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)) {

alejandro-colomar · 2024-12-23T10:14:44Z

lib/chkname.c

+	if (streq(name, "")
+	 || streq(name, ".")
+	 || streq(name, "..")
+	 || strpbrk(name, ",:")
+	 || strchriscntrl(name)
+	 || strisdigit(name))
+	{
+		errno = EINVAL;
+		return false;
+	}
+
 	if (allow_bad_names) {
 		return true;
 	}


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

Yeah that seems like a good plan to me.

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

Yes.

Acked-by: Chris Hofstaedtler [email protected]

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.

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

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 from 5ddbdca to 8be46b3 Compare January 18, 2025 22:44

alejandro-colomar added 3 commits January 24, 2025 16:13

lib/chkname.c: is_valid_name(): Use streq() instead of its pattern

f7c8369

Signed-off-by: Alejandro Colomar <[email protected]>

src/useradd.c: create_home(): Use !streq() instead of its pattern

d1d2263

Signed-off-by: Alejandro Colomar <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alejandro-colomar commented Dec 23, 2024 •

edited

Loading

alejandro-colomar Dec 23, 2024

zeha Dec 23, 2024

alejandro-colomar Dec 23, 2024

zeha Dec 23, 2024

alejandro-colomar commented Dec 23, 2024 •

edited

Loading

ikerexxe commented Dec 24, 2024

alejandro-colomar commented Dec 24, 2024 •

edited

Loading

Zugschlus commented Dec 25, 2024

alejandro-colomar commented Dec 26, 2024 via email

alejandro-colomar commented Dec 26, 2024 •

edited

Loading

hallyn commented Dec 31, 2024

alejandro-colomar commented Dec 31, 2024 •

edited

Loading

alejandro-colomar commented Dec 31, 2024 •

edited

Loading

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

Conversation

alejandro-colomar commented Dec 23, 2024 • edited Loading

alejandro-colomar Dec 23, 2024

Choose a reason for hiding this comment

zeha Dec 23, 2024

Choose a reason for hiding this comment

alejandro-colomar Dec 23, 2024

Choose a reason for hiding this comment

zeha Dec 23, 2024

Choose a reason for hiding this comment

alejandro-colomar commented Dec 23, 2024 • edited Loading

ikerexxe commented Dec 24, 2024

alejandro-colomar commented Dec 24, 2024 • edited Loading

Zugschlus commented Dec 25, 2024

alejandro-colomar commented Dec 26, 2024 via email

alejandro-colomar commented Dec 26, 2024 • edited Loading

hallyn commented Dec 31, 2024

alejandro-colomar commented Dec 31, 2024 • edited Loading

alejandro-colomar commented Dec 31, 2024 • edited Loading

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