Little Bobby Tables #1151

alejandro-colomar · 2024-12-09T12:33:55Z

Closes: #1149
Link: https://www.wired.com/story/null-license-plate-landed-one-hacker-ticket-hell/
Link: https://xkcd.com/327/
Link: https://www.youtube.com/watch?v=hNoS2BU6bbQ
Link: https://lwn.net/Articles/1001215/
Link: https://dwheeler.com/essays/fixing-unix-linux-filenames.html
Link: #121
Link: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=42874
Link: https://lists.debian.org/debian-devel/2024/11/msg00250.html
Link: https://lists.debian.org/debian-devel/2024/12/msg00012.html
Link: https://lwn.net/Articles/1000485/
Cc: @ikerexxe
Cc: @hallyn
Cc: @thesamesam
Cc: @jubalh
Cc: @zeha
Cc: @rbalint
Cc: @Zugschlus

While at it:

src/: Report errors in user or groups names more consistently

Don't print the user name; if it's bad, it might be dangerous.
Print the string "user" or "group" before the error message.
Print the errno string to be consistent.

v1b

Rebase

$ git range-diff master..gh/nobadnames shadow/master..nobadnames 
1:  be021c88 ! 1:  7fea8a83 Little Bobby Tables
    @@ Commit message
     
      ## lib/chkname.c ##
     @@
    - #include "chkname.h"
    + #include "string/strcmp/streq.h"
      
      
     -int allow_bad_names = false;
2:  df6999b5 = 2:  a33b3c02 src/: Report errors in user or groups names more consistently

v2

Rebase after lib/chkname.c, src/: Strictly disallow really bad names #1158, and also after some local patches I have which improve readability.

