D-Bus over TCP is a trap, but we don't document this well enough.
Created attachment 138787 [details] [review] 1/9] spec: Recommend Unix domain sockets for all non-Windows platforms
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).
Created attachment 138789 [details] [review] 3/9] spec: Expand on how tcp connections are normally authenticated
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.
Created attachment 138792 [details] [review] 5/9] spec, dbus-daemon(1): Recommend against remote TCP for debugging
Created attachment 138793 [details] [review] 6/9] spec: Describe the security properties of nonce-tcp in terms of tcp
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.
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.)
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).
(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 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 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 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 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 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 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 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.
(4/9, 6/9, 7/9 have not had any review yet - further reviews welcomed)
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 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
(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 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 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 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 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 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 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 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 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 on attachment 138796 [details] [review] 9/9] dbus-daemon(1): Recommend requiring EXTERNAL on non-Windows OSs Review of attachment 138796 [details] [review]: ----------------------------------------------------------------- r+
(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.
(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.
(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.
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).
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.
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 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 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 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 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
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.