Bug 34569

Summary:	nonce-tcp transport never works, except on Windows
Product:	dbus	Reporter:	Simon McVittie <smcv>
Component:	core	Assignee:	Simon McVittie <smcv>
Status:	RESOLVED FIXED	QA Contact:	John (J5) Palmieri <johnp>
Severity:	normal
Priority:	medium	CC:	hp, zeuthen
Version:	1.4.x	Keywords:	patch
Hardware:	Other
OS:	All
URL:	http://git.collabora.co.uk/?p=user/smcv/dbus-smcv.git;a=shortlog;h=refs/heads/nonce-tcp-34569
Whiteboard:
i915 platform:		i915 features:
Bug Depends on:	34570
Bug Blocks:	36074
Attachments:	DBusNonceFile: don't always fail when use_subdir=TRUE, i.e. on Unix regression test

Description Simon McVittie 2011-02-22 06:02:10 UTC

Created attachment 43657 [details] [review]
DBusNonceFile: don't always fail when use_subdir=TRUE, i.e. on Unix

While writing a simple black-box test for libdbus, I thought I'd try out all the possible transports. It turns out the nonce-tcp transport has never worked on non-Windows.

The Unix code is meant to create a file _dbus_get_tmpdir() + "/dbus-XXXXX/nonce" (where XXXXX is random), creating the intermediate directory first.

Due to a typo, it tries to mkdir the whole path (including the /nonce); this fails, because the intermediate directory doesn't exist yet.

