lib/chkname.c, src/: Strictly disallow really bad names #1158

alejandro-colomar · 2024-12-23T10:11:17Z

Some names are bad, and some names are really bad.  '--badname' should
only allow the mildly bad ones, which we can handle.  Some names are too
bad, and it's not possible to deal with them.  Reject them
unconditionally.

Cc: @zeha
Cc: @Zugschlus
Cc: @ikerexxe
Cc: @hallyn

Revisions:

v1b

Acked-by: @zeha

$ git range-diff streq gh/reallybadnames reallybadnames 
1:  1c5acb5f ! 1:  9af4a680 lib/chkname.c, src/: Strictly disallow really bad names
    @@ Commit message
         bad, and it's not possible to deal with them.  Reject them
         unconditionally.
     
    -    Cc: Chris Hofstaedtler <[email protected]>
    +    Acked-by: Chris Hofstaedtler <[email protected]>
         Cc: Marc 'Zugschlus' Haber <[email protected]>
         Cc: Iker Pedrosa <[email protected]>
         Cc: Serge Hallyn <[email protected]>

v2

Reject a leading hyphen-minus unconditionally. So many tools would not be able to handle this; probably including ourselves.

$ git range-diff streq 9af4a680 reallybadnames 
1:  9af4a680 ! 1:  8a17ba49 lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c
     +#include "string/ctype/strchrisascii/strchriscntrl.h"
     +#include "string/ctype/strisascii/strisdigit.h"
      #include "string/strcmp/streq.h"
    ++#include "string/strcmp/strprefix.h"
      
      
    + int allow_bad_names = false;
     @@ lib/chkname.c: login_name_max_size(void)
      static bool
      is_valid_name(const char *name)
    @@ lib/chkname.c: login_name_max_size(void)
     +   || streq(name, ".")
     +   || streq(name, "..")
     +   || strpbrk(name, ",:")
    ++   || strprefix(name, "-")
     +   || strchriscntrl(name)
     +   || strisdigit(name))
     +  {

v3

Reject spaces unconditionally. [@zeha ]
Spaces are used as structuring characters in several programs (e.g., id(1)). Don't mess with them.

$ git range-diff streq gh/reallybadnames reallybadnames 
1:  8a17ba49 ! 1:  5369182a lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c: login_name_max_size(void)
     +  if (streq(name, "")
     +   || streq(name, ".")
     +   || streq(name, "..")
    -+   || strpbrk(name, ",:")
    ++   || strpbrk(name, ",: ")
     +   || strprefix(name, "-")
     +   || strchriscntrl(name)
     +   || strisdigit(name))

v4

Add / to the ban. [@Zugschlus ]

$ git range-diff gh/streq gh/reallybadnames reallybadnames 
1:  5369182a ! 1:  810bc45c lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c: login_name_max_size(void)
     +  if (streq(name, "")
     +   || streq(name, ".")
     +   || streq(name, "..")
    -+   || strpbrk(name, ",: ")
    ++   || strpbrk(name, ",: /")
     +   || strprefix(name, "-")
     +   || strchriscntrl(name)
     +   || strisdigit(name))

v4b

Rebase

$ git range-diff alx/master..gh/reallybadnames master..reallybadnames 
1:  4aee5e06 = 1:  95b045d3 lib/chkname.c: is_valid_name(): Use streq() instead of its pattern
2:  dda02b8b = 2:  d2f54847 src/useradd.c: create_home(): Use !streq() instead of its pattern
3:  810bc45c ! 3:  5ddbdca8 lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c
     +#include "string/strcmp/strprefix.h"
      
      
    - int allow_bad_names = false;
    + #ifndef  LOGIN_NAME_MAX
     @@ lib/chkname.c: login_name_max_size(void)
      static bool
      is_valid_name(const char *name)

Whoops, it seems this was supposed to be based after the streq branch.

v4c

Rebase

$ git range-diff alx/master..gh/reallybadnames master..reallybadnames 
1:  95b045d3 = 1:  8aae1eed lib/chkname.c: is_valid_name(): Use streq() instead of its pattern
2:  d2f54847 = 2:  ddc631e5 src/useradd.c: create_home(): Use !streq() instead of its pattern
3:  5ddbdca8 = 3:  8be46b32 lib/chkname.c, src/: Strictly disallow really bad names

v4d

Rebase

$ git range-diff master..gh/reallybadnames shadow/master..reallybadnames 
1:  8aae1eed = 1:  f7c83692 lib/chkname.c: is_valid_name(): Use streq() instead of its pattern
2:  ddc631e5 = 2:  d1d2263b src/useradd.c: create_home(): Use !streq() instead of its pattern
3:  8be46b32 ! 3:  0716561b lib/chkname.c, src/: Strictly disallow really bad names
    @@ src/newusers.c: static int add_user (const char *name, uid_t uid, gid_t gid)
                                Prog, name);
     
      ## src/pwck.c ##
    -@@ src/pwck.c: static void check_pw_file (int *errors, bool *changed)
    +@@ src/pwck.c: static void check_pw_file (bool *errors, bool *changed)
                 */
      
                if (!is_valid_user_name(pwd->pw_name)) {

v5

Also disallow ". This is so that we don't need to call audit_value_needs_encoding(3) and audit_encode_value(3).

$ git range-diff master gh/reallybadnames reallybadnames 
1:  f7c83692 = 1:  f7c83692 lib/chkname.c: is_valid_name(): Use streq() instead of its pattern
2:  d1d2263b = 2:  d1d2263b src/useradd.c: create_home(): Use !streq() instead of its pattern
3:  0716561b ! 3:  74b54226 lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c: login_name_max_size(void)
     +  if (streq(name, "")
     +   || streq(name, ".")
     +   || streq(name, "..")
    -+   || strpbrk(name, ",: /")
    ++   || strpbrk(name, ",: /\"")
     +   || strprefix(name, "-")
     +   || strchriscntrl(name)
     +   || strisdigit(name))

v6

Disallow character @, and disallow a few case-insensitive names: none, all, and except. [@stoeckmann ]

$ git range-diff alx/master gh/reallybadnames reallybadnames 
1:  f7c83692 = 1:  f7c83692 lib/chkname.c: is_valid_name(): Use streq() instead of its pattern
2:  d1d2263b = 2:  d1d2263b src/useradd.c: create_home(): Use !streq() instead of its pattern
-:  -------- > 3:  f4b64e1e lib/string/strcmp/: strcaseeq(): Add function
3:  74b54226 ! 4:  831b11b1 lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c
     +#include "string/ctype/strchrisascii/strchriscntrl.h"
     +#include "string/ctype/strisascii/strisdigit.h"
      #include "string/strcmp/streq.h"
    ++#include "string/strcmp/strcaseeq.h"
     +#include "string/strcmp/strprefix.h"
      
      
    @@ lib/chkname.c: login_name_max_size(void)
     +  if (streq(name, "")
     +   || streq(name, ".")
     +   || streq(name, "..")
    -+   || strpbrk(name, ",: /\"")
    ++   || strcaseeq(name, "none")
    ++   || strcaseeq(name, "all")
    ++   || strcaseeq(name, "except")
     +   || strprefix(name, "-")
    ++   || strpbrk(name, ",: /@\"")
     +   || strchriscntrl(name)
     +   || strisdigit(name))
     +  {

v7

Reorder characters by ASCII code.
Add # to the ban. [@stoeckmann ]

$ git range-diff alx/master gh/reallybadnames reallybadnames 
1:  f7c83692 = 1:  f7c83692 lib/chkname.c: is_valid_name(): Use streq() instead of its pattern
2:  d1d2263b = 2:  d1d2263b src/useradd.c: create_home(): Use !streq() instead of its pattern
3:  f4b64e1e = 3:  f4b64e1e lib/string/strcmp/: strcaseeq(): Add function
4:  831b11b1 ! 4:  2e6ba45e lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c: login_name_max_size(void)
     +   || strcaseeq(name, "all")
     +   || strcaseeq(name, "except")
     +   || strprefix(name, "-")
    -+   || strpbrk(name, ",: /@\"")
    ++   || strpbrk(name, " \"#,/:@")
     +   || strchriscntrl(name)
     +   || strisdigit(name))
     +  {

v8

Add to the ban: |, &, ;, +, *, !

$ git range-diff alx/master gh/reallybadnames reallybadnames 
1:  f7c83692 = 1:  f7c83692 lib/chkname.c: is_valid_name(): Use streq() instead of its pattern
2:  d1d2263b = 2:  d1d2263b src/useradd.c: create_home(): Use !streq() instead of its pattern
3:  f4b64e1e = 3:  f4b64e1e lib/string/strcmp/: strcaseeq(): Add function
4:  2e6ba45e ! 4:  a1159f6b lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c: login_name_max_size(void)
     +   || strcaseeq(name, "all")
     +   || strcaseeq(name, "except")
     +   || strprefix(name, "-")
    -+   || strpbrk(name, " \"#,/:@")
    ++   || strpbrk(name, " !\"#&*+,/:;@|")
     +   || strchriscntrl(name)
     +   || strisdigit(name))
     +  {

v8b

Rebase

$ git range-diff master..gh/reallybadnames shadow/master..reallybadnames 
1:  f7c83692 < -:  -------- lib/chkname.c: is_valid_name(): Use streq() instead of its pattern
2:  d1d2263b < -:  -------- src/useradd.c: create_home(): Use !streq() instead of its pattern
3:  f4b64e1e = 1:  4eb2c509 lib/string/strcmp/: strcaseeq(): Add function
4:  a1159f6b = 2:  4be5a599 lib/chkname.c, src/: Strictly disallow really bad names

v9

Rebase. strcaseeq() is already available in master.

$ git range-diff master..gh/reallybadnames shadow/master..reallybadnames 
1:  4eb2c509 < -:  -------- lib/string/strcmp/: strcaseeq(): Add function
2:  4be5a599 = 1:  a1c47e47 lib/chkname.c, src/: Strictly disallow really bad names

v9b

Rebase

$ git range-diff master..gh/reallybadnames shadow/master..reallybadnames 
1:  a1c47e47 = 1:  50087342 lib/chkname.c, src/: Strictly disallow really bad names

v10

Rebase. strisdigit() is now available.

$ git range-diff master..gh/reallybadnames shadow/master..reallybadnames 
1:  50087342 ! 1:  4ececc33 lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c
      #include "defines.h"
      #include "chkname.h"
     +#include "string/ctype/strchrisascii/strchriscntrl.h"
    -+#include "string/ctype/strisascii/strisdigit.h"
    + #include "string/ctype/strisascii/strisdigit.h"
      #include "string/strcmp/streq.h"
     +#include "string/strcmp/strcaseeq.h"
     +#include "string/strcmp/strprefix.h"
    @@ lib/chkname.c: is_valid_name(const char *name)
     -         *
     -         * Also do not allow fully numeric names or just "." or "..".
               */
    --  int numeric;
      
    +-  if (strisdigit(name)) {
    +-          errno = EINVAL;
    +-          return false;
    +-  }
    +-
     -  if (streq(name, "") ||
     -      streq(name, ".") ||
     -      streq(name, "..") ||
    @@ lib/chkname.c: is_valid_name(const char *name)
                return false;
        }
      
    --  numeric = isdigit(*name);
    --
    -   while (!streq(++name, "")) {
    -           if (!((*name >= 'a' && *name <= 'z') ||
    -                 (*name >= 'A' && *name <= 'Z') ||
     @@ lib/chkname.c: is_valid_name(const char *name)
                      streq(name, "$")
                     ))
    @@ lib/chkname.c: is_valid_name(const char *name)
     +                  errno = EILSEQ;
                        return false;
                }
    --          numeric &= isdigit(*name);
    --  }
    --
    --  if (numeric) {
    --          errno = EINVAL;
    --          return false;
        }
    - 
    -   return true;
     
      ## src/newusers.c ##
     @@ src/newusers.c: static int add_user (const char *name, uid_t uid, gid_t gid)

v11

Add ~ to the ban list. (Debian forbid that character for a long time.)

$ git range-diff master gh/reallybadnames reallybadnames 
1:  4ececc33 ! 1:  02a2bb2b lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c: login_name_max_size(void)
     +   || strcaseeq(name, "all")
     +   || strcaseeq(name, "except")
     +   || strprefix(name, "-")
    -+   || strpbrk(name, " !\"#&*+,/:;@|")
    ++   || strpbrk(name, " !\"#&*+,/:;@|~")
     +   || strchriscntrl(name)
     +   || strisdigit(name))
     +  {

v11b

Rebase

$ git range-diff master..gh/reallybadnames shadow/master..reallybadnames 
1:  02a2bb2b = 1:  50485f7b lib/chkname.c, src/: Strictly disallow really bad names

v11c

Rebase

$ git rd
1:  50485f7b = 1:  5c3f1784 lib/chkname.c, src/: Strictly disallow really bad names

v11d

Rebase

$ git rd 
1:  5c3f1784 = 1:  db8d0a1b lib/chkname.c, src/: Strictly disallow really bad names

v11e

Rebase

$ git rd
1:  db8d0a1b = 1:  d19eba6d lib/chkname.c, src/: Strictly disallow really bad names

v11f

Rebase

$ git rd 
1:  d19eba6d = 1:  a5886986 lib/chkname.c, src/: Strictly disallow really bad names

v11g

Rebase

$ git rd
1:  a5886986 = 1:  5f9b7879 lib/chkname.c, src/: Strictly disallow really bad names

v11h

Rebase

$ git rd 
1:  5f9b7879 = 1:  06985d54 lib/chkname.c, src/: Strictly disallow really bad names

v11i

Rebase

$ git rd 
1:  06985d54 = 1:  566cea5c lib/chkname.c, src/: Strictly disallow really bad names

v11j

Rebase

$ git rd 
1:  566cea5c = 1:  ca200b77 lib/chkname.c, src/: Strictly disallow really bad names

v11k

Rebase

$ git rd 
1:  ca200b77 = 1:  538ddfbf lib/chkname.c, src/: Strictly disallow really bad names

v11l

Rebase

$ git rd 
1:  538ddfbf = 1:  2e1a85e3 lib/chkname.c, src/: Strictly disallow really bad names

v11m

Rebase

$ git rd 
1:  2e1a85e3 = 1:  a77c6672 lib/chkname.c, src/: Strictly disallow really bad names

v11n

Rebase

$ git rd 
1:  a77c6672 = 1:  4c606cdd lib/chkname.c, src/: Strictly disallow really bad names

v11o

Rebase

$ git rd 
1:  4c606cdd = 1:  3881973b lib/chkname.c, src/: Strictly disallow really bad names

v11p

Rebase

$ git rd 
1:  3881973b = 1:  b6fe5b47 lib/chkname.c, src/: Strictly disallow really bad names

v11q

Rebase

$ git rd 
1:  b6fe5b47 = 1:  cc7e081f lib/chkname.c, src/: Strictly disallow really bad names

v11r

Rebase

$ git rd 
1:  cc7e081f ! 1:  f34485e7 lib/chkname.c, src/: Strictly disallow really bad names
    @@ src/newusers.c: static int add_user (const char *name, uid_t uid, gid_t gid)
                                Prog, name);
     
      ## src/pwck.c ##
    -@@ src/pwck.c: static void check_pw_file (bool *errors, bool *changed)
    +@@ src/pwck.c: static void check_pw_file (bool *errors, bool *changed, struct option_flags *fla
                 */
      
                if (!is_valid_user_name(pwd->pw_name)) {
    @@ src/pwck.c: static void check_pw_file (bool *errors, bool *changed)
                        } else {
     
      ## src/useradd.c ##
    -@@ src/useradd.c: static void process_flags (int argc, char **argv)
    +@@ src/useradd.c: static void process_flags (int argc, char **argv, struct option_flags *flags)
      
                user_name = argv[optind];
                if (!is_valid_user_name(user_name)) {
    @@ src/useradd.c: static void process_flags (int argc, char **argv)
                                        Prog, user_name);
     
      ## src/usermod.c ##
    -@@ src/usermod.c: process_flags(int argc, char **argv)
    +@@ src/usermod.c: process_flags(int argc, char **argv, struct option_flags *flags)
                                /*@notreached@*/break;
                        case 'l':
                                if (!is_valid_user_name(optarg)) {

v11s

Add comments. [@zeha ]

$ git range-diff shadow/master gh/reallybadnames reallybadnames 
1:  f34485e7 ! 1:  7dae722c lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c: login_name_max_size(void)
     +  if (streq(name, "")
     +   || streq(name, ".")
     +   || streq(name, "..")
    -+   || strcaseeq(name, "none")
    -+   || strcaseeq(name, "all")
    -+   || strcaseeq(name, "except")
    ++   || strcaseeq(name, "none")  // access.conf(5)
    ++   || strcaseeq(name, "all")  // access.conf(5)
    ++   || strcaseeq(name, "except")  // access.conf(5)
     +   || strprefix(name, "-")
     +   || strpbrk(name, " !\"#&*+,/:;@|~")
     +   || strchriscntrl(name)

v11t

Sort access.conf(5) stuff alphabetically. [@stoeckmann ]
Acked-by: @stoeckmann

$ git range-diff shadow/master gh/reallybadnames reallybadnames 
1:  7dae722c ! 1:  a1344ae8 lib/chkname.c, src/: Strictly disallow really bad names
    @@ Commit message
         unconditionally.
     
         Acked-by: Chris Hofstaedtler <[email protected]>
    +    Acked-by: Tobias Stoeckmann <[email protected]>
         Cc: Marc 'Zugschlus' Haber <[email protected]>
         Cc: Iker Pedrosa <[email protected]>
         Cc: Serge Hallyn <[email protected]>
    @@ lib/chkname.c: login_name_max_size(void)
     +  if (streq(name, "")
     +   || streq(name, ".")
     +   || streq(name, "..")
    -+   || strcaseeq(name, "none")  // access.conf(5)
    -+   || strcaseeq(name, "all")  // access.conf(5)
    ++   || strcaseeq(name, "all")     // access.conf(5)
     +   || strcaseeq(name, "except")  // access.conf(5)
    ++   || strcaseeq(name, "none")    // access.conf(5)
     +   || strprefix(name, "-")
     +   || strpbrk(name, " !\"#&*+,/:;@|~")
     +   || strchriscntrl(name)

v11u

Rebase

$ git rd
1:  a1344ae89 = 1:  5f87a82ab lib/chkname.c, src/: Strictly disallow really bad names

v12

Use strspn(3) instead of strprefix(). Both are fine, and strspn(3) is standard. It's also more useful when more than one leading characters are of interest, which is quite common (see lib/, src/: Use strspn(3) and strcspn(3) instead of their patterns #1369).

$ git rd 
1:  5f87a82ab ! 1:  1dea42b9e lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c
       */
      
     @@
    + #include <limits.h>
    + #include <stdbool.h>
    + #include <stddef.h>
    ++#include <string.h>
    + #include <unistd.h>
      
      #include "defines.h"
      #include "chkname.h"
    @@ lib/chkname.c
      #include "string/ctype/strisascii/strisdigit.h"
      #include "string/strcmp/streq.h"
     +#include "string/strcmp/strcaseeq.h"
    -+#include "string/strcmp/strprefix.h"
      
      
      #ifndef  LOGIN_NAME_MAX
    @@ lib/chkname.c: login_name_max_size(void)
     +   || strcaseeq(name, "all")     // access.conf(5)
     +   || strcaseeq(name, "except")  // access.conf(5)
     +   || strcaseeq(name, "none")    // access.conf(5)
    -+   || strprefix(name, "-")
    ++   || strspn(name, "-")
     +   || strpbrk(name, " !\"#&*+,/:;@|~")
     +   || strchriscntrl(name)
     +   || strisdigit(name))

v12b

Rebase

$ git rd 
1:  1dea42b9e = 1:  429bdbed7 lib/chkname.c, src/: Strictly disallow really bad names

v12c

Rebase

$ git rd 
1:  429bdbed7 = 1:  39cc733c0 lib/chkname.c, src/: Strictly disallow really bad names

v13

Don't disallow all,except,none. [@hallyn]

$ git rd 
1:  39cc733c0 ! 1:  d333a6fa5 lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c: login_name_max_size(void)
     +  if (streq(name, "")
     +   || streq(name, ".")
     +   || streq(name, "..")
    -+   || strcaseeq(name, "all")     // access.conf(5)
    -+   || strcaseeq(name, "except")  // access.conf(5)
    -+   || strcaseeq(name, "none")    // access.conf(5)
     +   || strspn(name, "-")
     +   || strpbrk(name, " !\"#&*+,/:;@|~")
     +   || strchriscntrl(name)

v13b

Rebase

$ git rd 
1:  d333a6fa5 = 1:  235e72aed lib/chkname.c, src/: Strictly disallow really bad names

v13c

Rebase

$ git rd 
1:  235e72aed ! 1:  58f4dc970 lib/chkname.c, src/: Strictly disallow really bad names
    @@ src/newusers.c: static int add_user (const char *name, uid_t uid, gid_t gid)
                                Prog, name);
     
      ## src/pwck.c ##
    -@@ src/pwck.c: static void check_pw_file (bool *errors, bool *changed, struct option_flags *fla
    +@@ src/pwck.c: static void check_pw_file(bool *errors, bool *changed, const struct option_flags
                 */
      
                if (!is_valid_user_name(pwd->pw_name)) {

v14

Split into two commits.

alx@devuan:~/src/shadow/shadow/master$ git diff gh/reallybadnames 
alx@devuan:~/src/shadow/shadow/master$ git rd 
1:  58f4dc970 ! 1:  1d2c5ea5e lib/chkname.c, src/: Strictly disallow really bad names
    @@ Commit message
         bad, and it's not possible to deal with them.  Reject them
         unconditionally.
     
    +    -  A leading '-' is too dangerous.  It breaks things like execve(2), and
    +       almost every command.
    +
    +    -  Spaces are used for delimiting lists of users and groups.
    +
    +    -  '"' is special in many languages, including the shell.  Having it in
    +       user names would be unnecessarily dangerous.
    +
    +    -  '#' is used for delimiting comments in several of our config files.
    +       Having it in usernames could result in incorrect configuration files.
    +
    +    -  ',' is used for delimiting lists of users and groups.
    +
    +    -  '/' is used for delimiting files, and thus could result in incorrect
    +       handling of users and groups.
    +
    +    -  ':' is the main delimiter in /etc/shadow and /etc/passwd.
    +
    +    -  ';' is special in many languages, including the shell.  Having it in
    +       user names would be unnecessarily dangerous.
    +
    +    There are other characters that we should disallow, but they need more
    +    research to make sure we don't introduce regressions.  This set should
    +    be less problematic.
    +
         Acked-by: Chris Hofstaedtler <[email protected]>
         Acked-by: Tobias Stoeckmann <[email protected]>
         Cc: Marc 'Zugschlus' Haber <[email protected]>
    @@ lib/chkname.c: login_name_max_size(void)
     +   || streq(name, ".")
     +   || streq(name, "..")
     +   || strspn(name, "-")
    -+   || strpbrk(name, " !\"#&*+,/:;@|~")
    ++   || strpbrk(name, " \"#,/:;")
     +   || strchriscntrl(name)
     +   || strisdigit(name))
     +  {
-:  --------- > 2:  3b4d01e48 lib/chkname.c, src/: Strictly disallow really bad names

lib/chkname.c

alejandro-colomar · 2024-12-23T10:24:04Z

The CI fails because I need to merge several PRs before this one:

lib/, src/: Use streq() instead of its pattern #1136
Add strisdigit(), and use it instead of its pattern #1153
Plus another patch set which depends on the previous one, and which I have locally. This one adds strchriscntrl().

ikerexxe · 2024-12-24T09:14:13Z

Before reviewing this PR I would like to come to an agreement on the long term path for this option, so let's wait for people to come and give us feedback at #1149. It will take some time to come to an agreement, but I think this is the best way forward.

alejandro-colomar · 2024-12-24T10:16:15Z

Before reviewing this PR I would like to come to an agreement on the long term path for this option, so let's wait for people to come and give us feedback at #1149. It will take some time to come to an agreement, but I think this is the best way forward.

Hmmm, I was thinking the other way around:

Whatever the agreement is for the future, some things should never be allowed.
We may remove --badname in the future, or we may add support for UTF-8 in the future,
but we all seem to agree that some characters are too dangerous to have in a username ever.

The files . and .. are special. A leading - starts options to commands. An empty name is special. Control characters are too dangerous. , and : are used as structure separators in the shadow files. And white space is used as structure separators in commands (e.g., id(1) output). Purely numeric are misinterpreted as a uid. All of those can never be fully supported, and thus must be unconditionally rejected.

I would also include slashes and backslashes in the ban, but I don't know if this will mess with Microsoft Windows users. Anyone knows?

Zugschlus · 2024-12-25T19:27:28Z

Not allowing backslashes might cause issues with Windows, and Slashes in the user name are probably a bad idea because of usage in shell scripts. In any case, an implicitly generated home directory from a user name containing slashes is bad, so if slashes stay allowed in user names, the home directory MUST be explicitly given, or alternatively sanitized, e.g. s//_/g

alejandro-colomar · 2024-12-26T01:02:53Z

On Wed, Dec 25, 2024 at 11:27:49AM -0800, Zugschlus wrote: Not allowing backslashes might cause issues with Windows, and Slashes in the user name are probably a bad idea because of usage in shell scripts. In any case, an implicitly generated home directory from a user name containing slashes is bad, so if slashes stay allowed in user names, the home directory MUST be explicitly given, or alternatively sanitized, e.g. s/\/_/g

Okay, so I'll add forward slashes to the ban. Thanks!

…

-- Reply to this email directly or view it on GitHub: #1158 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

-- <https://www.alejandro-colomar.es/>

alejandro-colomar · 2024-12-26T21:38:27Z

On Wed, Dec 25, 2024 at 11:27:49AM -0800, Zugschlus wrote: Not allowing backslashes might cause issues with Windows, and Slashes in the user name are probably a bad idea because of usage in shell scripts. In any case, an implicitly generated home directory from a user name containing slashes is bad, so if slashes stay allowed in user names, the home directory MUST be explicitly given, or alternatively sanitized, e.g. s//_/g
Okay, so I'll add forward slashes to the ban. Thanks!
…
-- Reply to this email directly or view it on GitHub: #1158 (comment) You are receiving this because you authored the thread. Message ID: @.***>
-- https://www.alejandro-colomar.es/

@Zugschlus

Done in v4 of this patch.

hallyn · 2024-12-31T19:04:38Z

I'm good with the intent of the patch.

Would like to take a closer look at the details (have to look up the particulars of things like strpbrk every time) before merging.

alejandro-colomar · 2024-12-31T19:08:28Z

I'm good with the intent of the patch.

Thanks!

Would like to take a closer look at the details (have to look up the particulars of things like strpbrk every time) before merging.

Sure. I need to check them every now and then. :)

strpbrk() is kind of like strchr(), but with several chars for matching. The name is unfortunate, but that's a simple way of thinking about it.

Here's a comparison of the descriptions in the manual pages:

DESCRIPTION
     The strchr() function returns a pointer to the first occurrence of
     the character c in the string s.

DESCRIPTION
     The  strpbrk() function locates the first occurrence in the string
     s of any of the bytes in the string accept.

I might have called it strchrs() instead...

alejandro-colomar · 2024-12-31T19:14:32Z

BTW, some of the APIs used in this patch are not yet merged. They are proposed in other PRs, which is why this thing is still a draft. Just in case you look up some definitions, they're not yet available.

alejandro-colomar · 2025-01-27T10:58:09Z

I've added " to the ban. The audit logger cannot handle those and would need to encode them, so let's keep it simple and disallow them.

lib/chkname.c

alejandro-colomar · 2025-11-28T16:06:25Z

Hmmm. Debian has extra code to disallow \?

No.

Hmmm, did Debian have extra code to allow \ and was recently removed?
I don't remember having changed anything in shadow, so where did that regression originate?

Or is it because of not using --badname?

Does passwd have that?

Sorry, it doesn't. But I'm confused. How were \ accepted before and not now?

Ah, no, other way around - shadow 4.13 in debian oldstable has a patch which relaxes the username check. That patch has been dropped.

Upstream never allowed a \ in usernames.

Hmmm, now it makes sense.

hallyn · 2025-11-28T16:18:35Z

Hmmm. Debian has extra code to disallow \?

No.

Or is it because of not using --badname?

Does passwd have that? You can see for yourself in the bug report what the user did.

So that's an interesting question for the debian codebase. They used to allow some
extra characters, now they do not. Forbidding future account creations with bad names
is one thing, but if the account already exists, surely they should be able to change
the password on it still, right?

hallyn · 2025-11-28T18:23:50Z

@alejandro-colomar ok, so, I don't think we should do this. It's not our place to stop
admins from creating accounts with names that can be bad in some situations, but useful
in others.

I think we should a) allow --baduser to be specified twice to say "no, really", b) not allow creating a homedir named for the user, and c) relax the rules for passwd, bc if you can passwd, then the user already exists.

hallyn · 2025-11-28T18:53:23Z

Now the ':' actually fails anyway

root@shadow:/root/shadow-4.13+dfsg1# useradd u5\:1 --badname
useradd: failure while writing changes to /etc/passwd
root@shadow:/root/shadow-4.13+dfsg1# useradd u5:1 --badname
useradd: failure while writing changes to /etc/passwd

The only problem with , ~, /, and # appears to be that even though it doesn't create
the home directory (useradd without -create-home), it does list it as the homedir in
/etc/passwd. Not sure we care. And admins who know what they're doing will specify
-d.

ubuntu:x:1000:1000::/home/ubuntu:/bin/bash
u1:x:1001:1001:,,,:/home/u1:/bin/bash
u2\1:x:1002:1002::/home/u2\1:/bin/sh
u3~2:x:1003:1003::/home/u3~2:/bin/sh
u4#2:x:1004:1004::/home/u4#2:/bin/sh
u5:x:1005:1005::/home/u5:/bin/sh
root@shadow:/root/shadow-4.13+dfsg1# ls -l /home
total 12
drwxrwxr-x 3 root   root   4096 Nov 28 15:23 serge
drwxr-x--- 2 u1     u1     4096 Nov 28 15:00 u1
drwxr-x--- 2 ubuntu ubuntu 4096 Nov 28 08:01 ubuntu

I need to think more about |, &, and ;.

alejandro-colomar · 2025-11-28T20:47:20Z

@alejandro-colomar ok, so, I don't think we should do this. It's not our place to stop admins from creating accounts with names that can be bad in some situations, but useful in others.

I think we should a) allow --baduser to be specified twice to say "no, really", b) not allow creating a homedir named for the user, and c) relax the rules for passwd, bc if you can passwd, then the user already exists.

