interfaces: add ofono interface #2058

alfonsosanchezbeato · Oct 3, 2016

Add interface for ofono telephony daemon.

jdstrand · Oct 3, 2016

jdstrand requested changes Oct 3, 2016

View changes

Thanks for the PR. See inline comments on a number of changes that need to be done and reviewed by the security team.

Can you alphabetize this (and move FwupdInterface too)?

Done

Can you use const ofonoPermanentSlotAppArmor = ... instead and convert to []byte when you need a byte slice? I realize not all interfaces do this yet, but it is a requirement for new interfaces.

Done

The pulseaudio interface does this already.

@jhodapp - yes, this came up as part of the pulseaudio review.

Can you limit this to the devices you need based on https://www.kernel.org/doc/Documentation/devices.txt? See interfaces/builtin/opengl.go and browser_support.go for example rules.

Done

Hrmm, but ok.

This rule should be: /run/systemd/seats/{,*} r

But why does ofono care about the seat?

Changed. In some cases there are preferences that depend on the user in the seat, like the preferred SIM for voice/data in dual SIM devices.

Ok, can you then say something along the lines of # To get current seat preferences for choosing SIM based on user or similar?

Another hrm, but ok. Note that we do have the serial-port interface that makes things better than they used to be, but until the core snap adds implicit serial-port slots and the core snap supports dynamic plugging, I don't think we can do better than this.

@jdstrand Right, that was our assumption. Once we have better support from snapd for this we will remove those rules and switch to the serial-port interface.

+1 here, we will change this when feasible

network netlink covers the other two. Can you remove network netlink?

Done

Why no interface component here?

Changed

seccomp argument filter is implemented. What are you wanting to filter on?

Can you also add a comment as to what these are for? dbus? networking? etc?

Comment was a left over, removed. Added a comment to clarify what are the calls for.

Can you add an xml comment here on the provenance of this dbus bus policy?

Done

Can you also add to the default bus policy:

    <deny own="org.ofono"/>

Done

Can you provide a comment as to the provenance of these udev rules?

Done

You can drop this case since the default is to return nil, nil

Done

This assumes that the ofono snap will not be installable on classic. Is that the case?

Also, on classic this means that a connected plug will have this policy:

# Allow all access to ofono services
dbus (receive, send)
    bus=system
    path=/{,**}
    peer=(label=unconfined),

This is a security hole because this rule grants access to all dbus system services, which is wrong. I suggest instead you:

add an interface component to the plugs rule
create a separate variable to hold classic policy and if release.OnClassic append it to the policy

If you do this, you fix the security hole and make the interface work on all snaps, classic and classic with ofono snap.

Interface added to apparmor rule. But not sure about what to put there as classic policy, as the ofono package has no apparmor rules.

Thank you for fixing the interface omission.

As for classic, look at mpris.go. Notice how it creates both mprisConnectedSlotAppArmor and mprisConnectedSlotAppArmorClassic. Then look in PermanentSlotSnippet how it creates snippet and then appends to snippet when on classic. I suggest you:

keep ofonoConnectedPlugAppArmor as is
add ofonoConnectedPlugAppArmorClassic with:

# Allow all access to ofono services on classic
dbus (receive, send)
    bus=system
    path=/{,**}
    interface=org.ofono.*
    peer=(label=unconfined),

adjust ConnectedPlugSnippet to do:

old := []byte("###SLOT_SECURITY_TAGS###")
new := []byte("")
snippet := bytes.Replace([]byte(ofonoConnectedPlugAppArmor), old, new, -1)
if release.OnClassic {
    snippet = append(snippet, ofonoConnectedPlugAppArmorClassic...)
}
return snippet, nil

Please add tests for:

seccomp on plugs side
seccomp on slots side
apparmor on the slots side

That is what TestUsedSecuritySystems() was doing, so I it is not clear to me how tests should be. Which ones could I use as reference?

I mean for each of these, add a test that when connected you verify a rule is present. Eg, add TestConnectedPlugSnippetAppArmor():

    plug := &interfaces.Plug{}
    release.OnClassic = false
    snippet, err := s.iface.ConnectedPlugSnippet(s.plug, slot, interfaces.SecurityAppArmor)
    c.Assert(err, IsNil)
        // verify apparmor connected
        c.Assert(string(snippet), testutil.Contains, "#include <abstractions/dbus-strict>")

        // verify classic didn't connect
    c.Assert(string(snippet), Not(testutil.Contains), "peer=(label=unconfined),")

Now do the same for the others.

You can drop TestUsedSecuritySystems() - they aren't required any more.

This commented still stands.

Can you move this to alphabetical order?

Done

alfonsosanchezbeato · Oct 4, 2016

PR refreshed for another round

jdstrand · Oct 4, 2016

