Several LSMs (SELinux, AppArmor, Smack) want D-Bus clients to be able to ask dbus-daemon for the SO_PEERSEC getsockopt value of another client. For SELinux and Smack, it appears this can in fact be done generically, without relying on libselinux or libsmack. For AppArmor, the SO_PEERSEC is somewhat useful in its own right, but Tyler tells me that dbus-daemon should really still use libapparmor to parse the context string into a label and mode. Based on discussion on the linux-security-module mailing list, it appears that the SO_PEERSEC is like a filename: it is always a \0-terminated string with no embedded \0 characters, but is not guaranteed to be ASCII or UTF-8. Here is a straw-man API: org.freedesktop.DBus.GetConnectionCredentials would return a {string: variant} map as it does now, with the following keys all of which are optional: ---- UnixUserID (UINT32) (existing) The numeric Unix user ID, as defined by POSIX ---- ProcessID (UINT32) (existing) The numeric process ID, on platforms that have this concept. On Unix, this is the process ID defined by POSIX. ---- LinuxSecurityLabel (ARRAY of BYTE) (new) On Linux systems, the security label that would result from the SO_PEERSEC getsockopt call. The array contains the non-zero bytes of the string in an unspecified superset of ASCII, followed by a single zero byte. For example, the SELinux context "system_u:system_r:init_t:s0" (a string of length 27) would be encoded as 28 bytes ending with ':', 's', '0', '\x00'. (Note that this is not the same as the older GetConnectionSELinuxContext method, which does not add the zero byte. Adding the \0 allows callers to read the string from the message payload without copying.) On SELinux systems, this is the same string output by "ps -Z" or "ls -Z", such as "system_u:system_r:init_t:s0", "unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023" or "unconfined_u:unconfined_r:chrome_sandbox_t:s0-s0:c0.c1023". On Smack systems, values might include "_", "*", "User", "System" or "System::Shared". On AppArmor systems, values might include "unconfined", "/usr/bin/firefox (enforce)" or "user1 (complain)". Separate AppArmorContext and AppArmorMode keys might also be available. ---- AppArmorLabel (ARRAY of BYTE) (new) On Linux systems with AppArmor, the label part of the AppArmor context. The array contains the non-zero bytes of the string in an unspecified superset of ASCII, followed by a single zero byte. For example, "unconfined" would be encoded as an array of 11 bytes ending with 'e', 'd', '\x00'. Typical values include "unconfined", "/usr/bin/firefox" or "user1". ---- AppArmorMode (ARRAY of BYTE) (new) On Linux systems with AppArmor, the mode part of the AppArmor context. The array contains the non-zero bytes of the string in an unspecified encoding that is a superset of ASCII, followed by a single zero byte. If the AppArmor context does not specify a mode, this key will not be present. Typical values include "enforce" or "complain". ---- Optional C API: /* same API as dbus_connection_get_windows_user() */ dbus_bool_t dbus_connection_get_linux_security_label (DBusConnection *, char **);
(In reply to Simon McVittie from comment #0) > On SELinux systems, this is the same string output by "ps -Z" or "ls -Z", This should probably be more like: On SELinux systems this is the SELinux context, the same string output by... > On Smack systems, values might include "_", "*", "User", "System" or > "System::Shared". On Smack systems this is the Smack label; values might... > On AppArmor systems, values might include "unconfined", "/usr/bin/firefox > (enforce)" or "user1 (complain)". Separate AppArmorContext and AppArmorMode > keys might also be available. On AppArmor systems this is the complete AppArmor context, potentially including both the label (one or more profiles) and the mode. Values might... > AppArmorLabel (ARRAY of BYTE) (new) I have deliberately called this AppArmorLabel, not AppArmorContext, because John Johansen indicated on the linux-security-module list that pedantically, a context is like "/usr/bin/firefox (enforce)" and a label is like "/usr/bin/firefox".
Am I correct to assume that security label returned by GetConnectionCredentials will be the one that is obtained when connection is established? In this comment (https://bugs.freedesktop.org/show_bug.cgi?id=47581#c3) you pointed out that obtaining credentials on demand might pose a security issue because malicious application might exec setuid binary and thus perform actions on root's behalf. I believe equivalent attack might exist on smack because it allows to add SMACK64EXEC xattr to an executable. It contains smack label that will be given to executed program. I think it would be good to make this clear in GetConnectionCredentials specification
(In reply to Jacek Bukarewicz from comment #2) > Am I correct to assume that security label returned by > GetConnectionCredentials will be the one that is obtained when connection is > established? It will be whatever SO_PEERSEC does, retrieved at the same time that we retrieve SO_PEERCRED. Is SO_PEERSEC meant to be secure when treated like SO_PEERCRED?
Ok, thanks. That's what I thought but I wanted to check. IMHO it should be fine for smack.
Created attachment 113355 [details] [review] 1/5] bus_driver_handle_get_connection_credentials: do not assert on OOM dbus_connection_get_windows_user is documented to return TRUE but put NULL in its argument if OOM is reached.
Created attachment 113356 [details] [review] [1/4] New a{sv} helper for using byte arrays as the variant From: Tyler Hicks <tyhicks@canonical.com> Create a new helper for using a byte array as the value in the mapping from string to variant. Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> --- From Bug #75113
Created attachment 113357 [details] [review] 3/5] Add LSM-agnostic support for LinuxSecurityLabel credential
Created attachment 113358 [details] [review] [3/4] Add regression test for LinuxSecurityLabel credential --- Sample output, on a system with AppArmor: /creds: ** Message: ProcessID of this process is 28903 ** Message: UnixUserID of this process is 0 ** Message: LinuxSecurityLabel of this process is unconfined or under aa-exec -p /usr/sbin/bluetoothd (an arbitrarily chosen process with a complain-mode AppArmor profile): /creds: ** Message: ProcessID of this process is 28881 ** Message: UnixUserID of this process is 0 ** Message: LinuxSecurityLabel of this process is /usr/sbin/bluetoothd//null-1//null-7 (complain)
Created attachment 113359 [details] [review] [4/4] Add LinuxSecurityLabel to specification
Comment on attachment 113355 [details] [review] 1/5] bus_driver_handle_get_connection_credentials: do not assert on OOM Review of attachment 113355 [details] [review]: ----------------------------------------------------------------- ++ Technically, the other call to dbus_connection_get_windows_user() in bus/driver.c is broken, since it can result in a NULL windows_sid being passed to _dbus_windows_user_is_process_owner(). However, that function is pure on Windows, so it isn’t a bug in practice.
Comment on attachment 113356 [details] [review] [1/4] New a{sv} helper for using byte arrays as the variant Review of attachment 113356 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-asv-util.c @@ +274,5 @@ > + */ > +dbus_bool_t > +_dbus_asv_add_byte_array (DBusMessageIter *arr_iter, > + const char *key, > + const void *value, Why not const uint8_t *value? Would make it a bit more explicit that this is a byte array.
Comment on attachment 113357 [details] [review] 3/5] Add LSM-agnostic support for LinuxSecurityLabel credential Review of attachment 113357 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-credentials.c @@ +600,5 @@ > join = FALSE; > > + if (credentials->linux_security_label != NULL) > + { > + if (!_dbus_string_append_printf (string, "%slsm='%s'", Should we be escaping quotation marks in credentials->linux_security_label here, or is the outputted string just for debug? ::: dbus/dbus-sysdeps-unix.c @@ +1677,5 @@ > + dbus_bool_t oom = FALSE; > + > +#ifndef SO_PEERSEC > +# define SO_PEERSEC 31 > +#endif Are there any platforms where we expect this to not be defined?
Comment on attachment 113358 [details] [review] [3/4] Add regression test for LinuxSecurityLabel credential Review of attachment 113358 [details] [review]: ----------------------------------------------------------------- ::: test/dbus-daemon.c @@ +307,5 @@ > enum { > SEEN_UNIX_USER = 1, > SEEN_PID = 2, > + SEEN_WINDOWS_SID = 4, > + SEEN_LINUX_SECURITY_LABEL = 8 Nitpick: Could include a trailing comma on the LINUX_SECURITY_LABEL line so it doesn’t need to be modified again if the test is updated in future. @@ +429,5 @@ > + DBUS_TYPE_BYTE); > + dbus_message_iter_get_fixed_array (&ay_iter, &label, &len); > + g_message ("%s of this process is %s", name, label); > + g_assert_cmpuint (strlen (label) + 1, ==, len); > + seen |= SEEN_LINUX_SECURITY_LABEL; SEEN_LINUX_SECURITY_LABEL is never checked. Can we have a LINUX_SECURITY_LABEL_SHOULD_WORK?
Comment on attachment 113359 [details] [review] [4/4] Add LinuxSecurityLabel to specification Review of attachment 113359 [details] [review]: ----------------------------------------------------------------- ::: doc/dbus-specification.xml @@ +6023,5 @@ > + the non-zero bytes of the security label in an unspecified > + ASCII-compatible encoding<footnote> > + <para>It could be ASCII or UTF-8, but could also be > + ISO Latin-1 or any other encoding.</para> > + </footnote>, followed by a single zero byte.</para> Might be worthwhile saying that the dictionary entry is not present if the security label would be empty.
(In reply to Philip Withnall from comment #11) > Why not const uint8_t *value? Would make it a bit more explicit that this is > a byte array. Mainly to avoid signedness headaches, I suspect - in practice, the value is normally going to be a char *. (In reply to Philip Withnall from comment #12) > Should we be escaping quotation marks in credentials->linux_security_label > here, or is the outputted string just for debug? It's just for debug. > > +#ifndef SO_PEERSEC > > +# define SO_PEERSEC 31 > > +#endif > > Are there any platforms where we expect this to not be defined? Linux with outdated libc... although it would have to be really outdated. Perhaps better to use #if defined(__linux__) && defined(SO_PEERSEC). (In reply to Philip Withnall from comment #13) > Nitpick: Could include a trailing comma on the LINUX_SECURITY_LABEL line so > it doesn’t need to be modified again if the test is updated in future. I wish you were right, but ISO C says trailing commas are not valid (or possibly this was the case in C89 or something), and there seems to be a tendency for people to try compiling libdbus on obscure platforms, with compilers that prefer to quote chapter & verse of ISO C and give up (rather than applying the obvious extension and carrying on, as gcc does). > SEEN_LINUX_SECURITY_LABEL is never checked. Can we have a > LINUX_SECURITY_LABEL_SHOULD_WORK? Not without in-depth knowledge of specific LSMs, I don't think. It is checked to the extent that we assert we see it at most once :-) (In reply to Philip Withnall from comment #14) > Might be worthwhile saying that the dictionary entry is not present if the > security label would be empty. If the security label was present but of zero length (and if there's a LSM that lets you do that, which would be silly), we'd throw it in anyway, as a 1-byte array containing only '\0'. On non-LSM or non-Linux systems, we omit it, but that's the general rule for GetConnectionCredentials anyway: "only include what you know".
(In reply to Simon McVittie from comment #0) > AppArmorLabel (ARRAY of BYTE) (new) > AppArmorMode (ARRAY of BYTE) (new) Tyler said on the mailing list that he doesn't think these are necessary or desirable if we have LinuxSecurityLabel. I'm happy to not have them and make the parsing be libapparmor's problem :-) In practice, I suspect that Ubuntu and its derivatives will continue to ship an unmerged patch that adds them as a deprecated compatibility thing, with their Bug #75113 semantics (no trailing \0), in the same way that they currently provide a backwards-compatible GetConnectionAppArmorSecurityContext() method that is not intended to go upstream.
Comment on attachment 113357 [details] [review] 3/5] Add LSM-agnostic support for LinuxSecurityLabel credential Review of attachment 113357 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-unix.c @@ +1731,5 @@ > + _dbus_verbose ("subtracting trailing \\0\n"); > + len--; > + } > + > + _dbus_string_set_length (&buf, len); At this point I should add a check that the strlen() of the data in buf is exactly len. If it is less, then the kernel has given us a security label that we cannot adequately represent (contains embedded \0) and we should ignore it.
(In reply to Simon McVittie from comment #15) > (In reply to Philip Withnall from comment #11) > > Why not const uint8_t *value? Would make it a bit more explicit that this is > > a byte array. > > Mainly to avoid signedness headaches, I suspect - in practice, the value is > normally going to be a char *. *snip other replies* Those explanations all make sense to me. Sounds good.
Comment on attachment 113355 [details] [review] 1/5] bus_driver_handle_get_connection_credentials: do not assert on OOM This patch has now been applied to master. The rest are still in a queue.
Created attachment 113372 [details] [review] [fix for 3/5] do not try to support SO_PEERSEC on ye olde glibc --- to be squashed into 3/5
Created attachment 113373 [details] [review] [fix for 3/5] verify that security label has no embedded \0 --- also to be squashed
Comment on attachment 113372 [details] [review] [fix for 3/5] do not try to support SO_PEERSEC on ye olde glibc Review of attachment 113372 [details] [review]: ----------------------------------------------------------------- Will you want an equivalent change to the #ifndef __linux__ in _dbus_connection_get_linux_security_label() (patch 3/5)? Seems debatable for such a sanity check, but I thought I’d raise it just in case. Probably do want an equivalent change in test/dbus-daemon.c from patch 4/5.
Comment on attachment 113373 [details] [review] [fix for 3/5] verify that security label has no embedded \0 Review of attachment 113373 [details] [review]: ----------------------------------------------------------------- ++
(In reply to Philip Withnall from comment #22) > Will you want an equivalent change to the #ifndef __linux__ in > _dbus_connection_get_linux_security_label() (patch 3/5)? Seems debatable for > such a sanity check, but I thought I’d raise it just in case. No, I don't think that's necessary. > Probably do want an equivalent change in test/dbus-daemon.c from patch 4/5. I don't think that's necessary either. I assume you are referring to this in 3/5 + result = _dbus_transport_get_linux_security_label (connection->transport, + label_p); +#ifndef __linux__ + _dbus_assert (!result); +#endif and this in the test (4/5) + else if (g_strcmp0 (name, "LinuxSecurityLabel") == 0) + { +#ifdef __linux__ ... do a sanity check on its contents ... + seen |= SEEN_LINUX_SECURITY_LABEL; +#else + g_assert_not_reached (); +#endif There are 5 possibilities: - A. non-Linux - Linux with outdated kernel/libc headers (SO_PEERSEC undefined) B. LSM active at runtime anyway C. LSM not active - modern Linux D. LSM active at runtime E. LSM not active libdbus will compile the LinuxSecurityLabel code in cases D and E, and will actually provide the LinuxSecurityLabel in case D only. With the fallback #define for SO_PEERSEC, we would instead have compiled the LSL code in cases B, C, D and E, and actually provided the LinuxSecurityLabel in cases B and D. The two assertions I quoted in this comment just say "if we are in case A, assert that there is no LinuxSecurityLabel, because that would make no sense" (just like we forbid seeing a Unix uid on Windows, or a Windows SID on Unix). In cases B to E, they *allow* a LinuxSecurityLabel, but do not *require* it, because we can't tell whether a suitable LSM is present and active without reimplementing the logic in the test. If the regression test happens to be run on a system in case D, it does a sanity-check on the encoding of the label (which I tried on an AppArmor system, which happens to be the LSM with the most structured/complex labels); but if it is run on a Linux system where the label is not available, either because there is no LSM (case E, my Debian laptop) or because libc is too old for libdbus to know how to get it (cases B and C), it won't complain. This is a limitation in the test - full coverage requires manual interpretation of the results. Given that the test cannot distinguish between D and E to tell whether it should expect to see a label, I don't think it's worthwhile for the test to distinguish between (B,C) and (D,E) either. Do you find this reasoning convincing? (Thank you for your careful review, by the way - please don't interpret my response as criticism. Even if I'm right about this, it's valuable that you have made me justify my reasoning :-)
(In reply to Simon McVittie from comment #24) > - Linux with outdated kernel/libc headers (SO_PEERSEC undefined) > - modern Linux To give some perspective, https://bugzilla.redhat.com/show_bug.cgi?id=193997 says SO_PEERSEC support for datagram Unix sockets was in 2.6.18, which means the concept of SO_PEERSEC is at least that old. 2.6.18 was in Debian 4 and RHEL 5, both released in 2007. Anyone still running OSs older than that is likely to be sufficiently change-averse that they will never consider backporting modern dbus.
(In reply to Simon McVittie from comment #24) > > Probably do want an equivalent change in test/dbus-daemon.c from patch 4/5. > > I don't think that's necessary either. > > I assume you are referring to this in 3/5 *snip* Yes. > and this in the test (4/5) *snip* Yes. Sorry I wasn’t clearer. > Do you find this reasoning convincing? I do! Thanks for documenting it all.
Created attachment 113407 [details] [review] [2/4 v2] Add LSM-agnostic support for LinuxSecurityLabel credential Reviewed-by: Philip Withnall <philip.withnall@collabora.co.uk> Acked-by: Stephen Smalley <sds@tycho.nsa.gov> (for SELinux) Acked-by: John Johansen <john.johansen@canonical.com> (for AppArmor) Acked-by: Casey Schaufler <casey@schaufler-ca.com> (for Smack) --- With review fixes merged. I will apply this, and the rest of the patches, in the near future if no other maintainers veto it.
This is http://cgit.freedesktop.org/~smcv/dbus/log/?h=security-label-89041 for anyone who wants to try it out.
(In reply to Simon McVittie from comment #28) > This is http://cgit.freedesktop.org/~smcv/dbus/log/?h=security-label-89041 > for anyone who wants to try it out. Tested-by: Tyler Hicks <tyhicks@canonical.com> It works as expected. Thanks!
Fixed in git for 1.9.12. Thanks, everyone!
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.