4 ms·

This makes me think the musl people are right. Glibc is quite a large attack surface.

loeg 3y ago
Fwiw, the analogous logic in musl libc[1] uses log_ident set to the empty string if openlog() was called with NULL ident[2]. It also formats twice to buf[1024] without checking that the first format's length isn't in excess of the stack buffer but that's likely always safe given how short the stuff printed in the first format is. [1]: https://git.musl-libc.org/cgit/musl/tree/src/misc/syslog.c#n105 https://git.musl-libc.org/cgit/musl/tree/src/misc/syslog.c#n... [2]: https://git.musl-libc.org/cgit/musl/tree/src/misc/syslog.c#n68 https://git.musl-libc.org/cgit/musl/tree/src/misc/syslog.c#n...
- knorker 3y ago
  Interesting. The first snprintf() could return a negative value (at least by the glibc snprintf manpage), and then therefore the second vsnprintf will overwrite... something. Maybe their implementation can't return negative, or only if the fmt string (constant, here) is invalid? I wouldn't count on it, especially of it being true forever. That's pretty sloppy code.
  - loeg 3y ago
    I believe the first snprintf only returns a negative value if the format string is broken. Musl has decided to avoid that by not breaking the format string. If that changes, returning -1 is the least of their problems. You can see the implementation here: https://git.musl-libc.org/cgit/musl/tree/src/stdio/vsnprintf.c https://git.musl-libc.org/cgit/musl/tree/src/stdio/vsnprintf... and https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.c https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.... .
    - bodyfour 3y ago
      Probably the person you're replying to is just confused because before it was standardized some snprintf() implementations returned -1 on overflow. If you were trying to be portable and defensive you'd need to check for either error return. Not really a concern inside musl because those implementations are probably long gone and because it's calling its own snprintf() anyway.
      - knorker 3y ago
        I'm not confused. I know that some snprintf implementations returns/returned -1 on overflow. I assume that musl doesn't, because it's a fine library. > If you were trying to be portable and defensive you'd need to check for either error return. Including defensive against future changes. I'm sure thousands of bugs are being written every day because people don't check return values that "can't happen", because they know the code they call. Then 10-20 years later, someone changes that code they depend on, without violating the contract. I encounter these kinds of bugs all the time. There's a simple way to avoid them: Check the damn return values, even if just with an assert. The extra annoying ones are ones with a comment saying "Can't happen", that then does happen. The person who wrote that could have spent about the same number of characters simply handling the "can't happen". We can't get away from Hyrum's Law, but we sure can try to minimize its impact.
    - knorker 3y ago
      > I believe the first snprintf only returns a negative value if the format string is broken. I encounter bugs all the time that come from the fact that the caller knows the code they call, so they ignore the documented promises. Let's all at least try to not invoke Hyrum's Law, even for code in the same project.
- o11c 3y ago
  More relevant, it caps `log_ident` to a very short length. If my math is correct, the max length of the first snprintf is 72 bytes (actually less with real-world valid PIDs), and the buffer is 1024. As usual, MUSL is secure at the cost of giving up usefulness. A 32-byte buffer might seem like a lot, but there are dozens of programs with a name longer than 31 bytes on my system. `dbus-update-activation-environment` is the most interesting (most others are gcc/binutils), and I know dbus in general uses syslog, though I haven't traced this particular entry point.
  - loeg 3y ago
    Yeah. You have to math out the various fields but I agree it appears to end up a few orders of magnitude smaller than 1024. It's just not particularly explicit. > As usual, MUSL is secure at the cost of giving up usefulness. I would say 'Musl is deliberately simple at the cost of usefulness'. The simplicity lends itself to correctness and security. Btw, since Musl does not implicitly construct log_ident from argv[0], program name is sort of irrelevant unless that program explicitly openlog()s with argv[0]. dbus-update-activation-environment does not appear to openlog or use syslog at all: https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/tools/dbus-update-activation-environment.c https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/tools... Instead it prints error messages to stderr.
  - GoblinSlayer 3y ago
    Truncated to 31 bytes would be dbus-update-activation-environm. Is it ambiguous?
snvzz 3y ago
In a sane world, most distributions would have long switched to musl and libressl, from glibc and openssl.
- Arnavion 3y ago
  Distributions can't switch to libressl until all the software they package also supports it. libressl is incompatible with openssl, so programs that are compiled against the one cannot run against the other, and both cannot be loaded into the same address space. That's why the few distributions that did switch to libressl eventually switched back.
  - o11c 3y ago
    s/address space/link-map namespace/ See `dlmopen(3)`. Admittedly, multiple link-map namespaces are weird to use.
  - snvzz 3y ago
    >the few distributions that did switch to libressl eventually switched back Note that libressl supposedly improved openssl compatibility dramatically after that. Also note it would have been a very different scenario if Debian or Fedora actually made the switch as they should have, rather than a few smaller distributions. Instead, incompetence was rewarded, with plenty of money thrown openssl's way. This has, in hindsight, proven to not have been a good decision; code quality is still bad, and libressl is still a world better.
    - enormousness 3y ago
      Debian has a humongous package library, the biggest of any distro if I'm not mistaken. They need full drop-in compatibility between libressl and openssl if they hope to replace the latter any time soon for all those packages.
      - snvzz 3y ago
        The (current) compatibility is closer to drop-in than not. Most of these packages have maintainers and an upstream. It is just a matter of getting it done. There exist distros using libressl and musl, proving feasibility.
GoblinSlayer 3y ago
I wonder. su called PAM to authenticate the user, PAM called syslog to log authentication failure. Even if syslog was in another library, it would still be called, you would just have more libraries.

