Created attachment 58746 [details] [review]
Add Smack keyword to policy enforcement

Discussion here:

http://lists.freedesktop.org/archives/dbus/2012-January/014904.html

Comment on attachment 58745 [details] [review]
Add method to check smack context

Review of attachment 58745 [details] [review]:
-----------------------------------------------------------------

::: bus/driver.c
@@ +39,4 @@
>  #include <dbus/dbus-marshal-recursive.h>
>  #include <string.h>
>  
> +

Please don't randomly change whitespace in an unrelated commit.

@@ +1740,5 @@
>      bus_driver_handle_get_id },
> +  { "GetConnectionSmackContext",
> +    DBUS_TYPE_STRING_AS_STRING,
> +    DBUS_TYPE_STRING_AS_STRING,
> +    bus_smack_handle_get_connection_context },

This is putting the method in the main org.freedesktop.DBus interface. I know the SELinux context is there already, but I think that's probably a mistake - we should keep the SELinux one where it is for the moment, but future platform-specifics should be separate, IMO.

I'd prefer it to be in its own interface (o.fd.D.Smack, or in a DNS namespace controlled by the Smack project). Ideally, that entire interface would be absent if Smack is disabled.

(See the Stats interface in dbus master (1.5.x) for an example of how multiple interfaces work, and how to make them not be there if disabled.)

Ideally, this should be documented in the D-Bus Specification (part of the same source tree). From my point of view, that moves from "ideally" to "hard requirement" if it stays in o.fd.DBus.

::: bus/smack.c
@@ +35,5 @@
> +#endif
> +
> +#ifdef DBUS_ENABLE_SMACK
> +static char *
> +bus_smack_get_label (DBusConnection *connection)

I feel as though this method ought to "raise" a DBusError, rather than its caller having to guess which of the two possible error cases happened.

@@ +43,5 @@
> +
> +  if (!dbus_connection_get_socket(connection, &sock_fd))
> +    return NULL;
> +
> +  if (smack_new_label_from_socket(sock_fd, &label) < 0)

Suppose I write a malicious program like this (pseudocode):

conn = dbus_bus_get ()
conn.call_method_async("SomethingOnlyRootCanDo")
unset_flag (conn.get_fd(), CLOEXEC)
/* any setuid program will do */
exec ("/bin/ping", "example.com")

and the implementor of SomethingOnlyRootCanDo uses GetConnectionUnixUserID (directly, or via PolicyKit) to see whether they're root or not.

If D-Bus used an equivalent of smack_new_label_from_socket() on-demand to get the Unix uid, there'd be a race condition: if the malicious process can exec ping quickly enough, when dbus-daemon asks "who is at the other end of this socket?", the kernel will reply "root" and the privileged action takes place.

That's one of the reasons we don't do that for uids: we get the process's uid at the beginning of the connection, and remember it. (The SELinux code appears to do the same for SELinux contexts.)

This does mean that processes which drop privilege have to be careful not to keep a privileged D-Bus connection open, but processes which will drop privilege always have to be careful, so...

Are you vulnerable to an equivalent race here?

@@ +109,5 @@
> +  if (!bus_transaction_send_from_driver (transaction, connection, reply))
> +    goto oom;
> +
> +  dbus_message_unref (reply);
> +  dbus_free(label);

Always use matching free functions. I doubt libsmack calls dbus_new or dbus_malloc or whatever, so you should use whatever free-function libsmack tells you to use (possibly just libc free()).

(dbus_free() isn't necessarily just free() - a different malloc/free pair can be set up. For instance, debug builds do things differently, with guard bytes at the beginning and end of allocations and so on.)

@@ +120,5 @@
> +err:
> +  if (reply != NULL)
> +    dbus_message_unref (reply);
> +
> +  dbus_free(label);

As above.

::: configure.ac
@@ +188,5 @@
>        [Define to build test code into the library and binaries])
>  fi
>  
> +# call early to ensure availability
> +PKG_PROG_PKG_CONFIG

I thought I'd fixed this already, which version are you basing your patch on? But yes, moving this earlier is correct.

Comment on attachment 58746 [details] [review]
Add Smack keyword to policy enforcement

Review of attachment 58746 [details] [review]:
-----------------------------------------------------------------

(This is a low-level implementation review, not a high-level design review - I'll get to that in a bit.)

::: bus/config-parser.c
@@ +68,5 @@
> +      union
> +      {
> +        unsigned long gid_uid_or_at_console;
> +        char *smack_label;
> +      };

I'm pretty sure transparent unions aren't C89 (which we're stuck with until/unless people stop caring about MSVC++), so this union needs a name. Sorry.

If you're going to have to alter all uses from d.policy.gid_uid_or_at_console to d.policy.d.gid_uid_or_at_console anyway, please split up that horrible name into separate fields, so that it's d.policy.d.gid or whatever and we can stop typing all that :-)

Alternatively, you could un-union it: I think this is being union'd with something larger anyway, so it would be no problem to make these members occupy distinct memory?

@@ +1001,5 @@
>                                NULL))
>          return FALSE;
>  
> +      if (((context != NULL) + (user != NULL) + (group != NULL) +
> +          (smack != NULL) + (at_console != NULL)) != 1)

