Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alejandro-colomar
Copy link
Collaborator

@alejandro-colomar alejandro-colomar commented Dec 23, 2024

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
$ 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
$ 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

lib/chkname.c Outdated
Comment on lines 62 to 83
if (streq(name, "")
|| streq(name, ".")
|| streq(name, "..")
|| strpbrk(name, ",:")
|| strchriscntrl(name)
|| strisdigit(name))
{
errno = EINVAL;
return false;
}

if (allow_bad_names) {
return true;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zeha Do we have an agreement here? :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that seems like a good plan to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take that as an Acked-by, if you don't mind. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Acked-by: Chris Hofstaedtler [email protected]

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Dec 23, 2024

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

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

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
Copy link
Collaborator Author

alejandro-colomar commented Dec 24, 2024

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
Copy link

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
Copy link
Collaborator Author

alejandro-colomar commented Dec 26, 2024 via email

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Dec 26, 2024

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
Copy link
Member

hallyn commented Dec 31, 2024

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
Copy link
Collaborator Author

alejandro-colomar commented Dec 31, 2024

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
Copy link
Collaborator Author

alejandro-colomar commented Dec 31, 2024

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
Copy link
Collaborator Author

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.

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

This is queued after #1208.

The APIs we're using here are provided by that PR.

lib/chkname.c Outdated
|| strcaseeq(name, "all")
|| strcaseeq(name, "except")
|| strprefix(name, "-")
|| strpbrk(name, " !\"#&*+,/:;@|")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at the old Debian patches, and they also forbid a ~.

I guess it's a good idea to also forbid that character, to avoid problems with for example

$ cd ~alx;

Cc: @zeha

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that a problem for shadow?

Copy link
Collaborator Author

@alejandro-colomar alejandro-colomar Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not a problem for shadow, but for core packages.

Some other characters in that list may not be a problem for shadow, but are for other core packages (and sysadmin finger memory).

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]>
Cc: Marc 'Zugschlus' Haber <[email protected]>
Cc: Iker Pedrosa <[email protected]>
Cc: Serge Hallyn <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants