Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
interfaces/builtin: add location interface #1118
Conversation
zyga
reviewed
May 3, 2016
| +# gives privileged access to the system. | ||
| +# Usage: reserved | ||
| + | ||
| + # DBus accesses |
zyga
May 3, 2016
Contributor
Since we're seeing the same dbus rules copy-pasted in various places I was wondering if this should be something we can stick in a function somewhere and reuse. I'd also like to see at least one # comment above them to explain what each of those dbus (...) rules do so that other people don't have to wonder.
zyga
reviewed
May 3, 2016
| + dbus (receive, send) | ||
| + bus=system | ||
| + path=/ | ||
| + interface=org.freedesktop.DBus**, |
zyga
reviewed
May 3, 2016
| + } | ||
| + snippet, err := s.iface.ConnectedPlugSnippet(s.plug, slot, interfaces.SecurityAppArmor) | ||
| + c.Assert(err, IsNil) | ||
| + c.Assert(string(snippet), testutil.Contains, "peer=(label=snap.location-service.{app1,app2}),") |
zyga
May 3, 2016
•
Edited 1 time
-
zyga
May 3, 2016
Contributor
-
May 3, 2016
This will need small refresh after #1117 lands
niemeyer
reviewed
May 3, 2016
| +type LocationServiceInterface struct{} | ||
| + | ||
| +func (iface *LocationServiceInterface) Name() string { | ||
| + return "location-service" |
niemeyer
May 3, 2016
Contributor
Let's rename this to simply "location" please, and rename the code accordingly.
niemeyer
reviewed
May 3, 2016
| + interface=org.freedesktop.DBus.Properties | ||
| + peer=(label=unconfined), | ||
| + | ||
| + # Allow binding the service to the requested connection names |
niemeyer
May 3, 2016
Contributor
That seems right to be on the permanent slot. But why do we allow below for the service to be sending/receiving on its own interfaces permanently? Does it need to talk to itself when not connected to another snap? If not, maybe we can constrain these further and only allow the snap to talk to peers that are connected too. Something worth trying.
ssweeny
May 3, 2016
Contributor
Some of the included providers talk to the service over dbus, so L-S does need to talk to itself. It may not need to do so if there are no connected clients, so I'll look into that.
jdstrand
reviewed
May 3, 2016
| + path=/org/freedesktop/DBus | ||
| + interface=org.freedesktop.DBus | ||
| + member={Request,Release}Name | ||
| + peer=(name=org.freedesktop.DBus), |
jdstrand
May 3, 2016
Contributor
Should this use peer=(name=org.freedesktop.DBus, label=unconfined)?
jdstrand
reviewed
May 3, 2016
| + path=/org/freedesktop/DBus | ||
| + interface=org.freedesktop.DBus | ||
| + member=GetConnectionUnixUser | ||
| + peer=(label=unconfined), |
jdstrand
May 3, 2016
Contributor
These can be combined like so:
dbus (receive, send) bus=system path=/org/freedesktop/DBus interface=org.freedesktop.DBus member=GetConnectionUnix{ProcessID,User} peer=(label=unconfined),
That said, shouldn't this both just use (send)?
jdstrand
reviewed
May 3, 2016
| + bus=system | ||
| + path=/org/freedesktop/* | ||
| + interface=org.freedesktop.DBus.Properties | ||
| + peer=(label=unconfined), |
jdstrand
reviewed
May 3, 2016
| + dbus (receive, send) | ||
| + bus=system | ||
| + path=/com/ubuntu/location/Service{,/**} | ||
| + interface=com.ubuntu.location.Service*, |
jdstrand
May 3, 2016
•
Edited 1 time
-
jdstrand
May 3, 2016
Contributor
-
May 3, 2016
This is pretty broad and says anything can use this (globbed) interface and all paths. It is hard to recommend how to fix since it is so general. I suggest adding peer=(label=@{profile_name}) for these and then add ConnectedSlot rules for server side rules for specific clients for any other rules. In this manner, both sides of the connection are mediated (client to server and server to client).
I know with 15.04 we said that we'd have wide open policy for the service and let the client rules handle things, but with interfaces we can be much more precise. I think I suggested this with either bluez and/or nm, but it doesn't look like it was implemented. Sorry the recommendations are changing a bit but now that we are understanding how permanent slots, connected slots and connected plugs can work together, we should take advantage of it. (not quite sure how to utilize PermanentPlug yet, but we'll see).
Once this is worked out for location service, please follow up on bluez and network-manager.
Thanks!
jdstrand
reviewed
May 3, 2016
| + bus=system | ||
| + path=/sessions/* | ||
| + interface=com.ubuntu.location.Service.Session | ||
| + member={Start,Stop}VelocityUpdates, |
jdstrand
reviewed
May 3, 2016
| + dbus (receive, send) | ||
| + bus=system | ||
| + path=/com/ubuntu/location/Service{,/**} | ||
| + interface=org.freedesktop.DBus**, |
jdstrand
reviewed
May 3, 2016
| + | ||
| +dbus (send) | ||
| + bus=system | ||
| + peer=(name=com.ubuntu.location.Service, label=unconfined), |
jdstrand
May 3, 2016
Contributor
I don't understand this rule. It is too general and the label for unconfined is confusing (the server side should be handled be the previous rule). Is this for dbus-daemon interface and or paths? If so, please be more specific.
jdstrand
reviewed
May 3, 2016
| +# Allow all access to location service | ||
| +dbus (receive, send) | ||
| + bus=system | ||
| + peer=(label=###SLOT_SECURITY_TAGS###), |
jdstrand
May 3, 2016
Contributor
This is saying that the entire location-service API is and always will be ok for the location. I see you said "Allow all access to location service" but I wanted to call this out.
niemeyer
May 3, 2016
Contributor
Yeah, this seems rather dangerous. It assumes we know what will be exposed via this API in the future, which is always a bad bet in terms of security.
jdstrand
reviewed
May 3, 2016
| + | ||
| +dbus (send) | ||
| + bus=system | ||
| + peer=(name=com.ubuntu.location.Service.Session, label=unconfined), |
niemeyer
changed the title from
interfaces: builtin: Add location-service interface
to
interfaces/builtin: add location interface
May 3, 2016
jdstrand
reviewed
May 3, 2016
| +recvmsg | ||
| +sendmsg | ||
| +sendto | ||
| +`) |
jdstrand
May 3, 2016
Contributor
@zyga - in some ways I feel like it would be convenient if we could have a function for dbus seccomp slot/plug policy that all these snaps could use, since it is just getting copied around. The downside is it make policy audits quite a bit harder.
That should not block this PR.
jdstrand
reviewed
May 3, 2016
| + <allow send_destination="com.ubuntu.location.Service"/> | ||
| + <allow send_destination="com.ubuntu.location.Service.Session"/> | ||
| + <allow send_interface="com.ubuntu.location.Service"/> | ||
| + <allow send_interface="com.ubuntu.location.Service.Session"/> |
|
Several good comments from Jamie. Let's see if we can iron these details out until Jamie is pleased with this interface, and then take the lessons on to bluez and network-manager. |
jdstrand
reviewed
May 3, 2016
| + <allow send_destination="com.ubuntu.location.Service"/> | ||
| + <allow send_destination="com.ubuntu.location.Service.Session"/> | ||
| + <allow send_interface="com.ubuntu.location.Service"/> | ||
| + <allow send_interface="com.ubuntu.location.Service.Session"/> |
jdstrand
May 3, 2016
Contributor
In addition to the whitespace issues, how this is supposed to work is that the server side binds to com.ubuntu.location.Service and the client side to com.ubuntu.location.Service.Session. Clients talk to com.ubuntu.location.Service and the service can talk to clients via com.ubuntu.location.Service.Session?
If that is accurate, why does PermanentSlotAppArmor allow the server to bind to both? Shouldn't it only allow binding to com.ubuntu.location.Service and the ConnectedPlugAppArmor policy allow binding to com.ubuntu.location.Service.Session? Otherwise, can you elaborate on how this works?
ssweeny
added some commits
May 5, 2016
jdstrand
reviewed
May 6, 2016
| +dbus (receive, send) | ||
| + bus=system | ||
| + path=/ | ||
| + interface=org.freedesktop.DBus*, |
jdstrand
May 6, 2016
Contributor
If you put 'audit' in front of this rules (ie, 'audit dbus') and use location-service, it will generate a log entry. Can you paste these log entries here? I'm trying to understand what 'label=' should be use here).
ssweeny
May 6, 2016
Contributor
@jdstrand When I did this it added no additional lines to the log, so I tried removing it which had no ill effects. I'm dropping this stanza.
jdstrand
reviewed
May 6, 2016
| + bus=system | ||
| + path=/sessions/* | ||
| + interface=com.ubuntu.location.Service.Session | ||
| + member={Start,Stop}VelocityUpdates, |
jdstrand
May 6, 2016
Contributor
Please use quotes when using AARE with dbus rules. Put plainly, use: member="{Start,Stop}VelocityUpdates",
jdstrand
reviewed
May 6, 2016
| + bus=system | ||
| + path=/com/ubuntu/location/Service | ||
| + interface=org.freedesktop.DBus.Properties | ||
| + member=PropertiesChanged, |
jdstrand
May 6, 2016
Contributor
Nice! Note, you can combing many of these: eg, member="Update{Heading,Position,Velocity}"
jdstrand
May 6, 2016
Contributor
Now that we have all of these client rules, we should add peer=(label=###PLUG_SECURITY_TAGS###) for all of these (ie, the corresponding rule to your peer=(label=###SLOT_SECURITY_TAGS###) in locationConnectedPlugAppArmor).
jdstrand
reviewed
May 6, 2016
| + path=/com/ubuntu/location/Service | ||
| + interface=org.freedesktop.DBus.Properties | ||
| + member=Set | ||
| + peer=(label=###SLOT_SECURITY_TAGS###), |
jdstrand
reviewed
May 6, 2016
| + path=/sessions/* | ||
| + interface=com.ubuntu.location.Service.Session | ||
| + member=UpdateVelocity | ||
| + peer=(label=###SLOT_SECURITY_TAGS###), |
jdstrand
May 6, 2016
Contributor
These can be combined with: member="Update{Heading,Position,Velocity}"
jdstrand
reviewed
May 6, 2016
| + bus=system | ||
| + path=/ | ||
| + interface=org.freedesktop.DBus.ObjectManager | ||
| + peer=(label=unconfined), |
jdstrand
May 6, 2016
Contributor
This is probably ok, but I wonder if there are particular methods you are using and that might form a sort of boilerplate policy for all DBus services to ease maintenance.
|
The policy is really shaping up. Thanks! I'm curious it you have test scripts, etc for this that could perhaps be added to the integration tests. Ie, it would be great to have a client connect to the service and then exercise the API and make sure the policy is correct. This would be great to have for bluez, network-manager, pulseaudio, mir, etc, etc too. :) |
ssweeny
added some commits
May 6, 2016
|
My testing involves installing the snap in a VM (using @zyga's devtools) and running a couple of binaries to test the client/server connection. If that sort of thing is supported in the integration tests then I can add it. |
ssweeny
reviewed
May 6, 2016
| + | ||
| +func (iface *LocationInterface) ConnectedSlotSnippet(plug *interfaces.Plug, slot *interfaces.Slot, securitySystem interfaces.SecuritySystem) ([]byte, error) { | ||
| + switch securitySystem { | ||
| + // FIXME: This doesn't handle multiple plugs |
ssweeny
May 6, 2016
Contributor
@zyga I couldn't figure out how this should handle multiple plugs being connected to this slot.
zyga
May 17, 2016
Contributor
Remember that ConnectedSlotSnippet is called once for each connected (plug, slot) pair. Just return the data specific to a given connection here.
jdstrand
reviewed
May 6, 2016
| + } | ||
| + buf.WriteByte('"') | ||
| + return buf.Bytes() | ||
| +} |
jdstrand
May 6, 2016
Contributor
This function is identical to slotAppLabelExpr() except it takes a interfaces.Plug as an argument instead of interfaces.Slot. This code should be shared.
ssweeny
May 6, 2016
Contributor
I decided to make it a separate function in case changes would be required to handle multiple plugs.
jdstrand
reviewed
May 6, 2016
| + old := []byte("###PLUG_SECURITY_TAGS###") | ||
| + new := plugAppLabelExpr(plug) | ||
| + snippet := bytes.Replace(locationConnectedSlotAppArmor, old, new, -1) | ||
| + return snippet, nil |
jdstrand
May 6, 2016
Contributor
@zyga - not sure if this was considered when implementing this. There is a 1 slot to many plugs relationship so ConnectedPlug policy is fine to substitute ###SLOT_SECURITY_TAGS### for the slot, but ConnectedSlot policy needs to enumerate all the plugs that are connected to the slot to generate the correct label string for ###PLUG_SECURITY_TAGS###. @ssweeny and I weren't sure how to do that in the current code, if it was planned, etc.
zyga
May 18, 2016
Contributor
Hmm
This is not doable with the current API. The problem is that we'll get N snippets, not one snippet that understands there are N connections in place.
zyga
May 18, 2016
•
Edited 1 time
-
zyga
May 18, 2016
Contributor
-
May 18, 2016
Thinking about it, the pseudo-code for the process is as follows:
snippets := getAllSnippets(...)
blob := combineSnippets(snippets, "dbus")
Is there something we could do in the combine phase to merge such rules together?
Is there a solution that would not require us to combine this on snapd side and would make it somehow magically work on apparmor side?
EDIT: @jdstrand ^^
jdstrand
May 19, 2016
•
Edited 1 time
-
jdstrand
May 19, 2016
Contributor
-
May 19, 2016
"Is there a solution that would not require us to combine this on snapd side and would make it somehow magically work on apparmor side?"
Yes, not match by label which was what the original patch did. However, I requested this change because snapd is in a position to know all the labels so we can and should be much more precise. In other words, we go through all this trouble to connect a plug to a slot so the policy should fully enforce that as opposed to letting anything connect to the server slot and then only adding client plug rules. This allows us to be most correct and adds layered security (both sides instead of one).
EDIT: with frameworks we had no choice but to only have client rules, but with interfaces we can improve on that.
@ssweeny that's a perfect case for an integration test, to get started you can take a look at the readme https://github.com/ubuntu-core/snappy/blob/master/integration-tests/README.md, also the current tests have lots of examples of how things can be done https://github.com/ubuntu-core/snappy/tree/master/integration-tests/tests. Please do not hesitate to ping me or @elopio in case you need any help. |
ssweeny
added some commits
May 16, 2016
|
The security policy LGTM now (thanks @zyga for clarifying the point regarding ConnectedSlotSnippet being called for each connection-- it might be worth documenting that in docs/....). I do want to explicitly call out that compared to network-manager, this bus policy is both very simple and very open. I'm not saying that is a problem though since the interface is not autoconnected and apparmor will mediate unconnected snaps, just doing due diligence. :) |
|
Let's please talk about the "very open" question online @jdstrand. |
ssweeny
added some commits
May 19, 2016
|
Going in per discussions online on the channel today. @ssweeny please don't forget the tests, when you have a moment. |
ssweeny commentedMay 3, 2016
Add the location-service interface.
There were no pre-existing AppArmor or SecComp rules for the location-service so I had to write those from scratch, and I expect some cleanup will be needed.