Bug 106004 - spec and dbus-daemon(1) should strongly discourage non-local TCP
Summary: spec and dbus-daemon(1) should strongly discourage non-local TCP
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: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2018-04-12 13:18 UTC by Simon McVittie
Modified: 2018-04-25 15:59 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
1/9] spec: Recommend Unix domain sockets for all non-Windows platforms (1.41 KB, patch)
2018-04-12 13:18 UTC, Simon McVittie
Details | Splinter Review
[2/9] spec: Don't claim that the nonce-tcp transport is "secured" (2.43 KB, patch)
2018-04-12 13:19 UTC, Simon McVittie
Details | Splinter Review
3/9] spec: Expand on how tcp connections are normally authenticated (1.19 KB, patch)
2018-04-12 13:19 UTC, Simon McVittie
Details | Splinter Review
4/9] spec, dbus-daemon(1): Say that non-local TCP is insecure (3.54 KB, patch)
2018-04-12 13:20 UTC, Simon McVittie
Details | Splinter Review
5/9] spec, dbus-daemon(1): Recommend against remote TCP for debugging (2.15 KB, patch)
2018-04-12 13:20 UTC, Simon McVittie
Details | Splinter Review
6/9] spec: Describe the security properties of nonce-tcp in terms of tcp (2.14 KB, patch)
2018-04-12 13:20 UTC, Simon McVittie
Details | Splinter Review
7/9] spec, dbus-daemon(1): Mention and deprecate shared session buses (3.17 KB, patch)
2018-04-12 13:20 UTC, Simon McVittie
Details | Splinter Review
8/9] dbus-daemon(1): Put some scary warnings on <allow_anonymous/> (1.39 KB, patch)
2018-04-12 13:22 UTC, Simon McVittie
Details | Splinter Review
9/9] dbus-daemon(1): Recommend requiring EXTERNAL on non-Windows OSs (1.20 KB, patch)
2018-04-12 13:23 UTC, Simon McVittie
Details | Splinter Review
[3½/9] spec: Note that EXTERNAL is not *completely* impossible via TCP (1.23 KB, patch)
2018-04-23 17:37 UTC, Simon McVittie
Details | Splinter Review
[7/9] spec, dbus-daemon(1): Mention and deprecate shared session buses (3.28 KB, patch)
2018-04-23 17:38 UTC, Simon McVittie
Details | Splinter Review
[10] spec: Describe nonce-tcp as "nonce-authenticated", not "nonce-secured" (1.12 KB, patch)
2018-04-23 17:39 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 2018-04-12 13:18:23 UTC
D-Bus over TCP is a trap, but we don't document this well enough.
Comment 1 Simon McVittie 2018-04-12 13:18:52 UTC
Created attachment 138787 [details] [review]
1/9] spec: Recommend Unix domain sockets for all non-Windows  platforms
Comment 2 Simon McVittie 2018-04-12 13:19:24 UTC
Created attachment 138788 [details] [review]
[2/9] spec: Don't claim that the nonce-tcp transport is  "secured"

Like the normal TCP transport, it has no confidentiality or integrity
protection. The only difference is that it adds an extra layer of
authentication.

However, this extra authentication is easily defeated if an attacker
could be eavesdropping on the link between client and server (unlike
DBUS_COOKIE_SHA1, which for all its flaws does at least protect the
confidentiality of the magic cookie).
Comment 3 Simon McVittie 2018-04-12 13:19:39 UTC
Created attachment 138789 [details] [review]
3/9] spec: Expand on how tcp connections are normally  authenticated
Comment 4 Simon McVittie 2018-04-12 13:20:01 UTC
Created attachment 138791 [details] [review]
4/9] spec, dbus-daemon(1): Say that non-local TCP is insecure

With some fairly reasonable threat models (active or passive local
attacker able to eavesdrop on the network link, confidential
information being transferred via D-Bus), secure authentication is
insufficient to make this transport secure: it does not protect
confidentiality or integrity either.
Comment 5 Simon McVittie 2018-04-12 13:20:20 UTC
Created attachment 138792 [details] [review]
5/9] spec, dbus-daemon(1): Recommend against remote TCP for  debugging
Comment 6 Simon McVittie 2018-04-12 13:20:32 UTC
Created attachment 138793 [details] [review]
6/9] spec: Describe the security properties of nonce-tcp in  terms of tcp
Comment 7 Simon McVittie 2018-04-12 13:20:59 UTC
Created attachment 138794 [details] [review]
7/9] spec, dbus-daemon(1): Mention and deprecate shared  session buses

