Bug 98666 - clients that can't send messages to an activatable service should not be able to autostart it
Summary: clients that can't send messages to an activatable service should not be able...
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on: 98671
Blocks:
  Show dependency treegraph
 
Reported: 2016-11-09 19:40 UTC by Simon McVittie
Modified: 2016-11-29 12:33 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Add tests for activation when message send/receive is denied (11.79 KB, patch)
2016-11-09 19:41 UTC, Simon McVittie
Details | Splinter Review
Do not auto-activate services if we could not send a message (15.44 KB, patch)
2016-11-09 19:43 UTC, Simon McVittie
Details | Splinter Review
Mediate auto-activation attempts through AppArmor (10.35 KB, patch)
2016-11-09 19:45 UTC, Simon McVittie
Details | Splinter Review
Spec: document what auto-starting is, and recommend it (3.31 KB, patch)
2016-11-09 19:45 UTC, Simon McVittie
Details | Splinter Review
Spec: document systemd activation (2.32 KB, patch)
2016-11-09 19:45 UTC, Simon McVittie
Details | Splinter Review
Spec: document AppArmor mediation of auto-starting (3.93 KB, patch)
2016-11-09 19:45 UTC, Simon McVittie
Details | Splinter Review
Add an integration test for AppArmor mediating activation (20.52 KB, patch)
2016-11-09 19:46 UTC, Simon McVittie
Details | Splinter Review
Do not auto-activate services if we could not send a message (15.51 KB, patch)
2016-11-21 21:21 UTC, Simon McVittie
Details | Splinter Review
Mediate auto-activation attempts through AppArmor (9.95 KB, patch)
2016-11-21 21:21 UTC, Simon McVittie
Details | Splinter Review
Spec: document AppArmor mediation of auto-starting (4.59 KB, patch)
2016-11-21 21:24 UTC, Simon McVittie
Details | Splinter Review
Add an integration test for AppArmor mediating activation (21.12 KB, patch)
2016-11-21 21:25 UTC, Simon McVittie
Details | Splinter Review
Activation test: exercise what happens with nonexistent AppArmor labels (5.37 KB, patch)
2016-11-21 21:27 UTC, Simon McVittie
Details | Splinter Review
Spec: document AppArmor mediation of auto-starting (4.61 KB, patch)
2016-11-22 11:42 UTC, Simon McVittie
Details | Splinter Review
Don't test AppArmor mediation of activation if libapparmor < 2.10 (3.29 KB, patch)
2016-11-28 15:50 UTC, Simon McVittie
Details | Splinter Review
activation test: don't crash if AppArmor is built but unavailable (988 bytes, patch)
2016-11-29 01:01 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2016-11-09 19:40:44 UTC
This feature request was previously discussed in the thread started at <https://lists.freedesktop.org/archives/dbus/2015-October/016815.html>. See there for background.

The tl;dr version is: suppose the XML policy language or Linux LSM mediation (SELinux, AppArmor) prevents client C from sending messages to activatable service S, and also prevents that client from calling StartServiceByName(). It is (perhaps unexpectedly) still possible for C to autostart S by sending a message to S: the message will be rejected, and in particular S won't receive it, but S will still be started first.

The patch-set that I'm about to attach makes it possibly for the XML policy and LSMs to prevent this. Specifically:

* If the XML policy language would not allow C to send the message to S, the
  message is now rejected *before* starting S.

* Similarly, if AppArmor would not allow C to send the message to S, the
  message is now rejected *before* starting S.

  - If the D-Bus .service file has a new AssumedAppArmorLabel key, we
    will ask AppArmor to evaluate the policy rules as though S will
    have that AppArmor label.

  - If not, we will ask AppArmor to evaluate the policy rules as though
    S will have an unspecified AppArmor label. Any allow or deny rule
    that matches peer=(label=something) will not match; the worst case
    here is that C is allowed to make S be started, but sending the actual
    message is later denied (in other words, regressing to current behaviour).

