passwd: add UPN validation support

[ikerexxe]

Add User Principal Name (UPN) validation to allow passwd command to accept usernames in user@domain.com format. This addresses rejection of valid UPN usernames by implementing proper RFC-compliant domain validation.

Changes include:

• Add is_valid_upn(), is_valid_domain_name(), and is_valid_domain_label() functions to lib/chkname.c with RFC 1035/1123 compliant validation
• Update passwd.c to accept both traditional usernames and UPN format
• Add comprehensive unit tests covering UPN validation edge cases
• Domain names limited to 253 chars total and 63 chars per label
• Maintains backward compatibility with existing username validation

Fixes: 326889c (2024-10-22; "Fix coverity unbound buffer issues")
Closes: #1626
Reported-by: @nooreldeenmansour
Supersedes: #1627

[ikerexxe]

This is what I’ve managed to put in place with the time I had available...

[ikerexxe]

@MarcinDigitic do you mind testing?

[alejandro-colomar]

I've fixed several issues for you.

v2:

• Use strspn(3) and strcspn(3) instead of manual byte operations.
• Use strndup(3) instead of its pattern.
• Allow trailing dot.
• Allow non-FQDN.
• Remove dead code.
• Simplify documentation.
• Other minor improvements.

range-diff

$ git range-diff --creation-factor=99 HEAD^^^ origin/passwd-upn passwd-upn -w
1:  a3d59004 ! 1:  8943055e lib: add UPN validation support
    @@ Metadata
     Author: Iker Pedrosa <ipedrosa@redhat.com>
     
      ## Commit message ##
    -    lib: add UPN validation support
    +    lib/chkname.*: Add UPN validation support
     
         Add is_valid_upn() function to validate User Principal Name format. UPN
         validation splits on @ and validates the prefix using existing username
         rules and suffix part using RFC 1035/1123 compliant domain name
         validation.
     
    +    Co-authored-by: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
    +    Co-authored-by: Alejandro Colomar <alx@kernel.org>
    +    Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/chkname.c ##
    +@@
    + 
    + 
    + /*
    +- * is_valid_user_name(), is_valid_group_name() - check the new user/group
    +- * name for validity;
    ++ * check user/group names for valid syntax
    +  * return values:
    +  *   true  - OK
    +  *   false - bad name
    +@@
    +  *   EINVAL       Invalid name
    +  *   EILSEQ       Invalid name character sequence (acceptable with --badname)
    +  *   EOVERFLOW    Name longer than maximum size
    ++ *   ENOMEM     Cannot allocate memory
    +  */
    + 
    + 
     @@
      #include "sysconf.h"
      
    @@ lib/chkname.c: is_valid_group_name(const char *name)
     +
     +
     +/*
    -+ * SYNOPSIS
    -+ *        static bool is_valid_domain_label(const char *label, size_t len);
    -+ *
    -+ * ARGUMENTS
    -+ *        label   Domain label string to validate.
    -+ *        len     Length of the label string.
    -+ *
    -+ * DESCRIPTION
     + * Validate a single DNS domain label according to RFC 1035/1123.
     + * Domain labels are components separated by dots in a domain name.
     + * For example, in "example.com", both "example" and "com" are labels.
    -+ *
    -+ * RETURN VALUE
    -+ *        true    Valid domain label.
    -+ *        false   Invalid domain label.
    -+ *
    -+ * ERRORS
    -+ *        This function does not set errno.
     + */
     +static bool
    -+is_valid_domain_label(const char *label, size_t len)
    ++is_valid_domain_label(const char *label)
     +{
    -+  size_t i;
    ++  size_t  len;
     +
    -+  /* Empty label is invalid */
    -+  if (len == 0 || len > LABEL_MAXLEN) {
    ++  if (strlen(label) > LABEL_MAXLEN) {
    ++          errno = EOVERFLOW;
     +          return false;
     +  }
    -+
    -+  /* Must start and end with alphanumeric */
    -+  if (!((label[0] >= 'a' && label[0] <= 'z') ||
    -+        (label[0] >= 'A' && label[0] <= 'Z') ||
    -+        (label[0] >= '0' && label[0] <= '9'))) {
    ++  if (!strcspn(label, "-")) {
    ++          errno = EINVAL;
     +          return false;
     +  }
    -+
    -+  if (len > 1) {
    ++  len = strlen(label);
     +  if (!((label[len-1] >= 'a' && label[len-1] <= 'z') ||
     +        (label[len-1] >= 'A' && label[len-1] <= 'Z') ||
    -+                (label[len-1] >= '0' && label[len-1] <= '9'))) {
    ++        (label[len-1] >= '0' && label[len-1] <= '9')))
    ++  {
    ++          errno = EINVAL;
     +          return false;
     +  }
    -+  }
     +
    -+  /* Check middle characters (can include hyphen) */
    -+  for (i = 1; i < len - 1; i++) {
    ++  for (size_t i = 0; i < len; i++) {
     +          if (!((label[i] >= 'a' && label[i] <= 'z') ||
     +                (label[i] >= 'A' && label[i] <= 'Z') ||
     +                (label[i] >= '0' && label[i] <= '9') ||
    -+                label[i] == '-')) {
    ++                label[i] == '-'))
    ++          {
    ++                  errno = EINVAL;
     +                  return false;
     +          }
     +  }
    @@ lib/chkname.c: is_valid_group_name(const char *name)
     +
     +
     +/*
    -+ * SYNOPSIS
    -+ *        static bool is_valid_domain_name(const char *domain);
    -+ *
    -+ * ARGUMENTS
    -+ *        domain  Domain name string to validate.
    -+ *
    -+ * DESCRIPTION
     + * Validate a domain name according to RFC 1035/1123 by splitting
     + * it into labels and validating each label individually.
    -+ *        Requires FQDN format (at least one dot).
    -+ *
    -+ * RETURN VALUE
    -+ *        true    Valid domain name.
    -+ *        false   Invalid domain name.
    -+ *
    -+ * ERRORS
    -+ *        EINVAL          Invalid domain structure (empty, leading/trailing dots, consecutive dots).
    -+ *        EILSEQ          Invalid domain label.
    -+ *        EOVERFLOW       Domain too long.
     + */
     +static bool
     +is_valid_domain_name(const char *domain)
     +{
    -+  const char *start, *end;
    -+  size_t domain_len, label_len;
    -+  bool has_dot = false;
    ++  char        *d;
    ++  const char  *l;
     +
    -+  if (streq(domain, "")) {
    -+          errno = EINVAL;
    -+          return false;
    -+  }
    -+
    -+  domain_len = strlen(domain);
    -+  if (domain_len > DOMAIN_MAXLEN) {
    ++  if (strlen(domain) > DOMAIN_MAXLEN) {
     +          errno = EOVERFLOW;
     +          return false;
     +  }
    -+
    -+  /* Check for leading or trailing dots */
    -+  if (domain[0] == '.' || domain[domain_len-1] == '.') {
    ++  if (!strcspn(domain, ".")) {
     +          errno = EINVAL;
     +          return false;
     +  }
     +
    -+  /* Parse and validate each label */
    -+  start = domain;
    -+  while ((end = strchr(start, '.')) != NULL) {
    -+          has_dot = true;
    -+          label_len = end - start;
    ++  d = strdupa(domain);
     +
    -+          /* Check for consecutive dots */
    -+          if (label_len == 0) {
    -+                  errno = EINVAL;
    -+                  return false;
    -+          }
    -+
    -+          if (!is_valid_domain_label(start, label_len)) {
    -+                  errno = EILSEQ;
    -+                  return false;
    -+          }
    -+
    -+          start = end + 1;
    -+  }
    -+
    -+  /* Validate final label */
    -+  label_len = strlen(start);
    -+  if (label_len == 0 || !is_valid_domain_label(start, label_len)) {
    -+          errno = EILSEQ;
    -+          return false;
    -+  }
    -+
    -+  /* Require FQDN (at least one dot) */
    -+  if (!has_dot) {
    -+          errno = EINVAL;
    ++  while (NULL != (l = strsep(&d, "."))) {
    ++          if (d == NULL && streq(l, ""))  // trailing root label
    ++                  break;
    ++          if (!is_valid_domain_label(l))
     +                  return false;
     +  }
     +
    @@ lib/chkname.c: is_valid_group_name(const char *name)
     +
     +
     +/*
    -+ * SYNOPSIS
    -+ *        bool is_valid_upn(const char *name);
    ++ * is_valid_upn - is valid User Principal Name
     + *
    -+ * ARGUMENTS
    -+ *        name    UPN (User Principal Name) string to validate.
    -+ *
    -+ * DESCRIPTION
    -+ *        Check UPN format (user@domain) for validity. A UPN consists of a
    -+ *        prefix (before @) and suffix (after @). The prefix must follow the
    -+ *        same rules as traditional usernames. The suffix part must be a
    ++ * Check UPN format (user@domain) for validity.  The user must follow
    ++ * the same rules as traditional usernames.  The domain part must be a
     + * valid domain name.
     + *
     + * This function only validates syntax, not whether the UPN exists
     + * in any authentication system.
    -+ *
    -+ * RETURN VALUE
    -+ *        true    Valid UPN format.
    -+ *        false   Invalid UPN format or not a UPN.
    -+ *
    -+ * ERRORS
    -+ *        EINVAL          Invalid UPN structure (empty parts, multiple @).
    -+ *        EILSEQ          Invalid character sequence.
    -+ *        EOVERFLOW       Name longer than maximum size.
    -+ *        ENOMEM          Memory allocation failure.
     + */
     +bool
    -+is_valid_upn(const char *name)
    ++is_valid_upn(const char *upn)
     +{
    ++  bool        ret;
    ++  char        *u;
     +  const char  *at_pos;
    -+  char *local_part, *domain_part;
    -+  size_t local_len;
    -+  bool result = false;
     +
    -+  /* Find exactly one @ symbol */
    -+  at_pos = strchr(name, '@');
    -+  if (at_pos == NULL) {
    -+          return false; /* Not a UPN */
    -+  }
    -+
    -+  /* Check for multiple @ symbols */
    -+  if (strchr(at_pos + 1, '@') != NULL) {
    -+          errno = EINVAL;
    ++  at_pos = strchr(upn, '@');
    ++  if (at_pos == NULL)
     +          return false;
    -+  }
     +
    -+  /* Extract prefix */
    -+  local_len = at_pos - name;
    -+  if (local_len == 0) {
    -+          errno = EINVAL;
    ++  u = strndup(upn, at_pos - upn);
    ++  if (u == NULL)
     +          return false;
    -+  }
     +
    -+  local_part = malloc(local_len + 1);
    -+  if (local_part == NULL) {
    -+          errno = ENOMEM;
    -+          return false;
    -+  }
    ++  ret = false;
     +
    -+  strncpy(local_part, name, local_len);
    -+  local_part[local_len] = '\0';
    -+
    -+  /* Extract suffix */
    -+  domain_part = strdup(at_pos + 1);
    -+  if (domain_part == NULL) {
    -+          errno = ENOMEM;
    -+          free(local_part);
    -+          return false;
    -+  }
    -+
    -+  if (streq(domain_part, "")) {
    -+          errno = EINVAL;
    ++  if (!is_valid_user_name(u))
     +          goto cleanup;
    -+  }
    -+
    -+  /* Validate local part using existing username rules */
    -+  if (!is_valid_user_name(local_part)) {
    ++  if (!is_valid_domain_name(at_pos + 1))
     +          goto cleanup;
    -+  }
    -+
    -+  /* Validate domain part using RFC-compliant domain name rules */
    -+  if (!is_valid_domain_name(domain_part)) {
    -+          goto cleanup;
    -+  }
    -+
    -+  result = true;
     +
    ++  ret = true;
     +cleanup:
    -+  free(local_part);
    -+  free(domain_part);
    -+  return result;
    ++  free(u);
    ++  return ret;
     +}
     
      ## lib/chkname.h ##
2:  5aa78162 = 2:  d9f3bc29 tests: add unit tests for UPN validation
3:  4ea29c1b = 3:  97b1a5c0 passwd: add UPN validation support

[alejandro-colomar]

I've fixed a few more issues for you.

v3:

• The first check is now a length check. I test against compile-time constants, which provides guarantees that the maximum string is quite short, and thus fits in the stack.
• The above allows me to drop the malloc(3) call (called through strndup(3)). Now that the string is really short, we can strdupa(3). This avoids ENOMEM entirely, and a vulnerability with it: the user can't force a crash with a huge UPN.
• Use stpsep() to simplify.
• Set errno in all false cases.
• Further reduce documentation.

range-diff

$ git range-diff HEAD^^^ origin/passwd-upn passwd-upn 
1:  8943055e ! 1:  6020aa6c lib/chkname.*: Add UPN validation support
    @@ lib/chkname.c
       *   true  - OK
       *   false - bad name
     @@
    -  *   EINVAL       Invalid name
    -  *   EILSEQ       Invalid name character sequence (acceptable with --badname)
    -  *   EOVERFLOW    Name longer than maximum size
    -+ *   ENOMEM     Cannot allocate memory
    -  */
      
    - 
    -@@
    + #include "defines.h"
    + #include "chkname.h"
    ++#include "sizeof.h"
    + #include "string/ctype/strchrisascii/strchriscntrl.h"
    + #include "string/ctype/strisascii/strisdigit.h"
    + #include "string/strcmp/streq.h"
    + #include "string/strcmp/strcaseeq.h"
    ++#include "string/strtok/stpsep.h"
      #include "sysconf.h"
      
      
    @@ lib/chkname.c: is_valid_group_name(const char *name)
     +/*
     + * is_valid_upn - is valid User Principal Name
     + *
    -+ * Check UPN format (user@domain) for validity.  The user must follow
    -+ * the same rules as traditional usernames.  The domain part must be a
    -+ * valid domain name.
    ++ * Check UPN format (user@domain) for validity.
     + *
     + * This function only validates syntax, not whether the UPN exists
     + * in any authentication system.
    @@ lib/chkname.c: is_valid_group_name(const char *name)
     +bool
     +is_valid_upn(const char *upn)
     +{
    -+  bool        ret;
    -+  char        *u;
    -+  const char  *at_pos;
    ++  char  *u, *d;
     +
    -+  at_pos = strchr(upn, '@');
    -+  if (at_pos == NULL)
    ++  if (strlen(upn) >= LOGIN_NAME_MAX + STRLEN("@") + DOMAIN_MAXLEN) {
    ++          errno = EOVERFLOW;
     +          return false;
    ++  }
     +
    -+  u = strndup(upn, at_pos - upn);
    -+  if (u == NULL)
    ++  u = strdupa(upn);
    ++  d = stpsep(u, "@");
    ++  if (d == NULL) {
    ++          errno = EINVAL;
     +          return false;
    ++  }
     +
    -+  ret = false;
    -+
    -+  if (!is_valid_user_name(u))
    -+          goto cleanup;
    -+  if (!is_valid_domain_name(at_pos + 1))
    -+          goto cleanup;
    -+
    -+  ret = true;
    -+cleanup:
    -+  free(u);
    -+  return ret;
    ++  return is_valid_user_name(u) && is_valid_domain_name(d);
     +}
     
      ## lib/chkname.h ##
2:  d9f3bc29 = 2:  01d04883 tests: add unit tests for UPN validation
3:  97b1a5c0 = 3:  0428deec passwd: add UPN validation support

---

v3b:

• Move the length checks closer to the corresponding strdupa(3) calls, to make it easier to verify that strdupa(3) calls are safe.

range-diff

$ git range-diff HEAD^^^ origin/passwd-upn passwd-upn 
1:  6020aa6c ! 1:  cf710e51 lib/chkname.*: Add UPN validation support
    @@ lib/chkname.c: is_valid_group_name(const char *name)
     +  char        *d;
     +  const char  *l;
     +
    -+  if (strlen(domain) > DOMAIN_MAXLEN) {
    -+          errno = EOVERFLOW;
    -+          return false;
    -+  }
     +  if (!strcspn(domain, ".")) {
     +          errno = EINVAL;
     +          return false;
     +  }
     +
    ++  if (strlen(domain) > DOMAIN_MAXLEN) {
    ++          errno = EOVERFLOW;
    ++          return false;
    ++  }
     +  d = strdupa(domain);
     +
     +  while (NULL != (l = strsep(&d, "."))) {
    @@ lib/chkname.c: is_valid_group_name(const char *name)
     +          errno = EOVERFLOW;
     +          return false;
     +  }
    -+
     +  u = strdupa(upn);
    ++
     +  d = stpsep(u, "@");
     +  if (d == NULL) {
     +          errno = EINVAL;
2:  01d04883 = 2:  9635aa1b tests: add unit tests for UPN validation
3:  0428deec = 3:  d2f556b7 passwd: add UPN validation support

[alejandro-colomar]

I realize that one of the unit tests is failing, I'll fix it when related PRs are merged and I can base my work on stable APIs

I think the reason for the failing tests is that I changed the code to accept user@domain.com. with a trailing root label, and also user@domain with a non-fully-qualified domain.

[alejandro-colomar]

I've fixed a few more things for you.

v4:

• Rebase

range-diff

$ git range-diff origin/passwd-upn^^^..origin/passwd-upn upstream/master..passwd-upn 
1:  adc23074fe38 ! 1:  052644053b3d lib/chkname.*: Add UPN validation support
    @@ lib/chkname.c
      #include "defines.h"
      #include "chkname.h"
     +#include "sizeof.h"
    - #include "string/ctype/strchrisascii/strchriscntrl.h"
    - #include "string/ctype/strisascii/strisdigit.h"
    + #include "string/ctype/isascii.h"
      #include "string/strcmp/streq.h"
      #include "string/strcmp/strcaseeq.h"
     +#include "string/strtok/stpsep.h"
2:  f1f9f623dadd = 2:  522faa7a5861 tests: add unit tests for UPN validation
3:  d71db363028d = 3:  c3625ee0efcb passwd: add UPN validation support

v5:

• Test the last character against '-' to simplify.
• Use streq(stpspn(s, CTYPE_ALNUM_C "-"), "") to simplify.

range-diff

$ git range-diff origin/passwd-upn^^^..origin/passwd-upn upstream/master..passwd-upn 
1:  052644053b3d ! 1:  5e50c4a34637 lib/chkname.*: Add UPN validation support
    @@ lib/chkname.c: is_valid_group_name(const char *name)
     +static bool
     +is_valid_domain_label(const char *label)
     +{
    -+  size_t  len;
    -+
     +  if (strlen(label) > LABEL_MAXLEN) {
     +          errno = EOVERFLOW;
     +          return false;
     +  }
    -+  if (!strcspn(label, "-")) {
    ++  if (!strcspn(label, "-") || label[strlen(label)-1] == '-') {
     +          errno = EINVAL;
     +          return false;
     +  }
    -+  len = strlen(label);
    -+  if (!((label[len-1] >= 'a' && label[len-1] <= 'z') ||
    -+        (label[len-1] >= 'A' && label[len-1] <= 'Z') ||
    -+        (label[len-1] >= '0' && label[len-1] <= '9')))
    -+  {
    ++  if (!streq(stpspn(s, CTYPE_ALNUM_C "-"), "")) {
     +          errno = EINVAL;
     +          return false;
     +  }
     +
    -+  for (size_t i = 0; i < len; i++) {
    -+          if (!((label[i] >= 'a' && label[i] <= 'z') ||
    -+                (label[i] >= 'A' && label[i] <= 'Z') ||
    -+                (label[i] >= '0' && label[i] <= '9') ||
    -+                label[i] == '-'))
    -+          {
    -+                  errno = EINVAL;
    -+                  return false;
    -+          }
    -+  }
    -+
     +  return true;
     +}
     +
2:  522faa7a5861 = 2:  f0b23874316b tests: add unit tests for UPN validation
3:  c3625ee0efcb = 3:  48d9c38e8f0e passwd: add UPN validation support

Details

[alejandro-colomar]

I've fixed a bug for you.

v6:

• The first character in a label must be a letter. The comment correctly said this, but the code didn't do it. Fix the code, and remove the comment. Add a link to the specification in the commit message.
• Use 2 spaces as inter-sentence spacing in commit message.

range-diff

$ git range-diff origin/passwd-upn^^^..origin/passwd-upn upstream/master..passwd-upn 
1:  5e50c4a34637 ! 1:  b1822d4782a6 lib/chkname.*: Add UPN validation support
    @@ Metadata
      ## Commit message ##
         lib/chkname.*: Add UPN validation support
     
    -    Add is_valid_upn() function to validate User Principal Name format. UPN
    +    Add is_valid_upn() function to validate User Principal Name format.  UPN
         validation splits on @ and validates the prefix using existing username
         rules and suffix part using RFC 1035/1123 compliant domain name
         validation.
     
    +    Link: <https://datatracker.ietf.org/doc/html/rfc1035#section-2.3.1>
         Co-authored-by: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
         Co-authored-by: Alejandro Colomar <alx@kernel.org>
    @@ lib/chkname.c: is_valid_group_name(const char *name)
      }
     +
     +
    -+/*
    -+ * Validate a single DNS domain label according to RFC 1035/1123.
    -+ * Domain labels are components separated by dots in a domain name.
    -+ * For example, in "example.com", both "example" and "com" are labels.
    -+ *
    -+ * According to RFC 1035 2.3.1 labels must start with a letter, end with
    -+ * a letter or digit, and have as interior characters only letters, digits,
    -+ * and hyphen. Labels must be 63 characters or less.
    -+ */
    ++// Validate a single DNS domain label according to RFC 1035 2.3.1.
     +static bool
     +is_valid_domain_label(const char *label)
     +{
    @@ lib/chkname.c: is_valid_group_name(const char *name)
     +          errno = EOVERFLOW;
     +          return false;
     +  }
    -+  if (!strcspn(label, "-") || label[strlen(label)-1] == '-') {
    ++  if (!strspn(label, CTYPE_ALPHA_C) || label[strlen(label)-1] == '-') {
     +          errno = EINVAL;
     +          return false;
     +  }
2:  f0b23874316b = 2:  339dde667336 tests: add unit tests for UPN validation
3:  48d9c38e8f0e = 3:  23c7a5328623 passwd: add UPN validation support

[ikerexxe]

I'm actively looking into this PR now that the dependencies are merged. Please do not push

[ikerexxe]

This is ready for review again

[alejandro-colomar]

Here's a review of the first commit. Except for one comment, it looks good to me.

[alejandro-colomar]

Thanks! LGTM.

Reviewed-by: Alejandro Colomar <alx@kernel.org>

I only have some comments about the source-code comments, but the source code is good.

[ikerexxe]

@alejandro-colomar feel free to merge if you agree with the changes

[alejandro-colomar]

Merged.

BTW, I forgot... I had this in mind a few weeks ago, but forgot about it... Is passwd the only program that needs this? Do we need to do the same thing for other programs?

[ikerexxe]

Yes, I think so. passwd is the only tool that can change data to external users

[alejandro-colomar]

Thanks! Let's keep it as is, and if someone finds and reports issues with any other tool, we'll address them. :)