6 ms·

Thoughts on the Systemd Root Exploit

112 points by uggedal 10y ago

geofft 10y ago
> They have even replaced DNS with a dbus-based protocol, which they "strongly recommend" applications use instead of DNS. This seems inaccurate. The phrase "strongly recommend" appears once in the manpage, where it is strongly recommended that you use either the standard libc resolver API, with libnss_resolve, or the D-Bus API. Applications should be using the libc resolver API instead of implementing DNS themselves. There are some applications like Chrome that implement DNS themselves because they care very much about DNS; those applications presumably know how to do all the things systemd-resolved does. Everyone else should get name resolution functionality from libc. That's what you've been supposed to do for decades, and it's a standard UNIX interface. That standard interface supports things like LLMNR that you don't get if you implement DNS yourself. Unfortunately, the standard UNIX interface is synchronous, which is why libraries like ares or adns exist. If you want to use such a library, you can point it at 127.0.0.53, but you still have the limitations of what can be expressed in DNS. (And you're still using a nonstandard API to speak to libares or libadns.) No API exists that is standard, async, and does everything that libc getaddrinfo() is capable of doing. So systemd built one. That's pretty standard behavior for systemd: implement compatibility interfaces where they exist, recommend them if they're good (systemd explicitly recommends /etc/fstab over writing native mount units, because /etc/fstab is a perfectly good format), implement them anyway if they're not, and write a better API, based on D-Bus, when needed. The latter bit not going through a multi-implementer standards committee isn't great, but it's nowhere near as bad as presented. Anyway, this is completely irrelevant to the rest of the analysis, which seems absolutely correct, and I'm not sure why the author included this parting shot.
- j_s 10y ago
  I'm not sure why the author included this parting shot Everybody loves kicking systemd as they re-invent various wheels; see... even I can't resist!
- viraptor 10y ago
  They added the strongly from another paragraph, that's true. But the man page does recommend the dbus API over libc: > The native, fully-featured API systemd-resolved exposes on the bus. See the API Documentation[1] for details. Usage of this API is generally recommended to clients as it is asynchronous and fully featured
- agwa 10y ago
  Non-DNS resolution protocols like LLMNR are almost entirely irrelevant, particularly on servers. Even if you do need asynchronous LLMNR support, you do not need dbus and a particular process running as PID 1 to get it - applications can make LLMNR queries using an asynchronous library, just like many currently use an asynchronous DNS library. The fact that systemd keeps making decisions like this that are architecturally dubious and lead to lock-in is most certainly grounds for criticism.
  - noselasd 10y ago
    When people enter a hostname into my application, I'm not particularly keen on implementing name resolving myself via DNS, LDAP, hosts file and so on in the application - It's absurd to suggest applications should make such a decision. The current NSS system works nicely though, it just needs an async API.
  - natermer 10y ago
    Nothing systemd has done is preventing anybody from using any asynchronous library they feel like. It's providing local name resolution services. And for very good reasons. By your logic things like NSS is useless as well because programs themselves can read ldap configuration files and /etc/resolv.conf on their own using libraries or whatever else they feel like using.
- zx2c4 10y ago
  OpenBSD's libasr has been ported to Linux. http://man.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/asr_run.3 http://man.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/... https://github.com/OpenSMTPD/libasr https://github.com/OpenSMTPD/libasr
- bigbugbag 10y ago
  Reading the man page it is actually recommending systemd-resolved over other options. It says: - option 1 (recommended): use systemd-resolved API. - option 2: use glibc API with a glibc NSS module to resolve host names via systemd-resolved. - option 3 (not recommended): local DNS stub listener on loopback to connect direct request to systemd-resolved. Author included this part to illustrate how the real issue is that systemd is an unprecedented lock-in. Honestly an init process implementing a DNS resolver? Where is my kitchen sink ?
  - tapoxi 10y ago
    To be fair to systemd, systemd-resolved is not an init process. It is its own service that just happens to integrate with systemd and is part of the wider systemd project (with journald, timesyncd, etc.)
    - bigbugbag 10y ago
      to be fair to systemd, it is an init system with a severe case of feature creep to the point that it now includes a DNS resolver that came with vulnerabilities long fixed in the existing ones.
      - tapoxi 10y ago
        systemd-resolved is a separate package. It is not the init system and not a requirement of the init system.
        bigbugbag 10y ago
        Official systemd homepage[1] begs to differ, it says systemd is an init system including many features among which is name resolution: >systemd (...) provides a system and service manager that runs as PID 1 and starts the rest of the system. (...) Other parts include a logging daemon, (...), log forwarding, and name resolution. [1]: https://freedesktop.org/wiki/Software/systemd/ https://freedesktop.org/wiki/Software/systemd/ -edit- Not sure where it is a separate package, just checked debian and arch, the systemd package contains systemd-resolved. https://packages.debian.org/jessie/amd64/systemd/filelist https://packages.debian.org/jessie/amd64/systemd/filelist https://www.archlinux.org/packages/core/x86_64/systemd/ https://www.archlinux.org/packages/core/x86_64/systemd/
- comex 10y ago
  > No API exists that is standard, async, and does everything that libc getaddrinfo() is capable of doing. So systemd built one. They built an API that was async and does everything that getaddrinfo is capable of doing. They did not build an API that was standard. They did not build an API that even had the potential to become standard, because many systems do not use D-Bus, and they are not going to add it just for a slightly better DNS resolution API than what already exists. What could they have done instead? Either or both of: (1) Implement an extension to the DNS protocol that handles whatever extra bits they need. This is probably the best approach due to the multitude of applications that bypass libc already. Actually, I'm not convinced after reading the manpage that an extension is even necessary... what's the issue with link-local addresses? Can't they just have the DNS server on localhost synthesize records when needed? In fact, based on the rest of the manpage, aren't they already doing that? And what's the issue with Unicode? Can't they translate between DNS punycode and whatever encoding LLMNR uses? But if an extension to DNS really is needed, it has the potential to be proposed as a standard and eventually become ubiquitous, whereas an ad-hoc replacement interface does not. (2) (Worse idea, probably:) Propose a libc API that would be an async version of getaddrinfo with whatever enhancements are desired. Implement a portable polyfill library that either calls getaddrinfo on a thread or (if the API has extended functionality in addition to being async) uses their D-Bus stuff, depending on platform. Admittedly, both options seem more fiddly and more work than 'just' adding some D-Bus calls. But when the existing story for name resolution is largely fully cross-platform, it seems like a bad idea to abandon that just for the sake of small improvements.
  - noselasd 10y ago
    Why is (2) a worse idea ? As a programmer I would vastly prefer it over dealing with DBus myself or dealing with DNS directly
  - daurnimator 10y ago
    (2) already exists in at least glibc: getaddrinfo_a. Using it ~5 years ago though I found plenty of bugs in it (e.g. https://sourceware.org/ml/libc-help/2012-07/msg00024.html https://sourceware.org/ml/libc-help/2012-07/msg00024.html ) However, getaddrinfo is not a great inferface: you still can't use it to e.g. look up an MX record. For that you need res_query(3), which does not have an async interface in libc. Pottering himself wrote a library to use res_query in a separate thread http://0pointer.de/lennart/projects/libasyncns/ http://0pointer.de/lennart/projects/libasyncns/. However, I don't like threads, and will avoid them where possible in libraries (an example reason: I like to be in a defined state after fork()). Which means I need an async dns library that implements a resumable state machine. Lately I've been using http://25thandclement.com/~william/projects/dns.c.html http://25thandclement.com/~william/projects/dns.c.html
- deleted 10y ago
  [deleted]
blakesterz 10y ago
So he closes with > Unfortunately, the lock-in they're creating will deprive people of the ability to vote with their feet and switch to better alternatives. This might sound like a dumb question, and I'm not saying I agree with him, but if I did, I'd vote with my feet by... what? Don't most distros use it now? I'm genuinely curious if this is a practical option for me, though I'm not likely to do it.
- ktRolster 10y ago
  Reports are that Devuan works fine. Gentoo reportedly works well, and then there's my favorite distro (for other reasons besides systemd), Slackware.
  - LoneWolf 10y ago
    As a Gentoo user I can confirm it works well without systemd, using openrc, you can use systemd on Gentoo if you want, I personally avoid it.
    - jpgvm 10y ago
      As an upside OpenRC is simpler and even more configurable than systemd whilst being almost as fast, having had dependency based boot since before systemd was conceived.
      - LoneWolf 10y ago
        Wait does it mean other init systems did not have dependency based boot? I have always been a Gentoo user so always used OpenRC, if that's the case it explains why I never understood why systemd was so "revolutionary". I do find it harder to configure and understand than OpenRC, after I edit some configurations I can just restart the service and it's working, with systemd I have to run more than one command and they are not intuitive at all.
        digi_owl 10y ago
jmclnx 10y ago
Everytime I see someone mention rust or c++ I stop reading. Might as well mention COBOL or FORTRAN in the list of 'good' languages, language used is irrelevant. Anyway it is a local exploit on an old release, not a good thing but containable.
- gshulegaard 10y ago
  Apparently relevant reading from the article: > A language with a better type system, such as Rust or C++ (which has std::optional) can help prevent this kind of error. > That said, this is not about programming languages. > Rewriting systemd in a safer language would not transform it into quality software, ...
grabcocque 10y ago
Oh great another whine-about-systemd rant.
phantom_oracle 10y ago
Sincere question without picking a side: Is Systemd being coded with the same level of 'care' as OpenSSL was before being pwned?
- astrodust 10y ago
  These projects aren't very glamorous to work for, plus when everyone's shitting on your work every day it's hard to stay motivated. Anyone who works on systemd is doing difficult work. We should treat these teams better and give them support rather than just brow-beat them for their mistakes.
- angry_octet 10y ago
  Not at all. OpenSSL was suffering from a lack of effort/funding and a desire not to 'break' anything. Systemd has plenty of effort and breaks everything, but a lack of philosophy, a lack of introspection. It's ignoring plenty of hard lessons about security practice and being very Microsoft-y. Massive technical debt which we will collectively pay for for a decade or more.
  - yuhong 10y ago
    The buggy TLS heartbeat extension was new code I think though.
- viraptor 10y ago
  From my experience - not that much care. I've found a different (remote) DoS issue in systemd-resolved a few months ago. It was a really obvious parsing issue. Also no CVE or announcement.
- anonbanker 10y ago
  If only systemd was written with the level of care OpenSSL had...
deleted 10y ago
[deleted]
loeg 10y ago
> systemd was using a magic value (-1) to represent an invalid mode_t value, and C's type system did not prevent passing it to the mode argument of open The open syscall should reject unrecognized flags in the mode argument (EINVAL), rather than just truncating down to recognized flags. That would also prevent this specific problem with the sentinel value being used on accident.
- JdeBP 10y ago
  In the actual open and openat system calls, the mode parameter is a 16-bit unsigned integer. There are 12 permissions bits and 4 file type bits. There are no "unrecognized flags". All 16 bits have meaning. * http://lxr.free-electrons.com/source/include/uapi/linux/stat.h#L5 http://lxr.free-electrons.com/source/include/uapi/linux/stat... * http://lxr.free-electrons.com/source/include/linux/types.h#L18 http://lxr.free-electrons.com/source/include/linux/types.h#L...
  - loeg 10y ago
    Bits above 1<<11 (the non-permission bits) are not valid arguments to open(2), so I don't see what your point is. In this context, they are invalid. open(2) should reject values with bits outside of 07777 (== 0x0fff) set, including (mode_t)-1 (== 0xffff). Here is the specific place where Linux truncates the bogus mode, instead of rejecting it: http://lxr.free-electrons.com/source/fs/open.c#L906 http://lxr.free-electrons.com/source/fs/open.c#L906 (S_IALLUGO defined here: http://lxr.free-electrons.com/source/include/linux/stat.h#L9 http://lxr.free-electrons.com/source/include/linux/stat.h#L9 ) This change would fix this class of issue: --- a/fs/open.c +++ b/fs/open.c @@ -889,9 +889,11 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o int lookup_flags = 0; int acc_mode = ACC_MODE(flags); - if (flags & (O_CREAT | __O_TMPFILE)) + if (flags & (O_CREAT | __O_TMPFILE)) { + if ((mode & ~S_IALLUGO) != 0) + return -EINVAL; op->mode = (mode & S_IALLUGO) | S_IFREG; - else + } else op->mode = 0; /* Must never be set by userspace */
JdeBP 10y ago
> systemd was using a magic value (-1) to represent an invalid mode_t value This is not a correct description of what happened, note. Kay Sievers originally used 0 to represent an invalid value. Lennart Poettering changed this to -1, because 0 is clearly not an invalid value. The bug resulted because he missed changing one of the comparison-against-zero validity checks. See https://news.ycombinator.com/item?id=13472516 https://news.ycombinator.com/item?id=13472516
- agwa 10y ago
  > The bug resulted because he missed changing one of the comparison-against-zero validity checks. Yes, that's exactly the kind of mistake that happens when you use magic values to represent invalid values instead of distinct types.
  - JdeBP 10y ago
    You still have not got it right. This is an old class of problem, documented since the 1960s. It's nothing to do with types. It's nothing to do with what language one uses (since it has been documented in languages as diverse as RPG and Pascal). It's the use of unexplained constant literals. M. Poettering was in fact doing the right thing and correcting the problem, replacing the unexplained constant literals written by M. Sievers ("mode > 0") with named constants ("mode != MODE_INVALID"). It is an example of the problem, one of whose symptoms is the question "Well which of these is the specific constant and which just happens to also be that number?", that M. Poettering missed a "mode > 0" that also needed replacing. Make no mistake. M. Poettering was actually applying the long-time well-understood fix for this. Here's Ted Holt talking about unexplained constant literals in RPG 4: * https://www.itjungle.com/2004/08/18/fhg081804-story01/ https://www.itjungle.com/2004/08/18/fhg081804-story01/ This same problem, and the approach of turning unexplained literals into named constants to improve maintainability, is explained all over the place, from William Allan Wulf's Fundamental structures of computer science published in 1983, through Niklaus Wirth in the 1975 Proceedings of the IEEE Conference on Reliable Software and Clark and Horning in a SIGPLAN paper in September 1973, to several of Gary Cornell's books on QuickBASIC and Visual BASIC in the 1990s. This is not a new thing, not language-specific, (clearly!) not addressed by changing language, nor addressed by types.
amluto 10y ago
sigh I lost track of the kernel mitigation for the exploit. I'll go take care of it (i.e., I just wrote the patch and I'll get it reviewed).

112 points by uggedal 10y ago

> They have even replaced DNS with a dbus-based protocol, which they "strongly recommend" applications use instead of DNS. This seems inaccurate. The phrase "strongly recommend" appears once in the manpage, where it is strongly recommended that you use either the standard libc resolver API, with libnss_resolve, or the D-Bus API. Applications should be using the libc resolver API instead of implementing DNS themselves. There are some applications like Chrome that implement DNS themselves because they care very much about DNS; those applications presumably know how to do all the things systemd-resolved does. Everyone else should get name resolution functionality from libc. That's what you've been supposed to do for decades, and it's a standard UNIX interface. That standard interface supports things like LLMNR that you don't get if you implement DNS yourself. Unfortunately, the standard UNIX interface is synchronous, which is why libraries like ares or adns exist. If you want to use such a library, you can point it at 127.0.0.53, but you still have the limitations of what can be expressed in DNS. (And you're still using a nonstandard API to speak to libares or libadns.) No API exists that is standard, async, and does everything that libc getaddrinfo() is capable of doing. So systemd built one. That's pretty standard behavior for systemd: implement compatibility interfaces where they exist, recommend them if they're good (systemd explicitly recommends /etc/fstab over writing native mount units, because /etc/fstab is a perfectly good format), implement them anyway if they're not, and write a better API, based on D-Bus, when needed. The latter bit not going through a multi-implementer standards committee isn't great, but it's nowhere near as bad as presented. Anyway, this is completely irrelevant to the rest of the analysis, which seems absolutely correct, and I'm not sure why the author included this parting shot.

j_s 10y ago
I'm not sure why the author included this parting shot Everybody loves kicking systemd as they re-invent various wheels; see... even I can't resist!
viraptor 10y ago
They added the strongly from another paragraph, that's true. But the man page does recommend the dbus API over libc: > The native, fully-featured API systemd-resolved exposes on the bus. See the API Documentation[1] for details. Usage of this API is generally recommended to clients as it is asynchronous and fully featured
agwa 10y ago
Non-DNS resolution protocols like LLMNR are almost entirely irrelevant, particularly on servers. Even if you do need asynchronous LLMNR support, you do not need dbus and a particular process running as PID 1 to get it - applications can make LLMNR queries using an asynchronous library, just like many currently use an asynchronous DNS library. The fact that systemd keeps making decisions like this that are architecturally dubious and lead to lock-in is most certainly grounds for criticism.
- noselasd 10y ago
  When people enter a hostname into my application, I'm not particularly keen on implementing name resolving myself via DNS, LDAP, hosts file and so on in the application - It's absurd to suggest applications should make such a decision. The current NSS system works nicely though, it just needs an async API.
- natermer 10y ago
  Nothing systemd has done is preventing anybody from using any asynchronous library they feel like. It's providing local name resolution services. And for very good reasons. By your logic things like NSS is useless as well because programs themselves can read ldap configuration files and /etc/resolv.conf on their own using libraries or whatever else they feel like using.
zx2c4 10y ago
OpenBSD's libasr has been ported to Linux. http://man.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/asr_run.3 http://man.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/... https://github.com/OpenSMTPD/libasr https://github.com/OpenSMTPD/libasr
bigbugbag 10y ago
Reading the man page it is actually recommending systemd-resolved over other options. It says: - option 1 (recommended): use systemd-resolved API. - option 2: use glibc API with a glibc NSS module to resolve host names via systemd-resolved. - option 3 (not recommended): local DNS stub listener on loopback to connect direct request to systemd-resolved. Author included this part to illustrate how the real issue is that systemd is an unprecedented lock-in. Honestly an init process implementing a DNS resolver? Where is my kitchen sink ?
- tapoxi 10y ago
  To be fair to systemd, systemd-resolved is not an init process. It is its own service that just happens to integrate with systemd and is part of the wider systemd project (with journald, timesyncd, etc.)
  - bigbugbag 10y ago
    to be fair to systemd, it is an init system with a severe case of feature creep to the point that it now includes a DNS resolver that came with vulnerabilities long fixed in the existing ones.
    - tapoxi 10y ago
      systemd-resolved is a separate package. It is not the init system and not a requirement of the init system.
      - bigbugbag 10y ago
        Official systemd homepage[1] begs to differ, it says systemd is an init system including many features among which is name resolution: >systemd (...) provides a system and service manager that runs as PID 1 and starts the rest of the system. (...) Other parts include a logging daemon, (...), log forwarding, and name resolution. [1]: https://freedesktop.org/wiki/Software/systemd/ https://freedesktop.org/wiki/Software/systemd/ -edit- Not sure where it is a separate package, just checked debian and arch, the systemd package contains systemd-resolved. https://packages.debian.org/jessie/amd64/systemd/filelist https://packages.debian.org/jessie/amd64/systemd/filelist https://www.archlinux.org/packages/core/x86_64/systemd/ https://www.archlinux.org/packages/core/x86_64/systemd/
comex 10y ago
> No API exists that is standard, async, and does everything that libc getaddrinfo() is capable of doing. So systemd built one. They built an API that was async and does everything that getaddrinfo is capable of doing. They did not build an API that was standard. They did not build an API that even had the potential to become standard, because many systems do not use D-Bus, and they are not going to add it just for a slightly better DNS resolution API than what already exists. What could they have done instead? Either or both of: (1) Implement an extension to the DNS protocol that handles whatever extra bits they need. This is probably the best approach due to the multitude of applications that bypass libc already. Actually, I'm not convinced after reading the manpage that an extension is even necessary... what's the issue with link-local addresses? Can't they just have the DNS server on localhost synthesize records when needed? In fact, based on the rest of the manpage, aren't they already doing that? And what's the issue with Unicode? Can't they translate between DNS punycode and whatever encoding LLMNR uses? But if an extension to DNS really is needed, it has the potential to be proposed as a standard and eventually become ubiquitous, whereas an ad-hoc replacement interface does not. (2) (Worse idea, probably:) Propose a libc API that would be an async version of getaddrinfo with whatever enhancements are desired. Implement a portable polyfill library that either calls getaddrinfo on a thread or (if the API has extended functionality in addition to being async) uses their D-Bus stuff, depending on platform. Admittedly, both options seem more fiddly and more work than 'just' adding some D-Bus calls. But when the existing story for name resolution is largely fully cross-platform, it seems like a bad idea to abandon that just for the sake of small improvements.
- noselasd 10y ago
  Why is (2) a worse idea ? As a programmer I would vastly prefer it over dealing with DBus myself or dealing with DNS directly
- daurnimator 10y ago
  (2) already exists in at least glibc: getaddrinfo_a. Using it ~5 years ago though I found plenty of bugs in it (e.g. https://sourceware.org/ml/libc-help/2012-07/msg00024.html https://sourceware.org/ml/libc-help/2012-07/msg00024.html ) However, getaddrinfo is not a great inferface: you still can't use it to e.g. look up an MX record. For that you need res_query(3), which does not have an async interface in libc. Pottering himself wrote a library to use res_query in a separate thread http://0pointer.de/lennart/projects/libasyncns/ http://0pointer.de/lennart/projects/libasyncns/. However, I don't like threads, and will avoid them where possible in libraries (an example reason: I like to be in a defined state after fork()). Which means I need an async dns library that implements a resumable state machine. Lately I've been using http://25thandclement.com/~william/projects/dns.c.html http://25thandclement.com/~william/projects/dns.c.html
deleted 10y ago
[deleted]

blakesterz 10y ago

So he closes with > Unfortunately, the lock-in they're creating will deprive people of the ability to vote with their feet and switch to better alternatives. This might sound like a dumb question, and I'm not saying I agree with him, but if I did, I'd vote with my feet by... what? Don't most distros use it now? I'm genuinely curious if this is a practical option for me, though I'm not likely to do it.

ktRolster 10y ago
Reports are that Devuan works fine. Gentoo reportedly works well, and then there's my favorite distro (for other reasons besides systemd), Slackware.
- LoneWolf 10y ago
  As a Gentoo user I can confirm it works well without systemd, using openrc, you can use systemd on Gentoo if you want, I personally avoid it.
  - jpgvm 10y ago
    As an upside OpenRC is simpler and even more configurable than systemd whilst being almost as fast, having had dependency based boot since before systemd was conceived.
    - LoneWolf 10y ago
      Wait does it mean other init systems did not have dependency based boot? I have always been a Gentoo user so always used OpenRC, if that's the case it explains why I never understood why systemd was so "revolutionary". I do find it harder to configure and understand than OpenRC, after I edit some configurations I can just restart the service and it's working, with systemd I have to run more than one command and they are not intuitive at all.
      - digi_owl 10y ago

jmclnx 10y ago

Everytime I see someone mention rust or c++ I stop reading. Might as well mention COBOL or FORTRAN in the list of 'good' languages, language used is irrelevant. Anyway it is a local exploit on an old release, not a good thing but containable.

gshulegaard 10y ago
Apparently relevant reading from the article: > A language with a better type system, such as Rust or C++ (which has std::optional) can help prevent this kind of error. > That said, this is not about programming languages. > Rewriting systemd in a safer language would not transform it into quality software, ...

grabcocque 10y ago

Oh great another whine-about-systemd rant.

phantom_oracle 10y ago

Sincere question without picking a side: Is Systemd being coded with the same level of 'care' as OpenSSL was before being pwned?

astrodust 10y ago
These projects aren't very glamorous to work for, plus when everyone's shitting on your work every day it's hard to stay motivated. Anyone who works on systemd is doing difficult work. We should treat these teams better and give them support rather than just brow-beat them for their mistakes.
angry_octet 10y ago
Not at all. OpenSSL was suffering from a lack of effort/funding and a desire not to 'break' anything. Systemd has plenty of effort and breaks everything, but a lack of philosophy, a lack of introspection. It's ignoring plenty of hard lessons about security practice and being very Microsoft-y. Massive technical debt which we will collectively pay for for a decade or more.
- yuhong 10y ago
  The buggy TLS heartbeat extension was new code I think though.
viraptor 10y ago
From my experience - not that much care. I've found a different (remote) DoS issue in systemd-resolved a few months ago. It was a really obvious parsing issue. Also no CVE or announcement.
anonbanker 10y ago
If only systemd was written with the level of care OpenSSL had...

deleted 10y ago

[deleted]

loeg 10y ago

> systemd was using a magic value (-1) to represent an invalid mode_t value, and C's type system did not prevent passing it to the mode argument of open The open syscall should reject unrecognized flags in the mode argument (EINVAL), rather than just truncating down to recognized flags. That would also prevent this specific problem with the sentinel value being used on accident.

JdeBP 10y ago
In the actual open and openat system calls, the mode parameter is a 16-bit unsigned integer. There are 12 permissions bits and 4 file type bits. There are no "unrecognized flags". All 16 bits have meaning. * http://lxr.free-electrons.com/source/include/uapi/linux/stat.h#L5 http://lxr.free-electrons.com/source/include/uapi/linux/stat... * http://lxr.free-electrons.com/source/include/linux/types.h#L18 http://lxr.free-electrons.com/source/include/linux/types.h#L...
- loeg 10y ago
  Bits above 1<<11 (the non-permission bits) are not valid arguments to open(2), so I don't see what your point is. In this context, they are invalid. open(2) should reject values with bits outside of 07777 (== 0x0fff) set, including (mode_t)-1 (== 0xffff). Here is the specific place where Linux truncates the bogus mode, instead of rejecting it: http://lxr.free-electrons.com/source/fs/open.c#L906 http://lxr.free-electrons.com/source/fs/open.c#L906 (S_IALLUGO defined here: http://lxr.free-electrons.com/source/include/linux/stat.h#L9 http://lxr.free-electrons.com/source/include/linux/stat.h#L9 ) This change would fix this class of issue: --- a/fs/open.c +++ b/fs/open.c @@ -889,9 +889,11 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o int lookup_flags = 0; int acc_mode = ACC_MODE(flags); - if (flags & (O_CREAT | __O_TMPFILE)) + if (flags & (O_CREAT | __O_TMPFILE)) { + if ((mode & ~S_IALLUGO) != 0) + return -EINVAL; op->mode = (mode & S_IALLUGO) | S_IFREG; - else + } else op->mode = 0; /* Must never be set by userspace */

JdeBP 10y ago

> systemd was using a magic value (-1) to represent an invalid mode_t value This is not a correct description of what happened, note. Kay Sievers originally used 0 to represent an invalid value. Lennart Poettering changed this to -1, because 0 is clearly not an invalid value. The bug resulted because he missed changing one of the comparison-against-zero validity checks. See https://news.ycombinator.com/item?id=13472516 https://news.ycombinator.com/item?id=13472516

agwa 10y ago
> The bug resulted because he missed changing one of the comparison-against-zero validity checks. Yes, that's exactly the kind of mistake that happens when you use magic values to represent invalid values instead of distinct types.
- JdeBP 10y ago
  You still have not got it right. This is an old class of problem, documented since the 1960s. It's nothing to do with types. It's nothing to do with what language one uses (since it has been documented in languages as diverse as RPG and Pascal). It's the use of unexplained constant literals. M. Poettering was in fact doing the right thing and correcting the problem, replacing the unexplained constant literals written by M. Sievers ("mode > 0") with named constants ("mode != MODE_INVALID"). It is an example of the problem, one of whose symptoms is the question "Well which of these is the specific constant and which just happens to also be that number?", that M. Poettering missed a "mode > 0" that also needed replacing. Make no mistake. M. Poettering was actually applying the long-time well-understood fix for this. Here's Ted Holt talking about unexplained constant literals in RPG 4: * https://www.itjungle.com/2004/08/18/fhg081804-story01/ https://www.itjungle.com/2004/08/18/fhg081804-story01/ This same problem, and the approach of turning unexplained literals into named constants to improve maintainability, is explained all over the place, from William Allan Wulf's Fundamental structures of computer science published in 1983, through Niklaus Wirth in the 1975 Proceedings of the IEEE Conference on Reliable Software and Clark and Horning in a SIGPLAN paper in September 1973, to several of Gary Cornell's books on QuickBASIC and Visual BASIC in the 1990s. This is not a new thing, not language-specific, (clearly!) not addressed by changing language, nor addressed by types.

amluto 10y ago

sigh I lost track of the kernel mitigation for the exploit. I'll go take care of it (i.e., I just wrote the patch and I'll get it reviewed).