interfaces/builtin: add physical-memory-* and io-ports-control #2424

Merged
merged 11 commits into from Jan 11, 2017

Projects

None yet

4 participants

@tonyespy
Contributor
tonyespy commented Dec 6, 2016

Add physical-memory-control and -observe interfaces to gate access
to the physical memory device (/dev/mem_. Also add io-ports-control
to gate access to I/O ports (/dev/port).

interfaces/builtin/io_ports_control.go
+// NewIoPortsControlInterface returns a new "io-ports-control" interface.
+func NewIoPortsControlInterface() interfaces.Interface {
+ return &commonInterface{
+ name: "io-ports-control",
@morphis
morphis Dec 8, 2016 edited Contributor

I don't see this being added in snap/implicit.go

So how does it turn up on a system?

+
+const physicalMemoryControlConnectedPlugAppArmor = `
+# Description: With kernels with STRICT_DEVMEM=n, write access to all physical
+# memory.
@morphis
morphis Dec 8, 2016 Contributor

You also want

# Usage: reserved

here to flag the interface as a privilege one which not everybody should get.

@jdstrand
jdstrand Dec 8, 2016 Contributor

Actually, with the use of the base declaration, we can removed the Usage metatags here and everywhere (but keep a nice description for auditors and devs).

@morphis
morphis Dec 8, 2016 Contributor

Ok.

+# Description: With kernels with STRICT_DEVMEM=n, write access to all physical
+# memory.
+#
+# With STRICT_DEVMEM=y, allow writing to /dev/mem to access
@morphis
morphis Dec 8, 2016 Contributor

See comment about runtime detection below.

+)
+
+const physicalMemoryObserveConnectedPlugAppArmor = `
+# Description: With kernels with STRICT_DEVMEM=n, read-only access to all physical
@morphis
morphis Dec 8, 2016 Contributor

I guess you also want

# Usage: reserved

here to flag the interface as a privilege one which not everybody should get.

+
+const physicalMemoryObserveConnectedPlugAppArmor = `
+# Description: With kernels with STRICT_DEVMEM=n, read-only access to all physical
+# memory. With STRICT_DEVMEM=y, allow reading /dev/mem for read-only
@morphis
morphis Dec 8, 2016 Contributor

Wondering if we should enforce STRICT_DEVMEM=y in the interface. If there is some way to detect easily from userspace if that option is set we could simply not allow a connection in this case and make this even more secure. @jdstrand What do you think=

@jdstrand
jdstrand Dec 8, 2016 Contributor

@morphis - I'm not aware of a way to check for STRICT_DEVMEM at runtime but the snappy kernel porting guide states that STRICT_DEVMEM must be used to be a proper snappy kernel. If someone turns it off, my feeling is that the interface code shouldn't be the one verifying that anyway. I think this should be handled via a verification tool (perhaps the kernel team has thought through this?).

@morphis
morphis Dec 8, 2016 Contributor

Fine for me.

@jdstrand

Security policy and base declaration LGTM (with the few things I pointed out). I think there is some argument for making the base declaration stricter for physical-memory-*, but considering that snappy kernels need to have STRICT_DEVMEM to be compliant, leaving this as you have it makes sense to me.

+# (eg, X without KMS, dosemu, etc).
+capability sys_rawio,
+/dev/mem rw,
+
@jdstrand
jdstrand Dec 8, 2016 Contributor

Can you get rid of this newline?

+)
+
+const ioPortsControlConnectedPlugAppArmor = `
+# Description: Allow write access to all I/O ports.
@jdstrand
jdstrand Dec 8, 2016 Contributor

Can you add "See 'man 4 mem' for details"?

interfaces/builtin/io_ports_control.go
+# allows the process to disable interrupts. This will probably
+# crash the system, and is not recommended.
+
+iopl
@jdstrand
jdstrand Dec 8, 2016 Contributor

I think you are going to need ioperm here too.

@jdstrand
Contributor
jdstrand commented Dec 8, 2016

@niemeyer - sorta feeling like io-ports should be io-ports-control based on the ioperm, mem and iopl man pages. What do you think?

@tonyespy
Contributor

@jdstrand - all of your comments have been addressed.
@morphis - new interfaces added to the snap/implicit.go.

tonyespy added some commits Dec 13, 2016
@tonyespy tonyespy interfaces/builtin: add physical-memory-* and io-ports-control
Add physical-memory-control and -observe interfaces to gate access
to the physical memory device (/dev/mem_.  Also add io-ports-control
to gate access to I/O ports (/dev/port).
79d3b15
@tonyespy tonyespy snap: add io-ports-control and physical-memory-* to implicit slots
cada19a
@tonyespy tonyespy Merge remote-tracking branch 'origin/master' into add-dev-mem-interfaces
3555613
@tonyespy tonyespy interfaces/builtin: add sys_rawio capability to io-ports-control
d9de784
@jdstrand

LGTM as is, but should add the udev tagging either as part of this PR or the next one.

+
+capability sys_rawio, # required by iopl
+
+/dev/ports rw,
@jdstrand
jdstrand Dec 14, 2016 Contributor

I didn't think of this before, but you are going to want to also udev tag this since I suspect this interface is going to be used alongside things like i2c, iio, serial-port, etc. See #2459 (comment). I would be ok with merging this as is if 2.20 is a hard requirement and then fixing this for 2.21.

See https://github.com/snapcore/snapd/pull/2459/files#diff-0fc98d9f1337c18cf172845c6cd2a7b5R119 on how to do this.

+# BIOS code and data regions on x86, etc) for all common uses of /dev/mem
+# (eg, X without KMS, dosemu, etc).
+capability sys_rawio,
+/dev/mem rw,
@jdstrand
jdstrand Dec 14, 2016 Contributor

Also tag /dev/mem.

+# memory. With STRICT_DEVMEM=y, allow reading /dev/mem for read-only
+# access to architecture-specific subset of the physical address (eg, PCI,
+# space, BIOS code and data regions on x86, etc).
+/dev/mem r,
@jdstrand
jdstrand Dec 14, 2016 Contributor

Also tag /dev/mem.

@tonyespy tonyespy interface/builtin: add UDev TAG logic to io-ports-control interface
4986df2
@jdstrand

The approach is good. Please use const and feel free to do the same for mem as well. You might consider instead updating builtin/common.go to have connectedPlugUDev and then you could simply use:

func NewIoPortsControlInterface() interfaces.Interface {
	return &commonInterface{
		name: "io-ports-control",
		connectedPlugAppArmor: ioPortsControlConnectedPlugAppArmor,
		connectedPlugSecComp:  ioPortsControlConnectedPlugSecComp,
                connectedPlugUDev:     yourFuncForUDevShippets(),
		reservedForOS:         true,
`)

That isn't required for this PR.

interfaces/builtin/io_ports_control.go
"github.com/snapcore/snapd/interfaces"
)
-const ioPortsControlConnectedPlugAppArmor = `
+var ioPortsControlConnectedPlugAppArmor = []byte(`
@jdstrand
jdstrand Dec 19, 2016 Contributor

Can you change this back to const? While we don't use this consistently within the codebase (for older interfaces), this is the coding practice for new interfaces.

interfaces/builtin/io_ports_control.go
-const ioPortsControlConnectedPlugSecComp = `
+var ioPortsControlConnectedPlugSecComp = []byte(`
@jdstrand
jdstrand Dec 19, 2016 Contributor

Please use const.

+name: client-snap
+plugs:
+ plug-for-io-ports:
+ interface: io-ports-control
@jdstrand
jdstrand Dec 19, 2016 Contributor

This top-level plugs declaration is not needed since you use plugs: [io-ports-control] directly down below. I suggest just removing it (or use plugs: [plug-for-io-ports] down below).

@tonyespy
tonyespy Dec 23, 2016 Contributor

@jdstrand

I've updated io-ports-control security snippets to use const as suggested. I've also updated the two physical-memory-* interfaces to implement the UDev logic as well.

Re: the top-level plugs declaration. If I remove those lines from the consumingSnapInfo, I get the following panic from run-checks:

... Panic: plug is not of interface "io-ports-control" (PC=0x45F26E)

I also tried using "plugs: [plug-for-io-ports]" and "plugs: [io-ports-control, plug-for-io-ports]" and both still cause the same panic.

@jdstrand
jdstrand Jan 10, 2017 Contributor

@tonyespy - this issues with this section have to do with how the mocking is being done. Not enough is being mocked to do it in the way I described and there aren't any examples to mock an implicit slot. I suggest for clarity of intent in the test to simply change consumingSnapInfo to be:

name: client-snap                                                               
plugs:                                                                          
  plug-for-io-ports:                                                            
    interface: io-ports-control                                                 
apps:                                                                           
  app-accessing-io-ports:                                                       
    command: foo                                                                
    plugs: [plug-for-io-ports] 

In this manner you are plugging what you specified in toplevel 'plugs', which is how a developer would normally do this. Alternatively, you could revamp the setup to work like 'network-control'. That's harder and busy work at this point, so I suggest what I outlined above.

tonyespy added some commits Dec 22, 2016
@tonyespy tonyespy Merge remote-tracking branch 'origin/master' into add-dev-mem-interfaces afc65d0
@tonyespy tonyespy interface/builtin: add UDev TAG logic to physical-memory-* interfaces
b8c2290
@tonyespy tonyespy interfaces/builtin: make io-ports-control security vars const
321958b
@mvo5 mvo5 added this to the 2.21 milestone Jan 5, 2017
interfaces/builtin/io_ports_control.go
+
+ case interfaces.SecurityUDev:
+ var tagSnippet bytes.Buffer
+ const udevRule = `KERNEL=="/dev/ports", TAG+="%s"`
@jdstrand
jdstrand Jan 10, 2017 Contributor

This should be KERNEL=="ports" (we've trimmed '/dev/' in other interfaces).

+name: client-snap
+plugs:
+ plug-for-io-ports:
+ interface: io-ports-control
@jdstrand
jdstrand Jan 10, 2017 Contributor

@tonyespy - this issues with this section have to do with how the mocking is being done. Not enough is being mocked to do it in the way I described and there aren't any examples to mock an implicit slot. I suggest for clarity of intent in the test to simply change consumingSnapInfo to be:

name: client-snap                                                               
plugs:                                                                          
  plug-for-io-ports:                                                            
    interface: io-ports-control                                                 
apps:                                                                           
  app-accessing-io-ports:                                                       
    command: foo                                                                
    plugs: [plug-for-io-ports] 

In this manner you are plugging what you specified in toplevel 'plugs', which is how a developer would normally do this. Alternatively, you could revamp the setup to work like 'network-control'. That's harder and busy work at this point, so I suggest what I outlined above.

+iopl
+`)
+
+ expectedSnippet3 := []byte(`KERNEL=="/dev/ports", TAG+="snap_client-snap_app-accessing-io-ports"
@jdstrand
jdstrand Jan 10, 2017 Contributor

This should be KERNEL=="ports" (we've trimmed '/dev/' in other interfaces).

+
+ case interfaces.SecurityUDev:
+ var tagSnippet bytes.Buffer
+ const udevRule = `KERNEL=="/dev/mem", TAG+="%s"`
@jdstrand
jdstrand Jan 10, 2017 Contributor

This should be KERNEL=="mem" (we've trimmed '/dev/' in other interfaces).

+apps:
+ app-accessing-physical-memory:
+ command: foo
+ plugs: [physical-memory-control]
@jdstrand
jdstrand Jan 10, 2017 Contributor

Please adjust to:

   plugs: [plug-for-physical-memory]
+capability sys_rawio,
+/dev/mem rw,
+`)
+ expectedSnippet2 := []byte(`KERNEL=="/dev/mem", TAG+="snap_client-snap_app-accessing-physical-memory"
@jdstrand
jdstrand Jan 10, 2017 Contributor

This should be KERNEL=="mem" (we've trimmed '/dev/' in other interfaces).

+
+ case interfaces.SecurityUDev:
+ var tagSnippet bytes.Buffer
+ const udevRule = `KERNEL=="/dev/mem", TAG+="%s"`
@jdstrand
jdstrand Jan 10, 2017 Contributor

This should be KERNEL=="mem" (we've trimmed '/dev/' in other interfaces).

+apps:
+ app-accessing-physical-memory:
+ command: foo
+ plugs: [physical-memory-observe]
@jdstrand
jdstrand Jan 10, 2017 Contributor

Please adjust to:

    plugs: [plug-for-physical-memory]
+# space, BIOS code and data regions on x86, etc).
+/dev/mem r,
+`)
+ expectedSnippet2 := []byte(`KERNEL=="/dev/mem", TAG+="snap_client-snap_app-accessing-physical-memory"
@jdstrand
jdstrand Jan 10, 2017 Contributor

This should be KERNEL=="mem" (we've trimmed '/dev/' in other interfaces).

@tonyespy
tonyespy Jan 11, 2017 Contributor

@jdstrand requested changes made & pushed.

tonyespy added some commits Jan 11, 2017
@tonyespy tonyespy interfaces/builtin: drop /dev/ prefix in udev rules 337ed22
@tonyespy tonyespy interfaces/builtin: modify physical-memory-*-test plugs name b0a7951
@tonyespy tonyespy interfaces/builtin: update io-ports-control-test mocking (review comm…
…ents)
b51c3ec
@jdstrand
Contributor

Thanks for addressing the feedback @tonyespy! LGTM

@mvo5
Collaborator
mvo5 commented Jan 11, 2017

Thanks! Looks good to me as well.

@mvo5 mvo5 merged commit aec795f into snapcore:master Jan 11, 2017

6 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-i386 autopkgtest finished (success)
Details
xenial-ppc64el autopkgtest finished (success)
Details
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment