|
Description
Simon McVittie
2016-11-09 19:40:44 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. 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. 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. 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. 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. Created attachment 127880 [details] [review] Spec: document AppArmor mediation of auto-starting 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 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 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 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 on attachment 127879 [details] [review] Spec: document systemd activation Review of attachment 127879 [details] [review]: ----------------------------------------------------------------- Nice. 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 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.) 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. (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. (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. (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. (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 on attachment 127878 [details] [review] Spec: document what auto-starting is, and recommend it Moved to Bug #98671, and updated there Comment on attachment 127879 [details] [review] Spec: document systemd activation Moved to Bug #98671, and updated there Depends on Bug #98671, without which Attachment #127880 [details] won't apply (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. (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. 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 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. 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). 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. 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 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 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 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. (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 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 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 on attachment 128115 [details] [review] Activation test: exercise what happens with nonexistent AppArmor labels Review of attachment 128115 [details] [review]: ----------------------------------------------------------------- r+ 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 on attachment 128142 [details] [review] Spec: document AppArmor mediation of auto-starting Review of attachment 128142 [details] [review]: ----------------------------------------------------------------- r+ 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. 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. 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 on attachment 128243 [details] [review] Don't test AppArmor mediation of activation if libapparmor < 2.10 Review of attachment 128243 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 128259 [details] [review] activation test: don't crash if AppArmor is built but unavailable Review of attachment 128259 [details] [review]: ----------------------------------------------------------------- r+ 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.