loeg 3y ago

Fwiw, the analogous logic in musl libc[1] uses log_ident set to the empty string if openlog() was called with NULL ident[2]. It also formats twice to buf[1024] without checking that the first format's length isn't in excess of the stack buffer but that's likely always safe given how short the stuff printed in the first format is. [1]: https://git.musl-libc.org/cgit/musl/tree/src/misc/syslog.c#n105 https://git.musl-libc.org/cgit/musl/tree/src/misc/syslog.c#n... [2]: https://git.musl-libc.org/cgit/musl/tree/src/misc/syslog.c#n68 https://git.musl-libc.org/cgit/musl/tree/src/misc/syslog.c#n...

knorker 3y ago
Interesting. The first snprintf() could return a negative value (at least by the glibc snprintf manpage), and then therefore the second vsnprintf will overwrite... something. Maybe their implementation can't return negative, or only if the fmt string (constant, here) is invalid? I wouldn't count on it, especially of it being true forever. That's pretty sloppy code.
- loeg 3y ago
  I believe the first snprintf only returns a negative value if the format string is broken. Musl has decided to avoid that by not breaking the format string. If that changes, returning -1 is the least of their problems. You can see the implementation here: https://git.musl-libc.org/cgit/musl/tree/src/stdio/vsnprintf.c https://git.musl-libc.org/cgit/musl/tree/src/stdio/vsnprintf... and https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.c https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.... .
  - bodyfour 3y ago
    Probably the person you're replying to is just confused because before it was standardized some snprintf() implementations returned -1 on overflow. If you were trying to be portable and defensive you'd need to check for either error return. Not really a concern inside musl because those implementations are probably long gone and because it's calling its own snprintf() anyway.
    - knorker 3y ago
      I'm not confused. I know that some snprintf implementations returns/returned -1 on overflow. I assume that musl doesn't, because it's a fine library. > If you were trying to be portable and defensive you'd need to check for either error return. Including defensive against future changes. I'm sure thousands of bugs are being written every day because people don't check return values that "can't happen", because they know the code they call. Then 10-20 years later, someone changes that code they depend on, without violating the contract. I encounter these kinds of bugs all the time. There's a simple way to avoid them: Check the damn return values, even if just with an assert. The extra annoying ones are ones with a comment saying "Can't happen", that then does happen. The person who wrote that could have spent about the same number of characters simply handling the "can't happen". We can't get away from Hyrum's Law, but we sure can try to minimize its impact.
  - knorker 3y ago
    > I believe the first snprintf only returns a negative value if the format string is broken. I encounter bugs all the time that come from the fact that the caller knows the code they call, so they ignore the documented promises. Let's all at least try to not invoke Hyrum's Law, even for code in the same project.
o11c 3y ago
More relevant, it caps `log_ident` to a very short length. If my math is correct, the max length of the first snprintf is 72 bytes (actually less with real-world valid PIDs), and the buffer is 1024. As usual, MUSL is secure at the cost of giving up usefulness. A 32-byte buffer might seem like a lot, but there are dozens of programs with a name longer than 31 bytes on my system. `dbus-update-activation-environment` is the most interesting (most others are gcc/binutils), and I know dbus in general uses syslog, though I haven't traced this particular entry point.
- loeg 3y ago
  Yeah. You have to math out the various fields but I agree it appears to end up a few orders of magnitude smaller than 1024. It's just not particularly explicit. > As usual, MUSL is secure at the cost of giving up usefulness. I would say 'Musl is deliberately simple at the cost of usefulness'. The simplicity lends itself to correctness and security. Btw, since Musl does not implicitly construct log_ident from argv[0], program name is sort of irrelevant unless that program explicitly openlog()s with argv[0]. dbus-update-activation-environment does not appear to openlog or use syslog at all: https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/tools/dbus-update-activation-environment.c https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/tools... Instead it prints error messages to stderr.
- GoblinSlayer 3y ago
  Truncated to 31 bytes would be dbus-update-activation-environm. Is it ambiguous?

snvzz 3y ago

In a sane world, most distributions would have long switched to musl and libressl, from glibc and openssl.

Arnavion 3y ago
Distributions can't switch to libressl until all the software they package also supports it. libressl is incompatible with openssl, so programs that are compiled against the one cannot run against the other, and both cannot be loaded into the same address space. That's why the few distributions that did switch to libressl eventually switched back.
- o11c 3y ago
  s/address space/link-map namespace/ See `dlmopen(3)`. Admittedly, multiple link-map namespaces are weird to use.
- snvzz 3y ago
  >the few distributions that did switch to libressl eventually switched back Note that libressl supposedly improved openssl compatibility dramatically after that. Also note it would have been a very different scenario if Debian or Fedora actually made the switch as they should have, rather than a few smaller distributions. Instead, incompetence was rewarded, with plenty of money thrown openssl's way. This has, in hindsight, proven to not have been a good decision; code quality is still bad, and libressl is still a world better.
  - enormousness 3y ago
    Debian has a humongous package library, the biggest of any distro if I'm not mistaken. They need full drop-in compatibility between libressl and openssl if they hope to replace the latter any time soon for all those packages.
    - snvzz 3y ago
      The (current) compatibility is closer to drop-in than not. Most of these packages have maintainers and an upstream. It is just a matter of getting it done. There exist distros using libressl and musl, proving feasibility.

GoblinSlayer 3y ago

I wonder. su called PAM to authenticate the user, PAM called syslog to log authentication failure. Even if syslog was in another library, it would still be called, you would just have more libraries.