Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sd-bus: allow cross-uid-namespace connections #11785

Merged
merged 3 commits into from Mar 14, 2019

Conversation

5 participants
@dvdhrm
Copy link
Member

dvdhrm commented Feb 21, 2019

Hey

CC: @filbranden @giuseppe @mrguitar @poettering @teg

This PR allows sd-bus to connect to a message broker even if it runs in a different uid namespace. The problem was that sd-bus includes its own UID in the SASL authentication, even though this is not necessary. Since the uid-mappings might be different in both namespaces, the formatting does not necessarily match the information the kernel returns through auxiliary socket data.

The first patch fixes a bug in the sd-bus server SASL implementation.

The second patch improves sd-bus to no longer send uids verbatim. Instead, we send an empty argument to AUTH EXTERNAL, which is interpreted as the uid given through AF_UNIX auxiliary data.

Note that this PR will very likely break live-updates. The problem is that sd-bus currently is broken since it sends incorrect SASL lines as server. So if you merge this PR and run systemctl or any sd-bus based client that connects to an sd-bus based server (like the management socket of systemd), the SASL connection will fail until you reboot the machine.
Not sure what to do about this, though. Maybe we should make sure to backport patch #1 to distros now, so most machines have it already when patch #2 hits distributions? Dunno, suggestions welcome!

Thanks
David

@dvdhrm dvdhrm requested a review from poettering Feb 21, 2019

Show resolved Hide resolved src/libsystemd/sd-bus/bus-socket.c Outdated
Show resolved Hide resolved src/libsystemd/sd-bus/bus-socket.c
Show resolved Hide resolved src/libsystemd/sd-bus/bus-socket.c
Show resolved Hide resolved src/libsystemd/sd-bus/bus-socket.c Outdated
Show resolved Hide resolved src/libsystemd/sd-bus/bus-socket.c Outdated
@poettering

This comment has been minimized.

Copy link
Member

poettering commented Feb 21, 2019

Note that this PR will very likely break live-updates. The problem is that sd-bus currently is broken since it sends incorrect SASL lines as server. So if you merge this PR and run systemctl or any sd-bus based client that connects to an sd-bus based server (like the management socket of systemd), the SASL connection will fail until you reboot the machine.
Not sure what to do about this, though.

hmm, not sure i follow. i mean, the new behaviour is triggered by the client, no? i.e. old clients send the auth token along with the AUTH iiuc, and the server replies to that with a correct answer. new clients with your patch applied will send no auth token and instead expect a DATA reply, iiuc. hence, why not update the clients to that it can also deal with the missing DATA reply in that case? unless i am missing something this should make all clients work against all servers, no? because for old clients the server response on old and new is not differnt, and for new clients we can just make sure that both types of server responses (the old incorrect + the new correct) is accepted, no?

@dvdhrm

This comment has been minimized.

Copy link
Member Author

dvdhrm commented Feb 21, 2019

and for new clients we can just make sure that both types of server responses (the old incorrect + the new correct) is accepted, no?

Yes, we can make clients accept the "incorrect" response, but that will mean we accept weird server behavior. To be clear, we would then allow a server to respond with one of these:

OK 0123456789abcdef
OK 0123456789abcdef
AGREE_UNIX_FD

or

DATA
OK 0123456789abcdef
AGREE_UNIX_FD

Problem is, a non-pipelining client would actually not respond with "DATA " to a line that says "OK", but the broken server does require the client to respond with "DATA ".

So yeah, we can make the client accept both without causing any bigger issues other than possibly being unable to catch issues in other implementations (which I agree is probably negligible).

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Feb 21, 2019

i think compat trumps here

@martinpitt

This comment has been minimized.

Copy link
Contributor

martinpitt commented Feb 21, 2019

This also seems to break something fundamental, as the failed test log shows:

Setting up systemd (241-230-g096b4c8792-0) ...
Installing new version of config file /etc/systemd/journald.conf ...
Installing new version of config file /etc/systemd/logind.conf ...
Installing new version of config file /etc/systemd/resolved.conf ...
Installing new version of config file /etc/systemd/system.conf ...
Failed to retrieve unit state: Access denied
Failed to retrieve unit state: Access denied
Failed to retrieve unit state: Access denied
adduser: The user `systemd-timesync' already exists, but is not a system user. Exiting.
dpkg: error processing package systemd (--configure):

These three "access denied" are from trying to stop three services (systemctl stop systemd-networkd systemd-timesyncd systemd-resolved). Apparently upgrading from 237 to 241 causes all these to fail.

This reproduces perfectly locally, so this is a real regression indeed.

@giuseppe

This comment has been minimized.

Copy link
Contributor

giuseppe commented Feb 25, 2019

dropping the euid() from the request solves the issue I was seeing. Thanks @dvdhrm !

@poettering poettering added this to the v242 milestone Feb 25, 2019

@keszybz

This comment has been minimized.

Copy link
Member

keszybz commented Mar 5, 2019

What's the status here? It would be nice to include this in the v242-rc1.

@dvdhrm dvdhrm force-pushed the dvdhrm:implicit-sasl branch from b33ae0c to 4af04e6 Mar 14, 2019

sd-bus: avoid magic number in SASL length calculation
Lets avoid magic numbers and use a constant `strlen()` instead.

Signed-off-by: David Rheinsberg <david.rheinsberg@gmail.com>

@dvdhrm dvdhrm force-pushed the dvdhrm:implicit-sasl branch from 4af04e6 to 763971a Mar 14, 2019

dvdhrm added some commits Mar 14, 2019

sd-bus: fix SASL reply to empty AUTH
The correct way to reply to "AUTH <protocol>" without any payload is to
send "DATA" rather than "OK". The "DATA" reply triggers the client to
respond with the requested payload.

In fact, adding the data as hex-encoded argument like
"AUTH <protocol> <hex-data>" is an optimization that skips the "DATA"
roundtrip. The standard way to perform an authentication is to send the
"DATA" line.

This commit fixes sd-bus to properly send the "DATA" line. Surprisingly
no existing implementation depends on this, as they all pass the data
directly as argument to "AUTH". This will not work if we want to pass
an empty argument, though.

Signed-off-by: David Rheinsberg <david.rheinsberg@gmail.com>
sd-bus: skip sending formatted UIDs via SASL
The dbus external authentication takes as optional argument the UID the
sender wants to authenticate as. This uid is purely optional. The
AF_UNIX socket already conveys the same information through the
auxiliary socket data, so we really don't have to provide that
information.

Unfortunately, there is no way to send empty arguments, since they are
interpreted as "missing argument", which has a different meaning. The
SASL negotiation thus changes from:

    AUTH EXTERNAL <uid>
    NEGOTIATE_UNIX_FD                   (optional)
    BEGIN

to:

    AUTH EXTERNAL
    DATA
    NEGOTIATE_UNIX_FD                   (optional)
    BEGIN

And thus the replies we expect as a client change from:

    OK <server-id>
    AGREE_UNIX_FD                       (optional)

to:

    DATA
    OK <server-id>
    AGREE_UNIX_FD                       (optional)

Since the old sd-bus server implementation used the wrong reply for
"AUTH" requests that do not carry the arguments inlined, we decided to
make sd-bus clients accept this as well. Hence, sd-bus now allows
"OK <server-id>\r\n" replies instead of "DATA\r\n" replies.

Signed-off-by: David Rheinsberg <david.rheinsberg@gmail.com>
@dvdhrm

This comment has been minimized.

Copy link
Member Author

dvdhrm commented Mar 14, 2019

Updated:

  • drop magic numbers
  • allow legacy "OK ..." replies
  • minor style fixes
return -EPERM;
} else {
return -EPERM;
}

This comment has been minimized.

@poettering

poettering Mar 14, 2019

Member

nitpick: we generally avoid {} around single line if blocks

/* Nice! We got all the lines we need. First check the DATA line. */

if (d - (char*) b->rbuffer == 4) {
if (memcmp(b->rbuffer, "DATA", 4))

This comment has been minimized.

@poettering

poettering Mar 14, 2019

Member

nitpick: we usually avoid relying on C's downgrade-to-bool feature unless something is actually boolean or boolean-like, or is a pointer. This here isn't either really, so weed to an explicit memcmp() != 0

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Mar 14, 2019

nitpicks are just nitpicks. Let's merge.

@poettering poettering merged commit beb6196 into systemd:master Mar 14, 2019

10 checks passed

CentOS CI Build finished.
Details
CentOS CI (Vagrant) Build finished.
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
bionic-amd64 autopkgtest finished (success)
Details
bionic-i386 autopkgtest finished (success)
Details
bionic-s390x autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.