4 ms·

> "Tailscale SSH now rejects usernames with leading dashes." Really? That's the fix? A proper fix is to use "--" to separate arguments.

by cyberax 3mo ago

sedatk 3mo ago
Their fix just future-proofs it in case the same bug gets reintroduced.
- cyberax 3mo ago
  This is just a dirty fix. It adds weird restrictions and masks issues. Refactoring external invocations to use safe argument handling is a better way to fix it. Along with tests that exercise weird names.
  - sedatk 3mo ago
    I argue the opposite: there’s no better fix for this. You can write the most elegant fix, whatever it is, and prevent that from happening only on the codebase that’s fixed. That doesn’t mean that the codebase will always be the only authority on authentication. The username policy fixes this issue for good, regardless of whatever you write in the future, or whatever new mechanism is introduced. It’s a restriction for sure, but it’s not a nonsense restriction? Who would have a username starting with a hyphen? I didn’t even know it was possible until today.
    - turbert 3mo ago
      > I argue the opposite: there’s no better fix for this The better fix would be to not have the username pass through a parser looking for cli flags in the first place.
      - OJFord 3mo ago
        How do you propose to do that when username is a possible argument?
  - raggi 3mo ago
    http://github.com/tailscale/tailscale/commit/e4144230f410204aa2f43a07a57182c751239597 http://github.com/tailscale/tailscale/commit/e4144230f410204...
    - cyberax 3mo ago
      All the hallmarks of an LLM fix right there...
      - cyphar 3mo ago
        Indeed, but they did have code to use "os/user".Lookup() in a fallback path so I wonder if the issue is that they have some NSS module for user definitions (which wouldn't work with osusergo/static or musl-linked binaries)? If so, this is kind of an understandably ugly problem, though there is still a better option than shelling out -- systemd-userdb.
- turbert 3mo ago
  A correct implementation would be to just call glibc directly, this seems like a hasty fix to get the patch out the door. The history of vulns from bad shell escaping is as old as bash, whenever possible you probably shouldn't be mixing code and data, especially in a security critical application like this.
  - raggi 3mo ago
    The fact that there is no portable way to link the relevant functions that works reliably across all distributions of Linux is a failure of POSIX and GNU, and unfortunately is largely the Linux distribution story in a nutshell. Your answer is mostly correct, except that when you tug on that thread the shelf comes off the wall, the plaster comes with it, and then it cracks the water pipes on the way to the floor.
    - deleted 3mo ago
      [deleted]
valleyer 3mo ago
A proper fix is not to shell out to a command at all; use getpwnam(3) or similar.
- cyphar 3mo ago
  They actually do use "os/user".Lookup() as a fallback if fetching via getent(1) fails, so they might be using NSS modules to dynamically define users and then not linking their binaries with glibc (musl and the opt-in pure-Go implementation of "os/user".Lookup() do not support NSS). Still, requesting the information from systemd-userdb would be a better way of doing this if it's really necessary. EDIT: Yeah, some older comments confirm that was their reasoning. https://github.com/tailscale/tailscale/commit/1fc1077052dfa77e0efc7b5b308ead5e5e3e1965 https://github.com/tailscale/tailscale/commit/1fc1077052dfa7...
catlifeonmars 3mo ago
“--“ doesn’t work on all versions of getent. A better fix is to call “getent passwd” with no user controlled arguments and then parse the resulting list. This gets rid of the input sanitization problem entirely.
- cyberax 3mo ago
  Are there any actual systems that can run Tailscale and that have faulty getent?
  - raggi 3mo ago
    Yes
    - cyberax 3mo ago
      Which ones? Alpine supports it.
      - raggi 3mo ago
        Most variants don't, Alpine in fact does not, nor does the uclibc shell script, nor the commonly copied in implementation in many busybox environments. Reference: https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/main/musl/getent.c https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/m...
- tryauuum 3mo ago
  that's stupid. getent passwd will fetch the user list from the network depending on setup. Have fun fetching 40000 records from the network every call

> "Tailscale SSH now rejects usernames with leading dashes." Really? That's the fix? A proper fix is to use "--" to separate arguments.

by cyberax 3mo ago

> "Tailscale SSH now rejects usernames with leading dashes." Really? That's the fix? A proper fix is to use "--" to separate arguments.

sedatk 3mo ago

Their fix just future-proofs it in case the same bug gets reintroduced.

cyberax 3mo ago
This is just a dirty fix. It adds weird restrictions and masks issues. Refactoring external invocations to use safe argument handling is a better way to fix it. Along with tests that exercise weird names.
- sedatk 3mo ago
  I argue the opposite: there’s no better fix for this. You can write the most elegant fix, whatever it is, and prevent that from happening only on the codebase that’s fixed. That doesn’t mean that the codebase will always be the only authority on authentication. The username policy fixes this issue for good, regardless of whatever you write in the future, or whatever new mechanism is introduced. It’s a restriction for sure, but it’s not a nonsense restriction? Who would have a username starting with a hyphen? I didn’t even know it was possible until today.
  - turbert 3mo ago
    > I argue the opposite: there’s no better fix for this The better fix would be to not have the username pass through a parser looking for cli flags in the first place.
    - OJFord 3mo ago
      How do you propose to do that when username is a possible argument?
- raggi 3mo ago
  http://github.com/tailscale/tailscale/commit/e4144230f410204aa2f43a07a57182c751239597 http://github.com/tailscale/tailscale/commit/e4144230f410204...
  - cyberax 3mo ago
    All the hallmarks of an LLM fix right there...
    - cyphar 3mo ago
      Indeed, but they did have code to use "os/user".Lookup() in a fallback path so I wonder if the issue is that they have some NSS module for user definitions (which wouldn't work with osusergo/static or musl-linked binaries)? If so, this is kind of an understandably ugly problem, though there is still a better option than shelling out -- systemd-userdb.
turbert 3mo ago
A correct implementation would be to just call glibc directly, this seems like a hasty fix to get the patch out the door. The history of vulns from bad shell escaping is as old as bash, whenever possible you probably shouldn't be mixing code and data, especially in a security critical application like this.
- raggi 3mo ago
  The fact that there is no portable way to link the relevant functions that works reliably across all distributions of Linux is a failure of POSIX and GNU, and unfortunately is largely the Linux distribution story in a nutshell. Your answer is mostly correct, except that when you tug on that thread the shelf comes off the wall, the plaster comes with it, and then it cracks the water pipes on the way to the floor.
  - deleted 3mo ago
    [deleted]

valleyer 3mo ago

A proper fix is not to shell out to a command at all; use getpwnam(3) or similar.

cyphar 3mo ago
They actually do use "os/user".Lookup() as a fallback if fetching via getent(1) fails, so they might be using NSS modules to dynamically define users and then not linking their binaries with glibc (musl and the opt-in pure-Go implementation of "os/user".Lookup() do not support NSS). Still, requesting the information from systemd-userdb would be a better way of doing this if it's really necessary. EDIT: Yeah, some older comments confirm that was their reasoning. https://github.com/tailscale/tailscale/commit/1fc1077052dfa77e0efc7b5b308ead5e5e3e1965 https://github.com/tailscale/tailscale/commit/1fc1077052dfa7...

catlifeonmars 3mo ago

“--“ doesn’t work on all versions of getent. A better fix is to call “getent passwd” with no user controlled arguments and then parse the resulting list. This gets rid of the input sanitization problem entirely.

cyberax 3mo ago
Are there any actual systems that can run Tailscale and that have faulty getent?
- raggi 3mo ago
  Yes
  - cyberax 3mo ago
    Which ones? Alpine supports it.
    - raggi 3mo ago
      Most variants don't, Alpine in fact does not, nor does the uclibc shell script, nor the commonly copied in implementation in many busybox environments. Reference: https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/main/musl/getent.c https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/m...
tryauuum 3mo ago
that's stupid. getent passwd will fetch the user list from the network depending on setup. Have fun fetching 40000 records from the network every call