This might (?) have made sense behind a firewall in 2003; but now it's
2018, the typical threat model that we are defending against has
changed from "vandals want to feel proud of their l33t skills"
to "organised crime wants your money", and a "trusted" local LAN
probably contains an obsolete phone, tablet, games console or
Internet-of-Things-enabled toaster with remote root exploits.
This make network topologies that used to be acceptable look
increasingly irresponsible.
Comment 8 Simon McVittie 2018-04-12 13:22:49 UTC
Created attachment 138795 [details] [review]
8/9] dbus-daemon(1): Put some scary warnings on  <allow_anonymous/>

I'm far from convinced that this option should even *exist*, but it
should definitely be documented as a very bad thing.

---

Reviewer opinions also solicited on whether we should just delete <allow_anonymous/> from dbus-daemon in the 1.13.x branch. (This would leave DBusServer capable of accepting anonymous connections, which I think is OK - if you're using DBusServer then you are only providing access to your own process, not to the rest of a bus.)
Comment 9 Simon McVittie 2018-04-12 13:23:06 UTC
Created attachment 138796 [details] [review]
9/9] dbus-daemon(1): Recommend requiring EXTERNAL on  non-Windows OSs

This is the default, and blocks TCP-based attacks by making the
attacker fail to authenticate (while also preventing inadvisable
TCP-based configurations from working).
Comment 10 Simon McVittie 2018-04-12 13:27:22 UTC
(In reply to Simon McVittie from comment #8)
> Reviewer opinions also solicited on whether we should just delete
> <allow_anonymous/> from dbus-daemon in the 1.13.x branch.

Please note that this would be a feature regression: it would be impossible to use ANONYMOUS auth on a custom (non-system, non-session) bus, even if that bus has been locked down such that messages from anonymous peers can't escalate privileges or do bad things.

In principle we *could* make <allow_anonymous/> allowed or rejected according to bus_context_get_type(), but that seems maybe undesirable when the bus type is otherwise just a label that has no functional impact?
Comment 11 Ralf Habacker 2018-04-19 09:29:59 UTC
Comment on attachment 138787 [details] [review]
1/9] spec: Recommend Unix domain sockets for all non-Windows  platforms

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

looks good
Comment 12 Ralf Habacker 2018-04-19 09:30:23 UTC
Comment on attachment 138788 [details] [review]
[2/9] spec: Don't claim that the nonce-tcp transport is  "secured"

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

looks good
Comment 13 Ralf Habacker 2018-04-19 09:41:12 UTC
Comment on attachment 138789 [details] [review]
3/9] spec: Expand on how tcp connections are normally  authenticated

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

Dbus server on Windows has support for getting connected user credentials for local tcp connections, which makes it more secure.

Support for secure connections on Windows could be improved with the, waiting to be finished, sspi implementation.
Comment 14 Ralf Habacker 2018-04-19 09:55:42 UTC
Comment on attachment 138792 [details] [review]
5/9] spec, dbus-daemon(1): Recommend against remote TCP for  debugging

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

looks good
Comment 15 Ralf Habacker 2018-04-19 09:59:27 UTC
Comment on attachment 138795 [details] [review]
8/9] dbus-daemon(1): Put some scary warnings on  <allow_anonymous/>

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

from the patch I guess this is an addition to the ANONYMOUS auth - if so it looks good
Comment 16 Ralf Habacker 2018-04-19 09:59:50 UTC
Comment on attachment 138796 [details] [review]
9/9] dbus-daemon(1): Recommend requiring EXTERNAL on  non-Windows OSs

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

looks good
Comment 17 Simon McVittie 2018-04-19 13:16:08 UTC
Comment on attachment 138789 [details] [review]
3/9] spec: Expand on how tcp connections are normally  authenticated

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

> Dbus server on Windows has support for getting connected user credentials for local tcp connections

That's true. I can try to write something about how EXTERNAL actually *does* work over loopback TCP on Windows.

However, I'm not completely sure whether that's actually secure or not (see Bug #83499 starting from https://bugs.freedesktop.org/show_bug.cgi?id=83499#c2). If it has race conditions that could let an attacker trick us into accepting a connection that we should have rejected, then we should remove that support for EXTERNAL and go back to DBUS_COOKIE_SHA1 on Windows.

This isn't a regression in this spec patch: before this patch, the spec already said EXTERNAL isn't supported on TCP (which isn't entirely true). So I think on balance this patch already makes the spec better?

> Support for secure connections on Windows could be improved with the, waiting to be finished, sspi implementation.

When that implementation is ready, we can document it in the spec, but until then I'm just documenting what we have now.
Comment 18 Simon McVittie 2018-04-19 13:19:50 UTC
(4/9, 6/9, 7/9 have not had any review yet - further reviews welcomed)
Comment 19 Ralf Habacker 2018-04-20 06:22:07 UTC
Comment on attachment 138791 [details] [review]
4/9] spec, dbus-daemon(1): Say that non-local TCP is insecure

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

looks good
Comment 20 Ralf Habacker 2018-04-20 06:23:47 UTC
Comment on attachment 138793 [details] [review]
6/9] spec: Describe the security properties of nonce-tcp in  terms of tcp

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

