Bug 99828 - nonce-tcp transport makes unsafe use of shared temporary directory
Summary: nonce-tcp transport makes unsafe use of shared temporary directory
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-15 16:21 UTC by Simon McVittie
Modified: 2017-02-17 19:49 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Change _dbus_create_directory to fail for existing directories (5.47 KB, patch)
2017-02-15 16:34 UTC, Simon McVittie
Details | Splinter Review
Change _dbus_create_directory to fail for existing directories (4.73 KB, patch)
2017-02-15 18:47 UTC, Simon McVittie
Details | Splinter Review
activation test: Fix time-of-check/time-of-use bug waiting to happen (1.53 KB, patch)
2017-02-15 18:48 UTC, Simon McVittie
Details | Splinter Review
test: Delete directories like directories, not files (1023 bytes, patch)
2017-02-15 18:48 UTC, Simon McVittie
Details | Splinter Review
shell-test: Don't use _dbus_get_tmpdir() (1.28 KB, patch)
2017-02-15 18:49 UTC, Simon McVittie
Details | Splinter Review
Change _dbus_create_directory to fail for existing directories (6.22 KB, patch)
2017-02-16 13:02 UTC, Simon McVittie
Details | Splinter Review
shell-test: Don't use _dbus_get_tmpdir() (1.30 KB, patch)
2017-02-16 13:03 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2017-02-15 16:21:42 UTC
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.
Comment 1 Simon McVittie 2017-02-15 16:34:25 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.
Comment 2 Simon McVittie 2017-02-15 16:45:04 UTC
(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.
Comment 3 Simon McVittie 2017-02-15 17:13:09 UTC
(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.
Comment 4 Simon McVittie 2017-02-15 18:47:58 UTC
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.
Comment 5 Simon McVittie 2017-02-15 18:48:14 UTC
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.
Comment 6 Simon McVittie 2017-02-15 18:48:34 UTC
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.
Comment 7 Simon McVittie 2017-02-15 18:49:54 UTC
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.
Comment 8 Philip Withnall 2017-02-16 11:01:30 UTC
(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 9 Philip Withnall 2017-02-16 11:04:34 UTC
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 10 Philip Withnall 2017-02-16 11:07:05 UTC
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 11 Philip Withnall 2017-02-16 11:08:30 UTC
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 12 Philip Withnall 2017-02-16 11:09:01 UTC
Comment on attachment 129655 [details] [review]
shell-test: Don't use _dbus_get_tmpdir()

Review of attachment 129655 [details] [review]:
-----------------------------------------------------------------

++
Comment 13 Simon McVittie 2017-02-16 13:02:13 UTC
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.
Comment 14 Simon McVittie 2017-02-16 13:03:20 UTC
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.
Comment 15 Simon McVittie 2017-02-16 13:10:41 UTC
(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 16 Philip Withnall 2017-02-16 13:14:27 UTC
Comment on attachment 129667 [details] [review]
shell-test: Don't use _dbus_get_tmpdir()

Review of attachment 129667 [details] [review]:
-----------------------------------------------------------------

++
Comment 17 Philip Withnall 2017-02-16 13:16:52 UTC
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. ++
Comment 18 Simon McVittie 2017-02-16 13:26:26 UTC
(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.
Comment 19 Philip Withnall 2017-02-16 13:35:56 UTC
(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.
Comment 20 Simon McVittie 2017-02-16 13:57:30 UTC
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.
Comment 21 Simon McVittie 2017-02-16 15:26:58 UTC
Released 1.10.16. 1.11.10 will follow shortly.
Comment 22 Simon McVittie 2017-02-17 11:19:10 UTC
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.
Comment 23 Simon McVittie 2017-02-17 19:49:21 UTC
(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.