* If the XML policy language would not allow S to *receive* the message,
  the message is still not rejected until after S has started. (This is
  because until then, we can't know what the process attributes of S will be,
  in particular its uid and gid; and because more generally, we don't
  compute the list of access-control rules that will apply to S until it
  has actually connected, and we probably don't want to compute that list.)

  The XML policy language's receive rules are not used in practice, so this
  is not really a big problem.

* If AppArmor would not allow S to receive the message from C,
  the message is also still not rejected until after S has started, to be
  consistent with the policy language. It would be possible to be
  symmetrical with the send rules for services with AssumedAppArmorLabel,
  but not for services that do not have it (we cannot query an unspecified
  policy, so we would effectively have to assume 'unconfined'); I think
  that would probably be more confusing than the current situation.

I haven't implemented SELinux mediation because I don't use SELinux, but I did pass the same information to selinux.c as to apparmor.c, so that it would be easy for someone else to do.
Comment 1 Simon McVittie 2016-11-09 19:41:46 UTC
Created attachment 127875 [details] [review]
Add tests for activation when message send/receive is  denied

---

This does not change any functionality; it just sets a baseline for what we expect to happen at the moment.
Comment 2 Simon McVittie 2016-11-09 19:43:31 UTC
Created attachment 127876 [details] [review]
Do not auto-activate services if we could not send a  message

We specifically do not check recipient policies, because
the recipient policy is based on properties of the
recipient process (in particular, its uid), which we do
not necessarily know until we have already started it.

In this initial implementation we do not check LSMs either,
because we cannot know what LSM context the recipient process
is going to have. However, LSM support will need to be added
to make this feature useful, because StartServiceByName is
normally allowed in non-LSM environments, and is more
powerful than auto-activation anyway.

The StartServiceByName method does not go through this check,
because if access to that method has been granted, then
it's somewhat obvious that you can start arbitrary services.

---

This includes a change to the tests added in the previous commit, to assert that we have the newly-desired behaviour.
Comment 3 Simon McVittie 2016-11-09 19:45:04 UTC
Created attachment 127877 [details] [review]
Mediate auto-activation attempts through AppArmor

Because the recipient process is not yet available, we have to make some
assumption about its AppArmor profile. Parsing the first word of
the Exec value and then chasing symlinks seems like too much magic,
so I've gone for something more explicit. If the .service file contains

AssumedAppArmorLabel=/foo/bar

then we will do the AppArmor query on the assumption that the recipient
AppArmor label will be as stated. Otherwise, we will do a query
with an unspecified label, which means that AppArmor rules that do
specify a peer label will never match it.

Regardless of the result of this query, we will do an independent
AppArmor query when the activation has actually happened, this time
with the correct peer label; that second query will still be used
to decide whether to deliver the message. As a result, if this change
has any effect, it is to make the bus more restrictive; it does not
allow anything that would previously have been denied.
Comment 4 Simon McVittie 2016-11-09 19:45:23 UTC
Created attachment 127878 [details] [review]
Spec: document what auto-starting is, and recommend it

For something we recommend, that is important enough to have its own
header flag, it doesn't have very good documentation. Redo the text
to suggest that auto-starting is the normal thing and
StartServiceByName is the oddity. That's usually a good principle
to follow, since it dodges time-of-check/time-of-use issues, and the
method call that you presumably wanted to do needs to handle errors
anyway.
Comment 5 Simon McVittie 2016-11-09 19:45:38 UTC
Created attachment 127879 [details] [review]
Spec: document systemd activation

We didn't say that SystemdService existed. Now we do, together with
enough context to make it make sense.
Comment 6 Simon McVittie 2016-11-09 19:45:54 UTC
Created attachment 127880 [details] [review]
Spec: document AppArmor mediation of auto-starting
Comment 7 Simon McVittie 2016-11-09 19:46:48 UTC
Created attachment 127881 [details] [review]
Add an integration test for AppArmor mediating activation

---

Because this sets up AppArmor profiles, it has to run as root. It's gracefully skipped if we don't have AppArmor or are not root.
Comment 8 Philip Withnall 2016-11-12 22:49:09 UTC
Comment on attachment 127875 [details] [review]
Add tests for activation when message send/receive is  denied

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

Looks good to me.
Comment 9 Philip Withnall 2016-11-12 23:11:09 UTC
Comment on attachment 127876 [details] [review]
Do not auto-activate services if we could not send a  message

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

This looks good. A minor number of minor nitpicks which you can ignore if you want.

::: bus/bus.c
@@ +1513,5 @@
>   *
> + * NULL for sender definitely means the bus driver.
> + *
> + * NULL for proposed_recipient may mean the bus driver, or may mean
> + * we are checking whether service-activation is allowed.

Might want to provide a bit more detail here than "checking whether service-activation is allowed". How about "checking whether service-activation is allowed as a first pass before all the details of the activated service are known", or similar?

::: bus/bus.h
@@ +142,4 @@
>                                                                    DBusConnection   *addressed_recipient,
>                                                                    DBusConnection   *proposed_recipient,
>                                                                    DBusMessage      *message,
> +                                                                  BusActivationEntry *activation_entry,

Nitpick: You might want to reindent the argument names here? The downsides of aligning them all. :-(

(And in selinux.h if you do this.)
Comment 10 Philip Withnall 2016-11-12 23:18:01 UTC
Comment on attachment 127878 [details] [review]
Spec: document what auto-starting is, and recommend it

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

::: doc/dbus-specification.xml
@@ +4981,5 @@
>        </para>
> +
> +      <para>
> +        With D-Bus, starting a service is normally done by
> +        <firstterm>auto-starting</firstterm>. That is, applications send a

Perhaps mention that it's often (incorrectly?) called 'service activation', just to make sure that people grep to the right place in the document when keying off that term.

@@ +4983,5 @@
> +      <para>
> +        With D-Bus, starting a service is normally done by
> +        <firstterm>auto-starting</firstterm>. That is, applications send a
> +        message to a particular well-known name, such as
> +        <literal>com.example.TextEditor</literal>, without specifying the

Nitpick: Do we want to be pedantic enough to include version numbers in the example well-known names in the specification, to reinforce the versioning recommendations from https://dbus.freedesktop.org/doc/dbus-api-design.html#api-versioning? I could easily see it being an unnecessary distraction here though.

@@ +4995,4 @@
>        <para>
> +        It is also possible for applications to send an request to
> +        start a service. See
> +        <xref linkend="bus-messages-start-service-by-name"/> for details.

Unfortunately, the `bus-messages-start-service-by-name` mostly consists of a link back to this section to seek more information here.

Do you want to point out in this paragraph that `StartServiceByName` is racy and therefore less recommended than auto-starting?
Comment 11 Philip Withnall 2016-11-12 23:21:59 UTC
Comment on attachment 127879 [details] [review]
Spec: document systemd activation

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

Nice.
Comment 12 Philip Withnall 2016-11-12 23:28:27 UTC
Comment on attachment 127880 [details] [review]
Spec: document AppArmor mediation of auto-starting

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

::: doc/dbus-specification.xml
@@ +5185,5 @@
> +      <sect3 id="message-bus-starting-services-apparmor">
> +        <title>Mediating Activation with AppArmor</title>
> +
> +        <para>
> +          Please refer to AppArmor documentation for general information on

Can you add a link to the AppArmor documentation? http://wiki.apparmor.net/index.php/Documentation

@@ +5202,5 @@
> +          assumptions about the resulting process's credentials.
> +          If it does proceed with auto-starting, when the service appears, the
> +          <literal>dbus-daemon</literal> repeats the policy check (with
> +          the service's true credentials, which might not be identical)
> +          before delivering the message.

Perhaps add a sentence clarifying that the second check is guaranteed to be stronger (in the sense that it will reject a non-strict superset of what the first check would reject).

@@ +5216,5 @@
> +          on the assumption that the activated service will be confined
> +          under the specified label; in particular, rules of the form
> +          <literal>dbus send peer=(label=/usr/sbin/mydaemon)</literal> or
> +          <literal>deny dbus send peer=(label=/usr/sbin/mydaemon)</literal>
> +          will match it, allowing or denying as appropriate.

What happens if there is no profile defined for the given label? The implementation suggests that everything is rejected, but this should be documented.

@@ +5227,5 @@
> +          the form
> +          <literal>dbus send peer=(label=X)</literal> (for any value of X)
> +          will not allow the auto-start, and any rule of the form
> +          <literal>deny dbus send peer=(label=X)</literal>
> +          will not deny it.

Is it worth mentioning whether an undefined label is the same as `unconfined`?
Comment 13 Philip Withnall 2016-11-12 23:38:54 UTC
Comment on attachment 127877 [details] [review]
Mediate auto-activation attempts through AppArmor

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

The commit message could mention the libapparmor version bump.

Although why is a version bump happening? I don't see any change in how libapparmor is used in this patch.

The rest looks OK to me. It's a shame there are no unit tests for AppArmor in dbus-daemon, although it would only be possible to run them on AppArmor-enabled kernels. (I'm not asking you to write an entire new test suite.)
Comment 14 Philip Withnall 2016-11-12 23:43:15 UTC
I forget how you prefer the 'review' tags to be used in the whiteboard, so I haven't changed it.

tl;dr: Patches look fine; just a few really minor/nitpick comments.
Comment 15 Simon McVittie 2016-11-21 11:39:00 UTC
(In reply to Philip Withnall from comment #9)
> Might want to provide a bit more detail here than "checking whether
> service-activation is allowed". How about "checking whether
> service-activation is allowed as a first pass before all the details of the
> activated service are known", or similar?

Sounds good.

> Nitpick: You might want to reindent the argument names here? The downsides
> of aligning them all. :-(

Possibly. I tend to lean towards "if it looks halfway reasonable, it's sufficiently-aligned" to avoid giant diffstats, and it's hard to align long names without breaking the 80-column guideline.
Comment 16 Simon McVittie 2016-11-21 11:43:47 UTC
(In reply to Philip Withnall from comment #10)
> > +      <para>
> > +        With D-Bus, starting a service is normally done by
> > +        <firstterm>auto-starting</firstterm>. That is, applications send a
> 
> Perhaps mention that it's often (incorrectly?) called 'service activation',
> just to make sure that people grep to the right place in the document when
> keying off that term.

The shades of meaning obviously aren't made clear enough in the spec, because what you've said here doesn't agree with my understanding.

The way I think the terminology works (which could be wrong) is:

* activation is anything that makes dbus-daemon start a service on-demand
  (either directly, via the setuid helper, or via systemd)

* auto-starting is when com.example.Foo is activated as a side-effect
  of sending a message with destination=com.example.Foo and without
  the NO_AUTO_START flag

* StartServiceByName() is also activation, but is not auto-starting

> > +        With D-Bus, starting a service is normally done by
> > +        <firstterm>auto-starting</firstterm>. That is, applications send a
> > +        message to a particular well-known name, such as
> > +        <literal>com.example.TextEditor</literal>, without specifying the
> 
> Nitpick: Do we want to be pedantic enough to include version numbers in the
> example well-known names in the specification, to reinforce the versioning
> recommendations from
> https://dbus.freedesktop.org/doc/dbus-api-design.html#api-versioning? I
> could easily see it being an unnecessary distraction here though.

We have not traditionally done so. I wouldn't mind doing that as a separate change.

> Unfortunately, the `bus-messages-start-service-by-name` mostly consists of a
> link back to this section to seek more information here.
> 
> Do you want to point out in this paragraph that `StartServiceByName` is racy
> and therefore less recommended than auto-starting?

Yes, perhaps that would be good. I think that would be best as a separate change.
Comment 17 Simon McVittie 2016-11-21 11:47:19 UTC
(In reply to Philip Withnall from comment #12)
> > +          Please refer to AppArmor documentation for general information on
> 
> Can you add a link to the AppArmor documentation?
> http://wiki.apparmor.net/index.php/Documentation

Yes, I should.

> > +          If it does proceed with auto-starting, when the service appears, the
> > +          <literal>dbus-daemon</literal> repeats the policy check (with
> > +          the service's true credentials, which might not be identical)
> > +          before delivering the message.
> 
> Perhaps add a sentence clarifying that the second check is guaranteed to be
> stronger (in the sense that it will reject a non-strict superset of what the
> first check would reject).

That isn't actually entirely true, though. It is possible (although AIUI not recommended) to write AppArmor profiles that work on a blacklist basis ("allow everything except...", by using `deny` rules.

> > +          on the assumption that the activated service will be confined
> > +          under the specified label; in particular, rules of the form
> > +          <literal>dbus send peer=(label=/usr/sbin/mydaemon)</literal> or
> > +          <literal>deny dbus send peer=(label=/usr/sbin/mydaemon)</literal>
> > +          will match it, allowing or denying as appropriate.
> 
> What happens if there is no profile defined for the given label? The
> implementation suggests that everything is rejected, but this should be
> documented.

I haven't actually tried it, tbh.

> Is it worth mentioning whether an undefined label is the same as
> `unconfined`?

Probably. AIUI, the answer is that an undefined label is not the same as the special label `unconfined`: `dbus send peer=(label=unconfined)` matches messages sent to an unconfined peer, but does not match messages whose peer label is unspecified.
Comment 18 Simon McVittie 2016-11-21 11:58:08 UTC
(In reply to Philip Withnall from comment #13)
> The commit message could mention the libapparmor version bump.
> 
> Although why is a version bump happening? I don't see any change in how
> libapparmor is used in this patch.

This should probably actually have been part of Attachment #127881 [details], where it is needed for aa_features_new_from_kernel().

> It's a shame there are no unit tests for AppArmor
> in dbus-daemon, although it would only be possible to run them on
> AppArmor-enabled kernels. (I'm not asking you to write an entire new test
> suite.)

AppArmor is inherently system-wide (and testing it requires CAP_MAC_ADMIN), so unit tests are hard. The best we can do most of the time is to have integration (system-level) tests.

Unfortunately, the more general tests for AppArmor D-Bus mediation are in the apparmor source tree, and in Ubuntu-specific repositories <https://wiki.ubuntu.com/Process/Merges/TestPlans/AppArmor>.

My intention is to not duplicate any of the AppArmor or Ubuntu stuff (as you say, I'm not writing a whole new test suite!), but to tests to dbus where feasible when adding new functionality.
Comment 19 Simon McVittie 2016-11-21 20:38:10 UTC
Comment on attachment 127878 [details] [review]
Spec: document what auto-starting is, and recommend it

Moved to Bug #98671, and updated there
Comment 20 Simon McVittie 2016-11-21 20:38:16 UTC
Comment on attachment 127879 [details] [review]
Spec: document systemd activation

Moved to Bug #98671, and updated there
Comment 21 Simon McVittie 2016-11-21 20:38:49 UTC
Depends on Bug #98671, without which Attachment #127880 [details] won't apply
Comment 22 Simon McVittie 2016-11-21 20:56:16 UTC
(In reply to Philip Withnall from comment #9)
> Nitpick: You might want to reindent the argument names here? The downsides
> of aligning them all. :-(
> 
> (And in selinux.h if you do this.)

I have considered and rejected this particular comment. I don't want to reindent the whole lot, and the approach I've taken to indentation here has prior art in the declaration of bus_context_log_literal() and friends.
Comment 23 Philip Withnall 2016-11-21 20:57:57 UTC
(In reply to Simon McVittie from comment #15)
> (In reply to Philip Withnall from comment #9)
> > Nitpick: You might want to reindent the argument names here? The downsides
> > of aligning them all. :-(
> 
> Possibly. I tend to lean towards "if it looks halfway reasonable, it's
> sufficiently-aligned" to avoid giant diffstats, and it's hard to align long
> names without breaking the 80-column guideline.

Sounds like a reasonable compromise to me.

(In reply to Simon McVittie from comment #16)
> (In reply to Philip Withnall from comment #10)
> > > +      <para>
> > > +        With D-Bus, starting a service is normally done by
> > > +        <firstterm>auto-starting</firstterm>. That is, applications send a
> > 
> > Perhaps mention that it's often (incorrectly?) called 'service activation',
> > just to make sure that people grep to the right place in the document when
> > keying off that term.
> 
> The shades of meaning obviously aren't made clear enough in the spec,
> because what you've said here doesn't agree with my understanding.
> 
> The way I think the terminology works (which could be wrong) is:
> 
> * activation is anything that makes dbus-daemon start a service on-demand
>   (either directly, via the setuid helper, or via systemd)
> 
> * auto-starting is when com.example.Foo is activated as a side-effect
>   of sending a message with destination=com.example.Foo and without
>   the NO_AUTO_START flag
> 
> * StartServiceByName() is also activation, but is not auto-starting

I think the use of ‘starting’ and ‘activation’ is a bit confusing. If I’ve understood correctly, they are equivalent. (But not equivalent to ‘auto-starting’, of course.)

> > Nitpick: Do we want to be pedantic enough to include version numbers in the
> > example well-known names in the specification, to reinforce the versioning
> > recommendations from
> > https://dbus.freedesktop.org/doc/dbus-api-design.html#api-versioning? I
> > could easily see it being an unnecessary distraction here though.
> 
> We have not traditionally done so. I wouldn't mind doing that as a separate
> change.

+1 for doing it as a separate change.

> > Do you want to point out in this paragraph that `StartServiceByName` is racy
> > and therefore less recommended than auto-starting?
> 
> Yes, perhaps that would be good. I think that would be best as a separate
> change.

+1.

(In reply to Simon McVittie from comment #17)
> (In reply to Philip Withnall from comment #12)
> > Perhaps add a sentence clarifying that the second check is guaranteed to be
> > stronger (in the sense that it will reject a non-strict superset of what the
> > first check would reject).
> 
> That isn't actually entirely true, though. It is possible (although AIUI not
> recommended) to write AppArmor profiles that work on a blacklist basis
> ("allow everything except...", by using `deny` rules.

Erk. In that case, that should definitely be mentioned somewhere, as otherwise some idiot like me could easily assume what I said.
Comment 24 Simon McVittie 2016-11-21 21:21:00 UTC
Created attachment 128111 [details] [review]
Do not auto-activate services if we could not send a  message

We specifically do not check recipient policies, because
the recipient policy is based on properties of the
recipient process (in particular, its uid), which we do
not necessarily know until we have already started it.

In this initial implementation we do not check LSMs either,
because we cannot know what LSM context the recipient process
is going to have. However, LSM support will need to be added
to make this feature useful, because StartServiceByName is
normally allowed in non-LSM environments, and is more
powerful than auto-activation anyway.

The StartServiceByName method does not go through this check,
because if access to that method has been granted, then
it's somewhat obvious that you can start arbitrary services.

---

Expand comment as Philip suggested
Comment 25 Simon McVittie 2016-11-21 21:21:49 UTC
Created attachment 128112 [details] [review]
Mediate auto-activation attempts through AppArmor

Because the recipient process is not yet available, we have to make some
assumption about its AppArmor profile. Parsing the first word of
the Exec value and then chasing symlinks seems like too much magic,
so I've gone for something more explicit. If the .service file contains

AssumedAppArmorLabel=/foo/bar

then we will do the AppArmor query on the assumption that the recipient
AppArmor label will be as stated. Otherwise, we will do a query
with an unspecified label, which means that AppArmor rules that do
specify a peer label will never match it.

Regardless of the result of this query, we will do an independent
AppArmor query when the activation has actually happened, this time
with the correct peer label; that second query will still be used
to decide whether to deliver the message. As a result, if this change
has any effect, it is to make the bus more restrictive; it does not
allow anything that would previously have been denied.

---

Do not bump libapparmor dependency: that's only needed for the test.
Comment 26 Simon McVittie 2016-11-21 21:24:11 UTC
Created attachment 128113 [details] [review]
Spec: document AppArmor mediation of auto-starting

---

Link to AppArmor documentation as requested.

Describe the circumstances under which the pre-check could reject a message that the post-check would in fact have accepted (label-based "deny" rules basically).

Clarify that an unspecified label is not the same thing as "unconfined".

Confirm that label-based send rejection does not require that there is in fact a profile of that name loaded into the kernel (I've tried it and it works, patch to follow).
Comment 27 Simon McVittie 2016-11-21 21:25:43 UTC
Created attachment 128114 [details] [review]
Add an integration test for AppArmor mediating  activation

This requires libapparmor 2.10, for aa_features_new_from_kernel()
and related functions.

---

Move the required-version bump here.

I'm not bothering to make it conditional on installed-tests being enabled, because AIUI AppArmor D-Bus mediation only works on Ubuntu derivatives with their patched kernels, and if you're sufficiently serious about AppArmor to have patched your kernel for it, then you're certainly sufficiently serious about AppArmor to be tracking Ubuntu's libapparmor.
Comment 28 Simon McVittie 2016-11-21 21:27:31 UTC
Created attachment 128115 [details] [review]
Activation test: exercise what happens with nonexistent  AppArmor labels

---

It works like you would hope: AssumedAppArmorLabel being set to a label whose associated profile is not present in the kernel works just the same as if it was set to a label that *does* have a loaded profile. Either way, a simple textual match is done to decide whether peer=(label=X) rules match it.
Comment 29 Philip Withnall 2016-11-21 21:30:29 UTC
Comment on attachment 127881 [details] [review]
Add an integration test for AppArmor mediating activation

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

r+

::: test/data/dbus-installed-tests.aaprofile.in
@@ +1,1 @@
> +# Copyright © 2016 Collabora Ltd.

Copyright symbol seems to have been corrupted here, but I assume that’s Splinter/Bugzilla/git-bz, rather than in your actual git commit.

@@ +47,5 @@
> +    dbus (send, receive, bind),
> +    network,
> +    signal,
> +
> +    deny dbus send peer=(label=@DBUS_TEST_EXEC@/test-apparmor-activation//com.example.SendDeniedByAppArmorLabel),

Note to anyone else who reviews: the double-slash before the last component is not a typo, it’s the labelling syntax for hats. (http://wiki.apparmor.net/index.php/AppArmor_Core_Policy_Reference#Hat_and_Local_profile_names)
Comment 30 Philip Withnall 2016-11-21 21:33:11 UTC
Comment on attachment 128111 [details] [review]
Do not auto-activate services if we could not send a  message

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

r+, assuming the comment change was the only one.
Comment 31 Philip Withnall 2016-11-21 21:34:40 UTC
Comment on attachment 128112 [details] [review]
Mediate auto-activation attempts through AppArmor

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

r+, assuming that dropping the dependency change is the only change. What I would give for an interdiff, Splinter.
Comment 32 Simon McVittie 2016-11-21 21:38:00 UTC
(In reply to Philip Withnall from comment #29)
> Copyright symbol seems to have been corrupted here, but I assume that’s
> Splinter/Bugzilla/git-bz, rather than in your actual git commit.

Yes, Splinter is (still) defiantly Latin-1 :-(

(or should that be U+1F64D PERSON FROWNING?)

> > +    deny dbus send peer=(label=@DBUS_TEST_EXEC@/test-apparmor-activation//com.example.SendDeniedByAppArmorLabel),
> 
> Note to anyone else who reviews: the double-slash before the last component
> is not a typo, it’s the labelling syntax for hats.
> (http://wiki.apparmor.net/index.php/
> AppArmor_Core_Policy_Reference#Hat_and_Local_profile_names)

It is actually the syntax for local ("child") profiles, but hats are a special case of child profiles: you can put a hat on with aa_change_hat() without special privileges, and you can take the hat off by knowing the magic token you passed to aa_change_hat(), whereas transitioning into or out of a child profile declared with "profile whatever {...}" requires change_profile rights.
Comment 33 Philip Withnall 2016-11-21 21:40:19 UTC
Comment on attachment 128113 [details] [review]
Spec: document AppArmor mediation of auto-starting

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

::: doc/dbus-specification.xml
@@ +5241,5 @@
> +          without specifying any particular label. In particular, any rule of
> +          the form
> +          <literal>dbus send peer=(label=X)</literal> (for any value of X,
> +          including the special label <literal>unconfined</literal>)
> +          will not allow the auto-start, and any rule of the form

‘will not allow’ could be confused for ‘will deny’. Could we clarify this a bit — perhaps as ‘will not allow the auto-start if it would otherwise be denied’?

@@ +5243,5 @@
> +          <literal>dbus send peer=(label=X)</literal> (for any value of X,
> +          including the special label <literal>unconfined</literal>)
> +          will not allow the auto-start, and any rule of the form
> +          <literal>deny dbus send peer=(label=X)</literal>
> +          will not deny it.

Probably need to make a similar change to ‘will not deny it’ for symmetry.
Comment 34 Philip Withnall 2016-11-21 21:41:22 UTC
Comment on attachment 128114 [details] [review]
Add an integration test for AppArmor mediating  activation

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

r+ as per my previous review, assuming the configure.ac version bump is the only change.
Comment 35 Philip Withnall 2016-11-21 21:43:09 UTC
Comment on attachment 128115 [details] [review]
Activation test: exercise what happens with nonexistent  AppArmor labels

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

r+
Comment 36 Simon McVittie 2016-11-22 11:42:50 UTC
Created attachment 128142 [details] [review]
Spec: document AppArmor mediation of auto-starting

---

Rewrite the paragraph starting "Otherwise, AppArmor mediation of messages that auto-start...".

Previously, it said "will not allow" and "will not deny", which were intended to mean "will not be checked, preserving the allow/deny state resulting from other rules", but are too subtle: they could easily be misinterpreted as "will deny" and "will allow" (overruling other applicable rules).

Instead, phrase it as "will not influence" which is hopefully clearer about meaning these rules are not considered.
Comment 37 Philip Withnall 2016-11-23 10:33:11 UTC
Comment on attachment 128142 [details] [review]
Spec: document AppArmor mediation of auto-starting

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

r+
Comment 38 Simon McVittie 2016-11-28 15:50:59 UTC
Created attachment 128243 [details] [review]
Don't test AppArmor mediation of activation if libapparmor <  2.10

We need libapparmor 2.10 for the test, but not for the actual
functionality, for which 2.8.95 is enough. In particular this lets
us compile with AppArmor enabled on Ubuntu 14.04, which is still
the newest host platform available on travis-ci.

---

Otherwise, the build with AppArmor explicitly enabled (to check the code path where we have AppArmor and SELinux but no libaudit) fails: https://travis-ci.org/smcv/dbus/builds/179417330

With this change, the builds all succeed: https://travis-ci.org/smcv/dbus/builds/179417321

I also have a patch-set for testing dbus under Debian stable, Debian testing and Ubuntu LTS on travis-ci, by using Docker, which will exercise this new test; but that is going to need a bit more testing, and is really a separate topic.
Comment 39 Simon McVittie 2016-11-28 17:45:48 UTC
I've applied all the patches Philip reviewed (that's all of them except Attachment #128243 [details]) for dbus 1.11.8 (D-Bus Specification 0.30).

(In reply to Simon McVittie from comment #38)
> I also have a patch-set for testing dbus under Debian stable, Debian testing
> and Ubuntu LTS on travis-ci, by using Docker, which will exercise this new
> test

Actually no it won't, because the Docker container doesn't have access to AppArmor (I'm not sure whether AppArmor is even available on Travis-CI hosts).

I would still like to apply Attachment #128243 [details], but I don't consider it to be a release-blocker. I'll test the new dbus 1.11.8 on Ubuntu xenial before releasing.
Comment 40 Simon McVittie 2016-11-29 01:01:06 UTC
Created attachment 128259 [details] [review]
activation test: don't crash if AppArmor is built but  unavailable

Also don't try to clean up a process we didn't start.
Comment 41 Philip Withnall 2016-11-29 11:02:21 UTC
Comment on attachment 128243 [details] [review]
Don't test AppArmor mediation of activation if libapparmor <  2.10

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

r+
Comment 42 Philip Withnall 2016-11-29 11:03:16 UTC
Comment on attachment 128259 [details] [review]
activation test: don't crash if AppArmor is built but  unavailable

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

r+
Comment 43 Simon McVittie 2016-11-29 12:33:34 UTC
Feature available in 1.11.8; follow-up test fixes are in git master for 1.11.10.


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.