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

[alejandro-colomar]

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: @zeha
Acked-by: @stoeckmann
Cc: @Zugschlus
Cc: @ikerexxe
Cc: @hallyn

---

• This is a subset of lib/chkname.c, src/: Strictly disallow really bad names #1158 at v14.

---

Revisions:

v1b

• Rebase
• Reviewed-by @ikerexxe , @zeha

$ git rd 
1:  1d2c5ea5e ! 1:  e3d835320 lib/chkname.c, src/: Strictly disallow really bad names
    @@ Commit message
         research to make sure we don't introduce regressions.  This set should
         be less problematic.
     
    -    Acked-by: Chris Hofstaedtler <zeha@debian.org>
         Acked-by: Tobias Stoeckmann <tobias@stoeckmann.org>
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
    +    Reviewed-by: Chris Hofstaedtler <zeha@debian.org>
         Cc: Marc 'Zugschlus' Haber <mh+githubvisible@zugschlus.de>
    -    Cc: Iker Pedrosa <ipedrosa@redhat.com>
         Cc: Serge Hallyn <serge@hallyn.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     

v2

• Ban '.

$ git rd 
1:  e3d835320 ! 1:  266308458 lib/chkname.c, src/: Strictly disallow really bad names
    @@ Commit message
         -  '#' is used for delimiting comments in several of our config files.
            Having it in usernames could result in incorrect configuration files.
     
    +    -  "'" is special in many languages, including the shell.  Having it in
    +       user names would be unnecessarily dangerous.
    +
         -  ',' is used for delimiting lists of users and groups.
     
         -  '/' is used for delimiting files, and thus could result in incorrect
    @@ lib/chkname.c: login_name_max_size(void)
     +   || streq(name, ".")
     +   || streq(name, "..")
     +   || strspn(name, "-")
    -+   || strpbrk(name, " \"#,/:;")
    ++   || strpbrk(name, " \"#',/:;")
     +   || strchriscntrl(name)
     +   || strisdigit(name))
     +  {

v2b

• Rebase

$ git rd 
1:  266308458 ! 1:  defdaa950 lib/chkname.c, src/: Strictly disallow really bad names
    @@ lib/chkname.c: login_name_max_size(void)
                return true;
        }
     @@ lib/chkname.c: is_valid_name(const char *name)
    -          *
    -          * as a non-POSIX, extension, allow "$" as the last char for
    -          * sake of Samba 3.x "add machine script"
    --         *
    --         * Also do not allow fully numeric names or just "." or "..".
    -          */
    +    *
    +    * as a non-POSIX, extension, allow "$" as the last char for
    +    * sake of Samba 3.x "add machine script"
    +-   *
    +-   * Also do not allow fully numeric names or just "." or "..".
    +    */
      
     -  if (strisdigit(name)) {
     -          errno = EINVAL;

[alejandro-colomar]

This list of characters should be regression-free, I think.

I'll leave the remaining ones for a separate PR, for after the release.

[ikerexxe]

Can you update the documentation? It'd be nice to align the man page documents with the existing behaviour

[alejandro-colomar]

Can you update the documentation? It'd be nice to align the man page documents with the existing behaviour

The manual pages are still compatible with this change. They just say:

     -b, --badname
         Allow names that do not conform to standards.

Which is still true. We just allow less names with --badname after this change, but that text is still valid.

[ikerexxe]

You're absolutely right. My apologies, I was mistakenly focused on the specific character set mentioned in the CAVEATS section of the useradd man page, and overlooked that the description for --badname is still technically correct. No documentation update is needed for that specific flag.

[ikerexxe]

LGTM, but let's see what other people want to say before merging

[zeha]

LGTM, thanks.

[alejandro-colomar]

I'm going to add ' to the ban, because it is as dangerous as ", for the same exact reasons. I'll keep the Reviewed-by tags, as I don't expect a different opinion about it.

[alejandro-colomar]

Thanks! @hallyn , do you feel comfortable with this patch?

[hallyn]

Thanks.

[collinfunk]

This seems like a poor decision to me since you can just edit /etc/passwd and create a home directory manually to achieve the same thing.

I hope you do not consider further characters.

I have been considering adding tests to GNU coreutils which break implementations that assume UTF-8 encoding. For example, having \xc2 in a user name which is fine in some encodings:

$ LC_ALL=en_US.ISO-8859-1 printf '\xC2\n' | iconv --from-code=ISO-8859-1 --to-code=UTF-8 
Â

[alejandro-colomar]

This seems like a poor decision to me since you can just edit /etc/passwd and create a home directory manually to achieve the same thing.

Most of these characters either already failed in some cases (for example, : would have failed when counting the number of fields in /etc/shadow), or didn't fail but resulted in incorrect behavior (which would be worse). Spaces, tabs, and newlines are of course also terribly dangerous in user names. Etc.

I hope you do not consider further characters.

I'll investigate the source code, in case there are other characters that would result in shadow misbehaving already, such as possibly . and @.

I have been considering adding tests to GNU coreutils which break implementations that assume UTF-8 encoding. For example, having \xc2 in a user name which is fine in some encodings:

$ LC_ALL=en_US.ISO-8859-1 printf '\xC2\n' | iconv --from-code=ISO-8859-1 --to-code=UTF-8 
Â

We don't have in mind anything about UTF8. We're concerned about [[:cntrl:]], [[:space:]], and [[:punct:]].

[collinfunk]

I'll investigate the source code, in case there are other characters that would result in shadow misbehaving already, such as possibly . and @.

[...]

We don't have in mind anything about UTF8. We're concerned about [[:cntrl:]], [[:space:]], and [[:punct:]].

I see. That sounds reasonable. I think my idea of "future characters" was a bit more dramatic than yours. :)

Thanks for the quick response.