looks good
Comment 21 Ralf Habacker 2018-04-20 06:34:51 UTC
(In reply to Simon McVittie from comment #18)
> (4/9, 6/9, 7/9 have not had any review yet - further reviews welcomed)

I tried with several browsers on different os, but it looks that splinter is broken with some cryptic error messages in the web browser log. Reloading of the page does also not work. 

TypeError: commentEditorSeparator is null[Weitere Informationen]
9b08943e62aaf7d3363730e2d108bcd0.js:1283:2
TypeError: commentEditor is null[Weitere Informationen]
9b08943e62aaf7d3363730e2d108bcd0.js:1269:5
TypeError: Dom.get(...) is null[Weitere Informationen]
9b08943e62aaf7d3363730e2d108bcd0.js:1140:60
TypeError: commentEditor is null[Weitere Informationen]

Today I was able to publish a comment to patch 4, but publishing or adding additional notes to patch 7 is impossible, so reviews are appended here

Patch 7: 

> ... and for the same reasons, and should be considered strongly deprecated.
This looks broken. Do you want to say ?
 
This is insecure against an attacker on the same LAN, in the same ways as unencrypted remote X11 and NFSv2/NFSv and should be considered strongly deprecated for the same reasons.

Patch 8: looks good
Comment 22 Philip Withnall 2018-04-23 13:08:01 UTC
Comment on attachment 138787 [details] [review]
1/9] spec: Recommend Unix domain sockets for all non-Windows  platforms

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

r+
Comment 23 Philip Withnall 2018-04-23 13:08:18 UTC
Comment on attachment 138788 [details] [review]
[2/9] spec: Don't claim that the nonce-tcp transport is  "secured"

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

r+ with nitpicks

::: doc/dbus-specification.xml
@@ +3805,4 @@
>          read bytes do not match the nonce stored in the nonce file, the
>          server MUST immediately drop the connection.
>          If the nonce match the received byte sequence, the client is accepted
> +        and the transport behaves like an ordinary tcp transport.

Nitpick: s/tcp/TCP/ (I realise this is not your bug, but might as well fix it while you’re there).

@@ +3810,5 @@
>        <para>
>          After a successful connect to the server socket, the client MUST read
>          the nonce from the file published by the server via the noncefile=
>          key-value pair and send it over the socket. After that, the
> +        transport behaves like an ordinary tcp transport.

Same here.
Comment 24 Philip Withnall 2018-04-23 13:55:28 UTC
Comment on attachment 138789 [details] [review]
3/9] spec: Expand on how tcp connections are normally  authenticated

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

r+; I don’t think there’s anything in the patch which contradicts what Ralf is saying.
Comment 25 Philip Withnall 2018-04-23 13:56:44 UTC
Comment on attachment 138791 [details] [review]
4/9] spec, dbus-daemon(1): Say that non-local TCP is insecure

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

r+
Comment 26 Philip Withnall 2018-04-23 13:58:41 UTC
Comment on attachment 138792 [details] [review]
5/9] spec, dbus-daemon(1): Recommend against remote TCP for  debugging

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

r+ with a FIXME comment?

::: doc/dbus-daemon.1.xml.in
@@ +419,5 @@
> +  Developers are sometimes tempted to use remote TCP as a debugging
> +  tool. However, if this functionality is left enabled in finished
> +  products, the result will be dangerously insecure. Instead of
> +  using remote TCP, developers should <ulink
> +    url="https://lists.freedesktop.org/archives/dbus/2018-April/017447.html"

Nitpick: It would be good to have this written up somewhere more permanent and official than an archived mailing list post. However, this link is fine for now. Maybe add a FIXME comment?
Comment 27 Philip Withnall 2018-04-23 14:00:18 UTC
Comment on attachment 138793 [details] [review]
6/9] spec: Describe the security properties of nonce-tcp in  terms of tcp

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

r+
Comment 28 Philip Withnall 2018-04-23 14:01:03 UTC
Comment on attachment 138794 [details] [review]
7/9] spec, dbus-daemon(1): Mention and deprecate shared  session buses

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

r+
Comment 29 Philip Withnall 2018-04-23 14:01:35 UTC
Comment on attachment 138795 [details] [review]
8/9] dbus-daemon(1): Put some scary warnings on  <allow_anonymous/>

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