I've been thinking this evening about this, and my idea was:

Merge this patch. Some characters would be really forbidden (we can discuss the set of such characters, but there should be some that are really forbidden).
Allow --badname once for reading, but not for adding new users.
Maybe add --new-badname or something different (or maybe passing it twice?) for adding bad names.

That would allow using domain\user with --badname.

zeha · 2025-11-28T21:04:03Z

TBH I think for backslash the ship has sailed.

From my PoV a minimal set of bad characters (like : and ) should be absolutely forbidden with no override mechanism. And everything else left to the users, effectively removing --badname.

I think that was basically also where this entire thing started.

alejandro-colomar · 2025-11-28T21:21:50Z

TBH I think for backslash the ship has sailed.

From my PoV a minimal set of bad characters (like : and ) should be absolutely forbidden with no override mechanism. And everything else left to the users, effectively removing --badname.

I think that was basically also where this entire thing started.

I personally think \ is busted, I agree.

But maybe we could analyze that after the release and allow it (after all, Debian is proof that \ at least worked partially). But I'd leave that for the future. For now, I'd merge this PR.

hallyn · 2025-11-29T04:27:21Z

@zeha by 'the ship has sailed for backslash' do you mean people are using it and it
should continue to be supported at least in some way, or do you mean most distros
don't allow it and so it shouldn't be allowed?

