|
Description
Tyler Hicks
2014-02-17 19:15:58 UTC
Created attachment 94222 [details] [review] [PATCH 01/10] Document AppArmor enforcement in the dbus-daemon man page Created attachment 94223 [details] [review] [PATCH 02/10] Add apparmor element and attributes to the bus config dtd Created attachment 94224 [details] [review] [PATCH 03/10] Update autoconf file to build against libapparmor Created attachment 94225 [details] [review] [PATCH 04/10] Add apparmor element support to bus config parsing Created attachment 94226 [details] [review] [PATCH 05/10] Initialize AppArmor mediation Created attachment 94227 [details] [review] [PATCH 06/10] Store AppArmor label of bus during initialization Created attachment 94228 [details] [review] [PATCH 07/10] Store AppArmor label of connecting processes Created attachment 94229 [details] [review] [PATCH 08/10] Mediation of processes that acquire well-known names Created attachment 94231 [details] [review] [PATCH 09/10] Mediation of processes sending and receiving messages Created attachment 94232 [details] [review] [PATCH 10/10] Mediation of processes eavesdropping Comment on attachment 94224 [details] [review] [PATCH 03/10] Update autoconf file to build against libapparmor Review of attachment 94224 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +152,4 @@ > AC_ARG_ENABLE(doxygen-docs, AS_HELP_STRING([--enable-doxygen-docs],[build DOXYGEN documentation (requires Doxygen)]),enable_doxygen_docs=$enableval,enable_doxygen_docs=auto) > AC_ARG_ENABLE(abstract-sockets, AS_HELP_STRING([--enable-abstract-sockets],[use abstract socket namespace (linux only)]),enable_abstract_sockets=$enableval,enable_abstract_sockets=auto) > AC_ARG_ENABLE(selinux, AS_HELP_STRING([--enable-selinux],[build with SELinux support]),enable_selinux=$enableval,enable_selinux=auto) > +AC_ARG_ENABLE(apparmor, AS_HELP_STRING([--enable-apparmor],[build with AppArmor support]),enable_apparmor=$enableval,enable_apparmor=auto) There's lots of "underquoting" here (see the Autoconf manual). I know the existing code is less than ideal, but please quote new code correctly: the rule of thumb is one level of [] per level of (). @@ +1027,5 @@ > SELINUX_LIBS= > fi > > +# AppArmor detection > +if test x$enable_apparmor = xno ; then I'd prefer AS_IF for new code: AS_IF([test x$enable_apparmor = xno], [have_apparmor=no], [ AC_CHECK_LIB([apparmor], ...) ... ]) See the Autoconf manual for why this is better than if/fi. @@ +1032,5 @@ > + have_apparmor=no; > +else > + # See if we have Apparmor library > + AC_CHECK_LIB(apparmor, aa_is_enabled, > + have_apparmor=yes, have_apparmor=no) Does libapparmor not have a pkg-config file? :-( @@ +1038,5 @@ > + # see if we have the Apparmor header with the new D-Bus stuff in it > + if test x$have_apparmor = xyes ; then > + AC_MSG_CHECKING([for DBUS apparmor permissions in sys/apparmor.h]) > + AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <sys/apparmor.h>]], > + [[#ifdef AA_CLASS_DBUS return 0; Um... that's not how cpp syntax works. In practice, the "return 0" is either going to be ignored, or be a false negative. I think this could just be #ifndef AA_CLASS_DBUS # error AA_CLASS_DBUS not defined #endif Comment on attachment 94226 [details] [review] [PATCH 05/10] Initialize AppArmor mediation Review of attachment 94226 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +29,5 @@ > #ifdef HAVE_APPARMOR > > +#ifdef HAVE_ERRNO_H > +#include <errno.h> > +#endif /* HAVE_ERRNO_H */ Will there ever be any platform that has AppArmor but no errno.h? :-) @@ +87,5 @@ > + > +static dbus_bool_t > +aa_supports_dbus (void) > +{ > + char aa_dbus[PATH_MAX + 1]; We have DBusString. Please use it... @@ +94,5 @@ > + > + if (aa_find_mountpoint (&aa_securityfs) != 0) > + return FALSE; > + > + snprintf (aa_dbus, sizeof (aa_dbus), "%s/features/dbus/mask", aa_securityfs); ... because this is theoretically crashable: if strlen(aa_securityfs) == PATH_MAX - strlen("/features/dbus/mask"), then aa_dbus is not zero-terminated. Also, if the string somehow ends up truncated, I very much doubt that's what you want. Use DBusString and you won't have to think about this sort of rubbish. DBusString does require OOM checking, but if malloc() returns NULL while you're sufficiently early in the startup process to be doing this sort of check, then there's nothing you can do about it and you might as well have a fatal error. Comment on attachment 94228 [details] [review] [PATCH 07/10] Store AppArmor label of connecting processes Review of attachment 94228 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +308,5 @@ > + "Failed to get socket file descriptor of connection\n"); > + return NULL; > + } > + > + if (aa_getpeercon (fd, &context, &mode) == -1) Is this guaranteed to happen early enough in socket processing that it avoids the attack described in <https://bugs.freedesktop.org/show_bug.cgi?id=47581#c3>? (If it happens during the initial "handshake", before any D-Bus messages can have been sent by the peer, then the answer is yes.) @@ +315,5 @@ > + BUS_SET_OOM (error); > + else > + dbus_set_error (error, DBUS_ERROR_FAILED, > + "Failed to get AppArmor confinement information of socket peer: %s\n", > + _dbus_strerror (errno)); I think there's a _dbus_set_error_from_errno or something that you can use. The newline is unnecessary: DBusError.message is not expected to end with a newline (so anyone who will print one to stderr, or whatever, is expected to add their own anyway.) ::: bus/apparmor.h @@ +39,5 @@ > > +void bus_apparmor_confinement_unref (BusAppArmorConfinement *confinement); > +BusAppArmorConfinement* > +bus_apparmor_init_connection_confinement (DBusConnection *connection, > + DBusError *error); Coding style nitpicking: declarations look like this: BusAppArmorConfinement *bus_apparmor_init_connection_confinement (...) so that grepping for '^bus_apparmor_init_connection_confinement' only finds the definition, and pointers consistently look like this: foo *bar so that the whitespace suggests the precedence. ::: bus/connection.c @@ +648,5 @@ > + { > + /* This is a bit bogus because we pretend all errors > + * are OOM; this is done because we know that in bus.c > + * an OOM error disconnects the connection, which is > + * the same thing we want on any other error. Ick. Is this logic copied from SELinux? Comment on attachment 94229 [details] [review] [PATCH 08/10] Mediation of processes that acquire well-known names Review of attachment 94229 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +136,5 @@ > return TRUE; > } > + > +static dbus_bool_t > +modestr_to_complain (const char *mode) modestr_is_complain, surely? @@ +159,5 @@ > + { > + capng_get_caps_process (); > + if (capng_have_capability (CAPNG_EFFECTIVE, CAP_AUDIT_WRITE)) > + { > + char buf[PATH_MAX*2]; What does this have to do with paths? (which are not limited to PATH_MAX on Linux anyway - it's a convenient lie for poorly-written software) Why not DBusString? @@ +172,5 @@ > + } > +#endif /* HAVE_LIBAUDIT */ > + > + syslog (LOG_USER | LOG_INFO, "apparmor=\"%s\" operation=\"dbus_%s\" %s\n", > + mstr, op,_dbus_string_get_const_data (data)); syslog.h is only conditionally included, but you're using it unconditionally here. Space after comma (after "op,") please. Is USER|INFO really the right log level for an LSM denial? @@ +185,5 @@ > + if (!_dbus_string_append (auxdata, name)) > + return FALSE; > + if (!_dbus_string_append (auxdata, "=")) > + return FALSE; > + if (!_dbus_string_append_uint (auxdata, value)) We do have an append_printf() function. You'll probably find it useful. @@ +201,5 @@ > + return FALSE; > + if (!_dbus_string_append (auxdata, "=\"")) > + return FALSE; > + if (!_dbus_string_append (auxdata, value)) > + return FALSE; Is it OK that this is ambiguous if @value contains quotes? @@ +216,5 @@ > + return _dbus_append_pair_str (auxdata, "mask", "send"); > + else if (mask & AA_DBUS_RECEIVE) > + return _dbus_append_pair_str (auxdata, "mask", "receive"); > + else if (mask & AA_DBUS_BIND) > + return _dbus_append_pair_str (auxdata, "mask", "bind"); What if more than one bit is set? Do you actually want to include all of them? At the moment you only include one. @@ +251,5 @@ > + size += strlen (s); > + } > + va_end (ap); > + > + buffer = malloc (size + count + 1 + AA_QUERY_CMD_LABEL_SIZE); We have DBusString to avoid doing this sort of thing in a security-sensitive daemon. Please use it. I'm not going to review this function in detail until it's DBusString'd. @@ +277,5 @@ > + return size + count + AA_QUERY_CMD_LABEL_SIZE; > +} > + > +#define build_query(T, C, X...) \ > + (build_query) (T, C, __macroarg_counter (X), X) Is this portable to every major compiler used on platforms that have AppArmor? (I suspect that means "vaguely recent gcc and llvm".) @@ +558,5 @@ > + if (!_dbus_string_init (&auxdata)) > + goto oom; > + string_alloced = TRUE; > + > + if (bustype && !_dbus_append_pair_str (&auxdata, "bus", bustype ? bustype : "unknown")) You seem to be confused about whether bustype can be NULL. ::: bus/connection.c @@ +1129,5 @@ > return d->selinux_id; > } > > +BusAppArmorConfinement* > +bus_connection_get_apparmor_confinement (DBusConnection *connection) If it returns a new ref, I think it should be called _dup_ not _get_. ::: bus/services.c @@ +461,5 @@ > } > + > + if (!bus_apparmor_allows_acquire_service (connection, sid, > + (registry->context ? > + bus_context_get_type(registry->context) : NULL), This would be clearer if it used a temporary variable for this ternary expression, I think. @@ +465,5 @@ > + bus_context_get_type(registry->context) : NULL), > + _dbus_string_get_const_data (service_name), error)) > + { > + > + if (dbus_error_is_set (error) && So does bus_apparmor_allows_acquire_service have these possible results? TRUE with no error FALSE but not an error FALSE with an error That seems weird... @@ +471,5 @@ > + { > + goto out; > + } > + > + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, It is incorrect to "pile up" errors: if dbus_error_is_set(error) then it's invalid to set it to something else. Comment on attachment 94231 [details] [review] [PATCH 09/10] Mediation of processes sending and receiving messages Review of attachment 94231 [details] [review]: ----------------------------------------------------------------- > An example AppArmor rule that would allow a process to call the Hello > method of the session bus itself I think this is a poor example: the Hello method is very, very unusual (I think of it as more part of the bus-connection handshake than anything else). I'd prefer to discuss a rule that controls an ordinary method. SetActivationEnvironment might be a good example of something you might actually want to control. Comment on attachment 94231 [details] [review] [PATCH 09/10] Mediation of processes sending and receiving messages Review of attachment 94231 [details] [review]: ----------------------------------------------------------------- > An example AppArmor rule that would allow a process to call the Hello > method of the session bus itself I think this is a poor example: the Hello method is very, very unusual (I think of it as more part of the bus-connection handshake than anything else). I'd prefer to discuss a rule that controls an ordinary method. SetActivationEnvironment might be a good example of something you might actually want to control. ::: bus/apparmor.c @@ +604,5 @@ > + * addressed recipient, it could be an "eavesdropper" > + * > + * @param sender the sender of the message. > + * @param proposed_recipient the connection the message is to be sent to. > + * @returns whether to allow the send Does it do the libdbus (and GLib) idiom: either return TRUE without error, or FALSE with error? (By inspection, "no". That deserves a comment, at the very least; but I think you'd be better off returning a DBusError.) @@ +620,5 @@ > + const char *source, > + DBusError *error) > +{ > +#ifdef HAVE_APPARMOR > + BusAppArmorConfinement *scon = NULL, *tcon = NULL; What are "s" and "t"? It isn't at all obvious. If they're sender and target, or source and target, or something, then I'd prefer source_con and target_con (or whatever); but even better than that would be source_con and dest_con, which are an obvious abbreviation of the jargon already used for this function's parameters. @@ +623,5 @@ > +#ifdef HAVE_APPARMOR > + BusAppArmorConfinement *scon = NULL, *tcon = NULL; > + DBusString auxdata; > + dbus_bool_t sallow = FALSE, tallow = FALSE, saudit = TRUE, taudit = TRUE; > + dbus_bool_t string_alloced = FALSE; Which string? Do you mean "auxdata_initialized"? (Yes it's an API wart that you can't trivially initialize a DBusString to "empty, but safe to free" without malloc.) @@ +628,5 @@ > + unsigned long pid; > + int len, res, serrno = 0, terrno = 0; > + char *qstr = NULL; > + ssize_t qsize; > + uint32_t sperm = AA_DBUS_SEND, tperm = AA_DBUS_RECEIVE; A rather unfortunate abbreviation there :-P @@ +639,5 @@ > + scon = bus_connection_get_apparmor_confinement (sender); > + > + if (proposed_recipient) > + tcon = bus_connection_get_apparmor_confinement (proposed_recipient); > + else coding style nit: if the "else" has {}, I'd prefer the "if" to have them too, and vice versa @@ +653,5 @@ > + */ > + if (strcmp (msgtype, "error") == 0 || strcmp (msgtype, "method_return") == 0) > + { > + /* ignore reply messages and let dbus reply mapping handle them > + * as the send was already allowed I think you're blindly assuming that unrequested replies have already been dropped on the floor? That's part of the default configuration for the system bus, but not for the session bus, and a sufficiently insane sysadmin could change it. (They get to keep both pieces :-) @@ +661,5 @@ > + goto out; > + } > + > + /* do we want to include the msgtype in the encoding ??? what of tpid > + * so we can do conditional object owner perms I don't understand this comment, but hopefully you do. ::: bus/bus.c @@ +1516,5 @@ > + dest ? dest : DBUS_SERVICE_DBUS, > + src ? src : DBUS_SERVICE_DBUS, > + error)) > + { > + if (error != NULL && !dbus_error_is_set (error)) If the caller of bus_context_check_security_policy() is ignoring errors (which is mad, but theoretically valid) you won't complain about the message here... You'd probably be better off calling complain_about_message() from bus_apparmor_allows_send() if it denies. It's entirely possible that the existing code for SELinux, or for traditional XML policy, gets this wrong too. Comment on attachment 94232 [details] [review] [PATCH 10/10] Mediation of processes eavesdropping Review of attachment 94232 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +843,5 @@ > +#ifdef HAVE_APPARMOR > + BusAppArmorConfinement *con = NULL; > + DBusString auxdata; > + dbus_bool_t allow = FALSE, audit = TRUE; > + dbus_bool_t string_alloced = FALSE; As above @@ +914,5 @@ > + > + oom: > + BUS_SET_OOM (error); > + allow = FALSE; > + goto out; This is mishandled in its only caller, which will crash, because "piling up" errors is not allowed. Please use conventional GLib-style error behaviour (DBusError was based on GLib's GError). Pseudocode: if success don't set error return true or non-NULL or something else set error return false or NULL or something (In reply to comment #0) > I did have a patch that added a new bus method, > org.freedesktop.DBus.GetConnectionAppArmorSecurityContext, to get the > AppArmor > label string of a connection but I've dropped that patch from this set with > the intention of adding AppArmor support to the new GetConnectionCredentials > method (Bug #54445) in the near future. Yes please. Before doing this, you will have to clarify whether the AppArmor label string is guaranteed to be a valid D-Bus string (guaranteed to be UTF-8, with no embedded '\0' and no codepoints outside Unicode); if not, it will have to be transferred as a bytestring (byte array, 'ay'). (In reply to comment #0) > This patch set enables dbus-daemon to use the AppArmor LSM to mediate > activities on the bus. A general question about this stuff: do you intend to mediate the session bus, or just the system bus? The session bus is not typically considered to be a privilege boundary, and will require extra care. Comment on attachment 94224 [details] [review] [PATCH 03/10] Update autoconf file to build against libapparmor Review of attachment 94224 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +1034,5 @@ > + # See if we have Apparmor library > + AC_CHECK_LIB(apparmor, aa_is_enabled, > + have_apparmor=yes, have_apparmor=no) > + > + # see if we have the Apparmor header with the new D-Bus stuff in it This is because this patchset depends on an unreleased version of Apparmor, right? Do you already know which version of AppArmor will have D-Bus support or when it will be released? Does it {build,runtime}-depend on an non-vanilla kernel? What happens when the kernel does not support it? Comment on attachment 94224 [details] [review] [PATCH 03/10] Update autoconf file to build against libapparmor Review of attachment 94224 [details] [review]: ----------------------------------------------------------------- ::: configure.ac @@ +152,4 @@ > AC_ARG_ENABLE(doxygen-docs, AS_HELP_STRING([--enable-doxygen-docs],[build DOXYGEN documentation (requires Doxygen)]),enable_doxygen_docs=$enableval,enable_doxygen_docs=auto) > AC_ARG_ENABLE(abstract-sockets, AS_HELP_STRING([--enable-abstract-sockets],[use abstract socket namespace (linux only)]),enable_abstract_sockets=$enableval,enable_abstract_sockets=auto) > AC_ARG_ENABLE(selinux, AS_HELP_STRING([--enable-selinux],[build with SELinux support]),enable_selinux=$enableval,enable_selinux=auto) > +AC_ARG_ENABLE(apparmor, AS_HELP_STRING([--enable-apparmor],[build with AppArmor support]),enable_apparmor=$enableval,enable_apparmor=auto) The underquoting issues will be fixed in v2 of the series. @@ +1027,5 @@ > SELINUX_LIBS= > fi > > +# AppArmor detection > +if test x$enable_apparmor = xno ; then I've moved everything to AS_IF in v2. @@ +1032,5 @@ > + have_apparmor=no; > +else > + # See if we have Apparmor library > + AC_CHECK_LIB(apparmor, aa_is_enabled, > + have_apparmor=yes, have_apparmor=no) One has been recently created. It'll ship, for the first time, in the same upstream release that dbus will need to depend on. I've moved everything over to use the libapparmor pkg-config file in v2 of the patches. @@ +1034,5 @@ > + # See if we have Apparmor library > + AC_CHECK_LIB(apparmor, aa_is_enabled, > + have_apparmor=yes, have_apparmor=no) > + > + # see if we have the Apparmor header with the new D-Bus stuff in it Upstream AppArmor will be releasing a development snapshot, version 2.8.95, that will contain what dbus needs from libapparmor. There are only a few small kernel patches needed. They've been acked by the maintainer (John Johansen), we have been shipping them in Ubuntu (while we were shaking out the bugs in AppArmor D-Bus mediation), but John has not yet included them in an upstream pull request. If the kernel is old and/or missing the required patches, dbus-daemon detects that the kernel doesn't have support and continues without AppArmor mediation if the mode in the bus config file is "enabled". If the mode is "required" and dbus-daemon does not detect kernel support, the daemon will not start. See patches 4 and 5 for more details. @@ +1038,5 @@ > + # see if we have the Apparmor header with the new D-Bus stuff in it > + if test x$have_apparmor = xyes ; then > + AC_MSG_CHECKING([for DBUS apparmor permissions in sys/apparmor.h]) > + AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <sys/apparmor.h>]], > + [[#ifdef AA_CLASS_DBUS return 0; You're right. This was copied from the have_selinux check. This bit of code goes away with the move to using libapparmor's pkg-config. Comment on attachment 94226 [details] [review] [PATCH 05/10] Initialize AppArmor mediation Review of attachment 94226 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +29,5 @@ > #ifdef HAVE_APPARMOR > > +#ifdef HAVE_ERRNO_H > +#include <errno.h> > +#endif /* HAVE_ERRNO_H */ All platforms that have AppArmor should have errno.h. I've removed the cpp conditional in v2. @@ +94,5 @@ > + > + if (aa_find_mountpoint (&aa_securityfs) != 0) > + return FALSE; > + > + snprintf (aa_dbus, sizeof (aa_dbus), "%s/features/dbus/mask", aa_securityfs); I've moved everything that I could over to DBusString in v2 of the patches. Comment on attachment 94228 [details] [review] [PATCH 07/10] Store AppArmor label of connecting processes Review of attachment 94228 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +308,5 @@ > + "Failed to get socket file descriptor of connection\n"); > + return NULL; > + } > + > + if (aa_getpeercon (fd, &context, &mode) == -1) This happens very early, while setting up new connections. From what I can tell, there is no way for messages to be emitted from a connection prior to when aa_getpeercon() is called. @@ +315,5 @@ > + BUS_SET_OOM (error); > + else > + dbus_set_error (error, DBUS_ERROR_FAILED, > + "Failed to get AppArmor confinement information of socket peer: %s\n", > + _dbus_strerror (errno)); I've started using _dbus_error_from_errno() and removed the newline in v2. ::: bus/apparmor.h @@ +39,5 @@ > > +void bus_apparmor_confinement_unref (BusAppArmorConfinement *confinement); > +BusAppArmorConfinement* > +bus_apparmor_init_connection_confinement (DBusConnection *connection, > + DBusError *error); I've corrected this coding style mistake across all of the patches in v2. ::: bus/connection.c @@ +648,5 @@ > + { > + /* This is a bit bogus because we pretend all errors > + * are OOM; this is done because we know that in bus.c > + * an OOM error disconnects the connection, which is > + * the same thing we want on any other error. It is copied from SELinux. I have *not* changed this in v2. Comment on attachment 94229 [details] [review] [PATCH 08/10] Mediation of processes that acquire well-known names Review of attachment 94229 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +136,5 @@ > return TRUE; > } > + > +static dbus_bool_t > +modestr_to_complain (const char *mode) Renamed to modestr_is_complain() in v2. @@ +159,5 @@ > + { > + capng_get_caps_process (); > + if (capng_have_capability (CAPNG_EFFECTIVE, CAP_AUDIT_WRITE)) > + { > + char buf[PATH_MAX*2]; It has nothing to do with paths. It was mindlessly copied from SELinux. It goes away in v2, after switching to DBusString. @@ +172,5 @@ > + } > +#endif /* HAVE_LIBAUDIT */ > + > + syslog (LOG_USER | LOG_INFO, "apparmor=\"%s\" operation=\"dbus_%s\" %s\n", > + mstr, op,_dbus_string_get_const_data (data)); I've made syslog.h be unconditionally included in v2. I've added the space after "op," in v2. I've changed to USER|NOTICE in v2. NOTICE is what the Linux kernel's audit subsystem uses when sending messages to the syslog. @@ +185,5 @@ > + if (!_dbus_string_append (auxdata, name)) > + return FALSE; > + if (!_dbus_string_append (auxdata, "=")) > + return FALSE; > + if (!_dbus_string_append_uint (auxdata, value)) I felt like append_printf() was overkill for this situation. I cleaned up these helper functions a bit in v2 of the patches, but didn't use append_printf(). @@ +201,5 @@ > + return FALSE; > + if (!_dbus_string_append (auxdata, "=\"")) > + return FALSE; > + if (!_dbus_string_append (auxdata, value)) > + return FALSE; It is ok in the sense that this is already a mostly untrusted audit message generated from userspace. @@ +216,5 @@ > + return _dbus_append_pair_str (auxdata, "mask", "send"); > + else if (mask & AA_DBUS_RECEIVE) > + return _dbus_append_pair_str (auxdata, "mask", "receive"); > + else if (mask & AA_DBUS_BIND) > + return _dbus_append_pair_str (auxdata, "mask", "bind"); Only one mask bit is ever set in the apparmor.c code. However, to make it obvious that _dbus_append_mask() only expects one bit to be set, I've changed these tests from (mask & AA_DBUS_*) to (mask == AA_DBUS_*). @@ +251,5 @@ > + size += strlen (s); > + } > + va_end (ap); > + > + buffer = malloc (size + count + 1 + AA_QUERY_CMD_LABEL_SIZE); This functionality has been DBusString'd in v2. @@ +277,5 @@ > + return size + count + AA_QUERY_CMD_LABEL_SIZE; > +} > + > +#define build_query(T, C, X...) \ > + (build_query) (T, C, __macroarg_counter (X), X) I'm not sure and I also don't like the complexity of this function. I've greatly simplified it in v2. We only have three types of queries (send/receive, bind to service name, and eavesdropping), so doing variable length input parameters is overkill and isn't something that we need. @@ +558,5 @@ > + if (!_dbus_string_init (&auxdata)) > + goto oom; > + string_alloced = TRUE; > + > + if (bustype && !_dbus_append_pair_str (&auxdata, "bus", bustype ? bustype : "unknown")) Heh, fixed in v2. :) ::: bus/connection.c @@ +1129,5 @@ > return d->selinux_id; > } > > +BusAppArmorConfinement* > +bus_connection_get_apparmor_confinement (DBusConnection *connection) Renamed to bus_connection_dup_apparmor_confinement() in v2. ::: bus/services.c @@ +461,5 @@ > } > + > + if (!bus_apparmor_allows_acquire_service (connection, sid, > + (registry->context ? > + bus_context_get_type(registry->context) : NULL), Temp variable added in v2. @@ +465,5 @@ > + bus_context_get_type(registry->context) : NULL), > + _dbus_string_get_const_data (service_name), error)) > + { > + > + if (dbus_error_is_set (error) && It does. This was copied from bus_selinux_allows_acquire_service(). I've changed bus_apparmor_allows_acquire_service() to internally set the DBusError when returning FALSE. @@ +471,5 @@ > + { > + goto out; > + } > + > + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, Fixed in v2. Comment on attachment 94231 [details] [review] [PATCH 09/10] Mediation of processes sending and receiving messages Review of attachment 94231 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +604,5 @@ > + * addressed recipient, it could be an "eavesdropper" > + * > + * @param sender the sender of the message. > + * @param proposed_recipient the connection the message is to be sent to. > + * @returns whether to allow the send It didn't do the libdbus idiom, but it does in v2. All of the bus_apparmor_allows_*() functions have been updated to set the DBusError internally when returning FALSE in v2. @@ +620,5 @@ > + const char *source, > + DBusError *error) > +{ > +#ifdef HAVE_APPARMOR > + BusAppArmorConfinement *scon = NULL, *tcon = NULL; I'm not sure what John intended the "s" and "t" prefixes to mean. I had assumed source and target. I've renamed all "s" prefixes with "src_" and all "t" prefixes with "dst_". I used "dst_" instead of your suggested "dest_" because I like the variable names to be the same length. @@ +623,5 @@ > +#ifdef HAVE_APPARMOR > + BusAppArmorConfinement *scon = NULL, *tcon = NULL; > + DBusString auxdata; > + dbus_bool_t sallow = FALSE, tallow = FALSE, saudit = TRUE, taudit = TRUE; > + dbus_bool_t string_alloced = FALSE; Yes. I've renamed this bool to free_auxdata. When auxdata is allocated, free_auxdata gets set to TRUE. @@ +628,5 @@ > + unsigned long pid; > + int len, res, serrno = 0, terrno = 0; > + char *qstr = NULL; > + ssize_t qsize; > + uint32_t sperm = AA_DBUS_SEND, tperm = AA_DBUS_RECEIVE; Yes. :) That got "fixed" with the rename to src_perm. @@ +639,5 @@ > + scon = bus_connection_get_apparmor_confinement (sender); > + > + if (proposed_recipient) > + tcon = bus_connection_get_apparmor_confinement (proposed_recipient); > + else Fixed across all patches. @@ +653,5 @@ > + */ > + if (strcmp (msgtype, "error") == 0 || strcmp (msgtype, "method_return") == 0) > + { > + /* ignore reply messages and let dbus reply mapping handle them > + * as the send was already allowed Thank you for catching this! In v2 of the patches, I've moved around the call sites of bus_*_allows_send() so that they come after the determination of whether the message is a requested reply or not. This condtional is updated to only be true if requested_reply is also true. @@ +661,5 @@ > + goto out; > + } > + > + /* do we want to include the msgtype in the encoding ??? what of tpid > + * so we can do conditional object owner perms That was leftover from John's prototype. I've removed it in v2. ::: bus/bus.c @@ +1516,5 @@ > + dest ? dest : DBUS_SERVICE_DBUS, > + src ? src : DBUS_SERVICE_DBUS, > + error)) > + { > + if (error != NULL && !dbus_error_is_set (error)) SELinux gets this wrong, but I'm not sure about the XML policy. I've moved the complaining into bus_apparmor_allows_send() for v2. I didn't export and use complain_about_message() since we don't have access to the context variable and we really only needed a small bit of complain_about_message(). Comment on attachment 94232 [details] [review] [PATCH 10/10] Mediation of processes eavesdropping Review of attachment 94232 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +843,5 @@ > +#ifdef HAVE_APPARMOR > + BusAppArmorConfinement *con = NULL; > + DBusString auxdata; > + dbus_bool_t allow = FALSE, audit = TRUE; > + dbus_bool_t string_alloced = FALSE; Fixed in v2. @@ +914,5 @@ > + > + oom: > + BUS_SET_OOM (error); > + allow = FALSE; > + goto out; Fixed in v2. It now uses conventional GLib-style error behavior. (In reply to comment #19) > (In reply to comment #0) > > This patch set enables dbus-daemon to use the AppArmor LSM to mediate > > activities on the bus. > > A general question about this stuff: do you intend to mediate the session > bus, or just the system bus? The session bus is not typically considered to > be a privilege boundary, and will require extra care. We are mediating the session bus. The privilege boundary is provided by AppArmor. It will prevent confined applications from attacking the dbus-daemon process through IPC mediation, despite the applications and dbus-daemon running as the same user. Are there any other things to watch out for? Created attachment 95823 [details] [review] [PATCH 01/13 v2] Document AppArmor enforcement in the dbus-daemon man page Created attachment 95824 [details] [review] [PATCH 02/13 v2] Add apparmor element and attributes to the bus config dtd Created attachment 95825 [details] [review] [PATCH 03/13 v2] Update autoconf file to build against libapparmor Created attachment 95826 [details] [review] [PATCH 04/13 v2] Add apparmor element support to bus config parsing Created attachment 95827 [details] [review] [PATCH 05/13 v2] Initialize AppArmor mediation Created attachment 95828 [details] [review] [PATCH 06/13 v2] Store AppArmor label of bus during initialization Created attachment 95829 [details] [review] [PATCH 07/13 v2] Store AppArmor label of connecting processes Created attachment 95830 [details] [review] [PATCH 08/13 v2] Mediation of processes that acquire well-known names Created attachment 95831 [details] [review] [PATCH 09/13 v2] Do LSM checks after determining if the message is a requested reply Created attachment 95832 [details] [review] [PATCH 10/13 v2] Mediation of processes sending and receiving messages Created attachment 95833 [details] [review] [PATCH 11/13 v2] Mediation of processes eavesdropping Created attachment 95834 [details] [review] [PATCH 12/13] New a{sv} helper for using byte arrays as the variant Created attachment 95835 [details] [review] [PATCH 13/13] Add AppArmor support to GetConnectionCredentials The v2 patches work fine for me when applied on git master. I'm not a D-Bus maintainer, but here are my comments on the v2 patches: In commit logs, you can add a reference to the bug: Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 The default config files: bus/system.conf.in bus/session.conf.in should include the <apparmor> line, so it is easier for an admin to change it. [PATCH 4] > bus_apparmor_set_mode_from_config When dbus-daemon is compiled without AppArmor, the parser will accept invalid modes in the configuration such as: <apparmor mode="boo"/> I would prefer if it only accepts "disabled", "enabled" and NULL for future compatibility. If a future version of D-Bus implement a new mode "required_with_extra_checks" or something, dbus compiled without AppArmor support must reject that. Maybe something like the following? (untested) if (mode == NULL || strcmp (mode, "disabled") == 0 || strcmp (mode, "enabled") == 0) return TRUE; dbus_set_error (error, DBUS_ERROR_FAILED, "Mode attribute on <apparmor> must be \"disabled\" or " "\"enabled\" but cannot be \"%s\" when D-Bus is built " "without AppArmor support", mode); return FALSE; [PATCH 5] > /* Store the value of the AppArmor mediation mode in the bus configuration */ > static AppArmorConfigMode apparmor_config_mode = APPARMOR_ENABLED; Does config reload (SIGHUP) work? dbus-daemon(1) manpage says "Policy changes should take effect with SIGHUP." It's probably best to specify in the documentation whether changing the apparmor mode is supposed to work. > aa_supports_dbus - close(-1) might be harmless but since you test whether it is -1 just before, you can avoid it. > * Do early initialization; determine whether AppArmor is enabled. AFAIU, "early" means before reading the configuration. Is it what you mean? > apparmor_enabled = (aa_is_enabled () && aa_supports_dbus ()) ? TRUE : FALSE; aa_ prefix is for functions implemented in libapparmor such as aa_is_enabled. Since aa_supports_dbus() is not part of libapparmor, better rename it. Something like _bus_apparmor_aa_supports_dbus? > * Initialize user space > bus_apparmor_full_init I don't understand the comment about user space. > In function bus_apparmor_full_init: > if (bus_con == NULL) The test suggests bus_apparmor_full_init() could be called several times. Can it? Otherwise, it seems unnecessary. [PATCH 8] +dbus_bool_t bus_apparmor_allows_acquire_service (DBusConnection *connection, + BusSELinuxID *service_sid, the BusSELinuxID parameter seems to be a bad copy/paste and it's not used. [PATCH 10] +bus_apparmor_allows_send Documentation on @param missing. I think requested_reply is worth documenting. Other functions in apparmor.c have missing @param documentation. (In reply to comment #41) > The v2 patches work fine for me when applied on git master. > > I'm not a D-Bus maintainer, but here are my comments on the v2 patches: Thanks for the testing and review! I'll get a set of v3 patches tested and attached very soon. > > In commit logs, you can add a reference to the bug: > Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 Will do > > The default config files: > bus/system.conf.in > bus/session.conf.in > should include the <apparmor> line, so it is easier for an admin to change > it. Good idea > > [PATCH 4] > > > bus_apparmor_set_mode_from_config > > When dbus-daemon is compiled without AppArmor, the parser will accept > invalid modes in the configuration such as: > <apparmor mode="boo"/> > I would prefer if it only accepts "disabled", "enabled" and NULL for future > compatibility. If a future version of D-Bus implement a new mode > "required_with_extra_checks" or something, dbus compiled without AppArmor > support must reject that. > > Maybe something like the following? (untested) > > if (mode == NULL || strcmp (mode, "disabled") == 0 > || strcmp (mode, "enabled") == 0) > return TRUE; > > dbus_set_error (error, DBUS_ERROR_FAILED, > "Mode attribute on <apparmor> must be \"disabled\" or " > "\"enabled\" but cannot be \"%s\" when D-Bus is built " > "without AppArmor support", mode); > return FALSE; That looks good to me > > [PATCH 5] > > > /* Store the value of the AppArmor mediation mode in the bus configuration */ > > static AppArmorConfigMode apparmor_config_mode = APPARMOR_ENABLED; > > Does config reload (SIGHUP) work? dbus-daemon(1) manpage says "Policy > changes should take effect with SIGHUP." It's probably best to specify in > the documentation whether changing the apparmor mode is supposed to work. I'll have to test this and get back to you > > > aa_supports_dbus > > - close(-1) might be harmless but since you test whether it is -1 just > before, you can avoid it. Agreed > > > * Do early initialization; determine whether AppArmor is enabled. > > AFAIU, "early" means before reading the configuration. Is it what you mean? Yes > > > apparmor_enabled = (aa_is_enabled () && aa_supports_dbus ()) ? TRUE : FALSE; > > aa_ prefix is for functions implemented in libapparmor such as > aa_is_enabled. Since aa_supports_dbus() is not part of libapparmor, better > rename it. Something like _bus_apparmor_aa_supports_dbus? Sure > > > * Initialize user space > > bus_apparmor_full_init > > I don't understand the comment about user space. Me neither. I'll add something more descriptive. > > > In function bus_apparmor_full_init: > > if (bus_con == NULL) > > The test suggests bus_apparmor_full_init() could be called several times. > Can it? Otherwise, it seems unnecessary. It can be. In fact, it does get called several times by the tests in bus/dispatch.c. bus_context_new_test() -> bus_context_new() -> bus_apparmor_full_init() > > [PATCH 8] > > +dbus_bool_t bus_apparmor_allows_acquire_service (DBusConnection *connection, > + BusSELinuxID > *service_sid, > > the BusSELinuxID parameter seems to be a bad copy/paste and it's not used. It was intentional. When John initially wrote the prototype, his goal was to keep the same function prototypes among the SELinux and AppArmor functions. However, at this point, we've not kept the same prototypes so I'll remove the BusSELinuxID parameter. > > [PATCH 10] > > +bus_apparmor_allows_send > > Documentation on @param missing. I think requested_reply is worth > documenting. > Other functions in apparmor.c have missing @param documentation. I'll fix these up (In reply to comment #42) > (In reply to comment #41) > > [PATCH 5] > > > > > /* Store the value of the AppArmor mediation mode in the bus configuration */ > > > static AppArmorConfigMode apparmor_config_mode = APPARMOR_ENABLED; > > > > Does config reload (SIGHUP) work? dbus-daemon(1) manpage says "Policy > > changes should take effect with SIGHUP." It's probably best to specify in > > the documentation whether changing the apparmor mode is supposed to work. > > I'll have to test this and get back to you SIGHUP doesn't cause the mode to change. We're caching the AppArmor confinement context on each BusConnectionData at the initial connect operation so going from disabled to enabled could be problematic. For now, I'll document the lack of support for SIGHUP and I'll look into adding support in the future. (In reply to comment #43) > We're caching the AppArmor confinement context on each BusConnectionData at > the > initial connect operation so going from disabled to enabled could be > problematic. Given how intrusive LSMs are (system-wide, not just in D-Bus), I'd be OK with just documenting the fact that changing the mode needs a reboot. Would it ever make sense to have AppArmor enabled at the kernel level but not in dbus-daemon, or vice versa? If not, then perhaps there should be a mode that enables AppArmor if and only if the kernel says it is in use (which, if I understand correctly, is a mixture of kernel-compile-time configuration to have the functionality at all, and boot-time configuration on the kernel command line to switch it on)? (In reply to comment #42) > > I don't understand the comment about user space. > > Me neither. I'll add something more descriptive. If you're submitting patches, please review them yourself on the way :-) In particular, if you're submitting patches on behalf of someone else, please review them and fix anything that seems wrong, or at least ask the original author to comment on things that look like problem areas. (In reply to comment #44) > (In reply to comment #43) > > We're caching the AppArmor confinement context on each BusConnectionData at > > the > > initial connect operation so going from disabled to enabled could be > > problematic. > > Given how intrusive LSMs are (system-wide, not just in D-Bus), I'd be OK > with just documenting the fact that changing the mode needs a reboot. In the v3 patches that I'm about to attach, I made it clear in the man page that the mediation mode cannot be changed after the daemon is started (not even with SIGHUP). That implies that you must restart the daemon or reboot. I hope that's clear enough. > Would it ever make sense to have AppArmor enabled at the kernel level but > not in dbus-daemon, or vice versa? Yes. Someone may wish to have the AppArmor kernel module enforcing filesystem, network, and IPC rules but they may not be interested in D-Bus mediation. They can set the mode to "disabled" to achieve this. The inverse is not possible. If the kernel is not enforcing AppArmor rules, dbus-daemon can't be configured to enforce AppArmor D-Bus rules. > If not, then perhaps there should be a > mode that enables AppArmor if and only if the kernel says it is in use > (which, if I understand correctly, is a mixture of kernel-compile-time > configuration to have the functionality at all, and boot-time configuration > on the kernel command line to switch it on)? That is the default mode ("enabled"). At initialization, dbus-daemon checks to see if AppArmor is enabled or not and mimics the detected behavior. If an admin wants to disable enforcement, they can use "disabled" mode. If an admin wants to require enforcement, they can use "required" mode. (In reply to comment #45) > (In reply to comment #42) > > > I don't understand the comment about user space. > > > > Me neither. I'll add something more descriptive. > > If you're submitting patches, please review them yourself on the way :-) Oh, I have. :) I've made many changes and fixes. The main exception is the in-code comments. I generally tend to gloss over in-code comments as long as I understand the code itself. Sorry about that! > In particular, if you're submitting patches on behalf of someone else, > please review them and fix anything that seems wrong, or at least ask the > original author to comment on things that look like problem areas. We worked together very closely while getting these patches into shape, we're both on the Ubuntu Security Team, and we're in very close contact. I just didn't want to bother him over a comment that he quickly typed out over a year ago (and I have that good/bad habit of glossing over in-code comments). Created attachment 97982 [details] [review] [PATCH v3 01/13] Document AppArmor enforcement in the dbus-daemon man page Created attachment 97983 [details] [review] [PATCH v3 02/13] Add apparmor element and attributes to the bus config dtd Created attachment 97984 [details] [review] [PATCH v3 03/13] Update autoconf file to build against libapparmor Created attachment 97985 [details] [review] [PATCH v3 04/13] Add apparmor element support to bus config parsing Created attachment 97986 [details] [review] [PATCH v3 05/13] Initialize AppArmor mediation Created attachment 97987 [details] [review] [PATCH v3 06/13] Store AppArmor label of bus during initialization Created attachment 97988 [details] [review] [PATCH v3 07/13] Store AppArmor label of connecting processes Created attachment 97989 [details] [review] [PATCH v3 08/13] Mediation of processes that acquire well-known names Created attachment 97990 [details] [review] [PATCH v3 09/13] Do LSM checks after determining if the message is a requested reply Created attachment 97991 [details] [review] [PATCH v3 10/13] Mediation of processes sending and receiving messages Created attachment 97992 [details] [review] [PATCH v3 11/13] Mediation of processes eavesdropping Created attachment 97993 [details] [review] [PATCH v3 12/13] New a{sv} helper for using byte arrays as the variant Created attachment 97994 [details] [review] [PATCH v3 13/13] Add AppArmor support to GetConnectionCredentials (In reply to comment #46) > In the v3 patches that I'm about to attach, I made it clear in the man page > that the mediation mode cannot be changed after the daemon is started (not > even > with SIGHUP). That implies that you must restart the daemon or reboot. I hope > that's clear enough. Restarting the system dbus-daemon while booted is not a supported action; you must reboot. Restarting the session dbus-daemon mid-session is not a supported action; you must end the session. (Or, in the systemd user bus world, you must end all the sessions that share your uid.) > Someone may wish to have the AppArmor kernel module enforcing > filesystem, > network, and IPC rules but they may not be interested in D-Bus mediation. > They > can set the mode to "disabled" to achieve this. Makes sense. > The inverse is not possible. If the kernel is not enforcing AppArmor rules, > dbus-daemon can't be configured to enforce AppArmor D-Bus rules. Also makes sense. > That is the default mode ("enabled"). At initialization, dbus-daemon checks > to > see if AppArmor is enabled or not and mimics the detected behavior. If an > admin > wants to disable enforcement, they can use "disabled" mode. If an admin wants > to require enforcement, they can use "required" mode. OK. I'd have called these "auto", "disabled" and "enabled" respectively (or something with that general meaning), but if your way is more consistent with the SELinux glue, that's fine. I'll try to do a detailed review on this at some point, but D-Bus is not my main focus right now, so it might be a few days. In the meantime, reviews from other developers would be welcome, whether they are D-Bus maintainers or not - particularly people who understand AppArmor. (Alban, maybe you could take another look?) The v3 patches work fine for me when applied on dbus-1.8.0. My previous comments have been addressed and v3 looks good to me. (In reply to comment #61) > (In reply to comment #46) > > In the v3 patches that I'm about to attach, I made it clear in the man page > > that the mediation mode cannot be changed after the daemon is started (not > > even > > with SIGHUP). That implies that you must restart the daemon or reboot. I hope > > that's clear enough. > > Restarting the system dbus-daemon while booted is not a supported action; > you must reboot. Good to know. The man page patch in v3 should still be fine since it doesn't say that you can restart the daemon. It just says that you can't change the mode after starting the daemon. Hello! I wanted to give a friendly reminder that version 3 of the AppArmor patches are still waiting for review by a D-Bus maintainer. I understand that other features, fixes, and projects take precedence from time to time but I wanted to make sure that these patches aren't falling all the way off the radar. :) (In reply to comment #65) > Hello! I wanted to give a friendly reminder that version 3 of the AppArmor > patches are still waiting for review by a D-Bus maintainer. I understand > that other features, fixes, and projects take precedence from time to time > but I wanted to make sure that these patches aren't falling all the way off > the radar. :) They are on my list but I don't have time for a proper review right now; and since they are security-sensitive, a proper review is necessary. Comment on attachment 97985 [details] [review] [PATCH v3 04/13] Add apparmor element support to bus config parsing Review of attachment 97985 [details] [review]: ----------------------------------------------------------------- No obvious major issues up to this point in the series. ::: bus/apparmor.c @@ +2,5 @@ > + * apparmor.c AppArmor security checks for D-Bus > + * > + * Authors: John Johansen <john.johansen@canonical.com> > + * Tyler Hicks <tyhicks@canonical.com> > + * Based on: selinux.c by Matthew Rickard Copyright holder, please (you and John personally? Canonical Ltd.? some other legal entity?) - you don't need to rebase or anything, it can be a follow-up patch. Any copyright notices from selinux.c should also be transferred here. I personally think that a list of *copyright holders* is essential (it tells you who you need to ask for relicensing, etc.) but don't think a list of *authors* is very useful (since the key information about authors is who touched it recently, not who wrote the now-unrecognisable prototype a decade ago), which is why new modules I write get "© 2014 Collabora Ltd." but not usually "Author: Simon McVittie". If you prefer to credit authors in the file header and not just the git history, that's fine, but do say who holds the copyright too. ::: bus/config-parser-common.h @@ +49,5 @@ > ELEMENT_STANDARD_SYSTEM_SERVICEDIRS, > ELEMENT_KEEP_UMASK, > ELEMENT_SYSLOG, > + ELEMENT_ALLOW_ANONYMOUS, > + ELEMENT_APPARMOR, Avoid trailing commas in an enum; apparently some people's compilers like to quote chapter and verse of ISO C, and then stop the build. Comment on attachment 97986 [details] [review] [PATCH v3 05/13] Initialize AppArmor mediation Review of attachment 97986 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +98,5 @@ > + goto out; > + > + if (!_dbus_string_append (&aa_dbus, aa_securityfs) || > + !_dbus_string_append (&aa_dbus, "/features/dbus/mask")) > + goto out; If we run out of memory during bus initialization and return FALSE (i.e. "this apparmor version doesn't support mediating D-Bus"), does that mean that OOM results in putting the bus in a less secure mode? @@ +101,5 @@ > + !_dbus_string_append (&aa_dbus, "/features/dbus/mask")) > + goto out; > + > + mask_file = open (_dbus_string_get_const_data (&aa_dbus), > + O_RDONLY | O_CLOEXEC); Is it necessary to open it, as opposed to just stat()ing it? @@ +126,5 @@ > +#ifdef HAVE_APPARMOR > + apparmor_enabled = (aa_is_enabled () && _bus_apparmor_aa_supports_dbus ()); > +#endif > + > + return TRUE; Shouldn't this be returning FALSE if we couldn't determine whether AA supports D-Bus due to OOM? @@ +221,5 @@ > +{ > +#ifdef HAVE_APPARMOR > + return apparmor_enabled; > +#endif > + return FALSE; Unreachable if not enabled - I'd put this in an #else block ::: bus/bus.c @@ +912,5 @@ > > + if (!bus_apparmor_full_init ()) > + { > + dbus_set_error (error, DBUS_ERROR_FAILED, > + "AppArmor enabled but full initialization failed; check system log\n"); Minor: Any particular reason not to make bus_apparmor_full_init() able to "raise" a DBusError on its own? Or you could make bus_apparmor_full_init() use DBUS_SYSTEM_LOG_FATAL so it exits the dbus-daemon on failure, if it is always going to be fatal anyway. Comment on attachment 97987 [details] [review] [PATCH v3 06/13] Store AppArmor label of bus during initialization Review of attachment 97987 [details] [review]: ----------------------------------------------------------------- > It is important to note that libapparmor mallocs a single buffer to > store the con and mode strings and separates them with a NUL terminator. > Because of this, only con should be freed. The commit message doesn't seem like the right place for this information. ::: bus/apparmor.c @@ +68,5 @@ > +{ > + int refcount; /* Reference count */ > + > + char *context; /* AppArmor confinement context (label) */ > + char *mode; /* AppArmor confinement mode */ I think the fact that mode is freed automatically by freeing context deserves a comment here. Maybe mode should be a const char *. @@ +76,5 @@ > + > +static BusAppArmorConfinement *bus_con = NULL; > + > +static BusAppArmorConfinement* > +bus_apparmor_confinement_new (char *context, char *mode) The fact that context and mode are assumed to be allocated with apparmor's odd memory allocation scheme, where freeing context gets rid of mode too, deserves a comment. The fact that ownership is taken also deserves a comment. The "default" for C should normally be that parameters' ownership is not transferred unless documented otherwise. Comment on attachment 97988 [details] [review] [PATCH v3 07/13] Store AppArmor label of connecting processes Review of attachment 97988 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +276,5 @@ > return FALSE; > } > + > +void > +bus_apparmor_confinement_unref (BusAppArmorConfinement *confinement) (Why was this moved to a different place in the file?) @@ +320,5 @@ > + "Failed to get socket file descriptor of connection"); > + return NULL; > + } > + > + if (aa_getpeercon (fd, &context, &mode) == -1) I notice this won't work in mainline Linux (Linus' tree as of commit 44bf091), in which AppArmor does not yet implement socket_getpeersec_stream(). I assume you're trying to upstream the more complete implementation of AppArmor found in Ubuntu, but are not there yet? In practice, this means that enabling AppArmor (mandatory or not) on a mainline kernel will result in all connections to the bus being rejected. This seems ... non-ideal. Until the more full implementation of AppArmor is upstream, I think it should be runtime-disabled by default (regardless of whether it was compile-time-enabled or not), and documented as requiring a patched kernel such as Ubuntu's. Ubuntu can patch their system.conf to enable it. It would be possible to test for this at runtime (during dbus-daemon startup) by opening a socketpair() and trying aa_getpeercon() on it. If AA is in "enabled" mode (i.e. not "required"), perhaps it would be a good idea to do that, and flip AA to disabled if that fails? ---- A separate issue: I was concerned that this might return the *current* credentials of the process at the other end of the socket, but no, it seems that (at least in Ubuntu's patched kernel) it returns the credentials that *were* active when the socket was created, like SO_PEERCRED does for the uid/gid/pid. I should add some notes to our credentials-passing code that make it explicit that we rely on this for security. Could you please confirm that you will not upstream AppArmor into mainline kernels without preserving this property? Comment on attachment 97989 [details] [review] [PATCH v3 08/13] Mediation of processes that acquire well-known names Review of attachment 97989 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +171,5 @@ > + if (!capng_have_capability (CAPNG_EFFECTIVE, CAP_AUDIT_WRITE)) > + goto syslog; > + > + if (!_dbus_string_init (&avc)) > + goto syslog; Hmm. I know I said "DBusString is better, use DBusString" before, and I realise it's frustrating if I change my mind between reviews; but... This change from a static buffer (which cannot fail) to DBusString (which can) does have the disadvantage that if an operation is forbidden by policy, *but* the dbus-daemon reaches OOM while trying to log it to the audit log, it will silently go to the syslog instead. Is that considered to be a security issue? How mission-critical is the audit log considered to be? I realise this is largely theoretical, because Linux nearly always has overcommitting enabled, so malloc() basically won't ever return NULL. @@ +181,5 @@ > + _dbus_string_free (&avc); > + goto syslog; > + } > + > + /* FIXME: need to change this to show real user */ Is this something you plan to / need to fix, or copypasta from SELinux, or what? @@ +262,5 @@ > + > +static dbus_bool_t > +build_common_query (DBusString *query, const char *con, const char *bustype) > +{ > + return _dbus_string_set_length (query, AA_QUERY_CMD_LABEL_SIZE) && [Serious bug] As far as I can see, this sets @query to AA_QUERY_CMD_LABEL_SIZE uninitialized bytes + con + '\0' + AA_CLASS_DBUS + bustype. I don't think that's what you want. I suspect what you intended is to truncate the string (set its length to 0) and then append stuff. DBusString is a flexible/automatically-allocated string-buffer, like GString or GArray in GLib - it is not a fixed-length buffer. It will always expand its malloc()'d storage to be long enough for whatever you append to it. Comment on attachment 97991 [details] [review] [PATCH v3 10/13] Mediation of processes sending and receiving messages Review of attachment 97991 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +870,5 @@ > + !_dbus_append_pair_uint (&auxdata, "pid", pid)) > + goto oom; > + > + if (src_con->context && > + !_dbus_append_pair_str (&auxdata, "profile", src_con->context)) Is the standard terminology "context" or "profile"? (In reply to comment #70) > In practice, this means that enabling AppArmor (mandatory or not) on a > mainline kernel will result in all connections to the bus being rejected. > This seems ... non-ideal. Until the more full implementation of AppArmor is > upstream, I think it should be runtime-disabled by default (regardless of > whether it was compile-time-enabled or not), and documented as requiring a > patched kernel such as Ubuntu's. Ubuntu can patch their system.conf to > enable it. Here is one suitable implementation of what I suggest here: instead of <!-- Enable AppArmor mediation when it is available --> <apparmor mode="enabled"/> use <!-- AppArmor mediation requires a kernel with an AppArmor-specific socket_getpeersec_stream() implementation, which is not in mainline Linux as of 3.17-rc3. If you are using a patched kernel such as Ubuntu's, or a kernel where this has been upstreamed, change this to "enabled" to auto-detect AppArmor support, or "required" to make it mandatory. --> <apparmor mode="disabled"/> However, fixing the auto-detection to detect all the required functionality might be a better approach. (In reply to comment #13) > Is this guaranteed to happen early enough in socket processing that it > avoids the attack described in > <https://bugs.freedesktop.org/show_bug.cgi?id=47581#c3>? > > (If it happens during the initial "handshake", before any D-Bus messages can > have been sent by the peer, then the answer is yes.) To be clear: I had misunderstood the property we rely on for bus connection identity, which is really quite subtle. We are not relying on "things happening early enough": that would not be secure, because a message-sender could * queue up the entire handshake, plus a malicious message, in the outgoing socket buffer * make the D-Bus connection not close-on-exec * exec() a setuid process, or fd-pass the D-Bus connection to a more privileged process Instead, the security property we're relying on is that when a Unix socket is created (*BSD) or calls connect() or listen() (Linux), the kernel remembers the credentials that the process had at that moment, and stashes them in kernel memory; and then when someone gets SO_PEERCRED or calls getpeereid() or whatever, the kernel returns those values, *not* the ones that are current at the moment of querying. Similarly, for SCM_CREDS, the credentials that are sent are the ones that the peer explicitly sent, not the ones that are current at the moment of receiving. Because fds can be passed around between processes, resulting in having more than one peer, I don't think it would actually be possible to implement getpeereid() and friends in the way that would break our assumptions... but I'm still going to open a separate bug about documenting this better. Comment on attachment 97994 [details] [review] [PATCH v3 13/13] Add AppArmor support to GetConnectionCredentials Review of attachment 97994 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +584,5 @@ > + > + if (context != NULL) > + { > + if (!_dbus_asv_add_byte_array (array_iter, "AppArmorContext", > + context, strlen (context))) The GDBus/GVariant developers encourage protocol designers to encode bytestrings (strings of unspecified encoding that are guaranteed not to contain embedded \0) as a byte array that *does* include the trailing \0 in its length: so a context of "foo" would be (in GVariant notation) { 'AppArmorContext': @ay [102, 111, 111, 0] } This is because that way, the wire-protocol encoding of the message is guaranteed to have a \0 after the string data: so you can get a valid C string by pointing into the message buffer, without copying. GVariant-based tools like gdbus(1) pretty-print such strings, if their contents happen to be ASCII. tl;dr: consider using strlen (context) + 1. @@ +591,5 @@ > + > + if (mode != NULL) > + { > + if (!_dbus_asv_add_byte_array (array_iter, "AppArmorMode", > + mode, strlen (mode))) Here too. ::: doc/dbus-specification.xml @@ +5842,5 @@ > + <entry>ARRAY of BYTE</entry> > + <entry>The AppArmor confinement context of the connection. This > + byte string, which may not be UTF-8, is the same string as > + returned in the *con argument by the aa_getcon() family of > + AppArmor functions.</entry> ... and document whether the \0 is included. An example value would also be nice. @@ +5850,5 @@ > + <entry>ARRAY of BYTE</entry> > + <entry>The AppArmor confinement mode for the connection. This > + byte string, which may not be UTF-8, is the same string as > + returned in the *mode argument by the aa_getcon() family of > + AppArmor functions.</entry> ... and here (In reply to Simon McVittie from comment #67) > ::: bus/apparmor.c > @@ +2,5 @@ > > + * apparmor.c AppArmor security checks for D-Bus > > + * > > + * Authors: John Johansen <john.johansen@canonical.com> > > + * Tyler Hicks <tyhicks@canonical.com> > > + * Based on: selinux.c by Matthew Rickard > > Copyright holder, please (you and John personally? Canonical Ltd.? some > other legal entity?) - you don't need to rebase or anything, it can be a > follow-up patch. I'll add a line mentioning Canonical, Ltd. as the copyright holder. > Any copyright notices from selinux.c should also be transferred here. There are none. > I personally think that a list of *copyright holders* is essential (it tells > you who you need to ask for relicensing, etc.) but don't think a list of > *authors* is very useful (since the key information about authors is who > touched it recently, not who wrote the now-unrecognisable prototype a decade > ago), which is why new modules I write get "© 2014 Collabora Ltd." but not > usually "Author: Simon McVittie". > > If you prefer to credit authors in the file header and not just the git > history, that's fine, but do say who holds the copyright too. I agree with your opinion. I'll drop the author names. > ::: bus/config-parser-common.h > @@ +49,5 @@ > > ELEMENT_STANDARD_SYSTEM_SERVICEDIRS, > > ELEMENT_KEEP_UMASK, > > ELEMENT_SYSLOG, > > + ELEMENT_ALLOW_ANONYMOUS, > > + ELEMENT_APPARMOR, > > Avoid trailing commas in an enum; apparently some people's compilers like to > quote chapter and verse of ISO C, and then stop the build. Ack. (In reply to Simon McVittie from comment #68) > Comment on attachment 97986 [details] [review] [review] > [PATCH v3 05/13] Initialize AppArmor mediation > > Review of attachment 97986 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: bus/apparmor.c > @@ +98,5 @@ > > + goto out; > > + > > + if (!_dbus_string_append (&aa_dbus, aa_securityfs) || > > + !_dbus_string_append (&aa_dbus, "/features/dbus/mask")) > > + goto out; > > If we run out of memory during bus initialization and return FALSE (i.e. > "this apparmor version doesn't support mediating D-Bus"), does that mean > that OOM results in putting the bus in a less secure mode? It depends on the apparmor mode, which is specified in the bus config file. If the mode is "required", then bus_apparmor_full_init() will fail and the bus will exit with error. If the mode is "enabled" (the default mode), then the bus will run without AppArmor mediation. If the mode is "disabled", then the bus will run without AppArmor mediation. > > @@ +101,5 @@ > > + !_dbus_string_append (&aa_dbus, "/features/dbus/mask")) > > + goto out; > > + > > + mask_file = open (_dbus_string_get_const_data (&aa_dbus), > > + O_RDONLY | O_CLOEXEC); > > Is it necessary to open it, as opposed to just stat()ing it? No, stat() should be fine. I'll change this. > > @@ +126,5 @@ > > +#ifdef HAVE_APPARMOR > > + apparmor_enabled = (aa_is_enabled () && _bus_apparmor_aa_supports_dbus ()); > > +#endif > > + > > + return TRUE; > > Shouldn't this be returning FALSE if we couldn't determine whether AA > supports D-Bus due to OOM? I don't think so, but I need to think about this a bit more. The problem is that we don't yet know if AppArmor mediation is disabled in the bus config because the bus config has not been parsed at this point. We wouldn't want to error out if we couldn't check AA status but AA mediation is disabled in the bus config. I guess I could change apparmor_enabled from a bool to an int. -1 if AA status couldn't be checked (due to OOM, securityfs not being mounted, etc.), 0 if AA D-Bus mediation isn't available, and 1 if AA is available and supports D-Bus mediation. Then, bus_apparmor_full_init() could make the decision on whether or not to continue based on the bus config. That would mean that we'd want to make bus_apparmor_pre_init() return void. > @@ +221,5 @@ > > +{ > > +#ifdef HAVE_APPARMOR > > + return apparmor_enabled; > > +#endif > > + return FALSE; > > Unreachable if not enabled - I'd put this in an #else block Ack > > ::: bus/bus.c > @@ +912,5 @@ > > > > + if (!bus_apparmor_full_init ()) > > + { > > + dbus_set_error (error, DBUS_ERROR_FAILED, > > + "AppArmor enabled but full initialization failed; check system log\n"); > > Minor: Any particular reason not to make bus_apparmor_full_init() able to > "raise" a DBusError on its own? No particular reason other than following the style of the existing SELinux support. > Or you could make bus_apparmor_full_init() use DBUS_SYSTEM_LOG_FATAL so it > exits the dbus-daemon on failure, if it is always going to be fatal anyway. DBUS_SYSTEM_LOG_FATAL isn't handled as I expected it would be in bus_context_log(). If the <syslog> element is not present in the bus config, then bus_context_log() does not exit(1) when severity is DBUS_SYSTEM_LOG_FATAL. Is that the intended behavior? I'd have thought that exit(1) would happen no matter if the syslog was in use. (In reply to Simon McVittie from comment #69) > Comment on attachment 97987 [details] [review] [review] > [PATCH v3 06/13] Store AppArmor label of bus during initialization > > Review of attachment 97987 [details] [review] [review]: > ----------------------------------------------------------------- > > > It is important to note that libapparmor mallocs a single buffer to > > store the con and mode strings and separates them with a NUL terminator. > > Because of this, only con should be freed. > > The commit message doesn't seem like the right place for this information. It was meant to reinforce the comment added in the bus_apparmor_confinement_unref() function. > > ::: bus/apparmor.c > @@ +68,5 @@ > > +{ > > + int refcount; /* Reference count */ > > + > > + char *context; /* AppArmor confinement context (label) */ > > + char *mode; /* AppArmor confinement mode */ > > I think the fact that mode is freed automatically by freeing context > deserves a comment here. Maybe mode should be a const char *. Ack > @@ +76,5 @@ > > + > > +static BusAppArmorConfinement *bus_con = NULL; > > + > > +static BusAppArmorConfinement* > > +bus_apparmor_confinement_new (char *context, char *mode) > > The fact that context and mode are assumed to be allocated with apparmor's > odd memory allocation scheme, where freeing context gets rid of mode too, > deserves a comment. > > The fact that ownership is taken also deserves a comment. The "default" for > C should normally be that parameters' ownership is not transferred unless > documented otherwise. Ack (In reply to Simon McVittie from comment #70) > Comment on attachment 97988 [details] [review] [review] > [PATCH v3 07/13] Store AppArmor label of connecting processes > > Review of attachment 97988 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: bus/apparmor.c > @@ +276,5 @@ > > return FALSE; > > } > > + > > +void > > +bus_apparmor_confinement_unref (BusAppArmorConfinement *confinement) > > (Why was this moved to a different place in the file?) I don't know why that happened. Probably a mistake on my part. > > @@ +320,5 @@ > > + "Failed to get socket file descriptor of connection"); > > + return NULL; > > + } > > + > > + if (aa_getpeercon (fd, &context, &mode) == -1) > > I notice this won't work in mainline Linux (Linus' tree as of commit > 44bf091), in which AppArmor does not yet implement > socket_getpeersec_stream(). I assume you're trying to upstream the more > complete implementation of AppArmor found in Ubuntu, but are not there yet? Correct. John Johansen, the upstream AppArmor kernel maintainer, was the one that implemented that in Ubuntu. He will be upstreaming it very soon. > In practice, this means that enabling AppArmor (mandatory or not) on a > mainline kernel will result in all connections to the bus being rejected. > This seems ... non-ideal. Until the more full implementation of AppArmor is > upstream, I think it should be runtime-disabled by default (regardless of > whether it was compile-time-enabled or not), and documented as requiring a > patched kernel such as Ubuntu's. Ubuntu can patch their system.conf to > enable it. Enabling AppArmor support in dbus-daemon while running a mainline kernel will *not* result in all connections to the bus being rejected. This is because the mainline kernel will not have the apparmorfs/features/dbus/mask file until the mainline kernel has AppArmor getpeersec support. I've confirmed this with John. He (rightly) considers the getpeersec support to be a prereq for the dbus features file. > It would be possible to test for this at runtime (during dbus-daemon > startup) by opening a socketpair() and trying aa_getpeercon() on it. If AA > is in "enabled" mode (i.e. not "required"), perhaps it would be a good idea > to do that, and flip AA to disabled if that fails? > > ---- > > A separate issue: > > I was concerned that this might return the *current* credentials of the > process at the other end of the socket, but no, it seems that (at least in > Ubuntu's patched kernel) it returns the credentials that *were* active when > the socket was created, like SO_PEERCRED does for the uid/gid/pid. > > I should add some notes to our credentials-passing code that make it > explicit that we rely on this for security. > > Could you please confirm that you will not upstream AppArmor into mainline > kernels without preserving this property? After speaking with John, what he plans to upstream for getpeersec support will be a bit different than what is currently in Ubuntu. getpeersec will return a set of security contexts. The set will contain the context of the original process that opened the socket, as well as the context of any processes that have inherited the file. When dbus-daemon queries the AppArmor kernel code to see if it should allow an operation, the AppArmor kernel code will (safely) split the set of contexts up and check them one-by-one to be sure that each context is allowed to perform the operation. There will be no changes required to the dbus-daemon code. Since the original credential will be preserved, I think this will be sufficient in addressing your concern. (In reply to Simon McVittie from comment #71) > Comment on attachment 97989 [details] [review] [review] > [PATCH v3 08/13] Mediation of processes that acquire well-known names > > Review of attachment 97989 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: bus/apparmor.c > @@ +171,5 @@ > > + if (!capng_have_capability (CAPNG_EFFECTIVE, CAP_AUDIT_WRITE)) > > + goto syslog; > > + > > + if (!_dbus_string_init (&avc)) > > + goto syslog; > > Hmm. I know I said "DBusString is better, use DBusString" before, and I > realise it's frustrating if I change my mind between reviews; but... > > This change from a static buffer (which cannot fail) to DBusString (which > can) does have the disadvantage that if an operation is forbidden by policy, > *but* the dbus-daemon reaches OOM while trying to log it to the audit log, > it will silently go to the syslog instead. > > Is that considered to be a security issue? How mission-critical is the audit > log considered to be? > > I realise this is largely theoretical, because Linux nearly always has > overcommitting enabled, so malloc() basically won't ever return NULL. While not ideal, we're ok with the potential for falling back to syslog() if memory allocation fails. I think the increased safety provided by DBusString when building the query strings and audit messages is more of a win than using static char arrays to prevent allocation failures. I guess it is a bit of a toss-up. If someone feels strongly either way, then please speak up. > @@ +181,5 @@ > > + _dbus_string_free (&avc); > > + goto syslog; > > + } > > + > > + /* FIXME: need to change this to show real user */ > > Is this something you plan to / need to fix, or copypasta from SELinux, or > what? Copypasta that I wasn't planning on fixing atm. I suppose it may be as simple as passing the result of dbus_connection_get_unix_user() to log_message(), but it would need some more thought and testing. > @@ +262,5 @@ > > + > > +static dbus_bool_t > > +build_common_query (DBusString *query, const char *con, const char *bustype) > > +{ > > + return _dbus_string_set_length (query, AA_QUERY_CMD_LABEL_SIZE) && > > [Serious bug] > > As far as I can see, this sets @query to AA_QUERY_CMD_LABEL_SIZE > uninitialized bytes + con + '\0' + AA_CLASS_DBUS + bustype. > > I don't think that's what you want. I suspect what you intended is to > truncate the string (set its length to 0) and then append stuff. > > DBusString is a flexible/automatically-allocated string-buffer, like GString > or GArray in GLib - it is not a fixed-length buffer. It will always expand > its malloc()'d storage to be long enough for whatever you append to it. The good news is that this is intentional. The bad news is that it is a bit of a wart in the libapparmor query API. The aa_query_label() function fills that area in with a private value before sending query string on to the kernel. dbus-daemon is the first user of the userspace AppArmor query interface so the API was intentionally left in a barebones state while we better figure out what type of queries future userspace helpers will be doing. Then it can overlayed with nicer, higher level functions that are tailored to the specific type of query being performed. That said, it is poor form to leave the first AA_QUERY_CMD_LABEL_SIZE bytes uninitialized even if libapparmor will immediately initialize them. I'll change the call from _dbus_string_set_length() to _dbus_string_insert_bytes() and leave a comment about what is happening. (In reply to Simon McVittie from comment #72) > Comment on attachment 97991 [details] [review] [review] > [PATCH v3 10/13] Mediation of processes sending and receiving messages > > Review of attachment 97991 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: bus/apparmor.c > @@ +870,5 @@ > > + !_dbus_append_pair_uint (&auxdata, "pid", pid)) > > + goto oom; > > + > > + if (src_con->context && > > + !_dbus_append_pair_str (&auxdata, "profile", src_con->context)) > > Is the standard terminology "context" or "profile"? Good question. It changes depending on the context, for lack of a better word. User facing things, such as entries in the syslog or audit log, tend to use the term "profile". The term "context" is increasingly being used by developers when writing code and discussing AppArmor internals. As I mentioned in a previous comment, a context may soon consist of a set of profile names. (In reply to Simon McVittie from comment #73) > (In reply to comment #70) > > In practice, this means that enabling AppArmor (mandatory or not) on a > > mainline kernel will result in all connections to the bus being rejected. > > This seems ... non-ideal. Until the more full implementation of AppArmor is > > upstream, I think it should be runtime-disabled by default (regardless of > > whether it was compile-time-enabled or not), and documented as requiring a > > patched kernel such as Ubuntu's. Ubuntu can patch their system.conf to > > enable it. > > Here is one suitable implementation of what I suggest here: instead of > > <!-- Enable AppArmor mediation when it is available --> > <apparmor mode="enabled"/> > > use > > <!-- AppArmor mediation requires a kernel with an AppArmor-specific > socket_getpeersec_stream() implementation, which is not in mainline > Linux as of 3.17-rc3. If you are using a patched kernel such as > Ubuntu's, or a kernel where this has been upstreamed, change this > to "enabled" to auto-detect AppArmor support, or "required" > to make it mandatory. --> > <apparmor mode="disabled"/> > > However, fixing the auto-detection to detect all the required functionality > might be a better approach. As I mentioned in comment #79, the auto-detection logic is already sufficient. (In reply to Simon McVittie from comment #74) > (In reply to comment #13) > > Is this guaranteed to happen early enough in socket processing that it > > avoids the attack described in > > <https://bugs.freedesktop.org/show_bug.cgi?id=47581#c3>? > > > > (If it happens during the initial "handshake", before any D-Bus messages can > > have been sent by the peer, then the answer is yes.) > > To be clear: I had misunderstood the property we rely on for bus connection > identity, which is really quite subtle. > > We are not relying on "things happening early enough": that would not be > secure, because a message-sender could > > * queue up the entire handshake, plus a malicious message, in the outgoing > socket buffer > * make the D-Bus connection not close-on-exec > * exec() a setuid process, or fd-pass the D-Bus connection to a more > privileged process > > Instead, the security property we're relying on is that when a Unix socket > is created (*BSD) or calls connect() or listen() (Linux), the kernel > remembers the credentials that the process had at that moment, and stashes > them in kernel memory; and then when someone gets SO_PEERCRED or calls > getpeereid() or whatever, the kernel returns those values, *not* the ones > that are current at the moment of querying. > > Similarly, for SCM_CREDS, the credentials that are sent are the ones that > the peer explicitly sent, not the ones that are current at the moment of > receiving. > > Because fds can be passed around between processes, resulting in having more > than one peer, I don't think it would actually be possible to implement > getpeereid() and friends in the way that would break our assumptions... but > I'm still going to open a separate bug about documenting this better. Interesting. Thanks for expanding on this. However, it isn't a concern for the AppArmor contexts: 1) Confined processes can't simply change to another context. Its AppArmor profile must contain a rule that allows it to change to that specific context. 2) As I mentioned in comment #79, the socket labeling code that will go upstream will make it so that the original AppArmor context will follow the fd as it gets passed (over exec or with SCM_RIGHTS). If the fd gets passed a second time, the previous two contexts will follow it and so on... (In reply to Simon McVittie from comment #75) > Comment on attachment 97994 [details] [review] [review] > [PATCH v3 13/13] Add AppArmor support to GetConnectionCredentials > > Review of attachment 97994 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: bus/apparmor.c > @@ +584,5 @@ > > + > > + if (context != NULL) > > + { > > + if (!_dbus_asv_add_byte_array (array_iter, "AppArmorContext", > > + context, strlen (context))) > > The GDBus/GVariant developers encourage protocol designers to encode > bytestrings (strings of unspecified encoding that are guaranteed not to > contain embedded \0) as a byte array that *does* include the trailing \0 in > its length: so a context of "foo" would be (in GVariant notation) > > { 'AppArmorContext': @ay [102, 111, 111, 0] } > > This is because that way, the wire-protocol encoding of the message is > guaranteed to have a \0 after the string data: so you can get a valid C > string by pointing into the message buffer, without copying. > > GVariant-based tools like gdbus(1) pretty-print such strings, if their > contents happen to be ASCII. > > tl;dr: consider using strlen (context) + 1. Good idea. Will do. > > @@ +591,5 @@ > > + > > + if (mode != NULL) > > + { > > + if (!_dbus_asv_add_byte_array (array_iter, "AppArmorMode", > > + mode, strlen (mode))) > > Here too. > > ::: doc/dbus-specification.xml > @@ +5842,5 @@ > > + <entry>ARRAY of BYTE</entry> > > + <entry>The AppArmor confinement context of the connection. This > > + byte string, which may not be UTF-8, is the same string as > > + returned in the *con argument by the aa_getcon() family of > > + AppArmor functions.</entry> > > ... and document whether the \0 is included. > > An example value would also be nice. > > @@ +5850,5 @@ > > + <entry>ARRAY of BYTE</entry> > > + <entry>The AppArmor confinement mode for the connection. This > > + byte string, which may not be UTF-8, is the same string as > > + returned in the *mode argument by the aa_getcon() family of > > + AppArmor functions.</entry> > > ... and here Ack on all three above. Has there been any progress on a v4 patchset addressing my review concerns, other than those for which you have justified why I was wrong? I would like to get this merged, because having a rather major distribution shipping a significant patchset that touches security-sensitive code is not a great situation; but equally, I would like what I merge to be "as it should be". (In reply to Simon McVittie from comment #75) > The GDBus/GVariant developers encourage protocol designers to encode > bytestrings (strings of unspecified encoding that are guaranteed not to > contain embedded \0) as a byte array that *does* include the trailing \0 in > its length If you are able to make this change, it should be as soon as possible, because the longer you leave it, the more Ubuntu-specific things there could be that rely on it. (In reply to Tyler Hicks from comment #77) > DBUS_SYSTEM_LOG_FATAL isn't handled as I expected it would be in > bus_context_log(). If the <syslog> element is not present in the bus config, > then bus_context_log() does not exit(1) when severity is > DBUS_SYSTEM_LOG_FATAL. > Is that the intended behavior? I'd have thought that exit(1) would happen no > matter if the syslog was in use. This seems like an oversight tbh. I'll open a separate bug. In a non-LSM world, the session bus is not any kind of privilege boundary anyway, so the only bus where it can ever really matter is the system bus, and if you reconfigure that to not <syslog/> you can keep both pieces. I realise this is more significant if you're using LSMs to turn the session bus into a privilege boundary though. > > I notice this won't work in mainline Linux (Linus' tree as of commit > > 44bf091), in which AppArmor does not yet implement > > socket_getpeersec_stream(). ... > the > mainline kernel will not have the apparmorfs/features/dbus/mask file until > the > mainline kernel has AppArmor getpeersec support. I've confirmed this with > John. > He (rightly) considers the getpeersec support to be a prereq for the dbus > features file. Ack. OK, no objection on that basis, but if this statement turns out to be false I will consider the resulting regressions to be entirely your fault :-P (In reply to Tyler Hicks from comment #80) > While not ideal, we're ok with the potential for falling back to syslog() if > memory allocation fails. I think the increased safety provided by DBusString > when building the query strings and audit messages is more of a win than > using > static char arrays to prevent allocation failures. Sure. I'm happy with this if you are; you're the ones with the (potential for) non-obvious security requirements. (... and in any case, if malloc() ever returns NULL this is probably the least of your concerns :-) > > As far as I can see, this sets @query to AA_QUERY_CMD_LABEL_SIZE > > uninitialized bytes + con + '\0' + AA_CLASS_DBUS + bustype. > > The good news is that this is intentional. ... > That said, it is poor form to leave the first AA_QUERY_CMD_LABEL_SIZE bytes > uninitialized even if libapparmor will immediately initialize them. Yes. I would very much prefer AA_QUERY_CMD_LABEL_SIZE times '\0' and an informative comment, perhaps something like this: /* leave enough space for libapparmor to fill in its private data... */ if (!_dbus_string_set_length (query, AA_QUERY_CMD_LABEL_SIZE)) return FALSE; _dbus_string_zero (query); /* ... then append the actual query */ return _dbus_string_append (query, con) && _dbus_string_append_byte (query, '\0') && _dbus_string_append_byte (query, AA_CLASS_DBUS) && _dbus_string_append (query, bustype); > > Is the standard terminology "context" or "profile"? > > Good question. It changes depending on the context, for lack of a better > word [...] a context may soon consist of a set of profile names. If that's the case, please be extra-careful to avoid saying "profile" unless something is unambiguously a single profile. I would really, really appreciate seeing a realistic example wherever you document a getter for contexts/profiles. At the moment I have no idea whether a context (or a profile) typically/conventionally looks like "exe:/usr/bin/gnome-terminal" or "usr.bin.gnome-terminal" or "org.gnome.Terminal" or "user1000app654" or "user_tty_t" or what. A couple of "... such as ..." would go a long way. I realise that the in-kernel infrastructure supports arbitrary 0-terminated strings and does not enforce a policy for what they look like, but giving developers some idea what sort of thing they're looking for is valuable :-) (And yes I know the same could be said for SELinux; I'd be happy to receive patches saying something about the typical value of a SELinux context, too.) (In reply to Simon McVittie from comment #85) > Has there been any progress on a v4 patchset addressing my review concerns, > other than those for which you have justified why I was wrong? Yes - I made good progress months ago and then the patches were mothballed while I was trying to find the time to understand a bug (https://launchpad.net/bugs/1362469) related to the unrequested reply changes in v2 of this patchset. I was able to triage the bug yesterday so I'll start finalizing the v4 patchset and attach them to this bug. Thanks! (In reply to Simon McVittie from comment #18) > Before doing this, you will have to clarify whether the AppArmor > label string is guaranteed to be a valid D-Bus string (guaranteed to be > UTF-8, with no embedded '\0' and no codepoints outside Unicode); if not, it > will have to be transferred as a bytestring (byte array, 'ay'). I've been discussing this with Smack and SELinux developers on the linux-security-module list (plus a ping to Ubuntu's apparmor list), and they seem to think that the effective kernel ABI for SO_PEERCRED is "it's an ASCII string with no embedded \0". If true, then it's OK as a D-Bus string. I'm hoping this is the case, because strings are even more convenient than bytestrings - but if AppArmor profiles are inextricably tied to pathnames on disk, then that would mean there would be no way to have a profile for the hypothetical utility /usr/local/bin/check-©-notices. If the ABI of SO_PEERCRED was relaxed from "must be ASCII" to "must be UTF-8", you still couldn't do this if its on-disk name is in Latin-1 (i.e. if it was "check-\xa9-notices" rather than "check-\xc2\xa9-notices"). Nobody should be using non-UTF8 on Linux filesystems this decade, but people probably do, because legacy systems. We have also been discussing the possibility of having a single element in GetConnectionCredentials, "LinuxSecurityLabel" or "SecurityLabel" or something, shared between all LSMs, whose value is defined to be the same string (or bytestring) that you would get from SO_PEERCRED. (In reply to Tyler Hicks from comment #86) > while I was trying to find the time to understand a bug > (https://launchpad.net/bugs/1362469) related to the unrequested reply > changes in v2 of this patchset. For your reference, we do have a mailing list where you can ask questions about subtleties like "what's the deal with unrequested replies?". The D-Bus maintainers have traditionally treated it as a minor language-binding bug if a binding replies to a method call that didn't actually want a reply - this is because, just like your noisy denial messages in the audit log, we get noisy denial messages in the syslog. One note: now that I've landed support for non-insecure system bus monitoring which bypasses <allow> and <deny> rules (Bug #46787), you'll probably want to treat the new BecomeMonitor() method as eavesdropping, and call into bus_apparmor_allows_eavesdropping() from either bus_connection_be_monitor() or bus_driver_handle_become_monitor(). Based on a quick look at the v3 patch, the API you've used should make that easy to do. (In reply to Simon McVittie from comment #85) > (In reply to Tyler Hicks from comment #77) > > DBUS_SYSTEM_LOG_FATAL isn't handled as I expected it would be in > > bus_context_log(). If the <syslog> element is not present in the bus config, > > then bus_context_log() does not exit(1) when severity is > > DBUS_SYSTEM_LOG_FATAL. > > Is that the intended behavior? I'd have thought that exit(1) would happen no > > matter if the syslog was in use. > > This seems like an oversight tbh. I'll open a separate bug. Good news: this is fixed in master. Comment on attachment 97989 [details] [review] [PATCH v3 08/13] Mediation of processes that acquire well-known names Review of attachment 97989 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +266,5 @@ > + return _dbus_string_set_length (query, AA_QUERY_CMD_LABEL_SIZE) && > + _dbus_string_append (query, con) && > + _dbus_string_append_byte (query, '\0') && > + _dbus_string_append_byte (query, AA_CLASS_DBUS) && > + _dbus_string_append (query, bustype); Sorry, I didn't realise this issue until I looked back at the eavesdropping-mediation just now: The bus type might be NULL: if you are doing a bus other than the standard pair (system and session), it is valid to omit the <type> from the bus's config file altogether. At the moment you'd get an assertion failure on such buses, but you could use (bustype ? bustype : "") to circumvent that, if the empty string is acceptable. Similarly, whoever runs dbus-daemon can tell it an arbitrary type of their choice. The only constraint is that it's something you can express in XML (so no \0 or other low control characters). Is this a problem for you? (In reply to Simon McVittie from comment #87) > I've been discussing this with Smack and SELinux developers on the > linux-security-module list (plus a ping to Ubuntu's apparmor list) http://marc.info/?l=linux-security-module&m=142315527731162&w=2 https://lists.ubuntu.com/archives/apparmor/2015-February/007177.html (In reply to Simon McVittie from comment #87) > (In reply to Simon McVittie from comment #18) > > Before doing this, you will have to clarify whether the AppArmor > > label string is guaranteed to be a valid D-Bus string (guaranteed to be > > UTF-8, with no embedded '\0' and no codepoints outside Unicode); if not, it > > will have to be transferred as a bytestring (byte array, 'ay'). > > I've been discussing this with Smack and SELinux developers on the > linux-security-module list (plus a ping to Ubuntu's apparmor list), and they > seem to think that the effective kernel ABI for SO_PEERCRED is "it's an > ASCII string with no embedded \0". If true, then it's OK as a D-Bus string. > > I'm hoping this is the case, because strings are even more convenient than > bytestrings - but if AppArmor profiles are inextricably tied to pathnames on > disk, then that would mean there would be no way to have a profile for the > hypothetical utility /usr/local/bin/check-©-notices. If the ABI of > SO_PEERCRED was relaxed from "must be ASCII" to "must be UTF-8", you still > couldn't do this if its on-disk name is in Latin-1 (i.e. if it was > "check-\xa9-notices" rather than "check-\xc2\xa9-notices"). Nobody should be > using non-UTF8 on Linux filesystems this decade, but people probably do, > because legacy systems. As I mentioned in the thread on the linux-security-module list, we'll have to use bytestrings to represent AppArmor labels. > We have also been discussing the possibility of having a single element in > GetConnectionCredentials, "LinuxSecurityLabel" or "SecurityLabel" or > something, shared between all LSMs, whose value is defined to be the same > string (or bytestring) that you would get from SO_PEERCRED. Also as mentioned in the linux-security-module thread, libapparmor would need to provide a public API for splitting the string returned from the SO_PEERSEC interface so that applications can separate the label from the mode. This means that they'll now have to link against libapparmor. I have to say that I prefer having dbus-daemon call aa_getpeercon() and storing the label and mode separate. (In reply to Simon McVittie from comment #88) > (In reply to Tyler Hicks from comment #86) > > while I was trying to find the time to understand a bug > > (https://launchpad.net/bugs/1362469) related to the unrequested reply > > changes in v2 of this patchset. > > For your reference, we do have a mailing list where you can ask questions > about subtleties like "what's the deal with unrequested replies?". Higher priority issues were keeping me from looking at that bug. I thought the bug was in the AppArmor changes until I just recently instrumented the dbus code to dump certain debugging info. I'm not sure why I didn't realize earlier that the NO_REPLY_EXPECTED flag resulted in dbus-daemon treating the reply as an unrequested reply. I was somehow thinking that an unrequested reply was from a sender constructing a method_return or error message and trying to send it to an unexpecting connection on the bus. > The D-Bus maintainers have traditionally treated it as a minor > language-binding bug if a binding replies to a method call that didn't > actually want a reply - this is because, just like your noisy denial > messages in the audit log, we get noisy denial messages in the syslog. That's good to know. I'll look into fixing gdbus after I complete v4 of this patchset. (In reply to Simon McVittie from comment #89) > One note: now that I've landed support for non-insecure system bus > monitoring which bypasses <allow> and <deny> rules (Bug #46787), you'll > probably want to treat the new BecomeMonitor() method as eavesdropping, and > call into bus_apparmor_allows_eavesdropping() from either > bus_connection_be_monitor() or bus_driver_handle_become_monitor(). Based on > a quick look at the v3 patch, the API you've used should make that easy to > do. Thanks for the tip! (In reply to Simon McVittie from comment #91) > Comment on attachment 97989 [details] [review] [review] > [PATCH v3 08/13] Mediation of processes that acquire well-known names > > Review of attachment 97989 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: bus/apparmor.c > @@ +266,5 @@ > > + return _dbus_string_set_length (query, AA_QUERY_CMD_LABEL_SIZE) && > > + _dbus_string_append (query, con) && > > + _dbus_string_append_byte (query, '\0') && > > + _dbus_string_append_byte (query, AA_CLASS_DBUS) && > > + _dbus_string_append (query, bustype); > > Sorry, I didn't realise this issue until I looked back at the > eavesdropping-mediation just now: > > The bus type might be NULL: if you are doing a bus other than the standard > pair (system and session), it is valid to omit the <type> from the bus's > config file altogether. At the moment you'd get an assertion failure on such > buses, but you could use (bustype ? bustype : "") to circumvent that, if the > empty string is acceptable. > > Similarly, whoever runs dbus-daemon can tell it an arbitrary type of their > choice. The only constraint is that it's something you can express in XML > (so no \0 or other low control characters). > > Is this a problem for you? No. We already have working AppArmor policy for the "accessibility" bus. I was unaware of the possibility for a NULL bustype, though. I'll figure out if NULL or "" is most appropriate for building the query string in this situation. (In reply to Tyler Hicks from comment #77) > (In reply to Simon McVittie from comment #68) > > Comment on attachment 97986 [details] [review] [review] [review] > > [PATCH v3 05/13] Initialize AppArmor mediation > > > > Review of attachment 97986 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > @@ +126,5 @@ > > > +#ifdef HAVE_APPARMOR > > > + apparmor_enabled = (aa_is_enabled () && _bus_apparmor_aa_supports_dbus ()); > > > +#endif > > > + > > > + return TRUE; > > > > Shouldn't this be returning FALSE if we couldn't determine whether AA > > supports D-Bus due to OOM? > > I don't think so, but I need to think about this a bit more. The problem is > that we don't yet know if AppArmor mediation is disabled in the bus config > because the bus config has not been parsed at this point. We wouldn't want to > error out if we couldn't check AA status but AA mediation is disabled in the > bus config. > > I guess I could change apparmor_enabled from a bool to an int. -1 if AA > status > couldn't be checked (due to OOM, securityfs not being mounted, etc.), 0 if AA > D-Bus mediation isn't available, and 1 if AA is available and supports D-Bus > mediation. Then, bus_apparmor_full_init() could make the decision on whether > or > not to continue based on the bus config. > > That would mean that we'd want to make bus_apparmor_pre_init() return void. After looking at this again, I'm happy with the current logic. apparmor_enabled is false is AppArmor isn't enabled in the kernel, if the kernel doesn't support AppArmor D-Bus mediation, or if an error occurred while looking detecting AppArmor support. When the bus config is eventually parsed, the apparmor mode setting in the bus config is compared to apparmor_enabled and the bus exits if the two variables are incompatible. This is sufficient in my opinion. (In reply to Tyler Hicks from comment #96) > (In reply to Simon McVittie from comment #91) > > Comment on attachment 97989 [details] [review] [review] [review] > > [PATCH v3 08/13] Mediation of processes that acquire well-known names > > > > Review of attachment 97989 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > ::: bus/apparmor.c > > @@ +266,5 @@ > > > + return _dbus_string_set_length (query, AA_QUERY_CMD_LABEL_SIZE) && > > > + _dbus_string_append (query, con) && > > > + _dbus_string_append_byte (query, '\0') && > > > + _dbus_string_append_byte (query, AA_CLASS_DBUS) && > > > + _dbus_string_append (query, bustype); > > > > Sorry, I didn't realise this issue until I looked back at the > > eavesdropping-mediation just now: > > > > The bus type might be NULL: if you are doing a bus other than the standard > > pair (system and session), it is valid to omit the <type> from the bus's > > config file altogether. At the moment you'd get an assertion failure on such > > buses, but you could use (bustype ? bustype : "") to circumvent that, if the > > empty string is acceptable. > > > > Similarly, whoever runs dbus-daemon can tell it an arbitrary type of their > > choice. The only constraint is that it's something you can express in XML > > (so no \0 or other low control characters). > > > > Is this a problem for you? > > No. We already have working AppArmor policy for the "accessibility" bus. > > I was unaware of the possibility for a NULL bustype, though. I'll figure out > if NULL or "" is most appropriate for building the query string in this > situation. An empty string, "", is the best option. That'll cause AppArmor rules that specify a bus name from working with buses that don't specify a name. It will also allow AppArmor rules that don't specify a bus name to work with buses that don't specify a name. (In reply to Simon McVittie from comment #87) > We have also been discussing the possibility of having a single element in > GetConnectionCredentials, "LinuxSecurityLabel" or "SecurityLabel" or > something, shared between all LSMs, whose value is defined to be the same > string (or bytestring) that you would get from SO_PEERCRED. This is now implemented, and will be merged soon if there are no objections, so I'm considering this bug to be blocked by Bug #89041. The patch series for that bug already includes your patch 12/13 v3, "New a{sv} helper..." which it uses to attach the generic security label. (In reply to Simon McVittie from comment #75) > The GDBus/GVariant developers encourage protocol designers to encode > bytestrings (strings of unspecified encoding that are guaranteed not to > contain embedded \0) as a byte array that *does* include the trailing \0 in > its length I was worried about whether this would be a compatibility break or not; but IMO, LinuxSecurityLabel has relegated AppArmorContext and AppArmorMode to the same "retained for compatibility with older Ubuntu" status as GetConnectionAppArmorSecurityContext, so I don't intend to merge them upstream, and there is no need to risk breaking compatibility by adding the "\0". I need this patchset for an Ubuntu-derived project myself, so I'm looking into doing a rebase onto dbus master with my review comments fixed. I'll do a re-review as I go. (In reply to Simon McVittie from comment #68) > Comment on attachment 97986 [details] [review] > [PATCH v3 05/13] Initialize AppArmor mediation > > @@ +101,5 @@ > > + !_dbus_string_append (&aa_dbus, "/features/dbus/mask")) > > + goto out; > > + > > + mask_file = open (_dbus_string_get_const_data (&aa_dbus), > > + O_RDONLY | O_CLOEXEC); > > Is it necessary to open it, as opposed to just stat()ing it? According to the Ubuntu changelog, yes it is: * debian/patches/aa-mediation.patch: Attempt to open() the mask file in apparmorfs/features/dbus rather than simply stat() the dbus directory. This is an important difference because AppArmor does not mediate the stat() syscall. This resulted in problems in an environment where dbus-daemon, running inside of an LXC container, did not have the necessary AppArmor rules to access apparmorfs but the stat() succeeded so mediation was not properly disabled. (LP: #1238267) (In reply to Simon McVittie from comment #99) > (In reply to Simon McVittie from comment #87) > > We have also been discussing the possibility of having a single element in > > GetConnectionCredentials, "LinuxSecurityLabel" or "SecurityLabel" or > > something, shared between all LSMs, whose value is defined to be the same > > string (or bytestring) that you would get from SO_PEERCRED. > > This is now implemented, and will be merged soon if there are no objections, > so I'm considering this bug to be blocked by Bug #89041. The patch series > for that bug already includes your patch 12/13 v3, "New a{sv} helper..." > which it uses to attach the generic security label. > > (In reply to Simon McVittie from comment #75) > > The GDBus/GVariant developers encourage protocol designers to encode > > bytestrings (strings of unspecified encoding that are guaranteed not to > > contain embedded \0) as a byte array that *does* include the trailing \0 in > > its length > > I was worried about whether this would be a compatibility break or not; but > IMO, LinuxSecurityLabel has relegated AppArmorContext and AppArmorMode to > the same "retained for compatibility with older Ubuntu" status as > GetConnectionAppArmorSecurityContext, so I don't intend to merge them > upstream, and there is no need to risk breaking compatibility by adding the > "\0". I agree with this approach. But note that I plan to yank AppArmorContext and AppArmorMode out of Ubuntu. I don't know of anything that uses them and don't think that carrying the delta is worth the trouble. There are things that use GetConnectionAppArmorSecurityContext and I'll start migrating them to GetConnectionAppArmorSecurityContext once it is available in Ubuntu and libapparmor has the appropriate API to split the context up. (In reply to Simon McVittie from comment #100) > (In reply to Simon McVittie from comment #68) > > Comment on attachment 97986 [details] [review] [review] > > [PATCH v3 05/13] Initialize AppArmor mediation > > > > @@ +101,5 @@ > > > + !_dbus_string_append (&aa_dbus, "/features/dbus/mask")) > > > + goto out; > > > + > > > + mask_file = open (_dbus_string_get_const_data (&aa_dbus), > > > + O_RDONLY | O_CLOEXEC); > > > > Is it necessary to open it, as opposed to just stat()ing it? > > According to the Ubuntu changelog, yes it is: > > * debian/patches/aa-mediation.patch: Attempt to open() the mask file in > apparmorfs/features/dbus rather than simply stat() the dbus directory. > This is an important difference because AppArmor does not mediate the > stat() syscall. This resulted in problems in an environment where > dbus-daemon, running inside of an LXC container, did not have the > necessary AppArmor rules to access apparmorfs but the stat() succeeded > so mediation was not properly disabled. (LP: #1238267) Thanks for not pointing out that *I* wrote that. I forgot all about that bug so please ignore my response in comment #77. Created attachment 113585 [details] [review] [PATCH v4 14/13] Mediation of processes becoming a monitor Manually tested using the dbus-monitor that was updated to use BecomeMonitor. Created attachment 113586 [details] [review] [FIX FOR 04/13] fix: List copyright holder and remove author names Addresses feedback in comment #67 Created attachment 113587 [details] [review] [FIX FOR 04/13] fix: Drop trailing comma in config parser ElementType enum Addresses feedback in comment #67 Created attachment 113588 [details] [review] [FIX FOR 05/13] fix: fix unreachable return when AppArmor support is built Addresses feedback in comment #68 Created attachment 113589 [details] [review] [FIX FOR 05/13] fix: make bus_apparmor_full_init() able to raise a DBusError Addresses feedback in comment #68 Created attachment 113590 [details] [review] [FIX FOR 06/13] fix: set DBusError if bus context cannot be retrieved Addresses feedback in comment #68 Created attachment 113591 [details] [review] [FIX FOR 06/13] fix: make confinement mode ptr a const and document the semantics Addresses feedback in comment #69 Created attachment 113592 [details] [review] [FIX FOR 08/13] fix: initialize reserved area at the start of the query string Addresses feedback discussed in comment #80 Created attachment 113593 [details] [review] [FIX FOR 08/13] fix: Use empty string for NULL bustypes when building queries Addresses feedback in comment #91 Created attachment 113594 [details] [review] [FIX FOR 10/13] fix: Pass the message type to the AppArmor hook This is a change that prevents us from having to do strcmp() to determine the message type. I realize that I remove the strcmp()'s in this patch but the following patch reintroduces some message type comparisons. Created attachment 113595 [details] [review] [FIX FOR 10/13] fix: Don't audit unrequested reply message denials Addresses Ubuntu bug filed here: https://launchpad.net/bugs/1362469 Comment on attachment 97993 [details] [review] [PATCH v3 12/13] New a{sv} helper for using byte arrays as the variant Obsolete due to this patch living on in bug #89041 and no longer being needed in this bug. Comment on attachment 97994 [details] [review] [PATCH v3 13/13] Add AppArmor support to GetConnectionCredentials Obsolete due to the equivalent functionality being developed in bug #89041. Comment on attachment 113585 [details] [review] [PATCH v4 14/13] Mediation of processes becoming a monitor Review of attachment 113585 [details] [review]: ----------------------------------------------------------------- ::: bus/driver.c @@ +1940,5 @@ > if (!bus_driver_check_message_is_for_us (message, error)) > goto out; > > + context = bus_transaction_get_context (transaction); > + bustype = context ? bus_context_get_type (context) : NULL; I don't think the transaction's context can ever legitimately be NULL, but this is fine too. Comment on attachment 113586 [details] [review] [FIX FOR 04/13] fix: List copyright holder and remove author names Review of attachment 113586 [details] [review]: ----------------------------------------------------------------- fine Comment on attachment 113587 [details] [review] [FIX FOR 04/13] fix: Drop trailing comma in config parser ElementType enum Review of attachment 113587 [details] [review]: ----------------------------------------------------------------- yes Comment on attachment 113588 [details] [review] [FIX FOR 05/13] fix: fix unreachable return when AppArmor support is built Review of attachment 113588 [details] [review]: ----------------------------------------------------------------- yes Comment on attachment 113589 [details] [review] [FIX FOR 05/13] fix: make bus_apparmor_full_init() able to raise a DBusError Review of attachment 113589 [details] [review]: ----------------------------------------------------------------- sure Comment on attachment 113590 [details] [review] [FIX FOR 06/13] fix: set DBusError if bus context cannot be retrieved Review of attachment 113590 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +248,5 @@ > bus_con = bus_apparmor_confinement_new (context, mode); > if (bus_con == NULL) > { > + dbus_set_error (error, DBUS_ERROR_FAILED, > + "Error allocating bus AppArmor confinement\n"); Can this happen for any reason other than OOM? If it can't (and as far as I can see, it can't), BUS_SET_OOM (error) is better. Comment on attachment 113590 [details] [review] [FIX FOR 06/13] fix: set DBusError if bus context cannot be retrieved Review of attachment 113590 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +239,5 @@ > { > if (aa_getcon (&context, &mode) == -1) > { > + dbus_set_error (error, DBUS_ERROR_FAILED, > + "Error getting AppArmor context of bus: %s\n", Very minor, but DBusError messages shouldn't normally end with a newline: anything that prints one is expected to do that itself, if necessary. Comment on attachment 113591 [details] [review] [FIX FOR 06/13] fix: make confinement mode ptr a const and document the semantics Review of attachment 113591 [details] [review]: ----------------------------------------------------------------- ok Comment on attachment 113592 [details] [review] [FIX FOR 08/13] fix: initialize reserved area at the start of the query string Review of attachment 113592 [details] [review]: ----------------------------------------------------------------- This is much more elegant than the version I had :-) Comment on attachment 113593 [details] [review] [FIX FOR 08/13] fix: Use empty string for NULL bustypes when building queries Review of attachment 113593 [details] [review]: ----------------------------------------------------------------- OK Comment on attachment 113594 [details] [review] [FIX FOR 10/13] fix: Pass the message type to the AppArmor hook Review of attachment 113594 [details] [review]: ----------------------------------------------------------------- OK Comment on attachment 113595 [details] [review] [FIX FOR 10/13] fix: Don't audit unrequested reply message denials Review of attachment 113595 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +857,5 @@ > + * 2) The message is a reply type. Reply message are not audited because > + * the AppArmor policy language does not have the notion of a reply > + * message. Unrequested replies will be silently discarded if the sender > + * does not have permission to send to the receiver or if the receiver > + * does not have permission to receive from the sender. If this is the security model you want, that's fine from my point of view, but please be clear about the consequences: This means that if a D-Bus library is slightly buggy, and sends a reply to a method call that should have had NO_REPLY, it will not spam the audit log. That's a good thing, and presumably what you wanted to achieve. It also means that if a malicious client sends unsolicited "replies" (that are not actually a reply to anything at all) in an attempt to perform some attack (perhaps a DoS or a failed attempt to get an unmonitored side-channel), they will be rejected correctly, but the rejection will be silent - nothing in the audit log. That's potentially undesired, depending on your security model. Please confirm that you are OK with that happening. Created attachment 113602 [details] [review] [fix for 03] fix: configure.ac: avoid potential non-portability from "test EXPR -a EXPR" --- It's a GNUism, albeit a common one. Created attachment 113603 [details] [review] [fix for 05] fix: _bus_apparmor_aa_supports_dbus: document necessary kernel API guarantee Created attachment 113604 [details] [review] [fix for 05] fix: bus_apparmor_pre_init: distinguish between OOM and AppArmor not enabled Created attachment 113605 [details] [review] [fix for 05] fix: document why we open() and not just stat() Created attachment 113606 [details] [review] [fix for 06] fix: use BUS_SET_OOM Created attachment 113607 [details] [review] [fix for 06] fix: dbus_set_error doesn't need extra newlines Created attachment 113608 [details] [review] [06] Store AppArmor label of connecting processes From: Tyler Hicks <tyhicks@canonical.com> When processes connect the bus, the AppArmor confinement context should be stored for later use when checks are to be done during message sending/receiving, acquire a name, and eavesdropping. Code outside of apparmor.c will need to initialize and unreference the confinement context, so bus_apparmor_confinement_unref() can no longer be a static function. [Move bus_apparmor_confinement_unref back to its old location for a more reasonable diff -smcv] --- Not sure whether this is strictly necessary, but it's less diffstat, and I had to rebase some of this stuff anyway to deal with minor fuzz on the patches. Created attachment 113609 [details] [review] [15] apparmor: tighten up terminology for context vs. label vs. profile The thing returned by SO_PEERSEC (which we're calling LinuxSecurityLabel within D-Bus) can have a different meaning for each LSM. In AppArmor, according to discussion on the linux-security-module list, it's the AppArmor context, which is made up of an AppArmor label and an optional confinement mode; the label further subdivides into one or more profiles. In practice, the part that dbus-daemon wants is the label, and occasionally also the mode. --- I hope I got all this right... Created attachment 113610 [details] [review] [not for upstream] Add DBus method to return the AA context of a connection This is not intended for upstream inclusion. It implements a bus method (GetConnectionAppArmorSecurityContext) to get a connection's AppArmor security context but upstream D-Bus has recently added a generic way of getting a connection's security credentials (GetConnectionCredentials). Ubuntu should carry this patch until packages in the archive are moved over to the new, generic method of getting a connection's credentials. [Altered by Simon McVittie: survive non-UTF-8 contexts which would otherwise be a local denial of service, except that Ubuntu inherits a non-fatal warnings patch from Debian; new commit message taken from the Ubuntu changelog; do not emit unreachable code if AppArmor is disabled.] --- I expect you're going to want this for Ubuntu, and I need to build an Ubuntu-compatible package of dbus master to be able to run Ubuntu's AppArmor regression tests, so... Complete branch here: http://cgit.freedesktop.org/~smcv/dbus/log?h=apparmor-75113 More rebasing is possible, I haven't tested it on real AppArmor systems yet. My intention is that after review/testing, all "fix" patches (from either of us) will be squashed into the commit they're based on, either by you or by me. Comment on attachment 113602 [details] [review] [fix for 03] fix: configure.ac: avoid potential non-portability from "test EXPR -a EXPR" Review of attachment 113602 [details] [review]: ----------------------------------------------------------------- looks good (In reply to Simon McVittie from comment #137) > More rebasing is possible, I haven't tested it on real AppArmor systems yet. It seems good. Specifically, on an Ubuntu 14.04 (utopic) VM, it seems to pass the AppArmor and QRT tests that you linked back in Comment #0, with the exception that "sudo make check" fails during the QRT tests because root can't access the ordinary user's bus - but that isn't a regression, 1.8.8-1ubuntu2.1 has the same failure. Are there any newer tests for this stuff? I looked and couldn't find any, but I know Canonical/Ubuntu has been focusing on automated testing recently, so perhaps there are some autopkgtest cases that I didn't find? The specific package I tested, in case it's useful to you: dget http://people.collabora.com/~smcv/expires-201504/dbus_1.9.10-2ubuntu~smcv1.dsc I'll squash the fixes into the commits and do another review pass; hopefully the result will be something that, if you're happy to merge it, so am I. Once this gets merged, your options for Ubuntu will be to either base 15.04 on 1.8.x with a backport of this patch series, or upgrade to 1.9.x and ask me for a 1.10 stable branch with plenty of time to switch to it before you freeze. I'm happy to do new stable branches on request (with any reasonable time gap between them), but I am not willing to security-support a random snapshot of 1.9.x, so please don't ship 1.9.x in a stable release. Comment on attachment 113603 [details] [review] [fix for 05] fix: _bus_apparmor_aa_supports_dbus: document necessary kernel API guarantee Review of attachment 113603 [details] [review]: ----------------------------------------------------------------- I'll remind him about this and help to make sure it happens. Looks good. Comment on attachment 113604 [details] [review] [fix for 05] fix: bus_apparmor_pre_init: distinguish between OOM and AppArmor not enabled Review of attachment 113604 [details] [review]: ----------------------------------------------------------------- Nice cleanup! Looks good. Comment on attachment 113605 [details] [review] [fix for 05] fix: document why we open() and not just stat() Review of attachment 113605 [details] [review]: ----------------------------------------------------------------- Good idea to document this. Looks good. Comment on attachment 113606 [details] [review] [fix for 06] fix: use BUS_SET_OOM Review of attachment 113606 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 113607 [details] [review] [fix for 06] fix: dbus_set_error doesn't need extra newlines Review of attachment 113607 [details] [review]: ----------------------------------------------------------------- Thanks for fixing this. Looks good. Comment on attachment 113608 [details] [review] [06] Store AppArmor label of connecting processes Review of attachment 113608 [details] [review]: ----------------------------------------------------------------- Thanks for the cleanup. Looks good. Comment on attachment 113609 [details] [review] [15] apparmor: tighten up terminology for context vs. label vs. profile Review of attachment 113609 [details] [review]: ----------------------------------------------------------------- ::: bus/apparmor.c @@ +707,4 @@ > !_dbus_append_pair_uint (&auxdata, "pid", pid)) > goto oom; > > + if (con->label && !_dbus_append_pair_str (&auxdata, "label", con->label)) I can see that changing "profile" to "label" here could conceivably break third-party code - I'm not familiar enough with AppArmor to know how this is used - so I'd be fine with reverting that part of this change, preferably with an explanatory comment (but I'm afraid I don't know AppArmor well enough yet to write that comment myself). @@ +934,5 @@ > !_dbus_append_pair_uint (&auxdata, "pid", pid)) > goto oom; > > + if (src_con->label && > + !_dbus_append_pair_str (&auxdata, "label", src_con->label)) same @@ +943,5 @@ > !_dbus_append_pair_uint (&auxdata, "peer_pid", pid)) > goto oom; > > + if (dst_con->label && > + !_dbus_append_pair_str (&auxdata, "peer_label", dst_con->label)) same @@ +971,5 @@ > !_dbus_append_pair_uint (&auxdata, "pid", pid)) > goto oom; > > + if (dst_con->label && > + !_dbus_append_pair_str (&auxdata, "label", dst_con->label)) same @@ +979,5 @@ > !_dbus_append_pair_uint (&auxdata, "peer_pid", pid)) > goto oom; > > + if (src_con->label && > + !_dbus_append_pair_str (&auxdata, "peer_label", src_con->label)) same @@ +1102,4 @@ > !_dbus_append_pair_uint (&auxdata, "pid", pid)) > goto oom; > > + if (con->label && !_dbus_append_pair_str (&auxdata, "label", con->label)) same Comment on attachment 113609 [details] [review] [15] apparmor: tighten up terminology for context vs. label vs. profile Review of attachment 113609 [details] [review]: ----------------------------------------------------------------- It is worth noting that I've tried to address the confusion around context, label, and mode in the AppArmor project, as well. The patches in this thread have all been merged into trunk: https://lists.ubuntu.com/archives/apparmor/2015-February/007181.html ::: bus/apparmor.c @@ +75,1 @@ > const char *mode; /* AppArmor confinement mode (freed by freeing *context) */ This line should be changed to: const char *mode; /* AppArmor confinement mode (freed by freeing *label) */ (In reply to Tyler Hicks from comment #147) > It is worth noting that I've tried to address the confusion around context, > label, and mode in the AppArmor project, as well. Yes, thanks for doing that! This patch was my attempt at doing the same - I hope I've understood the terms well enough to be making the same changes you did :-) (In reply to Tyler Hicks from comment #147) > This line should be changed to: > > const char *mode; /* AppArmor confinement mode (freed by freeing *label) */ Yes, you're right. I'll wait til you've been through all my fixes, then do all the additional changes as a batch. (In reply to Simon McVittie from comment #149) > I'll wait til you've been through all my fixes Which, er, you have. Any opinion on "label", "peer_label" vs. "profile", "peer_profile"? I think the conservative thing to do at this point would be to not change them, and leave them like they are in deployed Ubuntu systems. (In reply to Simon McVittie from comment #150) > (In reply to Simon McVittie from comment #149) > > I'll wait til you've been through all my fixes > > Which, er, you have. Any opinion on "label", "peer_label" vs. "profile", > "peer_profile"? > > I think the conservative thing to do at this point would be to not change > them, and leave them like they are in deployed Ubuntu systems. I've discussed this with John and we both agree that switching to label and peer_label is the correct thing to do. Some of the AppArmor kernel code that John is in the process of upstreaming now uses "label" so third party tools are already in need of being updated. The only known third party tools that would care are the audit userspace utilities and the log parsing capabilities in libapparmor. The audit userspace utilities don't currently parse AppArmor denials (https://launchpad.net/bugs/1117804) so no worries there. The libapparmor audit log parsing code doesn't know about "label", so I've created a bug (https://launchpad.net/bugs/1423270) to get that fixed. (In reply to Simon McVittie from comment #136) > Created attachment 113610 [details] [review] [review] > [not for upstream] Add DBus method to return the AA context of a connection > I expect you're going to want this for Ubuntu, and I need to build an > Ubuntu-compatible package of dbus master to be able to run Ubuntu's AppArmor > regression tests, so... Nice catch. Thanks! (In reply to Simon McVittie from comment #127) > Comment on attachment 113595 [details] [review] [review] > [FIX FOR 10/13] fix: Don't audit unrequested reply message denials > > Review of attachment 113595 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: bus/apparmor.c > @@ +857,5 @@ > > + * 2) The message is a reply type. Reply message are not audited because > > + * the AppArmor policy language does not have the notion of a reply > > + * message. Unrequested replies will be silently discarded if the sender > > + * does not have permission to send to the receiver or if the receiver > > + * does not have permission to receive from the sender. > > If this is the security model you want, that's fine from my point of view, > but please be clear about the consequences: > > This means that if a D-Bus library is slightly buggy, and sends a reply to a > method call that should have had NO_REPLY, it will not spam the audit log. > That's a good thing, and presumably what you wanted to achieve. > > It also means that if a malicious client sends unsolicited "replies" (that > are not actually a reply to anything at all) in an attempt to perform some > attack (perhaps a DoS or a failed attempt to get an unmonitored > side-channel), they will be rejected correctly, but the rejection will be > silent - nothing in the audit log. That's potentially undesired, depending > on your security model. Please confirm that you are OK with that happening. I've spoken with John Johansen (AA kernel maintainer and original prototyper of this patch set) and Jamie Strandboge (AA userspace upstream and has written a lot of AA policy usinging dbus rules) about this one last time. We are all in agreement that this is the right approach. Thanks for double checking. (In reply to Simon McVittie from comment #149) > Yes, you're right. I'll wait til you've been through all my fixes, then do > all the additional changes as a batch. I think I've been through all of your fixes at this mount. (In reply to Tyler Hicks from comment #154) > (In reply to Simon McVittie from comment #149) > > Yes, you're right. I'll wait til you've been through all my fixes, then do > > all the additional changes as a batch. > > I think I've been through all of your fixes at this mount. s/mount/point/ The muscle memory problems of an OS developer... (In reply to Simon McVittie from comment #139) > (In reply to Simon McVittie from comment #137) > > More rebasing is possible, I haven't tested it on real AppArmor systems yet. > > It seems good. Specifically, on an Ubuntu 14.04 (utopic) VM, it seems to > pass the AppArmor and QRT tests that you linked back in Comment #0, with the > exception that "sudo make check" fails during the QRT tests because root > can't access the ordinary user's bus - but that isn't a regression, > 1.8.8-1ubuntu2.1 has the same failure. I'll run it through the tests on Vivid (the current development release). > > Are there any newer tests for this stuff? I looked and couldn't find any, > but I know Canonical/Ubuntu has been focusing on automated testing recently, > so perhaps there are some autopkgtest cases that I didn't find? The AppArmor and QRT tests are the latest and greatest for this code. > The specific package I tested, in case it's useful to you: > > dget > http://people.collabora.com/~smcv/expires-201504/dbus_1.9.10-2ubuntu~smcv1. > dsc Thanks! > I'll squash the fixes into the commits and do another review pass; hopefully > the result will be something that, if you're happy to merge it, so am I. Sounds great. > Once this gets merged, your options for Ubuntu will be to either base 15.04 > on 1.8.x with a backport of this patch series, or upgrade to 1.9.x and ask > me for a 1.10 stable branch with plenty of time to switch to it before you > freeze. I'm happy to do new stable branches on request (with any reasonable > time gap between them), but I am not willing to security-support a random > snapshot of 1.9.x, so please don't ship 1.9.x in a stable release. Feature freeze for Vivid (15.04) is tomorrow and I want to sleep tonight so I don't plan on upgrading to 1.9.x. I'll backport this patch series to 1.8.x. Created attachment 113625 [details] [review] fix for 10: when AA denies sending, label requested_reply as such, not as "matched rules" This appears to be copypasta from complain_about_message(), which is sometimes complaining about things rejected by dbus-daemon's own security policy language (hence "n matched rules"). The types coincidentally worked. Comment on attachment 113625 [details] [review] fix for 10: when AA denies sending, label requested_reply as such, not as "matched rules" Review of attachment 113625 [details] [review]: ----------------------------------------------------------------- Looks good. Created attachment 113628 [details] [review] [01/14] Document AppArmor enforcement in the dbus-daemon man page Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Created attachment 113629 [details] [review] [02/14] Add apparmor element and attributes to the bus config dtd From: Tyler Hicks <tyhicks@canonical.com> Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Created attachment 113630 [details] [review] [03/14] Update autoconf file to build against libapparmor From: Tyler Hicks <tyhicks@canonical.com> AppArmor support can be configured at build time with --enable-apparmor and --disable-apparmor. By default, the build time decision is automatically decided by checking if a sufficient libapparmor is available. A minimum required libapparmor is version 2.8.95. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 Signed-off-by: Tyler Hicks <tyhicks@canonical.com> [smcv: avoid potential non-portability from "test EXPR -a EXPR"] Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Reviewed-by: Tyler Hicks <tyhicks@canonical.com> Created attachment 113631 [details] [review] [04/14] Add apparmor element support to bus config parsing From: Tyler Hicks <tyhicks@canonical.com> The <apparmor> element can contain a single mode attribute that has one of three values: "enabled" "disabled" "required" "enabled" means that kernel support is autodetected and, if available, AppArmor mediation occurs in dbus-daemon. If kernel support is not detected, mediation is disabled. "disabled" means that mediation does not occur. "required" means that kernel support must be detected for dbus-daemon to start. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Created attachment 113632 [details] [review] [05/14] Initialize AppArmor mediation From: John Johansen <john.johansen@canonical.com> When starting dbus-daemon, autodetect AppArmor kernel support and use the results from parsing the busconfig to determine if mediation should be enabled. In the busconfig, "enabled" means that kernel support is autodetected and, if available, AppArmor mediation occurs in dbus-daemon. In "enabled" mode, if kernel support is not detected, mediation is disabled. "disabled" means that mediation does not occur. "required" means that kernel support must be detected for dbus-daemon to start. Additionally, when libaudit support is built into dbus-daemon, the AppArmor initialization routines set up the audit connection. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 Signed-off-by: John Johansen <john.johansen@canonical.com> [tyhicks: Honor enforcement modes and detect AppArmor dbus rule support] [tyhicks: fix unreachable return when AppArmor support is built] [tyhicks: make bus_apparmor_full_init() able to raise a DBusError] Signed-off-by: Tyler Hicks <tyhicks@canonical.com> [smcv: _bus_apparmor_aa_supports_dbus: document necessary kernel API guarantee] [smcv: bus_apparmor_pre_init: distinguish between OOM and AppArmor not enabled] [smcv: document why we open() and not just stat()] Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Reviewed-by: Tyler Hicks <tyhicks@canonical.com> Created attachment 113633 [details] [review] [06/14] Store AppArmor label of bus during initialization From: Tyler Hicks <tyhicks@canonical.com> During dbus-daemon initialization, the AppArmor confinement context should be stored for later use when checks are to be done on messages to/from the bus itself. AppArmor confinement contexts are documented in aa_getcon(2). They contain a confinement string and a mode string. The confinement string is typically the name of the AppArmor profile confining a given process. The mode string gives the current enforcement mode of the process confinement. For example, it may indicate that the confinement should be enforced or it may indicate that the confinement should allow all actions with the caveat that actions which would be denied should be audited. It is important to note that libapparmor mallocs a single buffer to store the con and mode strings and separates them with a NUL terminator. Because of this, only con should be freed. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 Signed-off-by: Tyler Hicks <tyhicks@canonical.com> [smcv: use BUS_SET_OOM] [smcv: dbus_set_error doesn't need extra newlines] Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Reviewed-by: Tyler Hicks <tyhicks@canonical.com> Created attachment 113634 [details] [review] [07/14] Store AppArmor label of connecting processes From: Tyler Hicks <tyhicks@canonical.com> When processes connect the bus, the AppArmor confinement context should be stored for later use when checks are to be done during message sending/receiving, acquire a name, and eavesdropping. Code outside of apparmor.c will need to initialize and unreference the confinement context, so bus_apparmor_confinement_unref() can no longer be a static function. [Move bus_apparmor_confinement_unref back to its old location for a more reasonable diff -smcv] Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Created attachment 113635 [details] [review] [08/14] Mediation of processes that acquire well-known names From: John Johansen <john.johansen@canonical.com> hen an AppArmor confined process wants to acquire a well-known name, a check is performed to see if the action should be allowed. The check is based on the connection's label, the bus type, and the name being requested. An example AppArmor rule that would allow the name "com.example.ExampleName" to be acquired on the system bus would be: dbus bind bus=system name=com.example.ExampleName, To let a process acquire any name on any bus, the rule would be: dbus bind, Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 Signed-off-by: John Johansen <john.johansen@canonical.com> [tyhicks: Use BusAppArmorConfinement, bug fixes, cleanup, commit msg] [tyhicks: initialize reserved area at the start of the query string] [tyhicks: Use empty string for NULL bustypes when building queries] Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Created attachment 113636 [details] [review] [09/14] Do LSM checks after determining if the message is a requested reply From: Tyler Hicks <tyhicks@canonical.com> Move the call to bus_selinux_allows_send() after the call to bus_connections_check_reply(). This allows LSMs to know if the message is a reply and whether or not it was requested. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Created attachment 113637 [details] [review] [10/14] Mediation of processes sending and receiving messages From: John Johansen <john.johansen@canonical.com> When an AppArmor confined process wants to send or receive a message, a check is performed to see if the action should be allowed. When a message is going through dbus-daemon, there are two checks performed at once. One for the sending process and one for the receiving process. The checks are based on the process's label, the bus type, destination, path, interface, and member, as well as the peer's label and/or destination name. This allows for the traditional connection-based enforcement, as well as any fine-grained filtering desired by the system administrator. It is important to note that error and method_return messages are allowed to cut down on the amount of rules needed. If a process was allowed to send a message, it can receive error and method_return messages. An example AppArmor rule that would be needed to allow a process to call the UpdateActivationEnvironment method of the session bus itself would be: dbus send bus=session path=/org/freedesktop/DBus interface=org.freedesktop.DBus member=UpdateActivationEnvironment peer=(name=org.freedesktop.DBus), To receive any message on the system bus from a process confined by the "confined-client" AppArmor profile: dbus receive bus=system peer=(label=confined-client), Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 Signed-off-by: John Johansen <john.johansen@canonical.com> [tyhicks: Use BusAppArmorConfinement, bug fixes, cleanup, commit msg] [tyhicks: Pass the message type to the AppArmor hook] [tyhicks: Don't audit unrequested reply message denials] Signed-off-by: Tyler Hicks <tyhicks@canonical.com> [smcv: when AA denies sending, don't label requested_reply as "matched rules"] Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Reviewed-by: Tyler Hicks <tyhicks@canonical.com> Created attachment 113638 [details] [review] [11/14] Mediation of processes eavesdropping From: Tyler Hicks <tyhicks@canonical.com> When an AppArmor confined process wants to eavesdrop on a bus, a check is performed to see if the action should be allowed. The check is based on the connection's label and the bus type. This patch adds a new hook, which was not previously included in the SELinux mediation, to mediate eavesdropping from bus_driver_handle_add_match(). A new function is added to bus/signals.c to see if a match rule is an eavesdropping rule since the rule flags field is private to signals.c. An example AppArmor rule that would allow a process to eavesdrop on the session bus would be: dbus eavesdrop bus=session, Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Created attachment 113639 [details] [review] [12/14] Mediation of processes becoming a monitor From: Tyler Hicks <tyhicks@canonical.com> When an AppArmor confined process wants to become a monitor, a check is performed to see if eavesdropping should be allowed. The check is based on the connection's label and the bus type. This patch reuses the bus_apparmor_allows_eavesdropping() hook. An example AppArmor rule that would allow a process to become a monitor on the system bus would be: dbus eavesdrop bus=system, Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 Signed-off-by: Tyler Hicks <tyhicks@canonical.com> Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Created attachment 113640 [details] [review] [13/14] apparmor: tighten up terminology for context vs. label vs. profile The thing returned by SO_PEERSEC (which we're calling LinuxSecurityLabel within D-Bus) can have a different meaning for each LSM. In AppArmor it's the AppArmor context, which is made up of an AppArmor label and an optional confinement mode; the label further subdivides into one or more profiles. See https://bazaar.launchpad.net/~apparmor-dev/apparmor/master/revision/2862 and subsequent commits for recent clarification of this terminology. In practice, the part that dbus-daemon deals with is the label, and occasionally also the mode. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=75113 --- This still needs review, but is just Attachment #113609 [details] with the comment correction you suggested. Created attachment 113641 [details] [review] [14/14, not for upstream] Add DBus method to return the AA context of a connection This is not intended for upstream inclusion. It implements a bus method (GetConnectionAppArmorSecurityContext) to get a connection's AppArmor security context but upstream D-Bus has recently added a generic way of getting a connection's security credentials (GetConnectionCredentials). Ubuntu should carry this patch until packages in the archive are moved over to the new, generic method of getting a connection's credentials. [Altered by Simon McVittie: survive non-UTF-8 contexts which would otherwise be a local denial of service, except that Ubuntu inherits a non-fatal warnings patch from Debian; new commit message taken from the Ubuntu changelog; do not emit unreachable code if AppArmor is disabled.] --- Not planning to merge this one. http://cgit.freedesktop.org/~smcv/dbus/log?h=apparmor-75113 updated. That patch series, minus the last commit, is what I intend to merge into master; I'll drop the last commit and hopefully mark 13/14 as Reviewed-by you when I do so. > The AppArmor and QRT tests are the latest and greatest for this code. Re-tested with: dget \ http://people.collabora.com/~smcv/expires-201504/dbus_1.9.10-2ubuntu~smcv3.dsc Same results. Comment on attachment 113640 [details] [review] [13/14] apparmor: tighten up terminology for context vs. label vs. profile Review of attachment 113640 [details] [review]: ----------------------------------------------------------------- Looks good. (In reply to Simon McVittie from comment #173) > http://cgit.freedesktop.org/~smcv/dbus/log?h=apparmor-75113 updated. That > patch series, minus the last commit, is what I intend to merge into master; > I'll drop the last commit and hopefully mark 13/14 as Reviewed-by you when I > do so. > > > The AppArmor and QRT tests are the latest and greatest for this code. > > Re-tested with: > > dget \ > http://people.collabora.com/~smcv/expires-201504/dbus_1.9.10-2ubuntu~smcv3. > dsc > > Same results. Re-tested in Vivid. Same results. Also tested with the unrequested replies test case (https://bugs.launchpad.net/ubuntu/+source/dbus/+bug/1362469/comments/5) and verified that they are not audited. Also manually tested the new dbus-monitor to verify that it falls back to eavesdropping and that an eavesdrop denial occurs when confined with this profile: $ echo "profile nomonitor { file, unix, dbus (send,receive), }" | \ sudo apparmor_parser -qr $ sudo aa-exec -p nomonitor -- dbus-monitor --system dbus-monitor: unable to enable new-style monitoring: org.freedesktop.DBus.Error.AccessDenied: "Connection ":1.58" is not allowed to eavesdrop due to AppArmor policy". Falling back to eavesdropping. ... Confining it with the following profile allows BecomeMonitor to succeed: $ echo "profile monitor { file, unix, dbus (send,receive,eavesdrop), }" | \ sudo apparmor_parser -qr Everything is working as expected. (In reply to Tyler Hicks from comment #175) > Everything is working as expected. Fixed in git for 1.9.12 \o/ This breaks the build without apparmor: http://build.gnome.org/continuous/buildmaster/builds/2015/02/18/73/build/log-dbus-bootstrap.txt I took a quick look at the code, but it's a pretty hairy mess of #ifdef, and I don't have a testing environment with apparmor. Please revert https://git.gnome.org/browse/gnome-continuous/commit/?id=ee2dc07149c726a8356b2dbf7fd8306149c8b36c when this is fixed. (In reply to Colin Walters from comment #177) > This breaks the build without apparmor: > > http://build.gnome.org/continuous/buildmaster/builds/2015/02/18/73/build/log- > dbus-bootstrap.txt @Simon: We found the reason why I inexplicably moved bus_apparmor_confinement_unref() in one of the v3 patches. Build testing a fix for this. Will attach it shortly. Created attachment 113646 [details] [review] [PATCH] apparmor: Fix build failure with --disable-apparmor Build tested all four possible combinations of --enable-selinux/--disable-selinux and --enable-apparmor/--disable-apparmor. Build failure (should be) fixed in git for 1.9.12, thanks. |
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.