r+
Comment 30 Philip Withnall 2018-04-23 14:01:59 UTC
Comment on attachment 138796 [details] [review]
9/9] dbus-daemon(1): Recommend requiring EXTERNAL on  non-Windows OSs

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

r+
Comment 31 Simon McVittie 2018-04-23 14:42:15 UTC
(In reply to Ralf Habacker from comment #21)
> Patch 7: 
> 
> > ... and for the same reasons, and should be considered strongly deprecated.
> This looks broken. Do you want to say ?
>  
> This is insecure against an attacker on the same LAN, in the same ways as
> unencrypted remote X11 and NFSv2/NFSv and should be considered strongly
> deprecated for the same reasons.

What I said is valid English, but could have been clearer.
Comment 32 Simon McVittie 2018-04-23 14:48:01 UTC
(In reply to Philip Withnall from comment #23)
> Nitpick: s/tcp/TCP/ (I realise this is not your bug, but might as well fix
> it while you’re there).

I deliberately didn't do that here, because I was talking about the tcp transport (an alternative to nonce-tcp and unix in D-Bus), not the TCP protocol (as described in RFC 793 and used by the D-Bus tcp transport).

Arguably the names of transports should be in <literal> or some other formatting, but the paragraph that I'm patching here consistently doesn't use any special formatting around nonce-tcp, and it would be confusing to <literal>ize tcp but not nonce-tcp.
Comment 33 Philip Withnall 2018-04-23 15:41:27 UTC
(In reply to Simon McVittie from comment #32)
> (In reply to Philip Withnall from comment #23)
> > Nitpick: s/tcp/TCP/ (I realise this is not your bug, but might as well fix
> > it while you’re there).
> 
> I deliberately didn't do that here, because I was talking about the tcp
> transport (an alternative to nonce-tcp and unix in D-Bus), not the TCP
> protocol (as described in RFC 793 and used by the D-Bus tcp transport).
> 
> Arguably the names of transports should be in <literal> or some other
> formatting, but the paragraph that I'm patching here consistently doesn't
> use any special formatting around nonce-tcp, and it would be confusing to
> <literal>ize tcp but not nonce-tcp.

That makes sense. I’d be in favour of a follow-up patch to consistently <literal>ise everything, but only if you can be bothered. r+ for this one now.
Comment 34 Simon McVittie 2018-04-23 17:37:25 UTC
Created attachment 139019 [details] [review]
[3½/9] spec: Note that EXTERNAL is not *completely* impossible via TCP

---

I applied everything that seemed uncontroversial for 1.13.4. Thanks for the reviews!

This hopefully addresses Ralf's comments about [3/9], by pointing out that it is possible to do EXTERNAL via loopback TCPv4 on Windows.

I'm not going to document the SSPI authentication mechanism until it lands (and maybe one day I'll have enough free time to review it).
Comment 35 Simon McVittie 2018-04-23 17:38:26 UTC
Created attachment 139020 [details] [review]
[7/9] spec, dbus-daemon(1): Mention and deprecate shared session buses

---

This adjusts the wording. Hopefully it's more clearly correct English now, with fewer subclauses.
Comment 36 Simon McVittie 2018-04-23 17:39:54 UTC
Created attachment 139021 [details] [review]
[10] spec: Describe nonce-tcp as "nonce-authenticated", not "nonce-secured"

nonce-tcp isn't really any more secure than tcp, unless you are
using ANONYMOUS authentication, which should not be considered
secure in any case. Avoid the word "secured" so that people don't
get the wrong idea.

---

Same idea as Attachment #138788 [details], but this time in the title, not the body text.
Comment 37 Philip Withnall 2018-04-23 17:54:13 UTC
Comment on attachment 139019 [details] [review]
[3½/9] spec: Note that EXTERNAL is not *completely* impossible via TCP

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

r+ by me, but Ralf should ack this too
Comment 38 Philip Withnall 2018-04-23 17:55:17 UTC
Comment on attachment 139020 [details] [review]
[7/9] spec, dbus-daemon(1): Mention and deprecate shared session buses

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

r+
Comment 39 Philip Withnall 2018-04-23 17:55:47 UTC
Comment on attachment 139021 [details] [review]
[10] spec: Describe nonce-tcp as "nonce-authenticated", not "nonce-secured"

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

r+, sure
Comment 40 Ralf Habacker 2018-04-24 08:49:21 UTC
Comment on attachment 139019 [details] [review]
[3½/9] spec: Note that EXTERNAL is not *completely* impossible via TCP

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

r+, see also appended patch to avoid pid reusage on Windows
Comment 41 Simon McVittie 2018-04-25 15:59:29 UTC
Thanks, all applied in git for 1.13.4, with the dbus-daemon man page bits (but not the spec) also applied to dbus-1.12 for 1.12.8.


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.