I'm aware that posix is quite strict, and systemd as well. I'm happy to have the
default be strict posix. Perhaps a new /etc/login.defs option should take the
place of the old --badname. Again, I do think (barring feedback from users to the
contrary) that it's fine to refuse to create homedirs with any of those characters.
And I continue to worry about scripts that, say, walk over ps output in an unsafe
way and getting tripped up by '#' etc in username.

@alejandro-colomar to be clear, I think you should not make any changes to the pr until
we have an agreement :)

zeha · 2025-11-29T11:10:34Z

it should continue to be supported at least in some way, or do you mean most distros
don't allow it and so it shouldn't be allowed?

@hallyn sorry for being unclear. I think most distros did not allow it and it should not be supported.

So if this ends up completely denying \:# or some other minimal set that would be a good start?

hallyn · 2025-11-29T13:50:05Z

it should continue to be supported at least in some way, or do you mean most distros
don't allow it and so it shouldn't be allowed?

@hallyn sorry for being unclear. I think most distros did not allow it and it should not be supported.

Ok, thanks for clarifying. I'm trying to poke a few people to find out whether there is
anything that really would require those. I'm guessing not - that sssd etc form the
\domainname for you and you can just use a very limited 'username' charset.

So if this ends up completely denying \:# or some other minimal set that would be a good start?

Ok, i think I'm good with this, but I'm thinking we should have two charsets, badset and
reallybadset, defined in a config file or configure.ac, so that distros or individuals
can rebuild differently if they're sure they know what they're doing.

I still think that passwd, userdel and usermod should support a wider set than useradd
and adduser. Else we're telling existing users that they have to either rebuild passwd,
or use it from a chroot, or just edit /etc/passwd by hand.

':' is not even worth considering since useradd doesn't even 'refuse', just fails to
save, the passwd file with a username with ':'. So yes it should always be in the
reallybadset.

I'm still wondering how we feel about other shell control chars - "&;|" in particular.
I think those belong in reallybadset, I see no legitimate use for those, and they're
more likely than not to mess up some user and process manipulation scripts somewhere.

zeha · 2025-11-29T15:45:09Z

I still think that passwd, userdel and usermod should support a wider set than useradd
and adduser. Else we're telling existing users that they have to either rebuild passwd,
or use it from a chroot, or just edit /etc/passwd by hand.

Agreed, provided that this cannot be abused to modify another user than intended.

alejandro-colomar · 2025-11-29T17:47:13Z

I still think that passwd, userdel and usermod should support a wider set than useradd and adduser. Else we're telling existing users that they have to either rebuild passwd, or use it from a chroot, or just edit /etc/passwd by hand.

