| Summary: | [PATCH] launch-helper: current implementation violates dbus spec | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Chengwei Yang <chengwei.yang.cn> |
| Component: | core | Assignee: | Havoc Pennington <hp> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | medium | CC: | chengwei.yang.cn, thiago, walters |
| Version: | 1.5 | Keywords: | patch |
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | review? | ||
| i915 platform: | i915 features: | ||
| Attachments: |
[PATCH 1/2] Fix: launch-helper voilate dbus spec
[PATCH 2/2] Test: add a test case for launch-helper [PATCH v2 1/2] Fix: launch-helper voilate dbus spec [PATCH v2 2/2] Test: add a test case for launch-helper spec: system services' service description files have constrained names spec: briefly describe Name, Exec and User keys [PATCH] Spec: two services offer the same service is only available for session bus [PATCH v2] Spec: document multiple .service files own the same well known name |
||
Basicly, there are two choices. a. change the arg[1] of servicehelper from bus name to filename, so we need do a lot of *rename* work in servicehelper to make less confusing. e.g. bus_name --> filename. b. keep current logic, so servicehelper find files (if multiple owner activation implemented #bug66484) from bus name. In this way, no rename needed, but I think we need export BusActivation visible to servicehelper, so it's very simple to find files from bus name, or just recurse all the service dirs and compare bus name one by one. (In reply to comment #1) > Basicly, there are two choices. > > a. change the arg[1] of servicehelper from bus name to filename, so we need > do a lot of *rename* work in servicehelper to make less confusing. e.g. > bus_name --> filename. > > b. keep current logic, so servicehelper find files (if multiple owner > activation implemented #bug66484) from bus name. In this way, no rename > needed, but I think we need export BusActivation visible to servicehelper, > so it's very simple to find files from bus name, or just recurse all the > service dirs and compare bus name one by one. c. fix this first considering to #bug66484 has a long way to go. So this patch just find the first owner and activate it. Created attachment 82205 [details] [review] [PATCH 1/2] Fix: launch-helper voilate dbus spec Created attachment 82206 [details] [review] [PATCH 2/2] Test: add a test case for launch-helper Created attachment 82314 [details] [review] [PATCH v2 1/2] Fix: launch-helper voilate dbus spec Created attachment 82315 [details] [review] [PATCH v2 2/2] Test: add a test case for launch-helper (In reply to comment #0) > E.g. a service file own bus name "org.test.name" must have file name > org.test.name.service. I think this might be a security feature? It certainly simplifies the launch helper, and the launch helper is security-sensitive, so simplification is welcome. See doc/system-activation.txt. > As I known, this is not a requirement of DBus spec. Then the spec (for system bus activation) should change. System bus activation is a security boundary, and should be as constrained as is practical. On the session bus, the fact that there can be multiple implementations of a name makes activation non-deterministic. System bus activation should be deterministic, and unlike session bus activation, it didn't have backwards-compatibility concerns (because it was a new feature). As you know, there are two kinds of well-known message bus, system bus and session bus. And for system bus, a setuid <servicehelper> is optional. When <servicehelper> is used, it has the constraintion that the .service file must named after the service name. If <servicehelper> isn't used and the system bus daemon running as root, for example, <user>root</user> specified by user, it has no the constraintion that the .service file must named after the service name. So it's inconsistent even in the system bus just because <servicehelper> used or not. The available combination can be summarised into below table. CASE BUS TEYPE USE SERVICEHELPER HAS ABOVE CONSTRAINTION? 1 system yes yes 2 system no no 3 session invalid no (In reply to comment #7) > (In reply to comment #0) > > E.g. a service file own bus name "org.test.name" must have file name > > org.test.name.service. > > I think this might be a security feature? It certainly simplifies the launch I think negative, since <servicehelper> is optional. <user>root</root> can be specified by user and no <servicehelper> is configured. > helper, and the launch helper is security-sensitive, so simplification is > welcome. See doc/system-activation.txt. Yes, simple is good, however, at least do the right thing. I think this patch isn't too complicated to apply, just tens of lines change but do the correct thing comply to the dbus specification. In addition, this patch unifies the above 3 situations into 1. In all cases there is no constraintion that the .service file must named after its service name. I do think that align our reference implementation with the spec is better than change the spec, especially if there are other implementations. > > > As I known, this is not a requirement of DBus spec. > > Then the spec (for system bus activation) should change. System bus > activation is a security boundary, and should be as constrained as is > practical. > > On the session bus, the fact that there can be multiple implementations of a > name makes activation non-deterministic. System bus activation should be > deterministic, and unlike session bus activation, it didn't have > backwards-compatibility concerns (because it was a new feature). (In reply to comment #8) > for system bus, a setuid <servicehelper> is optional. When > <servicehelper> is used, it has the constraintion that the .service file > must named after the service name. If <servicehelper> isn't used and the > system bus daemon running as root, for example, <user>root</user> specified > by user, it has no the constraintion that the .service file must named after > the service name. OS vendors and sysadmins are expected to either not change the system bus configuration, or understand the security implications if they do. Running the system dbus-daemon as root with the setuid servicehelper is inadvisable: it increases the amount of privileged code running, for no good reason. OS vendors are expected to set up a uid for the system dbus-daemon (typically called "dbus" or "messagebus"). Running the system dbus-daemon as root *without* the servicehelper is even less safe than that: it results in every activated system service running as root! Don't do that. I don't consider either of those configurations to be something we are willing to support, and I'm not going to increase the complexity of security-sensitive code without a very good reason. "The spec is more lenient than the implementation" is not a good enough reason for me unless there are also backward compatibility considerations, I'm afraid. The reference implementation of system bus activation is strict, and I'm not aware of any independent production-quality implementations of activation[1], let alone system activation, so any application relying on the existing spec wording clearly didn't actually work (and we aren't going to make it any worse). I'd much prefer to document that activatable system services' service files must be named after the bus name + ".service", and that it is recommended for activatable session services' service files to follow the same convention. If activating a system service can launch an arbitrarily chosen one of several implementations, that makes the OS more fragile. OSs are quite fragile enough already - we shouldn't exacerbate the problem! If D-Bus had a less strict approach to backwards compatibility (i.e. not breaking existing, working applications), I think we'd already have mandated that *all* services should use the system-style naming. "Don't break working applications" in library/service maintenance is just like "don't break userland" in the kernel: please stick to it. [1] GIO's gdbus-daemon doesn't implement activation at all; dbus-sharp's "managed dbus-daemon" implements session activation only, in a very simplistic way (it doesn't deal with two parallel activations of the same thing, for instance). Created attachment 85914 [details] [review] spec: system services' service description files have constrained names --- This documents current practice. I am not going to change the launch-helper to "do what I mean": it's security-sensitive, and any bugs here are probably vulnerabilities. Created attachment 85915 [details] [review] spec: briefly describe Name, Exec and User keys Comment on attachment 85914 [details] [review] spec: system services' service description files have constrained names Review of attachment 85914 [details] [review]: ----------------------------------------------------------------- Looks good to me. Comment on attachment 85915 [details] [review] spec: briefly describe Name, Exec and User keys Review of attachment 85915 [details] [review]: ----------------------------------------------------------------- Looks good. Created attachment 85957 [details] [review] [PATCH] Spec: two services offer the same service is only available for session bus Comment on attachment 85914 [details] [review] spec: system services' service description files have constrained names Applied for 1.7.6 (spec 0.22), thanks Comment on attachment 85915 [details] [review] spec: briefly describe Name, Exec and User keys Applied for 1.7.6 (spec 0.22), thanks Comment on attachment 85957 [details] [review] [PATCH] Spec: two services offer the same service is only available for session bus Review of attachment 85957 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-connection.c @@ +1098,4 @@ > * on our part. for now it doesn't matter, we will just > * end up back here eventually.) > */ > + _dbus_verbose ("timed out\n"); This is unrelated. ::: doc/dbus-specification.xml @@ +4664,5 @@ > find a service that will own that name. It then tries to spawn the > executable associated with it. If this fails, it will report an > + error. [FIXME what happens if two .service files offer the same service > + for the well-known session bus? what kind of error is reported, should > + we have a way for the client to choose one?] Strictly speaking the environment in which this error case is possible is "not the system bus" - as currently implemented, custom buses will have session-like semantics. I don't think it's really worth changing a FIXME comment. Fixing the FIXME by documenting current behaviour would be OK though, perhaps something like this: On the well-known system bus, it is not possible for two .service files in the same directory to offer the same service, because they are constrained to have names that match the service name. On the well-known session bus, if two .service files in the same directory offer the same service name, the result is undefined. Distributors should avoid this situation, for instance by naming session services' .service files according to their service name. If two .service files in different directories offer the same service name, the one in the higher-priority directory is used: for instance, on the system bus, .service files in /usr/local/share/dbus-1/system-services take precedence over those in /usr/share/dbus-1/system-services. Created attachment 87276 [details] [review] [PATCH v2] Spec: document multiple .service files own the same well known name Use Simon's words. Fixed in git for 1.7.6, thanks. I added some </para><para> to your patch so the paragraph breaks come out right. |
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.
In current activation service helper implementaion, it needs the service file named after its bus name with .service suffix. E.g. a service file own bus name "org.test.name" must have file name org.test.name.service. As I known, this is not a requirement of DBus spec. The implementation glue: activation.c /* join the helper path and the service name */ if (!_dbus_string_append (&command, servicehelper)) { _dbus_string_free (&command); BUS_SET_OOM (error); return FALSE; } if (!_dbus_string_append (&command, " ")) { _dbus_string_free (&command); BUS_SET_OOM (error); return FALSE; } if (!_dbus_string_append (&command, service_name)) { _dbus_string_free (&command); BUS_SET_OOM (error); return FALSE; } And in activation-helper.c if (!_dbus_string_append (&filename, name) || !_dbus_string_append (&filename, ".service")) { BUS_SET_OOM (error); goto out; } It just treats the service_name as .service filename without .service suffix. That's incorrectly.