Interface for modem manager

[alfonsosanchezbeato]

Initial proposal for interface to enable modem-manager snap.

Some additional changes needed for network-manager are included in this PR too.

[snappy-m-o]

Can one of the admins verify this patch?

[snappy-m-o]

Can one of the admins verify this patch?

[snappy-m-o]

Can one of the admins verify this patch?

[mvo5]

add to whitelist

[morphis]

Please add the full dbus policy file upstream ships here.

[alfonsosanchezbeato]

Upstream has actually two policy files, one used when policy kit is disabled and one used when policy kit is enabled. This is the upstream one for the case of policy kit disabled.

[morphis]

We will not switch to policykit here. So lets drop the comment.

[morphis]

We should drop all device specific rules and just leave the common ones in here. Any device specific rules need to be shipped with the gadget/kernel snap.

[niemeyer]

All the rules should actually ship in snapd. No snap, not even kernel or gadget, are allowed to inject security rules into snapd.

[morphis]

@niemeyer That wont work as we would have to adjust the interface for each customer specific device we're doing. Keep in mind that that product IDs are changing through simple things like rebranding etc. Both modem-manager and ofono are handling this thing through udev rules which will be device specific and we can't simply ship all of them in this interface. That is a simple maintenance hell.

[alfonsosanchezbeato]

Note that this device detection can be done in code from the snap too. For instance, ofono has two ways of detecting the modem type: the old one by using udev rules, and the new one, where it looks directly for vendor/product ID in udev events instead of looking for labels added by udev rules. It just happens that attaching labels used to be more convenient.

As I understand this, interfaces should be kind of general so this interface should be ideally reusable by programs that handle modems, say ModemManager AND ofono. So the ideal thing would be to put this application specific stuff (because these rules are MM rules, ofono has different ones - note again that only for a few modems) inside the snap, not in the gadget snap and not in the interface. As the snap should not add udev rules, this should be handled in code instead, as ofono does. No need for more permissions provided for the interface, as it is just a matter of listening to udev events.

However, this would require intensive modifications in ModemManager that we cannot assume. In the future world, if we get upstream buy-in, they would handle this. Meanwhile, I think this can be added to the interface, otherwise it would need to be added to every gadget snap, for which we do not necessarily will have control in the future. So somebody would install MM snap from the store and will find it is not able to detect the system modem.

[jdstrand]

Not blocking on the unimplemented idea I posted above, but if modem-manager can use the new way of looking for vendor/product ID in udev events, I think the dynamically generated apparmor rules for udev files would allow this to work and the old udev rules way of doing things can go away?

[morphis]

There is a abstraction in apparmor for those.

[alfonsosanchezbeato]

Which one?

[morphis]

Sorry, confused /sbin/resolvconf with the /etc/resolvconf*

[morphis]

For what do we need kernel module loading capabilities?

[alfonsosanchezbeato]

pppd loads some kernel modules.

[morphis]

@alfonsosanchezbeato ok, but which one for what?

[alfonsosanchezbeato]

ppp_deflate, ppp_async, maybe more depending on protocols (VPN case?). Sending packages through PPP and doing some tasks is at the kernel level due to efficiency reasons (otherwise you would need to get IP packets on user side and send them through the tty device back to the kernel).

[morphis]

I am not sure if the better thing would be (if we keep ppp in the core snap) to create a ppp interface which would be implicitly provided by the core snap. However we're not doing that for wpa-supplicant as of today which is used the same way from the network-manager snap. If @zyga @jdstrand or @niemeyer don't have anything against this I am fine to keep those for now.

[zyga]

If we had a ppp interface on the core snap, how would this make things better?

[morphis]

It would be usable by both ofono and modem-manager and we don't need to write the same rules into both interfaces.

[jdstrand]

As mentioned, very much against the snap loading modules. Fine with snapd doing this on the snap's behalf via this interface or a 'ppp' interface.

[morphis]

All those ppp ones should go into SNAP_DATA and ppp should come from the snap itself.

[alfonsosanchezbeato]

It is part of the core snap and interacts quite heavily with the kernel, so not sure if we should do that

[morphis]

We should ship our own ppp within the snap.

[alfonsosanchezbeato]

It is part of the core snap and interacts quite heavily with the kernel, so not sure if we should do that.

[niemeyer]

