interfaces: add fwupd interface

[timchen119]

An interface that allows fwupd upgrading UEFI firmware via UEFI capsule format.

[zyga]

There seems to be one space of indentation here. Can you please remove it?

[timchen119]

Do you mean the two space before the "#", or the space between "#" and "DBus" ? Do we have format conventions on AppArmor rules?

[jdstrand]

'#<2 spaces>privileged access to the fwupd service.' should be '#<one space>privileged access to the fwupd service.' is what Zygmunt meant. It doesn't mean anything to apparmor-- it is just the style we use.

[zyga]

This feels wrong. Those permissions will be handed to connected slot. Since the slot will not be on the core snap and instead on a snap that actually contains the fwupd software it seems more appropriate to put all of those permissions on the permanent slot snippet instead.

[timchen119]

When the system boot up fwupd service, the fwupd daemon only need the permissions in permanent slot snippet to start the service properly, those permissions listed in connected slot snippet are needed only when fwupdmgr cli communicate to fwupd.

[morphis]

@timchen119 I can see the point but I have the feeling that this will end up problematic. We should grant all these things with the permanent slot so the fwupmgr slot it able to perform tasks also without a connected plug.

[zyga]

You can probably move those to the connected slot snippet.

[timchen119]

Do you mean permanent slot snippet? Those permissions listed in connected slot snippet are needed only when fwupdmgr cli communicate to fwupd.

[jdstrand]

@timchen119 - @zyga I think was referring to everything from capability linux_immutable, to /sys/firmware/efi/fw_platform_size r,. If you add them in ConnectedSlot then they will get repeated for each connected client, which is not needed.

[zyga]

I'm not 100% sure but it seems that you can do exactly the same thing with apparmor instead. I don't recall any other interfaces taking advantage of the dbus security backend yet.

@jdstrand what do you think?

[timchen119]

Some interface do use them when I checked the current builtin interfaces

