create mir interface

[kgunnfront]

first cut at mir interface, i tested this locally based on the latest snapd & this wiki for the mir aspect https://developer.ubuntu.com/en/snappy/guides/mir-snaps/

[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?

[elopio]

@kgunn can you please document your test in integration-tests/manual-tests.md ?

[zyga]

run go fmt on all the files you've changed

[kgunnfront]

done

[zyga]

mir needs to ptrace?

[jdstrand]

If mir server needs ptrace, it should be more fine-grained than this and only ptrace for connected clients. Please use something like this in mirConnectedPlugAppArmor:
ptrace (tracedby) peer=###SLOT_SECURITY_TAGS###,

Then the mirConnectedSlotAppArmor would have:
ptrace (trace) peer=###PLUG_SECURITY_TAGS###,

The above assumes you need 'trace' Can you paste the apparmor denial that prompted this rule?

[kgunnfront]

removed ptrace references, i know we add these long ago, but appearantly they are not needed

[zyga]

Why is the socket in /tmp and not in /run? /tmp is private and per-app and won't work with snaps

[kgunnfront]

this was an old artifcat, removed /tmp references

[zyga]

What does \# mean there?

Could mir use shm entries that follow /dev/shm/snap.$SNAP_NAME?

[kgunnfront]

in this instance the # is actually the # in part of the directory name. for instance if i remove this line, I get the denial in the following way
apparmor="DENIED" operation="open" profile="snap.mir-server.mir-server" name="/dev/shm/#3" pid=5217 comm="mir-server" requested_mask="wr" denied_mask="wr" fsuid=0 ouid=0

also, mir is all server side allocated, so not sure the snap.$SNAP_NAME would work

[zyga]

Are all of those commands guaranteed to exist in the image? Should mir ship those tools internally?

[jdstrand]

All of these commands can be removed. They are in the default policy (interfaces/apparmor/template.go).

[zyga]

So is it /tmp/mir_socket or /run/mir_socket?

[kgunnfront]

only run, corrected

[zyga]

Why are those commented out entries here? Is mir using them?

[kgunnfront]

removed

[zyga]

This won't work, you have you use /run as each process gets a private /tmp that is invisible to anyone else.

[kgunnfront]

yep, removed /tmp for /run

[zyga]

Can this be made so that it is specific to the snap? Ideally mir would create buffers here that cannot be opened by any other process except the one that mir is talking to (so client A cannot look at shared objects of client B).

[zyga]

Does this mean the mir clients need ptrace?

[kgunnfront]

removed

[zyga]

Mir cliends should not run those applications. Can you expand on why they are here.

[kgunnfront]

was an artifact, since removed

[zyga]

Why do mir clients needs to read this? It seems like something mir server does

[jdstrand]

@zyga - I'm reading this as part of mirPermanentSlotAppArmor, so should just be the server.

[kgunnfront]

correct, i'm only focused on mir-server at the moment

[zyga]

Why does mir client need to chown files?

[jdstrand]

Again, this is the server, but I can't recall why the server needs to chown either. Can you comment in the code with a 'Note:'.

[kgunnfront]

artifact, i was able to remove

[zyga]

Same question as ptrace above

[jdstrand]

You repeated this rule. Please remove.

[kgunnfront]

removed

[zyga]

To be clear this should be different. The mir slot is restricted and can be only used by mir itself. The mir plug should not be restricted because mir is supposed to be more secure than X11.

[jdstrand]

In addition to what zyga said, this should have something in it besides the comment. It should contain something along the lines of what was in mir_client for seccomp on 15.04.

[kgunnfront]

removed, in the final efforts, not needed

[zyga]

Please run go fmt on this file.

[kgunnfront]

ran go fmt on all files

[zyga]

Why aren't you returning the snippets if you have defined above?

[jdstrand]

Ah, this is probably why Kevin had the policy in both places and had trouble. @kgunn, anywhere you defined AppArmor or Seccomp above you need to return non-nil in the corresponding snippets. So, this is ConnectedPlugSnippet, and you provided mirConnectedPlugAppArmor and mirConnectedPlugSeccomp above, so interfaces.SecurityAppArmor and interfaces.SecuritySeComp should return non-nil here.

[kgunnfront]

correct, after lots and lots of experimenting - returning the seccomp and aa profiles for mir server on PermanentPlugSnippet is what worked, returning on any other (PermSlotSnippet, ConnectedSlotSnippet, ConnectedPlugSnippet did not work and i tested each). Which honestly was not what i thought would occur - I believed that for Mir server it should have reported on PermanentSlot

[kgunnfront]

ok, this was related to me mistakenly using wrong label in yaml - so this is now correct

[zyga]

Ditto, return the permanent snippets defined above here please.

[kgunnfront]

fixed

[zyga]

IMHO mir should auto connect.

[jdstrand]

We've said the slot side should autoconnect, yes, and the plugs side of Mir is supposed to be safe for anyone, so yes, autoconnect.

[kgunnfront]

changed autoconnect to true

[zyga]

I'll skip reviewing this until the mir.go itself is updated.

[kgunnfront]

ok, zyga/jamie - i think it's probably ready for another round, spend large amount of today/y'day cleaning up

[zyga]

Please fix the mentioned bits and answer the few questions I've asked. Thank you for submitting this, one step closer to not having to use X11

[jdstrand]

This gets back to the opengl interface.

[jdstrand]

IIRC, this was going to be investigated and, again IIRC, @vosst said that Mir shouldn't be using these. Can someone confirm? If we need it, we should have a unix peer=(label=...) rule, likely one for slot and plug side. Can you post the denial that prompted this rule?

[kgunnfront]

commenting out "unix (receive, send) type=seqpacket addr=none,"
results in the following upon trying to run the client app

Jun 16 20:56:11 localhost ubuntu-core-launcher[4427]: Loading module: 'libubuntu_application_api_desktop_mirclient.so.3.0.0'
Jun 16 20:56:12 localhost kernel: [ 3126.446100] audit: type=1400 audit(1466110572.256:20): apparmor="DENIED" operation="open" profile="snap.mir-client.client-start" name="/usr/share/applications/" pid=4429 comm="clocks" requested_mask="r" denied_mask="r" fsuid=0 ouid=0

[jdstrand]

That denial is for something unrelated and is a noisy denial that shouldn't affect the app's functionality. Please leave out the seqpacket line.

[jdstrand]

Ping regarding: my question regarding 'unix (receive, send) type=seqpacket addr=none,'

[jdstrand]

Why is this needed?

[kgunnfront]

without it the following occurs on server launch

Jun 16 21:19:13 localhost ./devtools.snapd[5034]: taskrunner.go:238: DEBUG: Running task 100 on Do: Make snap "mir-server" available to the system
Jun 16 21:19:13 localhost systemd[1]: Reloading.
Jun 16 21:19:14 localhost systemd[1]: snapd.refresh.timer: Adding 1h 9min 46.277746s random time.
Jun 16 21:19:14 localhost systemd[1]: Reloading.
Jun 16 21:19:14 localhost systemd[1]: snapd.refresh.timer: Adding 9min 58.505083s random time.
Jun 16 21:19:14 localhost systemd[1]: Started Service for snap application mir-server.mir-server.
Jun 16 21:19:14 localhost ./devtools.snapd[5034]: daemon.go:181: DEBUG: uid=0;@ GET /v2/snaps 1.126997ms 200
Jun 16 21:19:14 localhost ubuntu-core-launcher[5138]: Mir server snap started
Jun 16 21:19:14 localhost ubuntu-core-launcher[5138]: [2016-06-16 21:19:14.438531] mirplatform: Found graphics driver: mir:mesa-kms (version 0.23.0)
Jun 16 21:19:14 localhost ubuntu-core-launcher[5138]: [2016-06-16 21:19:14.455655] mirplatform: Found graphics driver: mir:mesa-x11 (version 0.23.0)
Jun 16 21:19:14 localhost ubuntu-core-launcher[5138]: [2016-06-16 21:19:14.460754] mirserver: Starting
Jun 16 21:19:14 localhost ubuntu-core-launcher[5138]: [2016-06-16 21:19:14.461495] mircommon: Loading modules from: /snap/mir-server/x1/usr/lib/x86_64-linux-gnu/mir/server-platform
Jun 16 21:19:14 localhost ubuntu-core-launcher[5138]: [2016-06-16 21:19:14.461990] mircommon: Loading module: /snap/mir-server/x1/usr/lib/x86_64-linux-gnu/mir/server-platform/graphics-mesa-kms.so.9
Jun 16 21:19:14 localhost ubuntu-core-launcher[5138]: [2016-06-16 21:19:14.462408] mircommon: Loading module: /snap/mir-server/x1/usr/lib/x86_64-linux-gnu/mir/server-platform/server-mesa-x11.so.9
Jun 16 21:19:14 localhost ubuntu-core-launcher[5138]: [2016-06-16 21:19:14.463333] mircommon: Loading module: /snap/mir-server/x1/usr/lib/x86_64-linux-gnu/mir/server-platform/input-evdev.so.5
Jun 16 21:19:14 localhost ubuntu-core-launcher[5138]: [2016-06-16 21:19:14.465184] mirplatform: Found graphics driver: mir:mesa-kms (version 0.23.0)
Jun 16 21:19:14 localhost ubuntu-core-launcher[5138]: [2016-06-16 21:19:14.465662] mirplatform: Found graphics driver: mir:mesa-x11 (version 0.23.0)
Jun 16 21:19:14 localhost ubuntu-core-launcher[5138]: [2016-06-16 21:19:14.466394] mirserver: Selected driver: mir:mesa-kms (version 0.23.0)
Jun 16 21:19:14 localhost kernel: [ 4508.651864] audit: type=1400 audit(1466111954.459:24): apparmor="DENIED" operation="capable" profile="snap.mir-server.mir-server" pid=5140 comm="mir-server" capability=1 capname="dac_override"
Jun 16 21:19:14 localhost kernel: [ 4508.651871] audit: type=1400 audit(1466111954.459:25): apparmor="DENIED" operation="capable" profile="snap.mir-server.mir-server" pid=5140 comm="mir-server" capability=2 capname="dac_read_search"

[jdstrand]

I recall this was needed but I'm annoyed it is....

[kgunnfront]

i verified during clean up it's still needed.

[jdstrand]

This is very broad and gets back to the udev querying I've detailed in the opengl interface.

[kgunnfront]

I've done some clean up there, /sys/devices now removed

[jdstrand]

Can you add a comment: Note: this allows reading and inserting all input events

[kgunnfront]

done

[jdstrand]

These udev get back to the udev querying as detailed in the opengl interface.

[jdstrand]

Please use: @{PROC}/ r,

[kgunnfront]

got rid of proc

[jdstrand]

This is in the default policy, please remove.

[kgunnfront]

removed

[jdstrand]

This is in the default policy, please remove.

[kgunnfront]

removed

[jdstrand]

'r' is allowed by the default policy. You actually need 'w' to /sys/devices? Why?

[kgunnfront]

since removed

[jdstrand]

This is in the default policy. Please remove.

[kgunnfront]

removed

[jdstrand]

This gets back to opengl interface.

[kgunnfront]

uh, much of this since removed

[jdstrand]

Please put this next to the ptrace rule.

[kgunnfront]

much removed

[jdstrand]

This looks like the default policy in interfaces/seccomp/template.go. Can you list here only what isn't included there?

[kgunnfront]

went through fine tooth and cleaned up

[jdstrand]

The policy for mirConnectedPlugAppArmor is wrong. It should contain what the client needs, but this is all the privileged policy of the server. This should contain something along the lines of what was in mir_client for apparmor on 15.04.

[kgunnfront]

since cleaned up

[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?

[jdstrand]

This seems like an odd change for your PR.

[kgunnfront]

I've since reverted and pushed correction

[jdstrand]

Can you put this in alphabetically like you did in all_test.go?

[kgunnfront]

corrected

[jdstrand]

I suspect this can be limited based on https://www.kernel.org/doc/Documentation/devices.txt. What files is it accessing in /run/udev/data/?

[kgunnfront]

When I remove /run/udev/data/* r, the server will receive these denials

Aug 4 20:19:13 localhost ubuntu-core-launcher[1499]: [2016-08-04 20:19:13.694297] mirplatform: Found graphics driver: mir:mesa-x11 (version 0.23.5)
Aug 4 20:19:13 localhost ubuntu-core-launcher[1499]: Unknown command line options: --vt 1
Aug 4 20:19:13 localhost kernel: [ 589.117731] audit: type=1400 audit(1470341953.690:15): apparmor="DENIED" operation="open" profile="snap.mir-server.mir-server" name="/run/udev/data/+drm:card0-Virtual-1" pid=1501 comm="mir-server" requested_mask="r" denied_mask="r" fsuid=0 ouid=0
Aug 4 20:19:13 localhost kernel: [ 589.117859] audit: type=1400 audit(1470341953.690:16): apparmor="DENIED" operation="open" profile="snap.mir-server.mir-server" name="/run/udev/data/+drm:card0-Virtual-2" pid=1501 comm="mir-server" requested_mask="r" denied_mask="r" fsuid=0 ouid=0
Aug 4 20:19:13 localhost kernel: [ 589.117948] audit: type=1400 audit(1470341953.690:17): apparmor="DENIED" operation="open" profile="snap.mir-server.mir-server" name="/run/udev/data/+drm:card0-Virtual-3" pid=1501 comm="mir-server" requested_mask="r" denied_mask="r" fsuid=0 ouid=0

So is there some alternative?
note, i already have /dev/dri/card0 rw, in the list

[jdstrand]

You should probably just 'plugs: [ opengl ]' in the server, though I think you'll need to change snap/implicit.go to move opengl out of implicitClassicSlots and into implicitSlots. I think you'd also want to move /dev/dri/card0 rw, into opengl too. Not sure why it isn't there now....

If this is undesirable, can copy this from the opengl interface:

  # FIXME: this is an information leak and snapd should instead query udev for
  # the specific accesses associated with the above devices.
  /sys/bus/pci/devices/** r,
  /run/udev/data/+drm:card* r,
  /run/udev/data/+pci:[0-9]* r,

  # FIXME: for each device in /dev that this policy references, lookup the
  # device type, major and minor and create rules of this form:
  # /run/udev/data/<type><major>:<minor> r,
  # For now, allow 'c'haracter devices and 'b'lock devices based on
  # https://www.kernel.org/doc/Documentation/devices.txt
  /run/udev/data/c226:[0-9]* r,  # 226 drm

[jdstrand]

@niemeyer mentioned this is ready for re-review.

@kgunn - please respond to my questions regarding opengrl, /deg/dri/card0 and /run/udev/data

[kgunnfront]

OK, I rearrange to rely on the opengl i/f for the relevant paths.
I also made opengl part of implicitSlots

[jdstrand]

Please use this instead: /run/user/[0-9]*/mir_socket rw,

[kgunnfront]

changed

[jdstrand]

This is a bit weird, but I guess it is ok on the slot side.

[kgunnfront]

removed actually

[jdstrand]

Please remove this empty line.

[kgunnfront]

removed empty line

[jdstrand]

This is a bit broad. What is it for, anonymous sockets? Also, please remove the empty line.

[kgunnfront]

so for the kiosk style server, it uses /run/mir_socket
and empty line removed

[jdstrand]

This is also a bit broad. What is it for, anonymous sockets?

[kgunnfront]

same as before, /run/mir_socket is used by the kiosk style server

[jdstrand]

This is weird-- why would the client need to look in /usr/share/applications/ for connecting to mir?

[kgunnfront]

I honestly do not know. I just know I get the denial.

[jdstrand]

The connected plug should actually be 'common'.

[kgunnfront]

could you elaborate on what you mean here ?

[jdstrand]

A description of why these capabilities are needed would be ideal.

[kgunnfront]

add a note for the tty cap
i was able to remove dac_override and sys_admin

[jdstrand]

Typo: reserved

[kgunnfront]

corrected

[jdstrand]

The connected plug should be 'common'.

[kgunnfront]

again, some more context/description would be helpful

[jdstrand]

You have "# Usage: reserver" (mispelled) for mirConnectedPlugSecComp and mirConnectedPlugAppArmor. This should be "# Usage: common"

[jdstrand]

You need go fmt on the files again. This case statement is indented too much.

[jdstrand]

Thanks for the updates! This is getting really close in terms of security policy generation and the policy itself. Please see my comments from a few minutes ago.

[niemeyer]

Please respond to the points individually so we can track what has been done and what is still pending.

Marking as Reviewed.

[niemeyer]

@jdstrand This is ready for a re-review I think.

[jdstrand]

unix (receive, send) type=seqpacket addr=none, should move to mirConnectedSlotAppArmor and be: unix (receive, send) type=seqpacket addr=none peer=(label=###PLUG_SECURITY_TAGS###), then you need to adjust mirConnectedSlotAppArmor to substitute ###PLUG_SECURITY_TAGS### like you do ###SLOT_SECURITY_TAGS### in mirConnectedPlugAppArmor

[jdstrand]

Actually, I see you already have done that. Can you not just remove unix (receive, send) type=seqpacket addr=none, from mirPermanentSlotAppArmor() altogether?

[kgunnfront]

yep, removed the redundant unix (receive, send) type=seqpacket addr=none, from permanent slot

[jdstrand]

@niemeyer - done. Two remaining open questions.

[jdstrand]

For consistency, it would be nice if this was 'rw' instead of 'wr' (makes no difference to the policy).

However, /dev/tty* is, I think, too broad since USB serial devices might be found at /dev/ttyUSB*. Assuming you don't intend for mir to configure USN ttys, can you adjust this to be: /dev/tty[0-9]* rw,

(sorry I didn't notice this before).

[kgunnfront]

limited dev/tty and made it rw

[jdstrand]

"Description: Allow operating as the mir service." - please s/service/server/ so it is consistent with mirPermanentSlotAppArmor.

[kgunnfront]

changed to be consistent

[jdstrand]

Can you add an AutoConnect test?

func (s *NetworkInterfaceSuite) TestAutoConnect(c *C) {
    c.Check(s.iface.AutoConnect(), Equals, true)
}

[kgunnfront]

added

[jdstrand]

I think mir should be in implicitClassicSlots, no? On an all snaps system mir is provided as a snap (therefore not needed in implicitSlots) but on a classic system (eg, one with unity8) then mir would be part of the system (that said, there are I think changes that are needed to make this interface work on classic for the unix label rules, but those can be fixed in a later PR). If this is only for kiosks on all-snaps systems (or at least this PR is), perhaps don't have mir be an implicit slot at all.

[kgunnfront]

agreed, simply removed until we know more wrt personal and the future, will work through the classic case next

[jdstrand]

Whoops, I think github masked the '*' in my last comment. This should be:

/dev/tty[0-9]* rw,

[kgunnfront]

and corrected

[jdstrand]

+1 assuming the tests pass.

[mvo5]

This looks good now, thank you.