Just one note regarding having security snippets outside of snapd itself, otherwise this is a great review by @morphis. Thanks for that!

[jdstrand]

Totally minor nitpick, but it would be great since there are so many of these if they could be alphabetized.

[jdstrand]

@zyga - as a total aside, I wonder why we sometimes prefix 'New' here....

[jdstrand]

I really don't like these kinds of rules-- snapd should be querying udev and adding the /sys and /run/udev accesses that are assigned to the snap. That said, we don't have that yet, so this is ok. Can you adjust this: '/sys/bus/usb/devices** r,' to be '/sys/bus/usb/devices/** r,' though?

EDIT: Can you add a 'FIXME' note regarding how we should be querying udev instead of hard-coding this?

[jdstrand]

I was hoping that interfaces would solve this sort of thing and snapd would be more dynamic rather than hard-coding devices in this manner, but again, we aren't there yet. Without this dynamism, if there is another interface that needs /dev/tty* then two snaps using two different interfaces on the same system might cause problems for each other by accessing the same device. Can you add a FIXME for this? Also, what about /dev/ttyUSB*, is that handled differently?

[alfonsosanchezbeato]

FIXME added. The rule is to be able to access all /dev/tty* but virtual terminals (/dev/tty0, /dev/tty1, ...), as there are many variants possible: /dev/ttyS[N], /dev/ttyUSB[N], /dev/ttyACM[N], etc.

Just added /dev/cdc-* too, had forgotten that one.

[jdstrand]

Oh sigh, but at least this is a trusted interface. :) The seccomp argument filtering branch should be able to limit the ioctl to TIOCSSERIAL at least.

[jdstrand]

Please use: peer=(name=org.freedesktop.DBus, label=unconfined)

[jdstrand]