The Windows code blindly assumes that _dbus_get_tmpdir() is private and owned by the user, and creates a file _dbus_get_tmpdir() + "/dbus-XXXXX". This works, but is pointlessly divergent (if we used the Unix code path in both cases, we'd have found the bug sooner!), and following the Unix code path would protect us against the _dbus_get_tmpdir() returning C:\TEMP or something.

I suspect the idea of making the intermediate directory is to avoid symlink attacks. It may just be cargo-cult programming - we do use O_EXCL on Unix (and CREATE_NEW on Windows) - but if it's meant to be a defence against symlink attacks or whatever, there's no point in using mkdir() unless we treat EEXISTS as an error (which would mean we should introduce a "dbus_bool_t ok_if_exists" parameter to _dbus_create_directory).

Comment 1 Simon McVittie 2011-03-10 09:59:39 UTC

Created attachment 44313 [details] [review]
regression test

This requires the "modular tests" infrastructure from Bug #34570; it's the same as the test previously attached there.

Comment 2 Simon McVittie 2011-04-08 04:24:18 UTC

If people don't want Bug #34570 (integration tests) in 1.4, we could merge this in 1.5, and cherry-pick only the actual fix for 1.4.

Comment 3 David Zeuthen (not reading bugmail) 2011-04-08 07:24:23 UTC

Interestingly, GDBus supports nonce-tcp on Unix too. FWIW, I never tested interop with libdbus on Unix, only on Win32. Let me check your patch with the GDBus implementation...

Comment 4 David Zeuthen (not reading bugmail) 2011-04-08 07:43:29 UTC

(In reply to comment #3)
> Interestingly, GDBus supports nonce-tcp on Unix too. FWIW, I never tested
> interop with libdbus on Unix, only on Win32. Let me check your patch with the
> GDBus implementation...

OK, seems to work fine with both libdbus-1 and GDBus (except for some debug spew but I don't think that's important):

 [davidz@x61 bus]$ ./dbus-daemon --session --address=nonce-tcp: --print-address
nonce-tcp:host=localhost,port=38736,noncefile=/tmp/dbus_nonce-uxq2BqTq/nonce,guid=7de11038e49f80c15d7695e000004802
 ^Z
 [2]+  Stopped                 ./dbus-daemon --session --address=nonce-tcp: --print-address
 [davidz@x61 bus]$ bg
 [2]+ ./dbus-daemon --session --address=nonce-tcp: --print-address &

 [davidz@x61 bus]$ DBUS_SESSION_BUS_ADDRESS="nonce-tcp:host=localhost,port=38736,noncefile=/tmp/dbus_nonce-uxq2BqTq/nonce,guid=7de11038e49f80c15d7695e000004802" ../tools/dbus-send --session --print-reply --dest=org.freedesktop.DBus / org.freedesktop.DBus.GetId
Using your real home directory for testing, set DBUS_TEST_HOMEDIR to avoid
Using your real home directory for testing, set DBUS_TEST_HOMEDIR to avoid
method return sender=org.freedesktop.DBus -> dest=:1.0 reply_serial=2
   string "5dacb1acc49945cb1774243100004802"

 [davidz@x61 bus]$ DBUS_SESSION_BUS_ADDRESS="nonce-tcp:host=localhost,port=38736,noncefile=/tmp/dbus_nonce-uxq2BqTq/nonce,guid=7de11038e49f80c15d7695e000004802" gdbus call --session --dest org.freedesktop.DBus --object-path / --method org.freedesktop.DBus.GetId
('5dacb1acc49945cb1774243100004802',)

Comment 5 David Zeuthen (not reading bugmail) 2011-04-08 07:49:27 UTC

Off-topic, yet maybe still interesting: this also demonstrates that DBUS_COOKIE_SHA1 interop works (normally EXTERNAL with credentials is used):

$ G_DBUS_DEBUG=authentication DBUS_SESSION_BUS_ADDRESS="nonce-tcp:host=localhost,port=38736,noncefile=/tmp/dbus_nonce-uxq2BqTq/nonce,guid=7de11038e49f80c15d7695e000004802" gdbus call --session --dest org.freedesktop.DBus --object-path / --method org.freedesktop.DBus.GetId
GDBus-debug:Auth: CLIENT: initiating
GDBus-debug:Auth: CLIENT: didn't send any credentials
GDBus-debug:Auth: CLIENT: writing `AUTH\r\n'
GDBus-debug:Auth: CLIENT: WaitingForReject
GDBus-debug:Auth: CLIENT: WaitingForReject, read 'REJECTED EXTERNAL DBUS_COOKIE_SHA1 ANONYMOUS'
GDBus-debug:Auth: CLIENT: Trying to choose mechanism
GDBus-debug:Auth: CLIENT: Trying mechanism `EXTERNAL'
GDBus-debug:Auth: CLIENT: Mechanism `EXTERNAL' says it is not supported
GDBus-debug:Auth: CLIENT: Trying to choose mechanism
GDBus-debug:Auth: CLIENT: Trying mechanism `DBUS_COOKIE_SHA1'
GDBus-debug:Auth: CLIENT: writing `AUTH DBUS_COOKIE_SHA1 353030\r\n'
GDBus-debug:Auth: CLIENT: WaitingForData
GDBus-debug:Auth: CLIENT: WaitingForData, read=`DATA 6f72675f667265656465736b746f705f67656e6572616c20353734373432363730203837313933343837613738373062333665613131613032353864303239316638'
GDBus-debug:Auth: CLIENT: writing `DATA 487962796941436b544868646c7266612031666231626339626264643864373861646637626635376633396266653038363431353661666665\r\n'
GDBus-debug:Auth: CLIENT: WaitingForOK
GDBus-debug:Auth: CLIENT: WaitingForOK, read `OK 7de11038e49f80c15d7695e000004802'
GDBus-debug:Auth: CLIENT: writing `BEGIN\r\n'
GDBus-debug:Auth: CLIENT: Done, authenticated=1
('5dacb1acc49945cb1774243100004802',)

Comment 6 David Zeuthen (not reading bugmail) 2011-04-08 07:54:43 UTC

(In reply to comment #0)
> Created an attachment (id=43657) [details]
> DBusNonceFile: don't always fail when use_subdir=TRUE, i.e. on Unix
> 

Feel free to add

 Tested-by: David Zeuthen <davidz@redhat.com>

to the commit message.

Comment 7 Simon McVittie 2011-04-11 03:19:00 UTC

(In reply to comment #4)
> (In reply to comment #3)
> OK, seems to work fine with both libdbus-1 and GDBus (except for some debug
> spew but I don't think that's important):

Did you review the one-line patch? May I merge it? :-)

(Review of the regression test would also be appreciated; it requires Bug #34570.)

Comment 8 David Zeuthen (not reading bugmail) 2011-04-11 04:57:25 UTC

(In reply to comment #7)
> (In reply to comment #4)
> > (In reply to comment #3)
> > OK, seems to work fine with both libdbus-1 and GDBus (except for some debug
> > spew but I don't think that's important):
> 
> Did you review the one-line patch? May I merge it? :-)

Yup, I looked at the patch too. Looks good to merge... Feel free to also add Reviewed-by: davidz tag in addition to the Tested-by: davidz tag :-)

(Is there a flag I should have toggled to indicate this but didn't? I mean, if this was b.g.o I'd set the patch/attachment status to 'reviewed' or, in case I was the maintainer, 'accepted-commit-now'.)

> (Review of the regression test would also be appreciated; it requires Bug
> #34570.)

Sure, let me take a look.

Comment 9 David Zeuthen (not reading bugmail) 2011-04-11 05:06:43 UTC

(In reply to comment #1)
> Created an attachment (id=44313) [details]
> regression test
> 
> This requires the "modular tests" infrastructure from Bug #34570; it's the same
> as the test previously attached there.

This looks good to me.

(Though, this could fail if there's an extremely prohibitive firewall that doesn't allow listening/connecting on the loopback interface (think distro builder where you cannot trust the source / build scripts) but I think the answer here is then "don't run the D-Bus test suite, then".)

Reviewed-by: David Zeuthen <davidz@redhat.com>

Comment 10 Simon McVittie 2011-04-11 05:15:13 UTC

Thanks for reviewing!

(In reply to comment #8)
> (Is there a flag I should have toggled to indicate this but didn't? I mean, if
> this was b.g.o I'd set the patch/attachment status to 'reviewed' or, in case I
> was the maintainer, 'accepted-commit-now'.)

Any comment that indicates that you've reviewed it, really ("ship it" or "review+" or something). In Telepathy we often put review+ in the "Status Whiteboard" so it can show up in bug lists (at least for people like me who have the Status Whiteboard in their configured column-list).

Comment 11 Simon McVittie 2011-04-11 05:17:55 UTC

(In reply to comment #8)
> or, in case I was the maintainer

Oh, you're not in the approved-reviewer list in HACKING, are you? :-( Perhaps you should be...

Comment 12 David Zeuthen (not reading bugmail) 2011-04-11 05:20:14 UTC

(In reply to comment #11)
> (In reply to comment #8)
> > or, in case I was the maintainer
> 
> Oh, you're not in the approved-reviewer list in HACKING, are you? :-( Perhaps
> you should be...

I don't think I am, no.

Comment 13 Simon McVittie 2011-05-25 08:08:45 UTC

Now that davidz is in the reviewer cabal: fixed in git for 1.4.10, will be merged to master shortly (for 1.5.2).

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.