Little Bobby Tables

[alejandro-colomar]

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]

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]

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.

[alejandro-colomar]

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