On Windows, the server side of the nonce-tcp transport creates a randomly named file with random contents (the nonce) in the temporary directory returned by GetTempPathA(), which is user-specific in all non-obsolete versions of Windows. This is fine: because GetTempPathA() is user-specific, we only have to care about accidents, not malice. On Unix, it creates a randomly named, 0700-permissions directory in the temporary directory (in practice usually /tmp), then creates a file named "nonce" with random contents in that directory. This would be fine if we used mkdir() and checked for EEXIST: the worst that could happen would be that someone could cause DoS by filling the directory with lots of files or directories of the form we wanted to use (which we could mitigate with a retry loop, if we cared enough). However, we use _dbus_create_directory(), which unexpectedly treats EEXIST as success. This means an attacker could arrange for us to write into a directory they can read, or could arrange for us to write into a directory containing a symlink named "nonce" pointing to a file they want to overwrite. This is technically a security vulnerability, so I'm initially filing this as a closed-off bug. However, there are mitigations: * dbus-daemon on Unix does not allow nonce-tcp by default. * nonce-tcp on network transports other than loopback is trivially insecure anyway, because anyone on the local LAN can read the nonce from TCP traffic. * Prior to dbus 1.4.10 and 1.5.2, nonce-tcp didn't work on Unix due to Bug #34569. * nonce-tcp is a semi-pointless layering violation, because on ordinary TCP connections we use DBUS_COOKIE_SHA1 to do authentication, and nonce-tcp is just an extra layer of security theatre around that. I think the reasoning for it might have been that before Bug #61787 was fixed, D-Bus on Windows used to pretend all connections had successfully credentials-passed "I am the same uid as you", which was clearly wrong, but made EXTERNAL authentication work even though it should have failed. So I'm very tempted to make this a public bug, fix it in master, and move on, perhaps without doing a round of stable releases. Opinions? We should keep nonce-tcp working on Windows because it's what people expect, and we should keep at least basic functionality on Linux so that we can have test coverage, but I think it's OK if it ceases to work on other Unixes. --- Secondary, related issue: the "embedded tests" have similarly insecure behaviour in init_service_reload_test(). In init_service_reload_test() it's worse, because we even try to delete/re-create an existing directory, with some time-of-check/time-of-use fun. But we always tell people that the "embedded tests" are not meant to be compiled into a production dbus build anyway, being only a developer thing; so I think this one is clearly just a bug. I would be tempted to make this test require that a trusted directory we own is passed in via the environment, do its writing there instead of in TMPDIR, and skip that part of the test if the environment variable isn't set. "make check" would make sure to set it to something that's in the ${builddir}. --- Follow-up actions: This makes me want to revisit the idea of hard-coding dbus-daemon 1.11.x so that on Unix it will refuse to listen on non-AF_UNIX transports or carry out non-EXTERNAL authentication. People can still use DBusServer if they want to construct a fast-but-insecure D-Bus proxy. It also makes me tempted to try to delete _dbus_get_tmpdir() - if we ever try to use it without using mkdtemp() or mkstemp(), we are probably wrong. I'd be happy with nonce-tcp ceasing to work on platforms that don't have mkdtemp() in libc - I only want it on Unix at all so I can have some level of confidence that it works.
Created attachment 129643 [details] [review] Change _dbus_create_directory to fail for existing directories If we don't trap EEXIST and its Windows equivalent, we are unable to detect the situation where we create an ostensibly unique subdirectory in a shared /tmp, but an attacker has already created it. This affects dbus-nonce (the nonce-tcp transport) and the activation reload test. Add a new _dbus_ensure_directory() for the one case where we want it to succeed even on EEXIST: the DBUS_COOKIE_SHA1 keyring, which we know we are creating in our own trusted "official" $HOME. In the new transient service support on Bug #99825, ensure_owned_directory() would need the same treatment.
(In reply to Simon McVittie from comment #0) > However, we use _dbus_create_directory(), which unexpectedly treats EEXIST > as success. This means an attacker could arrange for us to write into a > directory they can read Actually, we use _dbus_string_save_to_file() which creates a new file with 0600 permissions, so we probably dodged this bullet. > or could arrange for us to write into a directory > containing a symlink named "nonce" pointing to a file they want to overwrite _dbus_string_save_to_file() opens a tempfile with O_EXCL | O_CREAT and uses the atomic-overwrite trick, so we're probably OK. > Secondary, related issue: the "embedded tests" have similarly insecure > behaviour in init_service_reload_test(). This one is vulnerable to symlink attacks if we lose a time-of-check/time-of-use race.
(In reply to Simon McVittie from comment #1) > Change _dbus_create_directory to fail for existing directories This currently fails tests, but might be a basis for an actually working solution.
Created attachment 129652 [details] [review] Change _dbus_create_directory to fail for existing directories If we don't trap EEXIST and its Windows equivalent, we are unable to detect the situation where we create an ostensibly unique subdirectory in a shared /tmp, but an attacker has already created it. This affects dbus-nonce (the nonce-tcp transport) and the activation reload test. Add a new _dbus_ensure_directory() for the one case where we want it to succeed even on EEXIST: the DBUS_COOKIE_SHA1 keyring, which we know we are creating in our own trusted "official" $HOME. In the new transient service support on Bug #99825, ensure_owned_directory() would need the same treatment.
Created attachment 129653 [details] [review] activation test: Fix time-of-check/time-of-use bug waiting to happen Creating a directory is atomic, stat'ing it to see whether to remove it is very much not.
Created attachment 129654 [details] [review] test: Delete directories like directories, not files Directories can't usefully appear in CLEANFILES, we have to delete them recursively in clean-local.
Created attachment 129655 [details] [review] shell-test: Don't use _dbus_get_tmpdir() There's no particular reason to be using a temporary directory (it's just some arbitrary string), and it's harder to eradicate uses of a temporary directory that is shared between users if we list it here. --- Unfortunately I didn't actually get as far as turning our only two other uses of _dbus_get_tmpdir() on Unix into a wrapper around mkdtemp(). That can happen later.
(In reply to Simon McVittie from comment #0) > So I'm very tempted to make this a public bug, fix it in master, and move > on, perhaps without doing a round of stable releases. Opinions? Given that it’s not a problem on Windows, and that _dbus_string_save_to_file() is used, the worst that can happen is that an attacker could get dbus-daemon to replace a file named ‘nonce’ in a directory of their choosing, right? I don’t think that needs the whole security dance. > I would be tempted to make this test require that a trusted directory we own > is passed in via the environment, do its writing there instead of in TMPDIR, > and skip that part of the test if the environment variable isn't set. "make > check" would make sure to set it to something that's in the ${builddir}. Seems reasonable. This definitely is just a bug rather than a security issue though, as you say. > This makes me want to revisit the idea of hard-coding dbus-daemon 1.11.x so > that on Unix it will refuse to listen on non-AF_UNIX transports or carry out > non-EXTERNAL authentication. People can still use DBusServer if they want to > construct a fast-but-insecure D-Bus proxy. I have no opinions on that. > It also makes me tempted to try to delete _dbus_get_tmpdir() - if we ever > try to use it without using mkdtemp() or mkstemp(), we are probably wrong. > I'd be happy with nonce-tcp ceasing to work on platforms that don't have > mkdtemp() in libc - I only want it on Unix at all so I can have some level > of confidence that it works. Sounds like a good plan.
Comment on attachment 129652 [details] [review] Change _dbus_create_directory to fail for existing directories Review of attachment 129652 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +2235,5 @@ > + * @param error initialized error object > + * @returns #TRUE on success > + */ > +dbus_bool_t > +_dbus_create_directory (const DBusString *filename, The hunk which renames the existing _dbus_create_directory() to _dbus_ensure_directory() in dbus-sysdeps-win.c seems to be missing.
Comment on attachment 129653 [details] [review] activation test: Fix time-of-check/time-of-use bug waiting to happen Review of attachment 129653 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 129654 [details] [review] test: Delete directories like directories, not files Review of attachment 129654 [details] [review]: ----------------------------------------------------------------- I was going to suggest that these also be listed in GITIGNOREFILES since they’re no longer in CLEANFILES, then realised dbus.git doesn’t use git.mk. Ignore me. ++
Comment on attachment 129655 [details] [review] shell-test: Don't use _dbus_get_tmpdir() Review of attachment 129655 [details] [review]: ----------------------------------------------------------------- ++
Created attachment 129666 [details] [review] Change _dbus_create_directory to fail for existing directories If we don't trap EEXIST and its Windows equivalent, we are unable to detect the situation where we create an ostensibly unique subdirectory in a shared /tmp, but an attacker has already created it. This affects dbus-nonce (the nonce-tcp transport) and the activation reload test. Add a new _dbus_ensure_directory() for the one case where we want it to succeed even on EEXIST: the DBUS_COOKIE_SHA1 keyring, which we know we are creating in our own trusted "official" $HOME. In the new transient service support on Bug #99825, ensure_owned_directory() would need the same treatment. We are not treating this as a serious security problem, because the nonce-tcp transport is rarely enabled on Unix and there are multiple mitigations. The nonce-tcp transport creates a new unique file with O_EXCL and 0600 (private to user) permissions, then overwrites the requested filename via atomic-overwrite, so the worst that could happen there is that an attacker could place a symbolic link matching the name of a directory we are going to create, causing a dbus-daemon configured for nonce-tcp to traverse the symlink and atomically overwrite a file named "nonce" in a directory of the attacker's choice, with new random contents that are not known to the attacker. This seems unlikely to be exploitable for anything worse than denial of service in practice. In mainline Linux since 3.6, this attack is also defeated by the fs.protected_symlinks sysctl, which many distributions enable by default. The activation reload test suffers from a classic symlink attack due to time-of-check/time-of-use errors in its implementation, but as part of the developer-only "embedded tests" that are only intended to be run on a trusted machine, it is not treated as security-sensitive. That code path will be fixed in a subsequent commit. --- Fixed Windows code path (compiles successfully, not tested). Longer commit message explaining security implications.
Created attachment 129667 [details] [review] shell-test: Don't use _dbus_get_tmpdir() There's no particular reason to be using a temporary directory (it's just some arbitrary string), and it will be harder for future changes to eradicate uses of a temporary directory that is shared between users if we list it here. --- Slightly better commit message.
(In reply to Simon McVittie from comment #13) > Fixed Windows code path (compiles successfully, not tested) Tests at least partly work in Wine. They don't all pass, but I don't think that's a regression (I'm pleasantly surprised they work at all, actually).
Comment on attachment 129667 [details] [review] shell-test: Don't use _dbus_get_tmpdir() Review of attachment 129667 [details] [review]: ----------------------------------------------------------------- ++
Comment on attachment 129666 [details] [review] Change _dbus_create_directory to fail for existing directories Review of attachment 129666 [details] [review]: ----------------------------------------------------------------- > We are not treating this as a serious security problem, because the > nonce-tcp transport is rarely enabled on Unix and there are multiple > mitigations. Maybe explicitly point people to the comment in this bug which contains the mitigations (https://bugs.freedesktop.org/show_bug.cgi?id=99828#c0). Everything else looks great. ++
(In reply to Philip Withnall from comment #17) > Maybe explicitly point people to the comment in this bug which contains the > mitigations (https://bugs.freedesktop.org/show_bug.cgi?id=99828#c0). I was referring to the mitigations described in the commit message, actually. The ones in #c0 are more like explaining *why* nonce-tcp is not used or useful on Unix.
(In reply to Simon McVittie from comment #18) > (In reply to Philip Withnall from comment #17) > > Maybe explicitly point people to the comment in this bug which contains the > > mitigations (https://bugs.freedesktop.org/show_bug.cgi?id=99828#c0). > > I was referring to the mitigations described in the commit message, actually. > > The ones in #c0 are more like explaining *why* nonce-tcp is not used or > useful on Unix. Ah, OK. I was considering the mitigation in #c0 to be ‘mitigate by not running nonce-tcp over non-loopback network transports’. I’m happy with the commit message as it stands.
Doing a round of releases. I'm not going to do the oss-security + embargo dance, but I think this does justify a 1.10.16, and we're overdue for 1.11.10 anyway. Probably not going to bother with 1.8.24 for this unless some distro asks me to.
Released 1.10.16. 1.11.10 will follow shortly.
Fixed in 1.11.10, 1.10.16. Fixes backported to 1.8.x for 1.8.24, but I am not intending to actually release 1.8.24 unless we find a security vulnerability affecting 1.8.x during Debian 8's support lifetime.
(In reply to Simon McVittie from comment #0) > However, we use _dbus_create_directory(), which unexpectedly treats EEXIST > as success. This means an attacker could arrange for us to write into a > directory they can read, or could arrange for us to write into a directory > containing a symlink named "nonce" pointing to a file they want to overwrite. This was in principle present since the introduction of nonce-tcp before dbus 1.3.1, but could not be accessed on Unix before 1.4.10/1.5.2 because it was also differently broken. > Secondary, related issue: the "embedded tests" have similarly insecure > behaviour in init_service_reload_test(). Present since shortly before dbus 0.21.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.