jdstrand requested changes Oct 4, 2016

View changes

Thanks for the updates, this is close. Please see my inline comments.

These aren't quite as fine-grained as I was hoping. It seems from the udev rules that only a handful of devices are known by ofono-- is that true? If so, can you determine the more fine-grained access? If you need help, simply remove these, run in devmode and paste the /run/udev/data denials and we can get more fine-grained.

No, ofono knows many more devices (see comment in ofonoPermanentSlotUdev). It uses libudev to get currently connected devices and processes udev events when devices are hot-plugged. When doing this libudev accesses to files in /run/udev/data, I have seen it accessing:

c2:*
c3:*
c188:*
c189:*
n*
+usb:*
+pci:*

An option to narrow this down is to modify ofono udev filter so it is more strict. Another one is to ignore some of the denials as some do not affect apparently the outcome. For instance, of the previous ones only reading char devices 188 and 189 seems critical (those are for /dev/ttyUSB* devices). However, I am a bit concerned about being too restrictive, as modem vendors might use non standard devices with unexpected major numbers.

Since this is the ofono service, it is less of a concern to let it have access to /run/udev/data. If you you feel the current accesses are sufficient, +1. It will be nice when we have the dynamic resolution code in place.

Great, then I think we can leave this in its current shape until that happens.

This assumes that the ofono snap will not be installable on classic. Is that the case?

Also, on classic this means that a connected plug will have this policy:

# Allow all access to ofono services
dbus (receive, send)
    bus=system
    path=/{,**}
    peer=(label=unconfined),

This is a security hole because this rule grants access to all dbus system services, which is wrong. I suggest instead you:

add an interface component to the plugs rule
create a separate variable to hold classic policy and if release.OnClassic append it to the policy

If you do this, you fix the security hole and make the interface work on all snaps, classic and classic with ofono snap.

Interface added to apparmor rule. But not sure about what to put there as classic policy, as the ofono package has no apparmor rules.

Thank you for fixing the interface omission.

As for classic, look at mpris.go. Notice how it creates both mprisConnectedSlotAppArmor and mprisConnectedSlotAppArmorClassic. Then look in PermanentSlotSnippet how it creates snippet and then appends to snippet when on classic. I suggest you:

keep ofonoConnectedPlugAppArmor as is
add ofonoConnectedPlugAppArmorClassic with:

# Allow all access to ofono services on classic
dbus (receive, send)
    bus=system
    path=/{,**}
    interface=org.ofono.*
    peer=(label=unconfined),

adjust ConnectedPlugSnippet to do:

old := []byte("###SLOT_SECURITY_TAGS###")
new := []byte("")
snippet := bytes.Replace([]byte(ofonoConnectedPlugAppArmor), old, new, -1)
if release.OnClassic {
    snippet = append(snippet, ofonoConnectedPlugAppArmorClassic...)
}
return snippet, nil

Typo: /Snipped/Snippet/

Please also test for c.Assert(string(snippet), testutil.Contains, "#include <abstractions/dbus-strict>") here.

Please add tests for:

seccomp on plugs side
seccomp on slots side
apparmor on the slots side

That is what TestUsedSecuritySystems() was doing, so I it is not clear to me how tests should be. Which ones could I use as reference?

I mean for each of these, add a test that when connected you verify a rule is present. Eg, add TestConnectedPlugSnippetAppArmor():

    plug := &interfaces.Plug{}
    release.OnClassic = false
    snippet, err := s.iface.ConnectedPlugSnippet(s.plug, slot, interfaces.SecurityAppArmor)
    c.Assert(err, IsNil)
        // verify apparmor connected
        c.Assert(string(snippet), testutil.Contains, "#include <abstractions/dbus-strict>")

        // verify classic didn't connect
    c.Assert(string(snippet), Not(testutil.Contains), "peer=(label=unconfined),")

Now do the same for the others.

alfonsosanchezbeato · Oct 5, 2016

Thanks for the review. PR refreshed addressing issues. Only remaining point should be permissions for /run/udev/data/ (see comment on that).

alfonsosanchezbeato · Oct 6, 2016

Branch refreshed adding permissions for unconfined apps in permanent slot.

jdstrand · Oct 7, 2016

jdstrand requested changes Oct 7, 2016

View changes

Thanks for the changes. A few more things remain. Please see inline comments.

This will allow everything to connect to ofono. Please use this instead:

# Allow traffic to/from our path and interface with any method for unconfined
# clients to talk to our ofono services.
dbus (receive, send)
    bus=system
    path=/{,**}
    interface=org.ofono.*
    peer=(label=unconfined),

Please adjust this comment to be:

# Allow access to the unconfined ofono services on classic.

Please remove: connect, getpeername, getsockname, setsockopt, socketpair and socket. They are all included in the default template.