$ grep -C 2 "policy user=\"root" interfaces/builtin/*.go
interfaces/builtin/bluez.go-
interfaces/builtin/bluez.go-var bluezPermanentSlotDBus = []byte(`
interfaces/builtin/bluez.go:<policy user="root">
interfaces/builtin/bluez.go-    <allow own="org.bluez"/>
interfaces/builtin/bluez.go-    <allow own="org.bluez.obex"/>
--
interfaces/builtin/fwupd.go-
interfaces/builtin/fwupd.go-var fwupdPermanentSlotDBus = []byte(`
interfaces/builtin/fwupd.go:<policy user="root">
interfaces/builtin/fwupd.go-    <allow own="org.freedesktop.fwupd"/>
interfaces/builtin/fwupd.go-</policy>
--
interfaces/builtin/location_control.go-
interfaces/builtin/location_control.go-var locationControlPermanentSlotDBus = []byte(`
interfaces/builtin/location_control.go:<policy user="root">
interfaces/builtin/location_control.go-    <allow own="com.ubuntu.location.Service"/>
interfaces/builtin/location_control.go-    <allow send_destination="com.ubuntu.location.Service"/>
--
interfaces/builtin/location_observe.go-
interfaces/builtin/location_observe.go-var locationObservePermanentSlotDBus = []byte(`
interfaces/builtin/location_observe.go:<policy user="root">
interfaces/builtin/location_observe.go-    <allow own="com.ubuntu.location.Service"/>
interfaces/builtin/location_observe.go-    <allow own="com.ubuntu.location.Service.Session"/>
--
interfaces/builtin/modem_manager.go-</policy>
interfaces/builtin/modem_manager.go-
interfaces/builtin/modem_manager.go:<policy user="root">
interfaces/builtin/modem_manager.go-    <allow own="org.freedesktop.ModemManager1"/>
interfaces/builtin/modem_manager.go-</policy>
--
interfaces/builtin/network_manager.go-var networkManagerPermanentSlotDBus = []byte(`
interfaces/builtin/network_manager.go-<!-- DBus policy for NetworkManager (upstream version 1.2.2) -->
interfaces/builtin/network_manager.go:<policy user="root">
interfaces/builtin/network_manager.go-    <allow own="org.freedesktop.NetworkManager"/>
interfaces/builtin/network_manager.go-    <allow send_destination="org.freedesktop.NetworkManager"/>

[morphis]

@zyga No you have to put a dbus policy file in place for every dbus service sitting on the dbus system bus. Remember the discussions we had for the bluez, network-manager, modem-manager interfaces.

[jdstrand]

The name of this interface doesn't quite fit with other interface names. Also, this should be in alphabetical order in interfaces.md. Please don't change the name of this interface yet, but what do people think of 'uefi-firmware-control' instead? @niemeyer, @zyga?

[jdstrand]

Ehh, I see this is very specific to fwupd, so 'fwupd' is the proper name of the interface (cc, @niemeyer and @zyga). Can you adjust interfaces.md to have: "Can access snaps providing the fwupd interface which gives privileged access to update UEFI capsule format firmware."

[morphis]

@jdstrand Right, I agreed with Tim that we put all these things in a fwupd interface for now and split the UEFI bits out into a proper uefi-control interface at a later point.

[jdstrand]

I'm surprised you didn't need member="GetConnectionUnix{ProcessID,User}"

[jdstrand]

I think you can drop receive here. If you can't, please comment.

[jdstrand]

For convenience you probably also want this:

# Allow unconfined to talk to us. The API for unconfined will be limited
# with DBus policy, below.
dbus (receive, send)
    bus=system
    path=/
    interface=org.freedesktop.DBus*
    peer=(label=unconfined),

This allows unconfined processes to talk to fwupd. Can you comment on why this is not needed?

[jdstrand]

"For convenience you probably also want this:

# Allow unconfined to talk to us. The API for unconfined will be limited
# with DBus policy, below.
dbus (receive, send)
    bus=system
    path=/
    interface=org.freedesktop.DBus*
    peer=(label=unconfined),

This allows unconfined processes to talk to fwupd. Can you comment on why this is not needed?
"

@timchen119 - can you comment on this?

[timchen119]

I've add this in my latest commit, originally the plan was to only limit the client with "connected plugs" to be able to connect the fwupd, like fwupdmgr. We currently don't have a use case with unconfined client to connect to fwupd, so add it or not I really have no opinion on this. @jdstrand do you think we can safely remove this if we see no use case?

[jdstrand]

If you don't need it, it is fine to leave out.

[jdstrand]

Can you use this instead: owner @{PROC}/@{pid}/mounts r,

[jdstrand]

Can this be fine-tuned? What do the denials look like without this rule?

[timchen119]

The current denial is

audit: type=1400 audit(1471503814.950:59): apparmor="DENIED" operation="open" profile="snap.uefi-fw-tools.fwupd" name="/sys/devices/pci0000:00/0000:00:13.0/ata1/host0/target0:0:0/0:0:0:0/block/sda/sda2/partition" pid=4369 comm="fwupd" requested_mask="r" denied_mask="r" fsuid=0 ouid=0

How about

/sys/devices/pci*/**/block/**/partition r,

?

[jdstrand]

Re "/sys/devices/pci*//block//partition r," > yes please.

[jdstrand]

Whoa, this gives complete read access to everything on the device. Why is this needed?

[timchen119]

Below is the error message:

$ /snap/bin/uefi-fw-tools.fwupdmgr update
Downloading 0.1.3.2 for $PRODUCT$..
Updating 0.1.3.2 on $PRODUCT$...
 * Decompressing firmware
Retrying as an offline update...
 * Idle
 * Decompressing firmware
 * Scheduling upgrade
UEFI firmware update failed: Unknown error -1