I'm not so sure. I think existing users should seriously consider transitioning to not using these characters at all. A couple of edits by hand in the transition wouldn't hurt.

In the long term, I would prefer if all programs have the same set of reallybad characters, and they all refuse them.

Otherwise, we risk having some bugs in passwd that could accidentally modify unintended users.

hallyn · 2025-11-29T20:05:05Z

I still think that passwd, userdel and usermod should support a wider set than useradd and adduser. Else we're telling existing users that they have to either rebuild passwd, or use it from a chroot, or just edit /etc/passwd by hand.

I'm not so sure. I think existing users should seriously consider transitioning to not using these characters at all. A couple of edits by hand in the transition wouldn't hurt.

If it was myself and my team, I'd strongly agree. But I don't feel that's acceptable
for our users.

In the long term, I would prefer if all programs have the same set of reallybad characters, and they all refuse them.

For reallybad characters, like :, those are fine to be the same across all
programs. Noone should have : in a username.

Out of bad characters, again, we have some that are bad because they can impact
shell flow control for admin programs (like &, |, ; and #), we have some that are
bad because if the username is used for homedir, they can cause issues (like / and ~).
And some that really are just dangerous for shadow itself (':').

Otherwise, we risk having some bugs in passwd that could accidentally modify unintended users.

Agreed for ':', but what else do you foresee?

alejandro-colomar · 2025-11-29T22:49:20Z

In the long term, I would prefer if all programs have the same set of reallybad characters, and they all refuse them.

For reallybad characters, like :, those are fine to be the same across all programs. Noone should have : in a username.

Out of bad characters, again, we have some that are bad because they can impact shell flow control for admin programs (like &, |, ; and #), we have some that are bad because if the username is used for homedir, they can cause issues (like / and ~). And some that really are just dangerous for shadow itself (':').

Otherwise, we risk having some bugs in passwd that could accidentally modify unintended users.

Agreed for ':', but what else do you foresee?

We also separate strings on , and also interpret especially whitespace, #, and @, so those could be risky too. (BTW, I just realized I included spaces but not tabs in the set of really bad characters; I forgot.)

And we use the home directory, so . and .. (entire names) could also do some harm.

And there might be others; we'd have to check the code more carefully.

$ grep -rhoi 'st.sep[a-z_2]* *([^3][^)]\+)' src/ lib* | grep -o '".*"' | sort | uniq
" \t"
" \t\n"
", "
", \t"
","
",:"
":"
"="
"@"
"\""
"\n"

alejandro-colomar · 2025-11-29T22:53:09Z

These two PRs would be interesting to make that research easier:

Please consider merging them soonish.

hallyn · 2025-12-01T04:06:33Z

Yes, but 1. '.' and '..' have always been excluded on their own, and will continue to be. As will any usernames which begin with '-', or are only digits. Those are not part of the 'reallybad' or 'bad' charsets, they're a separate check for the whole username. 2. only : is used to separate the username in shadow or passwd. I don't believe the other separators are really relevant.

alejandro-colomar · 2025-12-01T10:14:30Z

only : is used to separate the username in shadow or passwd. I don't believe the other separators are really relevant.

shadow/src/login_nopam.c

Line 225 in e562faf

host = stpsep(tok + 1, "@"); /* split user@host pattern */

That's not about /etc/passwd nor /etc/shadow, but it's code where we do username separation, and we use @. There are a few other similar cases in the same file.

Are you sure we'd be okay accepting @ in user names?

Also, while we don't separate users on commas, we do separate groups on commas (and whitespace), and we have the same validator for users and for groups, so the validator should probably reject commas (and whitespace).

alejandro-colomar · 2025-12-04T17:50:19Z

(BTW, I just realized I included spaces but not tabs in the set of really bad characters; I forgot.)

Self-correction: I didn't forget about tabs. Tabs (and newlines) are control characters, and thus rejected in the strchriscntrl() check in the next line.

alejandro-colomar · 2025-12-04T17:56:53Z

BTW, I've been wondering about that dollar we allow for Samba. There's a comment that says it's for Samba 3.0. I see that Samba is in version 4., and that 3. seems to be EOL. Does Samba 4.* still use that dollar? Do we want to keep supporting that? Does anyone know of a maintainer that knows anything about Samba that could comment?

hallyn · 2025-12-04T20:58:00Z

Good point about commas in groups. And about @. So both belong in reallybadchars.

alejandro-colomar · 2025-12-04T22:39:31Z

Good point about commas in groups. And about @. So both belong in reallybadchars.

Let's have a look at all the characters again:

" !\"#&*+,/:;@|~"

SPACE: It's commonly used for separating groups. Really bad.
!: We don't have any code handling !, so we could maybe allow this one.
": We care about " while parsing login.defs, which can have group names. Really bad.
#: We use this as a comment start in several configuration files, so we must be careful and not allow users having such a character (especially leading). Really bad.
&: We don't have any code handling &, so we could maybe allow this one.
*: We use * specially in several places (e.g., useradd(8), usermod(8), lib/port.c). Really bad.
+: I see some code in login_nopam checking for leading +. Really bad.
,: We already agreed it's really bad.
/: Directory delimiter. Really bad.
:: The baddest.
;: I think we're not using this one, so we could maybe allow it.
@: We already agreed it's really bad.
|: I think we don't use it, so maybe we could allow it.
~: I think we don't use it, so maybe we could allow it.

alejandro-colomar · 2025-12-04T22:42:32Z

One thing I'm not sure (I don't remember) is in which moment we run these checks. Because if we disallow the @ in code that should actually be accepting @ in the meaning that we assign to it, maybe we'd be breaking code. Maybe we need several different name checkers?

Does anyone have any system that uses @ (or any other of these characters) so that it can test that this commit is fine for them?

Some names are bad, and some names are really bad. '--badname' should only allow the mildly bad ones, which we can handle. Some names are too bad, and it's not possible to deal with them. Reject them unconditionally. - A leading '-' is too dangerous. It breaks things like execve(2), and almost every command. - Spaces are used for delimiting lists of users and groups. - '"' is special in many languages, including the shell. Having it in user names would be unnecessarily dangerous. - '#' is used for delimiting comments in several of our config files. Having it in usernames could result in incorrect configuration files. - ',' is used for delimiting lists of users and groups. - '/' is used for delimiting files, and thus could result in incorrect handling of users and groups. - ':' is the main delimiter in /etc/shadow and /etc/passwd. - ';' is special in many languages, including the shell. Having it in user names would be unnecessarily dangerous. There are other characters that we should disallow, but they need more research to make sure we don't introduce regressions. This set should be less problematic. Acked-by: Chris Hofstaedtler <[email protected]> Acked-by: Tobias Stoeckmann <[email protected]> Cc: Marc 'Zugschlus' Haber <[email protected]> Cc: Iker Pedrosa <[email protected]> Cc: Serge Hallyn <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

Some names are bad, and some names are really bad. '--badname' should only allow the mildly bad ones, which we can handle. Some names are too bad, and it's not possible to deal with them. Reject them unconditionally. Acked-by: Chris Hofstaedtler <[email protected]> Acked-by: Tobias Stoeckmann <[email protected]> Cc: Marc 'Zugschlus' Haber <[email protected]> Cc: Iker Pedrosa <[email protected]> Cc: Serge Hallyn <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>

alejandro-colomar force-pushed the reallybadnames branch from dfb9f11 to 1c5acb5 Compare December 23, 2024 10:11

alejandro-colomar commented Dec 23, 2024

View reviewed changes

lib/chkname.c Show resolved Hide resolved

alejandro-colomar force-pushed the reallybadnames branch 6 times, most recently from 5c45a5a to 5369182 Compare December 23, 2024 18:28

alejandro-colomar mentioned this pull request Dec 23, 2024

Little Bobby Tables #1151

Draft

alejandro-colomar force-pushed the reallybadnames branch from 5369182 to 810bc45 Compare December 26, 2024 21:35

alejandro-colomar force-pushed the reallybadnames branch from 810bc45 to 5ddbdca Compare January 10, 2025 13:44

alejandro-colomar force-pushed the reallybadnames branch 2 times, most recently from 8be46b3 to 0716561 Compare January 24, 2025 15:20

alejandro-colomar mentioned this pull request Jan 27, 2025

Audit update #1199

Merged

alejandro-colomar force-pushed the reallybadnames branch from 0716561 to 74b5422 Compare January 27, 2025 10:54

stoeckmann suggested changes Feb 5, 2025

View reviewed changes

lib/chkname.c Outdated Show resolved Hide resolved

alejandro-colomar force-pushed the reallybadnames branch 2 times, most recently from 831b11b to 2e6ba45 Compare February 5, 2025 19:05

stoeckmann reviewed Feb 5, 2025

View reviewed changes

lib/chkname.c Outdated Show resolved Hide resolved

lib/chkname.c Outdated Show resolved Hide resolved

alejandro-colomar force-pushed the reallybadnames branch from 2e6ba45 to a1159f6 Compare February 6, 2025 07:38

alejandro-colomar force-pushed the reallybadnames branch from d333a6f to 235e72a Compare December 4, 2025 17:44

alejandro-colomar marked this pull request as draft December 4, 2025 23:35

alejandro-colomar force-pushed the reallybadnames branch from 235e72a to 58f4dc9 Compare December 6, 2025 17:19

alejandro-colomar added 2 commits December 6, 2025 18:21

alejandro-colomar force-pushed the reallybadnames branch from 58f4dc9 to 3b4d01e Compare December 6, 2025 17:32

alejandro-colomar mentioned this pull request Dec 6, 2025

lib/chkname.c, src/: Strictly disallow the baddest names #1406

Open

alejandro-colomar self-assigned this Dec 11, 2025

lib/chkname.c, src/: Strictly disallow really bad names #1158

Are you sure you want to change the base?

lib/chkname.c, src/: Strictly disallow really bad names #1158

Uh oh!

Conversation

alejandro-colomar commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alejandro-colomar commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ikerexxe commented Dec 24, 2024

Uh oh!

alejandro-colomar commented Dec 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zugschlus commented Dec 25, 2024

Uh oh!

alejandro-colomar commented Dec 26, 2024 via email

Uh oh!

alejandro-colomar commented Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hallyn commented Dec 31, 2024

Uh oh!

alejandro-colomar commented Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alejandro-colomar commented Dec 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alejandro-colomar commented Jan 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alejandro-colomar commented Nov 28, 2025

Uh oh!

hallyn commented Nov 28, 2025

Uh oh!

hallyn commented Nov 28, 2025

Uh oh!

hallyn commented Nov 28, 2025

Uh oh!

alejandro-colomar commented Nov 28, 2025

Uh oh!

zeha commented Nov 28, 2025

Uh oh!

alejandro-colomar commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hallyn commented Nov 29, 2025

Uh oh!

zeha commented Nov 29, 2025

Uh oh!

hallyn commented Nov 29, 2025

Uh oh!

zeha commented Nov 29, 2025

Uh oh!

alejandro-colomar commented Nov 29, 2025

Uh oh!

hallyn commented Nov 29, 2025

Uh oh!

alejandro-colomar commented Nov 29, 2025

Uh oh!

alejandro-colomar commented Nov 29, 2025

Uh oh!

hallyn commented Dec 1, 2025

Uh oh!

alejandro-colomar commented Dec 1, 2025

Uh oh!

alejandro-colomar commented Dec 4, 2025

Uh oh!

alejandro-colomar commented Dec 4, 2025

Uh oh!

hallyn commented Dec 4, 2025

Uh oh!

alejandro-colomar commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

alejandro-colomar commented Dec 23, 2024 •

edited

Loading

alejandro-colomar commented Dec 23, 2024 •

edited

Loading

alejandro-colomar commented Dec 24, 2024 •

edited

Loading

alejandro-colomar commented Dec 26, 2024 •

edited

Loading

alejandro-colomar commented Dec 31, 2024 •

edited

Loading

alejandro-colomar commented Dec 31, 2024 •

edited

Loading

alejandro-colomar commented Nov 28, 2025 •

edited

Loading

alejandro-colomar commented Dec 4, 2025 •

edited

Loading

alejandro-colomar commented Dec 4, 2025 •

edited

Loading