Please remove: connect, getsockname and socket. They are all included in the default template.

Please use this instead: c.Check(string(snippet), testutil.Contains, "send\n") (getsockname is already in the default template).

You can drop TestUsedSecuritySystems() - they aren't required any more.

This commented still stands.

Please use this instead: c.Check(string(snippet), testutil.Contains, "peer=(label=snap (interface=org.ofono.* is in PermanentSlotAppArmor).

Please add: c.Check(string(snippet), testutil.Contains, "listen\n") (setsockopt is in default policy.

alfonsosanchezbeato · Oct 7, 2016

Thanks for the review. PR refreshed after addressing comments.

tonyespy · Oct 10, 2016

tonyespy reviewed Oct 10, 2016

View changes

A quick scan of the code also shows /dev/dsp being used by tools/huawei-audio, and /dev/chnlat11 being used by unit/test-caif.c. Might be worth adding these now.

Good point. Added, plus a couple of additional ones I found.

jdstrand · Oct 14, 2016

jdstrand reviewed Oct 14, 2016

View changes

+1 regarding security policy provided you fix the comment I mentioned needing fixing just now.

This comment is wrong. It should be // Let confined apps access unconfined ofono on classic

Comment changed and branch refreshed.

jdstrand · Nov 9, 2016

jdstrand requested changes Nov 9, 2016

View changes

Now that the base declaration is more finalized (also see #2269), please adjust this to be:

   ofono:                                                              
     allow-installation:                                                         
       slot-snap-type:                                                           
         - app                                                                   
         - core                                                                  
     deny-auto-connection: true                                                  
     deny-connection:                                                            
       on-classic: false

This says that this may be supplied as an app snap or via core (ie, on classic) and if supplied via an app snap, then a snap declaration is required to connect to the the slot implementation.

Please move this down below and use:

"ofono":       []string{"app", "core"}

You can drop 'Usage' now that we have the base declaration. Also, please adjust the whitespace before 'gives'.

For readability, can you move this rule and comment down below with the other /dev rules?

It looks like 'at_console' is deprecated: https://lists.fedoraproject.org/pipermail/devel/2014-May/199004.html. Considering the lack of policykit on Ubuntu Core it seems like removing it and relying on the root and default policy would be wise. Can you comment on why 'at_console' is here?

I copied this over from ofono default policy file. Removed this in the refreshed branch, as it is not really needed and does not make much sense anyway.

I've rethought this section since previous reviews. With the proposed implementation, classic systems with connected plugs get to talk to unconfined ofono as well as the security label of the ofono snap, but this snap won't exist on a classic system where the deb would be used instead. Can you confirm this?

If that is true, instead of using ofonoConnectedPlugAppArmorClassic, instead conditionally set new to unconfined when OnClassic and to slotAppLabelExpr(slot) when not and drop ofonoConnectedPlugAppArmorClassic altogether.

Well, I can see both things happening (ofono client using the snap or using the deb in a classic system). Maybe we should not be too restrictive and allow both things, if not an issue security-wise. Same should apply to the modem-manager interface changes: I have changed it back to the way you mention here, but maybe I should leave the change I just made. We can discuss on irc.

Re "ofono client using the snap or using the deb in a classic system"> if that is the case, then you can leave this as is.

Awesome, I will leave this bit as it is now.

You can drop this test-- you aren't adding seccomp policy for slots.

Feel free to drop this.

and this...

alfonsosanchezbeato · Nov 10, 2016

Branch refreshed after addressing comments.

jdstrand · Nov 10, 2016

Thanks for the changes!

jdstrand · Nov 10, 2016

jdstrand approved these changes Nov 10, 2016

View changes

alfonsosanchezbeato · Nov 11, 2016

@jdstrand, thanks for the review. No more changes on my side, as I have left the classic parts as they are now.

alfonsosanchezbeato · Nov 15, 2016

@jdstrand FYI I have added a couple of additional apparmor permissions:

+/run/udev/data/+usb-serial:* r,
+/run/udev/data/+platform:* r,
+/run/udev/data/+pnp:* r,

jdstrand · Nov 15, 2016

Those new accesses are fine.

chipaca · Nov 16, 2016

@@ -44,6 +44,7 @@ var allInterfaces = []interfaces.Interface{
          	&PulseAudioInterface{},
          	&UDisks2Interface{},
          	&FwupdInterface{},
+        +	&OfonoInterface{},

+        +/sys/bus/usb/devices/ r,
+        +# FIXME snapd should be querying udev and adding the /sys and /run/udev accesses
+        +# that are assigned to the snap, but we are not there yet.
+        +/sys/bus/usb/devices/** r,

+        +# FIXME snapd should be more dynamic to avoid conflicts between snaps trying to
+        +# access same ports.
+        +/dev/tty[^0-9]* rw,
+        +/dev/cdc-* rw,

+        +# Description: Allow operating as the ofono service. Reserved because this
+        +#  gives privileged access to the system.
+        +# Usage: reserved
+        +# TODO: add ioctl argument filters when seccomp arg filtering is implemented

+        +func (iface *OfonoInterface) ConnectedPlugSnippet(plug *interfaces.Plug, slot *interfaces.Slot, securitySystem interfaces.SecuritySystem) ([]byte, error) {
+        +	switch securitySystem {
+        +	case interfaces.SecurityDBus:
+        +		return nil, nil

snapcore/snapd

interfaces: add ofono interface #2058

alfonsosanchezbeato commented Oct 3, 2016

jdstrand requested changes Oct 3, 2016

View changes

alfonsosanchezbeato commented Oct 4, 2016

jdstrand requested changes Oct 4, 2016

View changes

alfonsosanchezbeato commented Oct 5, 2016

alfonsosanchezbeato commented Oct 6, 2016

jdstrand requested changes Oct 7, 2016

View changes

alfonsosanchezbeato commented Oct 7, 2016

tonyespy reviewed Oct 10, 2016

View changes

jdstrand reviewed Oct 14, 2016

View changes

jdstrand requested changes Nov 9, 2016

View changes

alfonsosanchezbeato commented Nov 10, 2016

jdstrand commented Nov 10, 2016

jdstrand approved these changes Nov 10, 2016

View changes

alfonsosanchezbeato commented Nov 11, 2016

alfonsosanchezbeato added some commits Sep 30, 2016

alfonsosanchezbeato commented Nov 15, 2016

jdstrand commented Nov 15, 2016

chipaca approved these changes Nov 16, 2016

View changes

chipaca merged commit `f94b419` into snapcore:master Nov 16, 2016
1 of 5 checks passed

1 of 5 checks passed

snapcore/snapd

Join GitHub today

interfaces: add ofono interface #2058

Conversation

alfonsosanchezbeato commented Oct 3, 2016

jdstrand requested changes Oct 3, 2016 View changes

jdstrand Oct 3, 2016 • edited Edited 1 time jdstrand Oct 5, 2016

jdstrand Oct 3, 2016 • edited Edited 1 time jdstrand Oct 3, 2016

alfonsosanchezbeato commented Oct 4, 2016

jdstrand requested changes Oct 4, 2016 View changes

jdstrand Oct 3, 2016 • edited Edited 1 time jdstrand Oct 3, 2016

alfonsosanchezbeato commented Oct 5, 2016

alfonsosanchezbeato commented Oct 6, 2016

jdstrand requested changes Oct 7, 2016 View changes

jdstrand Oct 7, 2016 • edited Edited 1 time jdstrand Oct 7, 2016

jdstrand Oct 7, 2016 • edited Edited 1 time jdstrand Oct 7, 2016

alfonsosanchezbeato commented Oct 7, 2016

tonyespy reviewed Oct 10, 2016 View changes

jdstrand reviewed Oct 14, 2016 View changes

jdstrand requested changes Nov 9, 2016 View changes

alfonsosanchezbeato commented Nov 10, 2016

jdstrand commented Nov 10, 2016

jdstrand approved these changes Nov 10, 2016 View changes

alfonsosanchezbeato commented Nov 11, 2016

alfonsosanchezbeato added some commits Sep 30, 2016

Some checks were not successful

alfonsosanchezbeato commented Nov 15, 2016

jdstrand commented Nov 15, 2016

chipaca approved these changes Nov 16, 2016 View changes

Hide details View details chipaca merged commit f94b419 into snapcore:master Nov 16, 2016 1 of 5 checks passed

1 of 5 checks passed

jdstrand requested changes Oct 3, 2016

View changes

jdstrand Oct 3, 2016 •

Edited 1 time

jdstrand
Oct 5, 2016

jdstrand Oct 3, 2016 •

Edited 1 time

jdstrand
Oct 3, 2016

jdstrand requested changes Oct 4, 2016

View changes

jdstrand Oct 3, 2016 •

Edited 1 time

jdstrand
Oct 3, 2016

jdstrand requested changes Oct 7, 2016

View changes

jdstrand Oct 7, 2016 •

Edited 1 time

jdstrand
Oct 7, 2016

jdstrand Oct 7, 2016 •

Edited 1 time

jdstrand
Oct 7, 2016

tonyespy reviewed Oct 10, 2016

View changes

jdstrand reviewed Oct 14, 2016

View changes

jdstrand requested changes Nov 9, 2016

View changes

jdstrand approved these changes Nov 10, 2016

View changes

chipaca approved these changes Nov 16, 2016

View changes

chipaca merged commit `f94b419` into snapcore:master Nov 16, 2016
1 of 5 checks passed