| Summary: | nonce-tcp transport makes unsafe use of shared temporary directory | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Simon McVittie <smcv> |
| Component: | core | Assignee: | Simon McVittie <smcv> |
| Status: | RESOLVED FIXED | QA Contact: | D-Bus Maintainers <dbus> |
| Severity: | normal | ||
| Priority: | medium | CC: | bugzilla |
| Version: | git master | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
Change _dbus_create_directory to fail for existing directories
Change _dbus_create_directory to fail for existing directories activation test: Fix time-of-check/time-of-use bug waiting to happen test: Delete directories like directories, not files shell-test: Don't use _dbus_get_tmpdir() Change _dbus_create_directory to fail for existing directories shell-test: Don't use _dbus_get_tmpdir() |
||
|
Description
Simon McVittie
2017-02-15 16:21:42 UTC
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.