From 3cacdab925c40a5d9b7cf3f67719201bbaa17f67 Mon Sep 17 00:00:00 2001 From: David Rheinsberg Date: Thu, 14 Mar 2019 13:26:50 +0100 Subject: [PATCH 1/3] sd-bus: avoid magic number in SASL length calculation Lets avoid magic numbers and use a constant `strlen()` instead. Signed-off-by: David Rheinsberg --- src/libsystemd/sd-bus/bus-socket.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-socket.c b/src/libsystemd/sd-bus/bus-socket.c index 89ac503ab8..8ee9250142 100644 --- a/src/libsystemd/sd-bus/bus-socket.c +++ b/src/libsystemd/sd-bus/bus-socket.c @@ -386,7 +386,9 @@ static int bus_socket_auth_verify_server(sd_bus *b) { if (line_begins(line, l, "AUTH ANONYMOUS")) { - r = verify_anonymous_token(b, line + 14, l - 14); + r = verify_anonymous_token(b, + line + strlen("AUTH ANONYMOUS"), + l - strlen("AUTH ANONYMOUS")); if (r < 0) return r; if (r == 0) @@ -398,7 +400,9 @@ static int bus_socket_auth_verify_server(sd_bus *b) { } else if (line_begins(line, l, "AUTH EXTERNAL")) { - r = verify_external_token(b, line + 13, l - 13); + r = verify_external_token(b, + line + strlen("AUTH EXTERNAL"), + l - strlen("AUTH EXTERNAL")); if (r < 0) return r; if (r == 0) From 2010873b4b49b223e0cc07d28205b09c693ef005 Mon Sep 17 00:00:00 2001 From: David Rheinsberg Date: Thu, 14 Mar 2019 13:33:28 +0100 Subject: [PATCH 2/3] sd-bus: fix SASL reply to empty AUTH The correct way to reply to "AUTH " 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 " 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 --- src/libsystemd/sd-bus/bus-socket.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-socket.c b/src/libsystemd/sd-bus/bus-socket.c index 8ee9250142..fd85960a41 100644 --- a/src/libsystemd/sd-bus/bus-socket.c +++ b/src/libsystemd/sd-bus/bus-socket.c @@ -395,7 +395,10 @@ static int bus_socket_auth_verify_server(sd_bus *b) { r = bus_socket_auth_write(b, "REJECTED\r\n"); else { b->auth = BUS_AUTH_ANONYMOUS; - r = bus_socket_auth_write_ok(b); + if (l <= strlen("AUTH ANONYMOUS")) + r = bus_socket_auth_write(b, "DATA\r\n"); + else + r = bus_socket_auth_write_ok(b); } } else if (line_begins(line, l, "AUTH EXTERNAL")) { @@ -409,7 +412,10 @@ static int bus_socket_auth_verify_server(sd_bus *b) { r = bus_socket_auth_write(b, "REJECTED\r\n"); else { b->auth = BUS_AUTH_EXTERNAL; - r = bus_socket_auth_write_ok(b); + if (l <= strlen("AUTH EXTERNAL")) + r = bus_socket_auth_write(b, "DATA\r\n"); + else + r = bus_socket_auth_write_ok(b); } } else if (line_begins(line, l, "AUTH")) From 1ed4723d38cd0d1423c8fe650f90fa86007ddf55 Mon Sep 17 00:00:00 2001 From: David Rheinsberg Date: Thu, 14 Mar 2019 13:34:13 +0100 Subject: [PATCH 3/3] 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 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 AGREE_UNIX_FD (optional) to: DATA OK 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 \r\n" replies instead of "DATA\r\n" replies. Signed-off-by: David Rheinsberg --- src/libsystemd/sd-bus/bus-socket.c | 101 ++++++++++++++++++----------- 1 file changed, 63 insertions(+), 38 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-socket.c b/src/libsystemd/sd-bus/bus-socket.c index fd85960a41..790b64010a 100644 --- a/src/libsystemd/sd-bus/bus-socket.c +++ b/src/libsystemd/sd-bus/bus-socket.c @@ -158,17 +158,25 @@ static int bus_socket_write_auth(sd_bus *b) { } static int bus_socket_auth_verify_client(sd_bus *b) { - char *e, *f, *start; + char *d, *e, *f, *start; sd_id128_t peer; unsigned i; int r; assert(b); - /* We expect two response lines: "OK" and possibly - * "AGREE_UNIX_FD" */ + /* + * We expect three response lines: + * "DATA\r\n" + * "OK \r\n" + * "AGREE_UNIX_FD\r\n" (optional) + */ - e = memmem_safe(b->rbuffer, b->rbuffer_size, "\r\n", 2); + d = memmem_safe(b->rbuffer, b->rbuffer_size, "\r\n", 2); + if (!d) + return 0; + + e = memmem(d + 2, b->rbuffer_size - (d - (char*) b->rbuffer) - 2, "\r\n", 2); if (!e) return 0; @@ -183,13 +191,30 @@ static int bus_socket_auth_verify_client(sd_bus *b) { start = e + 2; } - /* Nice! We got all the lines we need. First check the OK - * line */ + /* Nice! We got all the lines we need. First check the DATA line. */ - if (e - (char*) b->rbuffer != 3 + 32) + if (d - (char*) b->rbuffer == 4) { + if (memcmp(b->rbuffer, "DATA", 4)) + return -EPERM; + } else if (d - (char*) b->rbuffer == 3 + 32) { + /* + * Old versions of the server-side implementation of `sd-bus` replied with "OK " to + * "AUTH" requests from a client, even if the "AUTH" line did not contain inlined + * arguments. Therefore, we also accept "OK " here, even though it is technically the + * wrong reply. We ignore the "" parameter, though, since it has no real value. + */ + if (memcmp(b->rbuffer, "OK ", 3)) + return -EPERM; + } else { + return -EPERM; + } + + /* Now check the OK line. */ + + if (e - d != 2 + 3 + 32) return -EPERM; - if (memcmp(b->rbuffer, "OK ", 3)) + if (memcmp(d + 2, "OK ", 3)) return -EPERM; b->auth = b->anonymous_auth ? BUS_AUTH_ANONYMOUS : BUS_AUTH_EXTERNAL; @@ -197,8 +222,8 @@ static int bus_socket_auth_verify_client(sd_bus *b) { for (i = 0; i < 32; i += 2) { int x, y; - x = unhexchar(((char*) b->rbuffer)[3 + i]); - y = unhexchar(((char*) b->rbuffer)[3 + i + 1]); + x = unhexchar(d[2 + 3 + i]); + y = unhexchar(d[2 + 3 + i + 1]); if (x < 0 || y < 0) return -EINVAL; @@ -212,7 +237,7 @@ static int bus_socket_auth_verify_client(sd_bus *b) { b->server_id = peer; - /* And possibly check the second line, too */ + /* And possibly check the third line, too */ if (f) b->can_fds = @@ -611,39 +636,39 @@ static void bus_get_peercred(sd_bus *b) { } static int bus_socket_start_auth_client(sd_bus *b) { - size_t l; - const char *auth_suffix, *auth_prefix; + static const char sasl_auth_anonymous[] = { + /* + * We use an arbitrary trace-string for the ANONYMOUS authentication. It can be used by the + * message broker to aid debugging of clients. We fully anonymize the connection and use a + * static default. + */ + "\0AUTH ANONYMOUS\r\n" + /* HEX a n o n y m o u s */ + "DATA 616e6f6e796d6f7573\r\n" + }; + static const char sasl_auth_external[] = { + "\0AUTH EXTERNAL\r\n" + "DATA\r\n" + }; + static const char sasl_negotiate_unix_fd[] = { + "NEGOTIATE_UNIX_FD\r\n" + }; + static const char sasl_begin[] = { + "BEGIN\r\n" + }; + size_t i = 0; assert(b); - if (b->anonymous_auth) { - auth_prefix = "\0AUTH ANONYMOUS "; - - /* For ANONYMOUS auth we send some arbitrary "trace" string */ - l = 9; - b->auth_buffer = hexmem("anonymous", l); - } else { - char text[DECIMAL_STR_MAX(uid_t) + 1]; - - auth_prefix = "\0AUTH EXTERNAL "; - - xsprintf(text, UID_FMT, geteuid()); - - l = strlen(text); - b->auth_buffer = hexmem(text, l); - } - - if (!b->auth_buffer) - return -ENOMEM; + if (b->anonymous_auth) + b->auth_iovec[i++] = IOVEC_MAKE((char*) sasl_auth_anonymous, sizeof(sasl_auth_anonymous) - 1); + else + b->auth_iovec[i++] = IOVEC_MAKE((char*) sasl_auth_external, sizeof(sasl_auth_external) - 1); if (b->accept_fd) - auth_suffix = "\r\nNEGOTIATE_UNIX_FD\r\nBEGIN\r\n"; - else - auth_suffix = "\r\nBEGIN\r\n"; + b->auth_iovec[i++] = IOVEC_MAKE_STRING(sasl_negotiate_unix_fd); - b->auth_iovec[0] = IOVEC_MAKE((void*) auth_prefix, 1 + strlen(auth_prefix + 1)); - b->auth_iovec[1] = IOVEC_MAKE(b->auth_buffer, l * 2); - b->auth_iovec[2] = IOVEC_MAKE_STRING(auth_suffix); + b->auth_iovec[i++] = IOVEC_MAKE_STRING(sasl_begin); return bus_socket_write_auth(b); }