I endorse this refactoring. If you want to apply this change (without the addition of the smack bit) as a prior patch, I'll happily review and merge it straight away.

::: bus/policy.c
@@ +130,5 @@
> +  DBusList *default_rules;             /**< Default policy rules */
> +  DBusList *mandatory_rules;           /**< Mandatory policy rules */
> +  DBusHashTable *rules_by_uid;         /**< per-UID policy rules */
> +  DBusHashTable *rules_by_gid;         /**< per-GID policy rules */
> +  DBusHashTable *rules_by_smack_label; /**< per-SMACK label policy rules */

Either this should be #ifdef'd, or the comment should say "... or NULL if disabled".

@@ +612,5 @@
>  
> +#ifdef DBUS_ENABLE_SMACK
> +static DBusList **
> +get_list_string (DBusHashTable *table,
> +                 const char *key)

I don't think "get_list_string" is a good name for any function, particularly one with no doc-comment.

By inspection, this appears to be:

/*
 * Return a list head representing table[key],
 * creating it if it does not already exist.
 */

and I think it should be called ensure_list_for_key or something.

@@ +1372,4 @@
>  {
>    DBusList *link;
>    dbus_bool_t allowed;
> +

Please don't randomly change whitespace in lines not adjacent to your changes, however much it might offend you.

(I'd accept patches that deleted trailing whitespace and nothing else, though... trailing whitespace offends me too.)

::: bus/smack.c
@@ +36,5 @@
>  #endif
>  
> +#define SMACK_WRITE "W"
> +#define SMACK_READ "R"
> +#define SMACK_READ_WRITE "RW"

Are these magic tokens from libsmack, or something you invented?

If they're magic tokens from libsmack, please include its appropriate header. If it doesn't have an appropriate header, it probably should.

If they're something you invented for D-Bus, I'd prefer a 3-member enum. You could make the values SMACK_WRITE = (1 << 0), SMACK_READ = (1 << 1), SMACK_READ_WRITE = SMACK_WRITE|SMACK_READ if you want.

@@ +157,5 @@
> + */
> +DBusList**
> +bus_smack_generate_allowed_list (DBusConnection *connection,
> +                                 DBusHashTable  *rules_by_smack_label,
> +                                 dbus_bool_t *nomem_err)

I'd prefer this signature:

/* *append_here must be initialized on entry, usually to NULL */
dbus_bool_t
bus_smack_generate_allowed_list (DBusConnection  *connection,
                                 DBusHashTable   *rules_by_smack_label,
                                 DBusList       **append_here)

Then you'd call it like this:

DBusList *list = NULL;

if (!bus_smack_generate_allowed_list (connection, rules, &list))
  goto oom;

do_something_with_the_list (list);
_dbus_list_clear (&list);
/* now list == NULL again */

and not leak a dbus_malloc'd DBusList *. At the moment, the caller of this function leaks the DBusList *, and so does the error-unwinding code in this function.

@@ +189,5 @@
> +      link = _dbus_list_get_first_link (list);
> +      while (link != NULL)
> +        {
> +          BusPolicyRule *rule = link->data;
> +          link = _dbus_list_get_next_link (list, link);

I'd prefer this to be a for loop, to avoid link having an unexpected value in the rest of the body:

    for (link = _dbus_list_get_first_link (list);
         link != NULL;
         link = _dbus_list_get_next_link (list, link))
      {
        /* body */
      }

@@ +197,5 @@
> +            {
> +            case BUS_POLICY_RULE_OWN:
> +              is_allowed = bus_smack_has_access (subject_label,
> +                                                 object_label,
> +                                                 SMACK_READ_WRITE);

This looks dangerous to me.

Owning a well-known name means being able to impersonate a (potentially highly privileged) service perfectly. If you are able to send and receive messages to PolicyKit (which is trusted to enforce security policies), that shouldn't qualify you to impersonate PolicyKit!

This effectively means that you have to use separate object labels for "I'm allowed to be PolicyKit" and "I'm allowed to talk to PolicyKit".

Does Smack have any concept of verbs other than "read" and "write"?

@@ +207,5 @@
> +              break;
> +            case BUS_POLICY_RULE_RECEIVE:
> +              is_allowed = bus_smack_has_access (subject_label,
> +                                                 object_label,
> +                                                 SMACK_READ);

Mapping SEND to WRITE and READ to RECEIVE is not at all obvious, unfortunately.

If you receive signals from a service, you need to be able to receive its messages - that does look like reading.

If you can Get() or GetAll() a service's properties, or call a GetThings() method, that requires sending a method call message *and* receiving the reply. In your policy language, that would be checked as a write! (Receiving replies to your own method calls is normally allowed, even on the system bus, so you wouldn't normally need a read privilege.)

If you can Set() a property, or call some miscellaneous other method, that also requires sending a method call message (and if you want to find out whether it worked, receiving the reply). You'd also check that as a write, but in this case it's more or less expected.

@@ +225,5 @@
> +          _dbus_verbose ("permission request subject (%s) -> object (%s) : %s", subject_label, object_label, (is_allowed ? "GRANTED" : "REJECTED"));
> +        }
> +    }
> +
> +  dbus_free(subject_label);

I think this should be free(). Also, our coding style is space-before-parenthesis.

@@ +230,5 @@
> +  return allowed_list;
> +
> +nomem:
> +  if (allowed_list != NULL)
> +    _dbus_list_clear (allowed_list);

This clears the list, but does not free the DBusList* itself. You wouldn't have this bug with the signature I suggested above.

@@ +232,5 @@
> +nomem:
> +  if (allowed_list != NULL)
> +    _dbus_list_clear (allowed_list);
> +
> +  dbus_free(subject_label);

I think this should be free(). Also, our coding style is space-before-parenthesis.

Now the promised high-level review.

Something that strikes me about this patchset is: it's partly like SELinux, and partly not. I feel as though non-SELinux security mechanisms ought to be hooking into dbus-daemon in the same sort of places as SELinux - and sure enough, the bits that are like SELinux are mostly good (except for where our SELinux support is wrong - being on the main interface), and the bits that are not like SELinux I'm more suspicious about.

Part 1. GetConnectionSmackContext
---------------------------------

GetConnectionSmackContext is a natural method to want, paralleling GetConnectionUnixUser and GetConnectionSELinuxContext, although I don't think it should be on the core interface, and you'll have to be more careful about how you implement it to avoid attacks like the one I described.

I do wonder whether we should have an all-in-one method to get all available credentials (round trip reduction!), which *could* go on the o.fd.DBus interface, assuming it's documented properly in the spec:

    GetConnectionCredentials(s: Bus_Name) -> a{ss}: Credentials
    /* some example returns:
     * Traditional Unix:
     * { "unix-uid": "1000", "unix-gid": "1000", "unix-pid": "54321" }
     * SELinux:
     * { "unix-uid": "1000", "unix-gid": "1000", "unix-pid": "54321",
     *   "selinux-context": "user_u:staff_r:unconfined_t" }
     * SMACK:
     * { "unix-uid": "1000", "unix-gid": "1000", "unix-pid": "54321",
     *   "smack-context": "*" }
     * Windows:
     * { "windows-sid": "S-1-5-18" }
     */

Part 2. Access control
----------------------

There are two ways in which access control is done in dbus-daemon: the way it's done for "simple things", and the way it's done for SELinux. You've imitated the first, but I wonder whether the second would be more appropriate.

For "simple things", we make our own authorization decisions: when a new connection connects, we build a big list of every applicable ACL rule, and when it wants to do something, we iterate through that list and apply the result.

This is just optimization: in principle, we could just remember the credentials when the new connection connects (this bit needs to be done eagerly to avoid the attack I described), and iterate through all of the ACL rules whenever the process does something, checking whether they're applicable before we apply them. The important thing here is that because the OS has no obvious way to say whether something is allowed, just credentials, we have to build our own security policy and make our own decisions.

For SELinux, it's much simpler: libselinux already has a way to ask "may we do this?", so we call functions in selinux.c which all boil down to a call to avc_has_perm(). It appears Smack has an analogous function for "can this subject do this verb on this object?", which you use to build the ACL - but couldn't you just call out to that function every time, like SELinux does?

This gives avc_has_perm() the opportunity to log failed attempts to do things, and to have its own idea of whether it's strictly enforcing, or allowing everything but logging things that it wouldn't have allowed in enforcing mode.

Also, this keeps all of the SELinux policy together, instead of splitting it into the bits in system.d and the bits elsewhere - I think that's probably valuable.

This has a major advantage in environments where root is not all-powerful (like SELinux). Normally, root (by which I mean anyone with either uid 0 or CAP_FOWNER) can reconfigure the system dbus-daemon by writing files into /etc/dbus-1/system.d, and it will obey. If you don't want even root to be able to get around your security policy, this is obviously not ideal.

In D-Bus' SELinux setup, if arrangements have been made for it to be impossible to overwrite dbus-daemon or libselinux and get them to listen on /var/run/dbus/system_bus_socket next time you reboot, protecting system.d doesn't appear to be necessary, because the worst that can happen is a denial of service - the actual security check is made by calling into avc_has_perm(), which applies its own policy.

(In reply to comment #3)
> I thought I'd fixed this already, which version are you basing your patch on?

The answer should be "current git master", by the way: this is a feature, so it goes in the development branch. If you want to backport it to D-Bus 1.4 for Tizen or whatever, that's your decision, but as an upstream project we shouldn't.

(If someone reviews my various pending branches and/or finishes off Lennart's systemd support according to my review feedback, we can have a D-Bus 1.6 one day. Do you like D-Bus? Do you want to review things? :-)

(In reply to comment #2)
> Discussion here:
> 
> http://lists.freedesktop.org/archives/dbus/2012-January/014904.html

The tl;dr version of this thread is "no we're not going to have loadable modules, everything should be compiled in" - which is what these patches do, so that's good.

These patches don't generalize the current SELinux hooks into something common to both SELinux and Smack, but that's not necessarily a prerequisite.

(In reply to comment #5)
> I do wonder whether we should have an all-in-one method to get all available
> credentials (round trip reduction!)

I played around with this a bit and here is the result:

http://cgit.freedesktop.org/~smcv/dbus/log/?h=wip-get-all-creds

(Only tested briefly, no regression tests, might not actually work.)

Do any other D-Bus reviewers have opinions for or against the addition of this method?

> This has a major advantage in environments where root is not all-powerful
> (like SELinux).

For background: is Smack intended to be one of these environments?

Created attachment 60485 [details] [review]
get the Smack context of a connection

(In reply to comment #8)
> (In reply to comment #5)
> > I do wonder whether we should have an all-in-one method to get all available
> > credentials (round trip reduction!)
> 
> I played around with this a bit and here is the result:
> 
> http://cgit.freedesktop.org/~smcv/dbus/log/?h=wip-get-all-creds
> 
> (Only tested briefly, no regression tests, might not actually work.)

https://bugs.freedesktop.org/attachment.cgi?id=60485

This new patch obsoletes the previous patch to get the Smack context of a connection and it uses the new GetConnectionCredentials method.  I have tested for Smack enabled service and it returns correct, UID, PID and Smack context.

Comment on attachment 60485 [details] [review]
get the Smack context of a connection

Review of attachment 60485 [details] [review]:
-----------------------------------------------------------------

This presumably depends on my GetConnectionCredentials patch. Someone who isn't me will need to review that one.

I think this deserves to be two commits, really: one to get the Smack context of new connections, and one to include that in GetConnectionCredentials (and presumably you'll want a third commit later, which only depends on the first one, to check messages' adherence to Smack policies).

Is it called Smack or SMACK? Please be consistent when writing comments/docs/configure output.

::: bus/connection.c
@@ +637,5 @@
>                                                    &error);
> +
> +#ifdef DBUS_ENABLE_SMACK
> +  d->smack_label = bus_smack_get_label (connection, &error);
> +#endif

If getting the SELinux ID fails, and then getting the Smack label also fails, dbus-daemon will crash here. You'll need to follow a sequence like this:

    get_selinux_thing (..., &error);

    if (error is set)
      goto panic;

#ifdef DBUS_ENABLE_SMACK
    get_smack_thing (..., &error);

    if (error is set)
      goto panic;
#endif

DBusError is like GError: once an error has occurred, you have to either pass it on to your caller, or handle it and clear the variable. "Piling up" errors is not allowed. See the DBusError (or GError) API documentation.

You might be able to avoid the #ifdef by making your code path more like SELinux's - in the non-SELinux case, the function still exists, but always returns NULL without error.

I see you are halfway there: bus_smack_get_label() always exists, but you #ifdef here anyway. Make your mind up: either don't have it if it won't be called, or make it behave like the SELinux function and call it unconditionally.

The convention used in libdbus appears to be what the SELinux function does, so if you're not following it, please explain your justification for not doing so. My best guess is "it saves sizeof(char*) bytes per Connection", which I think is fairly unconvincing - I did #ifdef Stats, but those are rather larger.

@@ +738,5 @@
>        d->selinux_id = NULL;
>        
> +#ifdef DBUS_ENABLE_SMACK
> +  bus_smack_label_free (d->smack_label);
> +#endif

Something appears to be not right in the indentation here?

::: bus/smack.c
@@ +46,5 @@
> +    {
> +      dbus_set_error (error, DBUS_ERROR_FAILED,
> +                      "Failed to get the socket descriptor of the connection");
> +      _dbus_verbose ("Failed to get socket descriptor of connection for Smack check.\n");
> +    return NULL;

Something seems to be wrong with this indentation (the "return" seems to be 2 spaces to the left of the two function calls). Please use exclusively spaces (no tabs) in new code.

@@ +49,5 @@
> +      _dbus_verbose ("Failed to get socket descriptor of connection for Smack check.\n");
> +    return NULL;
> +    }
> +
> +  if (smack_new_label_from_socket (sock_fd, &label) < 0)

How does this function report errors? Does it set errno? If it does, please use _dbus_strerror (errno) to give users a hint.

On success, is the label guaranteed to be \0-terminated?

On success, is the label guaranteed to be valid UTF-8 as defined by dbus_validate_utf8(), or equivalently, by GLib's g_utf8_validate()?

(That means: UTF-8 with no overlong sequences, no surrogate pairs, no codepoints beyond the range of Unicode (<= 0x0010ffff), and no permanently-reserved noncharacter code points U+FDD0..U+FDEF or U+nFFFE..U+nFFFF for n in the range 0 to 0x10.)

If the label is not guaranteed to be valid UTF-8 you'll have to send it as a byte-array instead of a string, because the D-Bus "string" type guarantees UTF-8.

What does the syntax of a Smack context look like? Is there a length limit or a syntactic structure or anything?

@@ +63,5 @@
> +#endif
> +}
> +
> +void
> +bus_smack_label_free (void *label)

Surely this should be char *? Unless you have labels of some other type for some reason?

@@ +68,5 @@
> +{
> +#ifdef DBUS_ENABLE_SMACK
> +  if (label)
> +    free (label);
> +  label = NULL;

I don't see what this last line achieves.

::: cmake/bus/CMakeLists.txt
@@ +70,5 @@
>  	${BUS_DIR}/test.h					
>  	${BUS_DIR}/utils.c					
> +	${BUS_DIR}/utils.h
> +	${BUS_DIR}/smack.c
> +	${BUS_DIR}/smack.h

Please keep alphabetical order in lists of files, to minimize conflicts: if you keep them in order rather than adding at the end, we only get conflicts when two new files would go in the same place, whereas if everyone adds to the end, every branch that adds files automatically conflicts.

::: configure.ac
@@ +1684,5 @@
> +     [AC_MSG_ERROR([libsmack is required to enable smack support])])
> +fi
> +
> +AC_SUBST([LIBSMACK_CFLAGS])
> +AC_SUBST([LIBSMACK_LIBS])

You don't need to AC_SUBST these: PKG_CHECK_MODULES does it automatically.

(In reply to comment #11)
> If the label is not guaranteed to be valid UTF-8 you'll have to send it as a
> byte-array instead of a string, because the D-Bus "string" type guarantees
> UTF-8.

If it's not guaranteed to be UTF-8:

There's no strong convention for whether a stringy-but-not-necessarily-UTF-8 byte-array is sent with or without a trailing "\0", unfortunately.

The developers of GVariant seem to be trying to encourage including the trailing "\0" (it does make it a little easier to provide a nice C API), so if your context was "wombat" you'd send "wombat\0" in the D-Bus message, with a length of 7; I suggest following that convention.

(In reply to comment #11)
> This presumably depends on my GetConnectionCredentials patch. Someone who
> isn't me will need to review that one.

Blocked by Bug #54445 accordingly.

Unanswered questions for Brian:

> > This has a major advantage in environments where root is not all-powerful
> > (like SELinux).
> 
> For background: is Smack intended to be one of these environments?

(In reply to comment #11)
> Is it called Smack or SMACK?

> > +  if (smack_new_label_from_socket (sock_fd, &label) < 0)
> 
> How does this function report errors? Does it set errno?

> On success, is the label guaranteed to be \0-terminated?

> On success, is the label guaranteed to be valid UTF-8 as defined by
> dbus_validate_utf8(), or equivalently, by GLib's g_utf8_validate()?

> What does the syntax of a Smack context look like? Is there a length
> limit or a syntactic structure or anything?

I looked for answers to these on the Smack website, but the Smack website appears to be its author's home page, and contain no technical information except for a broken link to a MeeGo gitorious repository. This doesn't fill me with enthusiasm about it, tbh...

Created attachment 66729 [details] [review]
Query Smack context of connection

(In reply to comment #14)
> Unanswered questions for Brian:
> 
> > > This has a major advantage in environments where root is not all-powerful
> > > (like SELinux).
> > 
> > For background: is Smack intended to be one of these environments?

Yes.

> 
> (In reply to comment #11)
> > Is it called Smack or SMACK?
> 
> > > +  if (smack_new_label_from_socket (sock_fd, &label) < 0)
> > 
> > How does this function report errors? Does it set errno?

It is basically a wrapper around getsockopt(2).  It will use the error numbers that are set there.  The latest patch includes these in the error message string.

> > On success, is the label guaranteed to be \0-terminated?

Yes.
 
> > On success, is the label guaranteed to be valid UTF-8 as defined by
> > dbus_validate_utf8(), or equivalently, by GLib's g_utf8_validate()?

It is an ascii string.
 
> > What does the syntax of a Smack context look like? Is there a length
> > limit or a syntactic structure or anything?

A standard human readable ascii string.  e.g. "com.games.MellowMarsupials". the max length is 255 chars.
  
> I looked for answers to these on the Smack website, but the Smack website
> appears to be its author's home page, and contain no technical information
> except for a broken link to a MeeGo gitorious repository. This doesn't fill me
> with enthusiasm about it, tbh...

Modern technologies like Web pages and documentation are not the Smack maintainers strong points :)

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=Documentation/security/Smack.txt;h=a416479b8a1c23d6755cd5354027159af832a4dc;hb=080909503664641432cc8adf2ee2084775fd992a

There is a section there Smack Basics / Labels.

Created attachment 66731 [details] [review]
Query Smack context of connection

ping?

Comment on attachment 66731 [details] [review]
Query Smack context of connection

Review of attachment 66731 [details] [review]:
-----------------------------------------------------------------

Michael, does your request for a maintainer review indicate that you have reviewed this patch and approve it?

::: bus/connection.c
@@ +99,4 @@
>    long connection_tv_usec; /**< Time when we connected (microsec component) */
>    int stamp;               /**< connections->stamp last time we were traversed */
>  
> +  char *smack_label;

Mention "free with bus_smack_label_free()" in a comment, please - allocated strings in libdbus are usually meant to be freed with dbus_free(), but that's not guaranteed to be compatible.

@@ +633,5 @@
>    d->selinux_id = bus_selinux_init_connection_id (connection,
>                                                    &error);
> +
> +  if (!dbus_error_is_set (&error))
> +    d->smack_label = bus_smack_get_label(connection, &error);

coding style nitpicking: ...get_label (connection... with a space

@@ +1121,4 @@
>    return d->selinux_id;
>  }
>  
> +const char*

coding style nitpicking: const char * (with a space)

::: bus/driver.c
@@ +1571,5 @@
>  
> +  smack_ctx = bus_connection_get_smack_label (conn);
> +  if (smack_ctx)
> +    {
> +      if (!_dbus_asv_add_string(&array_iter, "SmackContext", smack_ctx))

Coding style nitpicking: "...add_string (&..." with a space.

If the standard jargon is "Smack label" then I'd prefer this called SmackLabel.

::: bus/smack.c
@@ +39,5 @@
> +#include <sys/smack.h>
> +#endif
> +
> +char *
> +bus_smack_get_label (DBusConnection *connection, DBusError *error)

I think "if there is no SMACK support, this returns NULL without setting the error" deserves a doc-comment. So does "the caller must free the label, if non-NULL, with bus_smack_label_free()".

@@ +45,5 @@
> +#ifdef DBUS_ENABLE_SMACK
> +  char *label;
> +  int sock_fd;
> +
> +  if (!dbus_connection_get_socket(connection, &sock_fd))

coding style nitpicking: space before parenthesis

@@ +48,5 @@
> +
> +  if (!dbus_connection_get_socket(connection, &sock_fd))
> +    {
> +      dbus_set_error (error, DBUS_ERROR_FAILED,
> +                      "Failed to get the socket descriptor of the connection.\n");

Conventionally, DBusError messages don't have a newline at the end.

@@ +54,5 @@
> +      return NULL;
> +    }
> +
> +  /* retrieve an ascii, null-terminated string that defines the Smack context of the connected socket */
> +  if (smack_new_label_from_socket(sock_fd, &label) < 0)

coding style nitpicking: space before parenthesis

::: bus/smack.h
@@ +28,5 @@
> +
> +#include "bus.h"
> +
> +char *
> +bus_smack_get_label (DBusConnection *connection, DBusError *error);

Coding style nitpicking: no newline after the return type for declarations, only for definitions. "char *bus_smack_..."

@@ +33,5 @@
> +
> +void
> +bus_smack_label_free (char *label);
> +
> +#endif // SMACK_H

Either /* SMACK_H */ or no comment, please. D-Bus is approximately C89.

::: configure.ac
@@ +206,5 @@
>        [Define to build test code into the library and binaries])
>  fi
>  
> +# call early to ensure availability
> +PKG_PROG_PKG_CONFIG

This change is fine, and feel free to make it, but it's unnecessary if AS_IF is used correctly.

@@ +1758,5 @@
>  fi
>  
> +#enable smack label support
> +AC_ARG_ENABLE([smack], [AS_HELP_STRING([--enable-smack], [enable Smack security checks])], [], [enable_smack=no])
> +if test "x$enable_smack" = xyes; then

AS_IF([test ...],
    [PKG_CHECK...])

(This makes sure PKG_PROG_PKG_CONFIG is evaluated even if Smack is disabled.)

::: doc/dbus-specification.xml
@@ +5649,5 @@
> +		<entry>SmackContext</entry>
> +		<entry>STRING</entry>
> +		<entry>This is a ascii string that defines the Smack context
> +		of the process that owns the connection.  Smack is a Manditory
> +		Access Control framework, that is part of the Linux kernel.

s/Manditory/Mandatory/

s/framework, that/framework that/

ASCII is an acronym, so, upper-case.

Why is this called a "context" here when Documentation/security/Smack.txt calls it a "label"? I'd prefer it called ...Label if that's the standard Smack jargon.

I'd prefer something like this, where _x_ represents an appropriate hyperlink into kernel.org cgit or something:

The process' label in Smack, a mandatory access control framework in the Linux kernel. The syntax of Smack labels is defined in _Documentation/security/Smack.txt_ in Linux source code; in particular, they are non-empty printable ASCII strings.

(In reply to comment #11)
> This presumably depends on my GetConnectionCredentials patch. Someone who
> isn't me will need to review that one.

Similarly, Michael/Brian: does your contribution here indicate that you think my patches on Bug #54445 are good to merge?

(In reply to comment #16)
> A standard human readable ascii string.  e.g. "com.games.MellowMarsupials".
> the max length is 255 chars.

Documentation/security/Smack.txt says 1-23 characters of ASCII - is that outdated?

If the maximum length has changed, do we have any guarantee that the format will not change to allow '\0' or non-UTF-8 in the future? (ASCII is trivially UTF-8 so that's OK.)

To defend against the potential for non-UTF-8 labels (even if the kernel currently guarantees that we won't receive any), something like this in bus_smack_get_label() would do nicely:

    if (!dbus_validate_utf8 (label, NULL))
      {
        ... do something ...
      }

I don't know what the "something" should be. You could either behave as if there was no label at all (return NULL without error, and omit it from the a{sv}, which should presumably cause clients to treat it as unprivileged), or raise a DBusError and make the request fail. Whichever you choose, please justify why :-)

The second parameter to dbus_validate_utf8() is a (pointer to return a) DBusError which you could use if appropriate, although at the moment it just says "invalid UTF-8" or something, so it isn't particularly informative.

(In reply to comment #19)
> The process' label in Smack, a mandatory access control framework in the
> Linux kernel. The syntax of Smack labels is defined in
> _Documentation/security/Smack.txt_ in Linux source code; in particular, they
> are non-empty printable ASCII strings.

Adding some typical examples like "... ASCII strings, such as 'com.example.MyApp', '_' or '^'" would be nice too. Please use example.(com|net|org) for any examples that are domain-name-shaped.

This is not blocked by Bug #54445 any more: there's some Windows stuff still to do on that bug, but the OS-independent and Unix parts are in git master.

This is a feature, so it isn't going to go in 1.6 in any case (although if there are Smack-reliant distributions that are unwilling to upgrade to 1.7, but willing to do their own QA on a backport, they're welcome to backport it).

My comments 19-22 are a code review. If you (for the most general possible values of "you") want this merged, please address these in a new patch. I'm not going to write Smack (or other LSM) code myself, because I wouldn't use it, but I'd be happy to review it.

Jacek, Patrick,
I hear you've been working on something similar to this for Tizen, so I've added you to Cc.

I'm happy to review patches to add LSM support to D-Bus, but I won't go looking for branches in vendors' patched D-Bus trees; so if you want to upstream this, please attach patches for review here.

Interesting code reviews and design discussion that might be relevant to your patches include the comments on this bug, the documentation added by Bug #83499, and the comments (except for Windows-specifics) on that bug.

The existing SELinux integration is also relevant, and so is Bug #75113 (the same thing as this, but for AppArmor).

Hi,

Security architecture has changed significantly with version of Tizen currently developed. It does not rely so heavily on Smack rules as it did previously and most changes proposed in this patch are not used (that is: <policy context="smack"> extension). Honestly, I don't have experience with this part so I'd prefer not to touch it :)

What we are still using is getting peer connection's smack label. Its implementation is as follows:
 - It is obtained in _dbus_connection_new_for_transport function (by calling libsmack function that essentially calls getsockopt(fd, SOL_SOCKET, SO_PEERSEC, ...). Smack label is saved in new DBusConnection structure field
 - new library function that reads cached value is introduced: dbus_connection_get_smack_label
 - cached smack label is also added to the result map returned in GetConnectionCredentials D-Bus daemon method

I am not the author of this patch (Patrick is), but I believe I could rework it so it merges with current D-Bus master branch if you like the approach described above.

(In reply to Jacek Bukarewicz from comment #25)
> Security architecture has changed significantly with version of Tizen
> currently developed. It does not rely so heavily on Smack rules as it did
> previously and most changes proposed in this patch are not used (that is:
> <policy context="smack"> extension).

I'm not going to merge it if nobody is using it.

Not merging that part is more in line with what I said in Comment #5 anyway, so that's good.

> What we are still using is getting peer connection's smack label.

If you want to do a respin of that patch, I'd review it. Some notes on things that would make me more likely to say "yes":

I would expect that the label (context?) should be part of the result of GetConnectionCredentials (Bug #54445), rather like the AppArmor context is added by the patches on Bug #75113. Please don't add a GetSmackContext() function - I consider GetConnectionCredentials() to be the replacement for all those.

Please choose one jargon word for the label/context, whichever one is considered more correct upstream, and stick to that. Please document it in the D-Bus Specification (next to the documentation for e.g. ProcessID), with brief notes on constraints (is it always UTF-8? can it contain \0? is it always non-empty if present? etc.) and some typical real-world values that it might take.

If it's always UTF-8 (or a UTF-8 subset such as ASCII), please verify that it is in fact UTF-8 (or omit it from the result of GetConnectionCredentials() if it isn't), and encode it as a string.

If it cannot contain \0 but is not constrained to UTF8 (like a Unix filename), please encode it as a byte-array, including the \0 terminator - e.g. { 'f', 'o', 'o', '\0' } if the bytestring you're given is "foo". You will probably want to depend on the patch "New a{sv} helper for using byte arrays as the variant" from Bug #75113 - I'm keen to encourage users of different LSMs to share their helper functions :-)

If it can contain \0 then you'd have to use a byte-array (or other appropriate data structure) and it would probably be most appropriate to not add an extra \0.

(In reply to Jacek Bukarewicz from comment #25)
>  - It is obtained in _dbus_connection_new_for_transport function (by calling
> libsmack function that essentially calls getsockopt(fd, SOL_SOCKET,
> SO_PEERSEC, ...).

I vaguely wonder whether we should say "this is Linux's API for peer security contexts", call the underlying getsockopt directly, and have dbus_connection_get_linux_security_context() rather than linking to libsmack, libapparmor and a series of other tiny libraries... although I suspect that might reduce the guarantees we can provide about the contents of the context, e.g. whether it is \0-terminated.

Is there anywhere that LSMs in general are discussed, where authors of LSM-sensitive things (e.g. dbus) can get advice from SELinux, Smack and AppArmor people all at the same time, rather than having to solve everything three times?

I believe http://vger.kernel.org/vger-lists.html#linux-security-module is the most appropriate place for getting answers for LSM-related questions.

I also was wondering if "GetConnectionCredentials" could attach connection's LSM security context without doing LSM-specific processing. It probably could and IMHO it's reasonable to expect it from this method. It would definitely be usable by systems using smack. I don't know about other LSMs, but it probably also would.
In case of kdbus process' LSM security context can also be passed as part of message metadata (and probably also queried). An excerpt from kdbus docs:

"KDBUS_ITEM_SECLABEL
Contains the LSM label of a task, stored as null-terminated string in item.str. Its length can also be derived from the item's total size."

Looking at getsockopt(... SO_PEERSEC) implementation and kdbus code where "seclabel" is obtained it seems like they should return the same thing (but this is just my analysis which might be wrong).

Also, thanks for paying attention to context/label consistency. Label is preferred in case of Smack (basing on its documentation and kernel source code). I see some references to context in libsmack, but it would probably be better to be more consistent and stick to "label".

(In reply to Jacek Bukarewicz from comment #27)
> I believe http://vger.kernel.org/vger-lists.html#linux-security-module is
> the most appropriate place for getting answers for LSM-related questions.

Asking there.

(In reply to Simon McVittie from comment #26)
> (In reply to Jacek Bukarewicz from comment #25)
> > Security architecture has changed significantly with version of Tizen
> > currently developed. It does not rely so heavily on Smack rules as it did
> > previously and most changes proposed in this patch are not used (that is:
> > <policy context="smack"> extension).
> 
> I'm not going to merge it if nobody is using it.

Yes, please don't. I already wanted to comment here along that line, but then forgot.

> > What we are still using is getting peer connection's smack label.
> 
> If you want to do a respin of that patch, I'd review it. Some notes on
> things that would make me more likely to say "yes":
> 
> I would expect that the label (context?) should be part of the result of
> GetConnectionCredentials (Bug #54445), rather like the AppArmor context is
> added by the patches on Bug #75113. Please don't add a GetSmackContext()
> function - I consider GetConnectionCredentials() to be the replacement for
> all those.

I had already reworked the patch to use GetConnectionCredentials(), but wanted to see it getting used in Tizen before submitting upstream. Jacek then took over the work.

Jacek's idea of making GetConnectionCredentials() LSM agnostic sounds interesting. +1 for investigating that first.



> 
> Please choose one jargon word for the label/context, whichever one is
> considered more correct upstream, and stick to that. Please document it in
> the D-Bus Specification (next to the documentation for e.g. ProcessID), with
> brief notes on constraints (is it always UTF-8? can it contain \0? is it
> always non-empty if present? etc.) and some typical real-world values that
> it might take.
> 
> If it's always UTF-8 (or a UTF-8 subset such as ASCII), please verify that
> it is in fact UTF-8 (or omit it from the result of
> GetConnectionCredentials() if it isn't), and encode it as a string.
> 
> If it cannot contain \0 but is not constrained to UTF8 (like a Unix
> filename), please encode it as a byte-array, including the \0 terminator -
> e.g. { 'f', 'o', 'o', '\0' } if the bytestring you're given is "foo". You
> will probably want to depend on the patch "New a{sv} helper for using byte
> arrays as the variant" from Bug #75113 - I'm keen to encourage users of
> different LSMs to share their helper functions :-)
> 
> If it can contain \0 then you'd have to use a byte-array (or other
> appropriate data structure) and it would probably be most appropriate to not
> add an extra \0.
> 
> (In reply to Jacek Bukarewicz from comment #25)
> >  - It is obtained in _dbus_connection_new_for_transport function (by calling
> > libsmack function that essentially calls getsockopt(fd, SOL_SOCKET,
> > SO_PEERSEC, ...).
> 
> I vaguely wonder whether we should say "this is Linux's API for peer
> security contexts", call the underlying getsockopt directly, and have
> dbus_connection_get_linux_security_context() rather than linking to
> libsmack, libapparmor and a series of other tiny libraries... although I
> suspect that might reduce the guarantees we can provide about the contents
> of the context, e.g. whether it is \0-terminated.
> 
> Is there anywhere that LSMs in general are discussed, where authors of
> LSM-sensitive things (e.g. dbus) can get advice from SELinux, Smack and
> AppArmor people all at the same time, rather than having to solve everything
> three times?

(In reply to Patrick Ohly from comment #29)
> Jacek's idea of making GetConnectionCredentials() LSM agnostic sounds
> interesting. +1 for investigating that first.

I implemented this in dbus-daemon 1.9.12 (Bug #89041).

Is there anything else that people who like SMACK would want to see in dbus-daemon, for instance a "mediation" layer (ability to allow/prevent individual messages) like the parts of the SELinux and AppArmor code that remain LSM-specific?

If not, let's close this as WORKSFORME or something.

It would still be useful for the Smack/Cynara integration to get asynchronous policy checking into the core D-Bus code (i.e. ask a separate process, wait for reply without blocking), or even the full Cynara changes.

However, I only mention it for the sake of completeness. I understand that this was already brought up elsewhere and considered too intrusive, and besides, it is unrelated to the patches proposed here, so I'm okay with closing this issues as "WORKSFORME".

(In reply to Patrick Ohly from comment #31)
> It would still be useful for the Smack/Cynara integration to get
> asynchronous policy checking into the core D-Bus code (i.e. ask a separate
> process, wait for reply without blocking)

If you propose patches, I'll consider them. If you don't propose patches, they will never be merged :-)

However, this seems likely to be a very significant amount of extra complexity in "hot paths" in dbus-daemon, so I suspect the answer might be "no".

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/65.