This should be moved to ConnectedSlotAppArmor with: peer=(label=###PLUG_SECURITY_TAGS###). See location_observe.go as an example.

I realize you probably did this based on bluez and network-manager, but we learned with location-observe that we could do better (this type of change is what the nm branch I gave you is doing btw (and we'll need another for bluez)).

[jdstrand]

Same here (move to ConnectedSlotAppArmor with: peer=(label=###PLUG_SECURITY_TAGS###))

[jdstrand]

Just to be explicit-- this is granting all of modem manager's DBus API to a connected clients. Is that intentional?

[alfonsosanchezbeato]

Yes, it is. I do not see a real reason to limit access to MM DBus interface from clients (see https://www.freedesktop.org/software/ModemManager/api/latest/).

[jdstrand]

Can you add a: # TODO: add ioctl argument filters when seccomp arg filtering is implemented

[jdstrand]

@zyga - I wonder if these seccomp rules for talking to dbus should be added to PermanentPlugSeccomp rules instead. Idea being, plugging clients will be killed if they try to use dbus if we only put these in ConnectedPlugSeccomp since they aren't connected on install whereas if we allowed them in PermanentPlugSeccomp they would not be killed, get a DBus denied message and be in a position to handle the condition gracefully. Another way to do it is if an interface uses the DBus backend, just add these seccomp rules unconditionally to the policy.

None of this should block this PR.

[zyga]

Could we constrain those so that those are not equivalent to a connected network slot? If so then yeah, we could move them to permanent.

[jdstrand]

We will be able to do something like that with 'socket' once seccomp arg filtering lands.

[jdstrand]

How do these rules correspond to the /dev/ accesses you gave in the apparmor policy? It seems that we are missing an opportunity here to use our snappy udev device assignment implementation and instead giving blanket apparmor rules with different tagging. If you are unfamiliar with what I am talking about, please see 'devices cgroup' from: https://raw.githubusercontent.com/ubuntu-core/snap-confine/master/README.md

[alfonsosanchezbeato]

Modem access is done via /dev/tty* and /dev/cdc-*. Not really sure how this should be modified by reading the link you posted. Note also that modem detection must be dynamic (think of insertion of modem dongle). Would your comment still apply in this case?

[jdstrand]

snapd is supposed to be able to handle dynamic detection and AIUI, this is the function of the gadget snap (mapping snappy names to device names) such that snapd can tag these devices accordingly and assign them to the slot via the udev backend.

[zyga]

IMHO we cannot and won't be able to do dynamic slots anytime soon. Even with hook support we won't see a distinct serial-port slot for each serial on the device that is not explicitly listed in the gadget snap. If you plug in a new one later it will not show up as a new slot.

[jdstrand]

This gives network-manager ring 0 access to the system. If you can load a kernel module, then you can disable all security, etc, etc. We should try to avoid this at all costs and note also that module autoloading should work fine and not require this access. 'snappy config ubuntu-core' in 15.04 allowed you to load kernel modules and IIRC this was exposed in the gadget snap. 16 removed all of this but we should be designing a system where snaps don't load modules themselves. There are a number of ways to handle this that could all be implemented:

• AIUI, 'snap config' will be coming back and it could support module loading
• the gadget snap could support module loading
• the interface could encode a list of modules to load and upon snap install/connect, snapd could load the modules. I discussed this relative to firewall-control here: https://bugs.launchpad.net/snappy/+bug/1583514
• snapd and the snap implementing a particular slot could talk over a carefully designed protocol for having snapd load requested modules (perhaps this is all abstracted behind a rewritten modprobe)

The main points of all of the above is that snaps don't load modules themselves and if snapd is expanded to load modules on behalf of snaps, that should be carefully controlled.

@tyhicks - fyi, I think you'll want to keep an eye on this

[alfonsosanchezbeato]

@jdstrand , the one actually loading modules is pppd, which is part of the core snap and then trusted. How about executing it unconfined (ux)? This would remove most of this new NM rules, so NM process would not have ring 0 access.

[jdstrand]

I'd prefer to have snapd add the rules rather than using a Ux/ux rule on pppd. This just shifts the attack from modem manager to pppd.

[jdstrand]

See my comments above about these blanket accesses.

[alfonsosanchezbeato]

See my comment there

[jdstrand]

Is modem manager really needed on classic or is this just something that works with network-manager? I'm trying to understand what snaps would legitimately access modem manager on classic.

[alfonsosanchezbeato]

NM, but also alternatives like connman. And, you could also manually configure modem connections.

[alfonsosanchezbeato]

PR refreshed after addressing comments.

[zyga]

We might be able to do it with serial port interface but for now I think this is somewhat OK. Caveat: modem-manager has a bad history of poking anything that's not blacklisted and sometimes causing issues but I guess we have no other choice yet.

[zyga]

Can you tell me more about how this should look like in the end?

[jdstrand]

The idea is this:

• if snapd knows the device in /dev, do udevadm info /dev/foo otherwise if snapd knows the device in /sys/devices, do udevadm info /sys/devices/.../foo. Eg:

  $ udevadm info /dev/kmsg
  P: /devices/virtual/mem/kmsg
  N: kmsg
  E: DEVMODE=0644
  E: DEVNAME=/dev/kmsg
  E: DEVPATH=/devices/virtual/mem/kmsg
  E: MAJOR=1
  E: MINOR=11
  E: SUBSYSTEM=mem
  

• at this point we can tag this device as being assigned to the snap, using the udev backend

• with the output from udevadm info we have enough information to grant precise access to /run/udev. Eg, for kmsg above:

  /run/udev/data/c1:11 r, # based on MAJOR and MINOR

Opengl is similar:

$ udevadm info /dev/dri/card0
P: /devices/pci0000:00/0000:00:02.0/drm/card0
N: dri/card0
E: DEVNAME=/dev/dri/card0
E: DEVPATH=/devices/pci0000:00/0000:00:02.0/drm/card0
E: DEVTYPE=drm_minor
E: ID_FOR_SEAT=drm-pci-0000_00_02_0
E: ID_PATH=pci-0000:00:02.0
E: ID_PATH_TAG=pci-0000_00_02_0
E: MAJOR=226
E: MINOR=0
E: SUBSYSTEM=drm
E: TAGS=:master-of-seat:seat:uaccess:
E: USEC_INITIALIZED=3062574

Add:

  /run/udev/data/c226:0 r, # based on MAJOR and MINOR
  /sys/devices/pci0000:00/0000:00:02.0/{,**} r, # based on DEVPATH
  /run/udev/data/+pci:0000:00:02.0 r, # based on ID_PATH

We'll have to have some light per-interface mappings (eg, when ID_PATH is pci-0000:00:02.0, use +pci:0000:00:02.0 in /run/data, but there might already be libraries and udev queries that would do this for us). We might also have to recursively descend directories in /sys/devices to realpath() the files so we know what to add, but that isn't hard to do either. All the pieces are there.

[zyga]

(just keep scrolling).

By now I am convinced we need something better. Since we plan to talk about this tomorrow I won't comment on the rest.

[zyga]

Is /etc/ppp/ writable?

[alfonsosanchezbeato]

Apparently yes.

[zyga]

Please merge with master, this is no longer something you have to change.

[alfonsosanchezbeato]

I have done some checks on the kernel modules. No options are used for them. The modules that ppp needs are:

ppp_deflate bsd_comp ppp_async crc_ccitt option usb_wwan usbserial

I have removed these lines in the NM interface:

/proc/sys/kernel/modprobe r,
/bin/kmod ix,
capability sys_module,

Then I have loaded the modules with commands:

sudo modprobe ppp_deflate
sudo modprobe bsd_comp
sudo modprobe ppp_async
sudo modprobe option

Finally, I have inserted the modem dongle, and was able to establish a connection with no apparmor denials. Therefore, this

/usr/sbin/pppd ix,
/etc/ppp/* rwix,
/dev/ppp rw,
/dev/tty[^0-9]* rw,
/run/udev/data/* r,
/run/lock/tty[^0-9] rw,
/run/ppp* rw,
/var/run/ppp* rw,
/var/log/ppp* rw,
/bin/run-parts ix,
@{PROC}/@{pid}/loginuid r,
capability setgid,
capability setuid,
/sbin/resolvconf rix,
/run/resolvconf* rw,
/etc/resolvconf/** rw,
/etc/resolvconf/update.d/* ix,
/lib/resolvconf/* ix,

is what we still need to add to the NM interface to establish a connection. So if we can pre-load said modules we can remove the loading modules permissions. Are we good with the permissions that still need to be added to NM interface?

[alfonsosanchezbeato]

There is still the auto-connection problem. One option here would be to add DBus calls between NM and MM in the permanent slots for both interfaces, which actually removes the need for creating manually the connection. Thoughts?

[morphis]

Afaik the outcome of the discussion yesterday was that we implement a ppp interface which we delegate the module loading to and the m-m snap will be connected too.

@alfonsosanchezbeato Lets not add hacks/workarounds in the interface to overcome the missing auto-connect functionality. Auto-connection is a feature that is a requirement to get snaps included like nm or mm by default in a device image and has to be implemented. @zyga already has some plans for this if I remember well.

For the detection of the modem-manager service from network-manager through a dbus name watch we should really only let the trigger go to network-manager once the modem-manager snap is connected of the interface to the network-manager snap and not before. @jdstrand is there a mechanism in apparmor's dbus mediation to cover those things?

[alfonsosanchezbeato]

PR refreshed after creating an implicit ppp interface that will eventually use the future snapd backend for loading kernel modules. All permissions needed to run pppd have been transferred to that interface from network-manager one.

[jdstrand]

A snapd interface attribute could specify /dev/tty[0-9]* and /dev/cdc-* such that (pseudocode):

for a in interface_attribs; do
    for i in /dev/tty* ; do
        add_apparmor_udev_paths_for_device($i) # calls 'udevadm info $i', et al
    done
done

[jdstrand]

Thank you for making the requested changes. In terms of security policy, I'm ok with this as is since it is using what we have, but having udev querying is going to help the policy to be more precise for all kinds of interfaces that need access to devices and we should be looking into that sooner than later IMHO.

[morphis]

Lets drop this now that we agreed on a interim solution until module loading is available through snapd.

[alfonsosanchezbeato]

Refreshed after removing the module loading part from ppp interface so this can be finally merged.

[jdstrand]

For these /dev accesses, we want to use the udev backend. If this must land now, feel free to add a FIXME note but ideally you'd use the udev backend in this PR.

[jdstrand]

Eh, these are static devices so using the udev backend isn't required. +1 as is.

[jdstrand]

I'm mildly surprised that there isn't a ConnectedPlugSeccomp, but hey, I'm not complaining! :)

[jdstrand]

ppp interface security policy LGTM. If adding a FIXME comment, +1 from me. If using udev backend in this PR, I'd like to take another look.

UPDATE: these are static devices so udev is not required. +1 as is.

[zyga]

Let's land it and iterate on getting those FIXMEs reduced over time