audit: type=1400 audit(1471506960.850:58): apparmor="DENIED" operation="open" profile="snap.uefi-fw-tools.fwupd" name="/dev/sda" pid=1565 comm="fwupd" requested_mask="r" denied_mask="r" fsuid=0 ouid=0

This was needed by the efivar library used in libfwup,
I'll dig into this to see how to prevent this.

[timchen119]

efivar lib need full disk access to get the partition guid and size info, I made a patch at https://github.com/timchen119/efivar/commit/331e4102249987e93e88185ae9e3103b5a2b9404 as a proof of concept to use udev runtime data to get these info, so we may change this to use this following rules without open the full disk access:

/run/udev/data/b* r,

However honestly I'm looking for other suggestions as this is really a quick patch which need much more time to polish.

[jdstrand]

@timchen119 - I would very much prefer the patch to gather what you need from udev runtime data. It is the difference between the implementation having access to some information on block devices and having read access to all data from the system, snaps and users.

[timchen119]

That patch is tested and worked in the gateway platform I had, however I just modified the code path I need in this certain platform as a proof of concept, so to polish this patch I need to spend some time to study a bit more and discuss a bit with upstream maintainer to understand if that would break other things in the library and hopefully upstream it in the future -- and that might take some time.

[morphis]

@timchen119 As we're building a snap with fwupmgr we can include the patch just in the snap and let the discussion with upstream go in parallel. I don't see any reason to wait for upstream to accept, merge and release a new version before we can use it. We could also do a platform specific snap for the moment if you want to put the snap into the public store and switch back later or leave the public snap be specific for a single platform for a limited time.

[jdstrand]

Some bus policy will be needed so dbus-daemon will allow the connection and this is pretty bare bones. I prefer the way that location-control did it though-- put the root policy in PermanentSlotDBus and put the default policy in ConnectedPlugDBus.

That said, I have questions and observations:

• context=default should have <deny own="org.freedesktop.fwupd"/>
• Are org.freedesktop.DBus.Properties, org.freedesktop.DBus.Introspectable and org.freedesktop.DBus.Peer all needed here or is org.freedesktop.fwupd enough?
• shouldn't user="root" have <allow send_destination="org.freedesktop.fwupd" send_interface="org.freedesktop.fwupd"/> (and the other 3 if they are required)?
• should non-root be able to talk to fwupd at all? If not, a <deny send_destination="org.freedesktop.fwupd" send_interface="org.freedesktop.fwupd"/> might be nice here

[timchen119]

Got it, I'll check location-control and polish this part a bit.

[jdstrand]

Actually, location-control may not be the perfect example. It is actually meant to be used by non-root users, but fwupd is clearly not. I'm going to amend my previous comment and say, please change the end result of the dbus bus policy (after connections are made) to be:

<policy user="root">
    <allow own="org.freedesktop.fwupd"/>
    <allow send_destination="org.freedesktop.fwupd" send_interface="org.freedesktop.fwupd"/>
    <allow send_destination="org.freedesktop.fwupd" send_interface="org.freedesktop.DBus.Properties"/>
    <allow send_destination="org.freedesktop.fwupd" send_interface="org.freedesktop.DBus.Introspectable"/>
    <allow send_destination="org.freedesktop.fwupd" send_interface="org.freedesktop.DBus.Peer"/>
</policy>
<policy context="default">
    <deny own="org.freedesktop.fwupd"/>
    <deny send_destination="org.freedesktop.fwupd" send_interface="org.freedesktop.fwupd"/>
</policy>

As it stands, when fwupd is installed, any non-snap process of any UID can talk to udisks which would provide easy privilege escalation. On a classic system we might use policykit to control this access, but all snaps does not have policy kit. If there are APIs that are meant to be accessible to everyone and are no-risk, feel free to list them in the default policy.

Note that leaving out the apparmor rule for unconfined I mentioned elsewhere does not actually prevent the privilege escalation since unconfined users can change_profile (eg, aa-exec -p snap.fwupmgr.fwupmgr -- <something nasty>).