$ git range-diff master..gh/nobadnames chkname..nobadnames 
1:  7fea8a83 ! 1:  51a73833 Little Bobby Tables
    @@ Commit message
     
      ## lib/chkname.c ##
     @@
    - #include "string/strcmp/streq.h"
    +  *   false - bad name
    +  * errors:
    +  *   EINVAL       Invalid name
    +- *   EILSEQ       Invalid name character sequence (acceptable with --badname)
    ++ *   EILSEQ       Invalid name character sequence
    +  *   EOVERFLOW    Name longer than maximum size
    +  */
    + 
    +@@
    + #include "string/strcmp/strprefix.h"
      
      
     -int allow_bad_names = false;
    @@ lib/chkname.c
      size_t
      login_name_max_size(void)
      {
    -@@ lib/chkname.c: login_name_max_size(void)
    - static bool
    - is_valid_name(const char *name)
    - {
    +@@ lib/chkname.c: is_valid_name(const char *name)
    +   if (streq(name, "")
    +    || streq(name, ".")
    +    || streq(name, "..")
    +-   || strpbrk(name, ",: ")
    +    || strprefix(name, "-")
    +-   || strchriscntrl(name)
    +    || strisdigit(name))
    +   {
    +           errno = EINVAL;
    +           return false;
    +   }
    + 
     -  if (allow_bad_names) {
     -          return true;
     -  }
    @@ src/newusers.c: static int add_user (const char *name, uid_t uid, gid_t gid)
      
     -  /* Check if this is a valid user name */
        if (!is_valid_user_name(name)) {
    --          if (errno == EINVAL) {
    +-          if (errno == EILSEQ) {
     -                  fprintf(stderr,
     -                          _("%s: invalid user name '%s': use --badname to ignore\n"),
     -                          Prog, name);
    @@ src/pwck.c: static void check_pw_file (int *errors, bool *changed)
     -           */
     -
                if (!is_valid_user_name(pwd->pw_name)) {
    --                  if (errno == EINVAL) {
    +-                  if (errno == EILSEQ) {
     -                          printf(_("invalid user name '%s': use --badname to ignore\n"),
     -                                 pwd->pw_name);
     -                  } else {
    @@ src/useradd.c: static void process_flags (int argc, char **argv)
      
                user_name = argv[optind];
                if (!is_valid_user_name(user_name)) {
    --                  if (errno == EINVAL) {
    +-                  if (errno == EILSEQ) {
     -                          fprintf(stderr,
     -                                  _("%s: invalid user name '%s': use --badname to ignore\n"),
     -                                  Prog, user_name);
    @@ src/usermod.c: process_flags(int argc, char **argv)
                                /*@notreached@*/break;
                        case 'l':
                                if (!is_valid_user_name(optarg)) {
    --                                  if (errno == EINVAL) {
    +-                                  if (errno == EILSEQ) {
     -                                          fprintf(stderr,
     -                                                  _("%s: invalid user name '%s': use --badname to ignore\n"),
     -                                                  Prog, optarg);
2:  a33b3c02 = 2:  d45dc827 src/: Report errors in user or groups names more consistently

v2b

Rebase

$ git range-diff alx/chkname..gh/nobadnames chkname..nobadnames 
1:  51a73833 = 1:  f423a89e Little Bobby Tables
2:  d45dc827 = 2:  eee679fe src/: Report errors in user or groups names more consistently

v2c

Rebase

$ git range-diff alx/chkname..gh/nobadnames chkname..nobadnames 
1:  f423a89e ! 1:  c892957c Little Bobby Tables
    @@ lib/chkname.c: is_valid_name(const char *name)
        if (streq(name, "")
         || streq(name, ".")
         || streq(name, "..")
    --   || strpbrk(name, ",: ")
    +-   || strpbrk(name, ",: /")
         || strprefix(name, "-")
     -   || strchriscntrl(name)
         || strisdigit(name))
2:  eee679fe = 2:  1cd7313f src/: Report errors in user or groups names more consistently

hallyn · 2024-12-10T01:57:15Z

I'd prefer aliasing --allow-badnames to --allow-unsafe-names. (But keeping the old name so anyone using it doesn't get their scripts broken).

Calling it allow-unsafe-names gives the clear message that we don't advise doing it. But I don't think it's our place to try to force people to stop.

If shadow itself is breaking on such names, then we should fix those places in shadow. The conern is that other userspace will break on the unsafe names. If someone makes a very tight full system where they're convinced unsafe names are fine for them, that's their prerogative.

zeha · 2024-12-10T11:06:40Z

I'd prefer aliasing --allow-badnames to --allow-unsafe-names. (But keeping the old name so anyone using it doesn't get their scripts broken).

Calling it allow-unsafe-names gives the clear message that we don't advise doing it. But I don't think it's our place to try to force people to stop.

If shadow itself is breaking on such names, then we should fix those places in shadow.

I agree whatever breaks in shadow should be fixed in shadow. It might be debatable what will be called broken though.

useradd -m --badname ../foo? (this makes a user called ../foo with a homedir of /home/../foo, resulting in /foo existing)
useradd -m --badname "$(printf '\n')"? (this makes a user with a zero-length name; cannot be deleted with deluser "$(printf '\n')")

The conern is that other userspace will break on the unsafe names. If someone makes a very tight full system where they're convinced unsafe names are fine for them, that's their prerogative.

I agree, but I see it as not pushing the rest of userspace forward to a world where more names will be allowed in general.

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

alejandro-colomar · 2024-12-10T23:02:46Z

If shadow itself is breaking on such names, then we should fix those places in shadow.

That's easier said than done. If one insists on using characters such as a newline, can we really "work"?

root@359f6c0bddb0:/# cp /etc/passwd /tmp/passwd
root@359f6c0bddb0:/# useradd --badname '\n'

root@359f6c0bddb0:/# diff -u /tmp/passwd /etc/passwd
--- /tmp/passwd	2024-12-10 22:56:06.914745615 +0000
+++ /etc/passwd	2024-12-10 22:56:57.662149407 +0000
@@ -16,3 +16,4 @@
 irc:x:39:39:ircd:/run/ircd:/usr/sbin/nologin
 _apt:x:42:65534::/nonexistent:/usr/sbin/nologin
 nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin
+\n:x:1000:1000::/home/\n:/bin/sh

root@359f6c0bddb0:/# useradd --badname '^M'

root@359f6c0bddb0:/# diff -u /tmp/passwd /etc/passwd
--- /tmp/passwd	2024-12-10 22:56:06.914745615 +0000
+++ /etc/passwd	2024-12-10 22:57:43.377648075 +0000
@@ -16,3 +16,5 @@
 irc:x:39:39:ircd:/run/ircd:/usr/sbin/nologin
 _apt:x:42:65534::/nonexistent:/usr/sbin/nologin
 nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin
+\n:x:1000:1000::/home/\n:/bin/sh
:/bin/sh1001::/home/

root@359f6c0bddb0:/# useradd --badname ':,'
useradd: failure while writing changes to /etc/passwd
root@359f6c0bddb0:/# useradd --badname ':'
useradd: failure while writing changes to /etc/passwd
root@359f6c0bddb0:/# useradd --badname ','

root@359f6c0bddb0:/# diff -u /tmp/passwd /etc/passwd
--- /tmp/passwd	2024-12-10 22:56:06.914745615 +0000
+++ /etc/passwd	2024-12-10 22:58:45.041004746 +0000
@@ -16,3 +16,6 @@
 irc:x:39:39:ircd:/run/ircd:/usr/sbin/nologin
 _apt:x:42:65534::/nonexistent:/usr/sbin/nologin
 nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin
+\n:x:1000:1000::/home/\n:/bin/sh
:/bin/sh1001::/home/
+,:x:1002:1002::/home/,:/bin/sh

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

Link: <https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/V1_chap03.html#tag_03_265> Signed-off-by: Alejandro Colomar <alx@kernel.org>

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

In the first case, we can do the transformation because a few lines above, we explicitly reject a name starting with a '-'. In the second case, we're obviously using ispfchar() instead of its pattern. Signed-off-by: Alejandro Colomar <alx@kernel.org>

lib/, man/, src/: Do not allow bad names Closes: <shadow-maint#1149> Link: <https://www.wired.com/story/null-license-plate-landed-one-hacker-ticket-hell/> Link: <https://xkcd.com/327/> Link: <https://www.youtube.com/watch?v=hNoS2BU6bbQ> Link: <https://lwn.net/Articles/1001215/> Link: <https://dwheeler.com/essays/fixing-unix-linux-filenames.html> Link: <shadow-maint#121> Link: <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=42874> Link: <https://lists.debian.org/debian-devel/2024/11/msg00250.html> Link: <https://lists.debian.org/debian-devel/2024/12/msg00012.html> Link: <https://lwn.net/Articles/1000485/> Cc: Iker Pedrosa <ipedrosa@redhat.com> Cc: Serge Hallyn <serge@hallyn.com> Cc: Sam James <sam@gentoo.org> Cc: Michael Vetter <jubalh@iodoru.org> Cc: Chris Hofstaedtler <zeha@debian.org> Cc: Balint Reczey <rbalint@debian.org> Cc: Marc Haber <mh+githubvisible@zugschlus.de> Signed-off-by: Alejandro Colomar <alx@kernel.org>

- Don't print the user name; if it's bad, it might be dangerous. - Print the string "user" or "group" before the error message. - Print the errno string to be consistent. Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar force-pushed the nobadnames branch from 2e94516 to 08ea278 Compare December 9, 2024 14:55

alejandro-colomar changed the title ~~lib/, man/, src/: Do not allow bad names~~ Little Bobby Tables Dec 9, 2024

alejandro-colomar force-pushed the nobadnames branch 2 times, most recently from ca3d0f2 to df6999b Compare December 9, 2024 22:44

alejandro-colomar added 2 commits December 10, 2024 15:59

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

4aee5e0

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

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

dda02b8

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

alejandro-colomar force-pushed the nobadnames branch 3 times, most recently from d45dc82 to eee679f Compare December 24, 2024 00:48

alejandro-colomar added 7 commits December 26, 2024 22:31

lib/ctype/: ispfchar(): Add function

2a18b2e

Link: <https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/V1_chap03.html#tag_03_265> Signed-off-by: Alejandro Colomar <alx@kernel.org>

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

29dde7d

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

lib/chkname.c: is_valid_name(): Split Samba check

531dd1e

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

src/: Report errors in user or groups names more consistently

1cd7313

- Don't print the user name; if it's bad, it might be dangerous. - Print the string "user" or "group" before the error message. - Print the errno string to be consistent. Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar force-pushed the nobadnames branch from eee679f to 1cd7313 Compare December 26, 2024 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Little Bobby Tables #1151

Little Bobby Tables #1151

alejandro-colomar commented Dec 9, 2024 •

edited

Loading

hallyn commented Dec 10, 2024

zeha commented Dec 10, 2024 •

edited

Loading

alejandro-colomar commented Dec 10, 2024

Little Bobby Tables #1151

Are you sure you want to change the base?

Little Bobby Tables #1151

Conversation

alejandro-colomar commented Dec 9, 2024 • edited Loading

hallyn commented Dec 10, 2024

zeha commented Dec 10, 2024 • edited Loading

alejandro-colomar commented Dec 10, 2024

alejandro-colomar commented Dec 9, 2024 •

edited

Loading

zeha commented Dec 10, 2024 •

edited

Loading