interfaces: add unity8 plug permissions

[mikix]

This builds upon the u8-base PR (#2786) to add some permissions for unity8 apps that plug into the interface.

It's mostly some dbus allowances for session services that the unity8 session will provide. I tried to be somewhat minimal: I tested that a confined ubuntu-calculator-app can launch in a unity8 session with this. I haven't done extensive testing beyond that. I imagine we'll add more to this over time.

(Some of this is Ted's work repackaged.)

@jdstrand can you look at this?

[jdstrand]

In addition to the above changes, please add some tests to make sure the connected policy is present. Please see TestConnectedPlugSnippetAppArmor and TestConnectedPlugSnippetSecComp from ofono_test.go (though feel free to just use one function, TestConnectedPlugSnippets, that tests both in one function).

[jdstrand]

Why isn't the unity8-session snap using /run/user/[0-9]*/mir_socket? What this rule seems to be saying is that the location of the mir socket will vary based on the slot implementation, which is a tight coupling between a specific slot implementation and the snap and the dependency required to make this tight coupling work cannot be expressed in snap/snapcraft.yaml. More concretely, this means that the slot's socket is in /run/user/1000/snap.unity8-session/mir_socket and a unity8 plugging app would need to look for it there. An alternate implementation of unity8 would be at /run/user/1000/snap.unity8-session-somethingelse/mir_socket and all snaps that use 'plugs: [ unity8 ]' will not be able to find it.

The purpose of interfaces is that they are a contract between the slot side and the plug side such that a plug doesn't have to care what slot snap it is connecting to and vice versa.

[mikix]

So this is an interesting issue. Unity8 by default just puts the socket in XDG_RUNTIME_DIR, which is how that path ended up being what it is. That's certainly changeable to be hardcoded instead.

However, this does not necessarily create a tight coupling to our slot implementation. That socket path is exported to apps via an environment variable, passed by ubuntu-app-launch I believe. So there's no hard-coding on the plug side. (Although it does mean we rely on our slot's implementation of ubuntu-app-launch which probably isn't good.)

So because of needing UAL, and because my approach does leak the name of the slot, it makes sense to use the global path instead.

There is one downside to that though, which is that we can't have nested Mir servers that way. This mir interface allows a top-level (/run/mir_socket), one nested one (user socket) and no more. If we allowed each slot to have its own socket, then we can be a little future proof in terms of what we want to allow with Mir servers.

But unless I hear a change of heart, I'll go ahead with your recommendation.

[jdstrand]

With the base declaration, you can drop the 'Usage' meta tag.

[jdstrand]

I suspect you will also want:

/var/cache/fontconfig/   r,
/var/cache/fontconfig/** mr,

[jdstrand]

This is a tad unfortunate and also not required on Touch (ie, this rule isn't in apparmor-easyprof-ubuntu policy). Can you comment? Perhaps this is only needed if the command fails?

[mikix]

Ah, I figured out why this was needed. Turns out it isn't, and I will remove. But backstory:

I tested first with unity8+x11 before moving to unity8+mir. Turns out that under X11, QInputInfo (in qtsystems) will call udev_monitor_enable_receiving, which connects to udev via a netlink socket. Under Mir, it doesn't do this. So this isn't a unity8 interface problem, and I'll drop this from here.

However, maybe this bit should be added to the x11 and/or unity7 interfaces. Without it (or, more likely, without just adding the network interface), qml snaps can't launch. But that's a separate PR.

[jdstrand]

There is now a maliit interface in development (#2793). Either it should be moved here or this access removed.

[mikix]

Will remove in favor of the maliit interface.

[jdstrand]

What is launching usensord? If it is the unity8-session snap then this label won't match.

[mikix]

Looking at the snap, we don't include usensord yet. I'll just remove this bit until we add it back (who knows, maybe it will end up in a different snap or want its own interface...)

[jdstrand]

What is launching url-dispatcher? If it is the unity8-session snap then this label won't match.

[jdstrand]

What is sending to the app? If it is the unity8-session snap then this label won't match. (Also note the dbus interface is a super-set of this-- this only has Open though, which is more strict).

[jdstrand]

What is starting the launcher? If unity8-session snap then this label won't match.

[jdstrand]

These rules make it look like a snap is able to put and get anything from the keyboard. On Touch this was known as com.canonical.QtMir.Clipboard, but this is com.ubuntu.content.dbus.Service. Can you discuss the clipboard handling especially in reference to https://bugs.launchpad.net/ubuntu/+source/unity8/+bug/1371170?

Also, what is starting this service? If unity8-session snap then this label won't match.

[mikix]

This is the "pasteboard" service we implemented in content-hub. See https://launchpad.net/bugs/1471998. I think it replaces the old com.canonical.QtMir.Clipboard idea?

Tyler Hicks was the security reviewer for it. I was not involved in the implementation at all, so I can't speak to how it is secured. But I believe it uses Mir surface ids to validate dbus requests.

Good point about the label matching here and elsewhere in this PR. I'll try to fix those up.

[jdstrand]

@tyhicks - can you comment on com.ubuntu.content.dbus.Service as the "pasteboard" service that replaced com.canonical.QtMir.Clipboard?

[jdstrand]

Why do you need bind? You don't have any apparmor rules for unix or dbus that use bind...

[jdstrand]

Please drop this. It is back in 2.22.2.

[jdstrand]

Please adjust to:

recvfrom
recvmsg
sendmsg
sendto

[mikix]

"In addition to the above changes, please add some tests to make sure the connected policy is present."

I do in "TestUsedSecuritySystems" from unity8_test.go, right?

[jdstrand]

"In addition to the above changes, please add some tests to make sure the connected policy is present."
I do in "TestUsedSecuritySystems" from unity8_test.go, right?

You are checking for non-Nil (which is good), but I was also wanting specific string tests (eg, see TestUsedSecuritySystems in network_control_test.go).

[mikix]

"You are checking for non-Nil (which is good), but I was also wanting specific string tests (eg, see TestUsedSecuritySystems in network_control_test.go)."

Got it, will do.

[mikix]

OK, I did a more comprehensive testing run of all the interfaces listed (except accessibility -- that and fonts were just cargo-culted from unity7).

But specifically the three main dbus APIs now exposed by this interface (content-hub for copy/paste, url dispatching, and launcher item futzing) were all tested and worked when confined.

I have to expand the unity8_test.go file to be more useful, in light of some of the new code added. But the policy itself can be re-reviewed. I'll do tests tomorrow.

[mikix]

OK, tests expanded for this new content. All should be ready for a re-review. Thanks for all your comments!

This obviously isn't the full set of APIs that apps need. But it's a decent start -- copy/paste, content file sharing, launcher icons, and opening URLs.

There's the open question of whether url-dispatcher or content-hub should be separate interfaces. They are both user services that happen to be bundled into u8 right now. Conceptually I could imagine splitting the interface out and having u8 slot both. But content-hub is needed for copy and paste to work. And url-dispatcher is needed for Qt.openUrlExternally() to work. Both seem foundational enough to be part of the main u8 interface. But I thought the same about maliit. So I'm open to suggestions here.

The Launcher dbus API clearly belongs to unity8, at least!

[jdstrand]

I think it is fine to leave url-dispatcher and content-hub in here. Do note that the base declaration restrictions should not be removed until the decision for those is made because we don't want 3rd-party snaps to break when they are using unity8 and suddenly require unity8+content-hub+url-dispatcher. Ie, moving out content-hub and url-dispatacher would be a flag day for all consumers of unity8 (or we'd have the same policy in both unity8 interface and content-hub/url-dispatcher to maintain compatibility).

[jdstrand]

Can you also add this recent addition to the unity7 interface to silence common denials:

# The snapcraft desktop part may look for schema files in various locations, so 
# allow reading system installed schemas.
/usr/share/glib*/schemas/{,*}              r,
/usr/share/gnome/glib*/schemas/{,*}        r,
/usr/share/ubuntu/glib*/schemas/{,*}       r,

[mikix]

OK

[jdstrand]

What is providing the accessibility bus in unity8? I think atm this can be removed if the answer is 'nothing' and then if/when unity8 ships it, you can re-add with peer=(label=###SLOT_SECURITY_TAGS###).

[jdstrand]

We have a lot more interesting policy on Touch for content transfer. I suspect content-hub will be a separate interface that unity8 will implement because, iirc, content-hub may want to be used elsewhere for file transfer. For now, can you adjust this to be something along the lines of:

# Note: content-hub may become its own interface, but for now include it here
# Pasteboard via Content Hub
dbus (receive, send)
     bus=session
     interface="com.ubuntu.content.dbus.Service"
     path="/"
     member="{CreatePaste,GetPasteData,GetLatestPasteData,PasteFormats,"
     peer=(name=com.ubuntu.content.dbus.Service,label=###SLOT_SECURITY_TAGS###),
dbus (receive)
     bus=session
     interface="com.ubuntu.content.dbus.Service"
     path="/"
     member="PasteFormatsChanged"
     peer=(name=com.ubuntu.content.dbus.Service,label=###SLOT_SECURITY_TAGS###),

# File transfer via Content Hub
dbus (send)
     bus=session
     interface=org.freedesktop.DBus
     path=/org/freedesktop/DBus
     member={Request,Release}Name
     peer=(label=unconfined),
dbus (bind)
     bus=session
     name=com.ubuntu.content.handler.@{PROFILE_DBUS},
dbus (receive)
     bus=session
     path=/com/ubuntu/content/handler/@{PROFILE_DBUS}
     interface=com.ubuntu.content.dbus.Handler
     peer=(label=###SLOT_SECURITY_TAGS###),

# File transfer source via Content Hub
dbus (receive, send)
     bus=session
     interface=com.ubuntu.content.dbus.Transfer
     path=/transfers/@{PROFILE_DBUS}/export/*
     peer=(label=###SLOT_SECURITY_TAGS###),

# File transfer destination via Content Hub
dbus (receive, send)
     bus=session
     interface=com.ubuntu.content.dbus.Transfer
     path=/transfers/@{PROFILE_DBUS}/import/*
     peer=(label=###SLOT_SECURITY_TAGS###),

# LP: #1293771
# Since fd delegation doesn't exist in the form that we need it at this time,
# content-hub will create hard links in 'HubIncoming' for volatile data. As
# such, snaps should not have write access to anything in this directory
# otherwise they would be able to change the source content.
audit deny @{HOME}/snap/@{SNAP_NAME}/**/HubIncoming/** w,

I used a glob for HubIncoming because I don't know where that is these days. It could be more specific, but we'd have to make sure that the content-hub implementation never changed it without changing this interface.

Lastly, the Touch policy also had this rule which covers all of the paste rules:

dbus (receive, send)
     bus=session
     interface=com.ubuntu.content.dbus.Service
     peer=(label=unconfined),

This is close to the rule you used, but I'd prefer to leave it out for now. Can you use the recommended policy and report back any issues (pasting the denials) so that we can work through additional policy?

[mikix]

Alright, I can use your general constructions (though I'll add some just-landing commands for paste support).

That HubIncoming bit though -- there must be a tighter construction we could use? I haven't used that part of content-hub yet. Your current glob would limit a snap from using an unrelated directory called HubIncoming (though I don't know how likely it is, it seems like a big namespace grab on our part).

[mikix]

Oh sorry just saw now your comment about the HubIncoming glob. Fine for me for now.

[jdstrand]

Let's make this rule more precise (there is no such thing as /var/dev/shm):

deny /{dev,run,var/run}/shm/lttng-ust-* r,

I think you just copied that from unity7; I'll fix the unity7 interface.

[mikix]

OK

[jdstrand]

Nitpick: I just noticed that there are 2 spaces preceding all the rules. We aren't very consistent on this, but we should be. Can you remove the two spaces?

[mikix]

OK

[jdstrand]

Please also add send and recv

[mikix]

OK

[jdstrand]

There is a utility function for this in interfaces/dbus/dbus.go called SafePath. Also, @{PROFILE_DBUS} is one of the template variables (template_vars.go) and can be used instead I think. Did you try it? In snappy, the notion of the app id is different. On Touch it was name_appname_version, but on snappy it is snap.name.command (ie, no version). @{PROFILE_DBUS} gives you the dbusified snap.name.command so I think it needs to be used instead.

[mikix]

Ah, SafePath is great, thanks. See my other comment for why I didn't use PROFILE_DBUS.

[jdstrand]

I don't think this is going to work quite right unless I'm missing something. You should be using simply @{PROFILE_DBUS} instead of building up a list of strings. Can you comment on why you didn't use @{PROFILE_DBUS}?

[mikix]

So the reason I don't use @{PROFILE_DBUS} is because that's not what the session uses. Various bits of Touch code is registering and looking for those DBus paths using Touch's AppIDs.

I'm sure you know most of this already, but just for posterity... Ubuntu App Launch is the ultimate arbiter of what a valid AppID is. It currently supports a bunch of different "backends" -- legacy, click, libertine, and snap -- each with their own AppID format, though they take similar form and tend to use underscores as separators.

So far, UAL is using name_command_revision as snap AppIDs. It doesn't use the snap.name.command used elsewhere in snappy. So that's why we see those paths for UAL and the Unity launcher and why I needed a new variable.

[jdstrand]

I strongly feel UAL should be moved to snap.name.command for the AppID. Snappy is a lot different than click. What they share is that on click name_command_revision was the AppID everywhere and on snappy snap.name.command is the AppID everywhere. The current snapd approach was very actively chosen not to use revision as part of the AppID because there is only ever supposed to be one ID for the snap's commands at any given time. Remember, revision changes with local installs without snap assertion (ie, --dangerous), 'snap try', 'snap revert', 'snap refresh', etc. Trying to keep all that straight to map a revision in UAL is adding a lot of complexity. Finally, if you are using name_command_revision as the AppID, the developer has to always keep that context in mind-- that would be meaningless in most security policy (and therefore interfaces). Trusted helpers are no longer able to use libapparmor to get the label of the connecting process and use it as the AppID-- instead they will all have to be modified to maintain a translation.

Is it a requirement that UAL use name_command_revision or is it that it just hasn't happened yet? If it just hasn't happened yet, please do it. If it is a hard requirement, I strongly feel that @niemeyer, @vosst and other stakeholders should work through this now and come up with a proper redesign so that we aren't pulling in tech debt.

[ted-gould]

I understand that the Snapd AppID is different than the Unity8 AppID, but I don't think that we should require Unity8 to change to be able to run on snapd. There are likely other desktop environments that have their own application identifiers that we should honor and allow them to use as well. So while we could discuss changing the AppID in Unity8 (unfortunately this would probably be a hard change as there's no shared implementation) I don't think that it should restrict whether different application identifiers are allowed by projects using snapd.

I also think that for Unity8 to move to snapd AppIDs we'll need them to include the revision. That way we can properly handle running applications while they're upgraded, removed or installed. But that is a much larger conversation that this PR.

As discussed on IRC, think that a way forward is to use a glob instead of specifying the AppID format today. It will provide less specificity in the short term, but this interface is also restricted currently, so there won't be many users. That way we can move forward with getting apps and desktops moved over to using snapd confined.

[jdstrand]

+1 on adding a glob while the details are worked out.

I agree that snapd in general shouldn't care about what snaps are doing on top and I wouldn't have expressed on opinion on this at all except for the fact that Unity8's concept of AppID trickled into the snapd interface code, which alerted me to maintenance concerns, potential problems and developer confusion. Importantly, trusted helpers on Touch used the security label as the AppID and could trust that. If you are using a different AppID for Personal, then the security label won't correspond to Personal's AppID and the implementation will be considerably more complex.

While unity8 is in some ways 'just a snap', Ubuntu Personal should be working in concert with snapd, not against it. Areas where snapd currently is not meeting Personal's needs should be closely examined, perhaps with redesign. This will only help us solve problems for other desktop environments moving forward.

As for handling running applications while they're upgraded, remove or installed, that is a separate issue and UAL's current behavior is just one way of managing that, but it is incomplete (see https://bugs.launchpad.net/snappy/+bug/1616650 for one example). We should strive to solve those issues in snapd independently of UAL/unity8 interface so that all snaps will benefit.

[mikix]

OK, I've updated this branch per your comments.

• I've restricted content hub access to just the specific pasteboard calls. (I've also dropped the entire file transfer part for now. My primary original interest was the pasteboard. Until we decide exactly where the content-hub goes, I'm fine leaving most of it out of this.)

• I've dropped the accessibility bits for now -- I never tested that, it was just copy paste from unity7.

So this is just down to font/schema readonly access, URL dispatching, the Pasteboard, and the Launcher. A good start at least.

[jdstrand]

Thanks for the changes! ###PLUG_DBUS_APPIDS### is still an open discussion point, but I like the other changes. Couple small comments.

[jdstrand]

Can we make it clear that this pasteboard access is safe? Eg:

# Pasteboard via Content Hub. Unity8 with mir has safeguards that ensure snaps
# only may get/set the pasteboard with user-driven actions.  

[jdstrand]

Please merge with trunk then you can drop recv* and send* (leaving just shutdown).

[mikix]

OK, glob in use, plus your other comments addressed.

[jdstrand]

Couple small changes and then I think this will be ready.

[jdstrand]

I'd prefer that we have the intended rule and the documented workaround rule for transparency. Ie:

dbus (receive)
     bus=session
     path=/@{PROFILE_DBUS}
     interface=org.freedesktop.Application
     member=Open
     peer=(label=###SLOT_SECURITY_TAGS###),

# FIXME: workaround rule while UAL is using click-style AppID. This does not
# provide isolation since 'foo' can access 'barfoobaz'
dbus (receive)
     bus=session
     path=/###PLUG_DBUS_APPIDS###
     interface=org.freedesktop.Application
     member=Open
     peer=(label=###SLOT_SECURITY_TAGS###),

[jdstrand]

I'd prefer that we have the intended rule and the documented workaround rule for transparency. Ie:

# Unity launcher (e.g. app counter, progress, alert)
dbus (receive, send)
     bus=session
     path=/com/canonical/Unity/Launcher/@{PROFILE_DBUS}
     peer=(name=com.canonical.Unity.Launcher,label=###SLOT_SECURITY_TAGS###),

# FIXME: workaround rule while UAL is using click-style AppID. This does not
# provide isolation since 'foo' can access 'barfoobaz'
dbus (receive, send)
     bus=session
     path=/com/canonical/Unity/Launcher/###PLUG_DBUS_APPIDS###
     peer=(name=com.canonical.Unity.Launcher,label=###SLOT_SECURITY_TAGS###),

[jdstrand]

Please also add "This name glob does not provide isolation because 'foo' can access 'barfoobaz'. The base declaration restriction cannot be lifted until this is properly mediated."

[mikix]

OK, requested changes made.

[mikix]

Neither of the test failures (travis or i386 autopkgtest) seem related. autopkgtest is an error in overlordSuite.TestEnsureLoopPruneRunsMultipleTimes and travis seemed to be about unavailable servers...

[chipaca]

this now has conflicts; please merge master

[mikix]

OK, merged trunk.

All the tests failed, but that seems to be due to an unrelated issue: 'snap "core" has no "configure" hook'

[jdstrand]

Please use 'const' instead of 'var' for anything security policy related. There are some older interfaces that don't do this, but this is the new style.

[mikix]

Fixed.

[jdstrand]

Please use const.

[mikix]

Fixed.

[niemeyer]

This seems problematic, because with or without the base-declaration, the only reason to have this in place is to ship actual snaps that will break if this rule is changed. So we're putting something in place which is over-permissive and can't be touched without breaking people's working applications once snapd is transparently and automatically updated behind their back. That doesn't sound good.

We need a better solution here.

[niemeyer]

One option is to add an extra field in the interface declaration with the actual old-school application ID. We'll still need to keep reviews tight because it means snaps could impersonate different applications by tweaking this field alone, but that means we can properly tell what specific application ID is being shipped in the snap and being connected to unity8, and also gradually phase out that possibility if/when we decide to in the future.

@jdstrand Does that make sense, or am I missing something?

[jdstrand]

@niemeyer - I have a strong preference that unity8 migrate away from the old Touch APP_IDs and use proper ids that are compatible with snappy and I therefore very much dislike this rule-- which is why there is all the scary language around it.

I think it is possible to do this with an attribute, and even thought about it before, but I was disinclined to add an attribute because I then felt this temporary code would potentially become more permanent and wouldn't be able to be taken away easily. In other words, because of my strong preference for moving away from Touch IDs, I didn't want them to gradually go away-- I wanted them to go away ASAP and not linger in the snapd codebase and requiring the migration to happen before the base declaration is updated is incentive to do that.

I agreed to the current approach with the understanding that the broken rules would be incompatibly removed before the base declaration was updated for general use, and mentioned that any snaps may break when core changes and the unity8-session (slot implementation), snaps plugging unity8 and (I guess) the platform snap are updated (ie, your concerns). Since Canonical is driving the implementation of the interface, the unity8-session slot implementation and the platform snap, the agreement was that any snaps plugging this interface would also have to come from Canonical, and Canonical would be responsible for the transition and have to handle any breakage (there are <10 snaps currently that have a snap declaration to plugs the unity8 interface at this time). The Personal team agreed to these terms.

[mikix]

In the interests of getting some of this landed, I have taken out the two dbus permissions that dealt with appids (receiving url dispatches and interacting with the unity8 launcher).

We can discuss the appid issue separately.

[niemeyer]

@jdstrand Your sentiment matches mine pretty much. Our only disagreement lies here:

I agreed to the current approach with the understanding that the broken rules would be incompatibly removed before the base declaration was updated for general use,

Reality tends to be unforgiving in those cases. Code easily gets shipped in production which such caveats, and then we're left with broken code that we can't drop, and we wouldn't even have the ability to statically analyze which snaps would actually be broken or not, because good and bad would look the same.

@mikix Thanks for that!

[mikix]

The two failing autopkgtest architectures failed due to classic-ubuntu-core-transition-auth, which I believe is unrelated to this branch.

Otherwise, I think this branch should be good to go. Anything controversial has been excised and the remainder is sufficient to allow a unity8 app to run confined.

[jdstrand]

Can you make this explicit deny this instead:

deny /{dev,run,var/run}/shm/lttng-ust-* rw,

I noticed this while looking at kiosk issues this week.

[mikix]

Done.