[jdstrand]

I'm surprised there is no seccomp policy since that is needed for talking to dbus. Did you test this using plugs: [ network ] or similar?

[morphis]

Please add necessary shell commands to connect the relevant slot/plug.

[timchen119]

Yes I did add the network and network-bind plug currently in the snap. Should we add those into the interface? Was thinking fwupd interface is not an implicit interface as the potential uefi-control interface we have in mind and we probably won't need dbus and its policy for uefi-control interface. But if thats need I could add it.

OK, I'll add the shell commands in the doc, the shell command is:
snap connect uefi-fw-tools:fwupdmgr uefi-fw-tools:fwupd

[jdstrand]

IIRC, fwupd does fetch things from over the internet, so it will always need 'network'. However, it would be great if:

• you could remove all other plugs for fwupdmgr and list the needed syscalls here (and ideally a general idea why they are needed-- but don't waste time on that-- just a high level (eg, it "binds to a port", "it calls out over the network on launch", etc)). These calls may be added to PermanentSlotSecComp
• Now that the daemon has started, have the cli command talk to fwupdmgr and list those calls required for fwupdmgr to talk to the client. These would go in ConnectedSlotSecComp
• you could remove all other plugs from the cli command and talk to fwupdmgr and list those calls. These would go in ConnectedPlugSecComp.

[jdstrand]

"IIRC, fwupd does fetch things from over the internet, so it will always need 'network'. However, it would be great if:

you could remove all other plugs for fwupdmgr and list the needed syscalls here (and ideally a general idea why they are needed-- but don't waste time on that-- just a high level (eg, it "binds to a port", "it calls out over the network on launch", etc)). These calls may be added to PermanentSlotSecComp
Now that the daemon has started, have the cli command talk to fwupdmgr and list those calls required for fwupdmgr to talk to the client. These would go in ConnectedSlotSecComp
you could remove all other plugs from the cli command and talk to fwupdmgr and list those calls. These would go in ConnectedPlugSecComp."

@timchen119, did you have a chance to look at this yet?

[timchen119]

I've remove all the plugs (network,network-bind) and added all the syscall needed in the code.

[morphis]

Why is the snap named uefi-fw-tools and not not fwupdmgr?

[timchen119]

Because it was the snap name back in 15.04 (we removed the project prefix though.)

[jdstrand]

Marking as 'Blocked' until feedback is addressed.

[morphis]

@timchen119 Do you have time to finalize the implementation with @jdstrand review comments?

[timchen119]

@morphis Thanks for all the review comments, was holiday until now. Will recommit this week.

[mvo5]

Just curious, why is this commented out?

[timchen119]

/dev/sd[a-z]* r,

The upstream version of efivar lib need full disk access to get the partition guid and size info, I made a patch at timchen119/efivar@331e410 as a proof of concept to use udev runtime data to get these info, so we may change this in our snap to use this following rules without open the full disk access:

/run/udev/data/b* r,

Was seeking for a better suggestion instead of a modified efivars library as that would need more work to polish the patch and may break hw I don't have.

[jdstrand]

Yes, please remove the /dev/sd[a-z]* r, and use this instead:

# Introspect the block devices to get partition guid and size information
/run/udev/data/b[0-9]*:[0-9]* r,

[mvo5]

👍 (just one question about a commented out line in the apparmor code, if we don't need it, maybe we should just rmeove it? or add a comment why its commented out?).

[jdstrand]

I see you added this already. Please use my slightly more fine-grained rule and update the comment for what you are looking for. Thanks!

[jdstrand]

Please adjust the comment and udev rule per my comment, and feel free to remove the unconfined dbus rule per my other comment, then +1.

[morphis]

LGTM, @jdstrand any further comments?

[jdstrand]

+1

[morphis]

@zyga @niemeyer @mvo5 Anymore things you want to comment on or can we merge this?