interfaces/mir: allow Wayland socket and non-root sockets #4365

gerboland · Dec 6, 2017

Mir 0.28 and greater implements the Wayland protocol, and needs to create its socket.

Mir running as non-root will create those sockets in $XDG_RUNTIME_DIR which is typically /run/users/*, so allow that too.

jdstrand · Dec 6, 2017

jdstrand requested changes Dec 6, 2017

View changes

I'm surprised we didn't notice this access before since we allow it in mirConnectedPlugAppArmor.

Yep, I found that funny too

I think the wayland interface should be updated to be implicit on classic and providable as a snap on non-classic (similar to how avahi-observe was recently converted) with these rules in the waylandPermanentSlotAppArmor policy. Then the mir snap would slots both mir and wayland.

Yep I initially tried the Mir snap providing the wayland interface, but it failed as it was not the core snap. I agree your proposal is much clearer, I'll do that then

zyga · Dec 7, 2017

zyga reviewed Dec 7, 2017

View changes

I agree with Jamie.

jdstrand · Dec 7, 2017

jdstrand requested changes Dec 7, 2017

View changes

This is a great start, thanks! A few questions which I think will lead to code changes inline.

You want this for auto-connection:

deny-auto-connection:
  on-classic: false

Is this access actually needed by wayland? I recall it was a mir-specific implementation detail.

The TTY stuff is needed by weston, but not shm

Right, was only talking about the shm access.

mutter doesn't seem to expose this on my system. Is this mir-specific? Something else?

Nope, my bad, removed

Is this needed by wayland?

Just accept4 by weston

This was also a mir-specific anonymous socket. Is it required by wayland?

You're right, I have removed it

Also mir-specific I think.

Yep, removed

This code will change if the unix rule goes away.

Done

This code will change if the unix rule goes away.

and done

gerboland · Dec 8, 2017

Ok, iteration 2 ready. I've whipped up a quick Weston snap to test it with:
https://code.launchpad.net/~gerboland/+git/weston-snap/+ref/master

jdstrand · Dec 8, 2017

jdstrand reviewed Dec 8, 2017

View changes

This wasn't fixed in the manner needed. Please adjust to be:

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

Is this access actually needed by wayland? I recall it was a mir-specific implementation detail.

The TTY stuff is needed by weston, but not shm

Right, was only talking about the shm access.

jdstrand · Dec 8, 2017

jdstrand requested changes Dec 8, 2017

View changes

Thanks for the updates! This is really coming along. Can you also update wayland_test.go and address the inline comments?

I think you will need:

/run/user/[0-9]*/wayland-shared-* rw,
/run/user/[0-9]*/wayland-cursor-shared-* rw,

I've been tracking these down, these are files for shared memory created by wayland (another one being xwayland-shared), to pass data between client and compositor, for example pass pixel buffers, cursor images.

The tricky thing is that clients will create them in their XDG_RUNTIME_DIR. For a snapped app, this directory is per-snap. Non-snapped apps use the normal /run/user/id -u

It is correct that the Wayland socket is in a shared place, so multiple clients can connect to it. But these shared memory files I think can be confined inside the snap's XDG_RUNTIME_DIR, as I don't believe they are ever shared between clients, they are purely for client-server communication.

For snapped apps, I think in addition to your proposal we should add

  /run/user/[0-9]*/snap.*/wayland-shared-* rw,
  /run/user/[0-9]*/snap.*/wayland-cursor-shared-* rw,
  /run/user/[0-9]*/snap.*/xwayland-shared-* rw,

(XWayland clients create the later). WDYT?

It would be fine for the server to do this, but note that @jhenstridge is working on user mounts. This will allow the snap's XDG_RUNTIME_DIR to be /run/user/id -u on the inside, but bind mounted on top of a snap-specific directory, then snapd/snap-confine/snap-update-ns would bind mount the wayland socket into the snap-specific dir. This would mean we could get rid of the desktop helper bits for the compat symlink (this also more closely follows the upstream designs).

How these intersect with the above accesses... I'm not sure. With blackbox testing, it seems to be initiated on the client side (they do an open() then unlink()) in the client's XDG_RUNTIME_DIR and I think the client must pass the fd over to the server. Ie, if I add an appropriate 'audit deny' rule, I can see denials like this:

apparmor="DENIED" operation="mknod" profile="snap.gedit.gedit" name="/run/user/1000/snap.gedit/wayland-cursor-shared-Hcwp8Q" pid=4896 comm="gedit" requested_mask="c" denied_mask="c" fsuid=1000 ouid=1000

as such, what you should do is add waylandConnectedSlotAppArmor policy, like so:

const waylandConnectedSlotAppArmor = `
# Allow access to client Wayland sockets for connected snaps
owner /run/user/[0-9]*/###PLUG_SECURITY_TAGS###/wayland-shared-* rw,
owner /run/user/[0-9]*/###PLUG_SECURITY_TAGS###/wayland-cursor-shared-* rw,
owner /run/user/[0-9]*/###PLUG_SECURITY_TAGS###/xwayland-shared-* rw,

This will allow the server to access these sockets for connected snaps. If my blackbox testing analysis is correct, this should continue to work after user mounts are in effect, because the slot side will continue to see these in /run/user/id -u/snap.*.

In addition to the above, the waylandPermanentSlotAppArmor policy would need this to handle unconfined applications:

owner /run/user/[0-9]*/wayland-shared-* rw,
owner /run/user/[0-9]*/wayland-cursor-shared-* rw,
owner /run/user/[0-9]*/xwayland-shared-* rw,

LP: #1656340 was about not creating /run/user/<uid>/snap.$SNAP, not /run/user/<uid>. The session manager is supposed to create this directory, not the snap. Why did you add this rule?

Mir is running as root, and I'm seeing /run/user/0 does not exist. Well with
[ -n "$XDG_RUNTIME_DIR" ] && mkdir -p $XDG_RUNTIME_DIR -m 700
I get
audit[6690]: AVC apparmor="DENIED" operation="mkdir" profile="snap.mir-kiosk.mir-kiosk" name="/run/user/0/" pid=6690 comm="mkdir" requested_mask="c" denied_mask="c" fsuid=0 ouid=0
mir-kiosk.mir-kiosk[6286]: mkdir: cannot create directory ‘/run/user/0’: Permission denied

Session Manager bug?

In this case I assume you mean that you are starting mir as a daemon. In this case, there is no user session and IMO it would be snappy's responsibility to create this directory. We had a previous PR for this in snap-confine, but it was never committed for various reasons. @zyga - can you comment? IIRC, the issue was the parent directories of /run/user/<uid> but I think if we just create /run/user/<uid> as 0700 as per the specification and log (perhaps not fail) if /run/user does not exist, we should be fine.

Correct assumption. Daemonized Mir just needs an accessible XDG_RUNTIME_DIR somehow.

Pease add accept back for the architectures that use it instead of accept4.

This should be:

owner /run/user/[0-9]*/wayland-[0-9]* rw,

I think you will also need:

owner /run/user/[0-9]*/wayland-shared-* rw,
owner /run/user/[0-9]*/wayland-cursor-shared-* rw,

You removed this from waylandPermanentSlotAppArmor, but left it here. This should be removed too, no?

I'm building my Chromium snap to incorporate XWayland, which will create that socket on startup. I was thinking it handiest to add support for any XWayland client to the Wayland interface. But in retrospect that's not very secure.

I'll iterate this a bit more.

Yes you right, this is pointless, removing.

These should be if !release.OnClassic { too I think.

Including tty too?

Yes. Note that this is the UDevPermanentSlot policy-- ie, what to tag for the slot provider so that the slot provider has these devices added to its device cgroup so it may function. When OnClassic, weston, mutter, etc are not running under snappy and are implicitly provided by the classic distro, and therefore there is no 'slot provider snap' where we need to tag these devices.

Thanks for the explanation. Learning lots..

codecov-io · Dec 13, 2017

Codecov Report

Merging #4365 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4365      +/-   ##
==========================================
+ Coverage   78.02%   78.04%   +0.01%     
==========================================
  Files         452      452              
  Lines       31278    31306      +28     
==========================================
+ Hits        24404    24432      +28     
  Misses       4830     4830              
  Partials     2044     2044

Impacted Files	Coverage Δ
interfaces/builtin/mir.go	`82.85% <ø> (ø)`	⬆️
interfaces/builtin/wayland.go	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa15490...7126f92. Read the comment docs.

gerboland · Jan 2, 2018

Assuming tests pass on CI, this should be good for another review pass. Thanks

jdstrand · Jan 3, 2018

jdstrand requested changes Jan 3, 2018

View changes

Thanks for all the updates! This is looking really good. Just a few small comments inline.

Can you update this comment to add 'from non-snap clients'?

Shouldn't this be 'owner' match? I expected it and the server side has owner match....

This ends up in the client's AppArmor policy, yeah? I didn't add owner as I was leaving open the possibility that the wayland socket may have different uid/gid than the client. But then you're right in that I should remove the "owner" from the client sockets. I think I'll do what you recommend

It is in the client's apparmor policy, yes. AIUI, part of the design of wayland was to not need for it to run as root (like X required for a long time) and indeed, mutter, plasma/wayland and sway all run their compositors under the uid of the user, which is why I thought this could use 'owner' match. What is the use case for a client running under one uid to use a wayland socket created/used by a compositor running as a different user?

You mentioned before that the client creates them in XDG_RUNTIME_DIR for the client, so it makes sense that ConnectedSlotAppArmor policy uses this:

owner /run/user/[0-9]*/###PLUG_SECURITY_TAGS###/wayland-shared-* rw,
owner /run/user/[0-9]*/###PLUG_SECURITY_TAGS###/wayland-cursor-shared-* rw,
owner /run/user/[0-9]*/###PLUG_SECURITY_TAGS###/xwayland-shared-* rw,

XDG_RUNTIME_DIR is (currently) expected to be set to /run/user/<uid>/snap.$SNAP and the default policy already allows:

owner /run/user/[0-9]*/snap.@{SNAP_NAME}/** mrwklix,

so it seems to me that these three rules for wayland-shared, wayland-cursor-shared and xwayland-shared should not be needed in ConnectedPlugAppArmor. Once we bind mount the snap-specific XDG_RUNTIME_DIR onto /run/user/<uid>, then you should have these rules, but today, we aren't doing that. Feel free to add a comment for this.

I realize I said I thought you might need these paths, but that was before your investigation that these are created client side.

Yes you're totally right.

Not a blocker, but the command here should not be required.

Typically you only need to test for one rule that is not shared with the other snippets, eg, here the drirc rule would work. Sometimes it is nice to also test other types of rules (eg, the security label on unix sockets, snap-specific file paths, etc). This applies to the ConnectedSlot and PermanentSlot policy below too. Not a blocker.

jdstrand · Jan 4, 2018

jdstrand approved these changes Jan 4, 2018

View changes

Thanks for making all these changes! I see that you added 'owner' match but there is an open question on it. I'm going to approve this as is for now since it is taking my recommendation of being more strict.

Since you used 'owner' above now, it would be nice to adjust this for owner so that if we change the above rule to remove it, this would fail.

Done. Sorry I missed that.
Thanks for the patient review!

gerboland · Jan 5, 2018

CI failing on fedora 26 setup...

gerboland changed the title from interfaces/mir: allow Wayland socket and non-root sockets to WIP: interfaces/mir: allow Wayland socket and non-root sockets Dec 13, 2017

gerboland changed the title from WIP: interfaces/mir: allow Wayland socket and non-root sockets to interfaces/mir: allow Wayland socket and non-root sockets Jan 2, 2018

Minor interface tweaks and beef up tests

b46c7f6

Address second round of review comments

559a788

Update test to match including 'owner'

abf76cd

Merge branch 'master' into mir-add-wayland-socket

7126f92

@@ -49,6 +49,9 @@ capability sys_tty_config,
          /{dev,run}/shm/\#* mrw,
          /run/mir_socket rw,
+        +/run/user/[0-9]*/mir_socket rw,

@@ -49,6 +49,9 @@ capability sys_tty_config,
          /{dev,run}/shm/\#* mrw,
          /run/mir_socket rw,
+        +/run/user/[0-9]*/mir_socket rw,
+        +/run/wayland* rw,
+        +/run/user/[0-9]*/wayland* rw,

+        +# Needed by server upon client connect
+        +accept
+        +accept4
+        +shmctl

          `
          const waylandConnectedPlugAppArmor = `
-        -# Description: Can access compositors supporting the wayland protocol
+        +# Description: Permit clients to connect to compositors supporting the Wayland protocol
+        +unix (receive, send) type=seqpacket addr=none peer=(label=###SLOT_SECURITY_TAGS###),

+        +	} else {
+        +		new = slotAppLabelExpr(slot)
+        +	}
+        +	snippet := strings.Replace(waylandConnectedPlugAppArmor, old, new, -1)

snapcore/snapd

interfaces/mir: allow Wayland socket and non-root sockets #4365

gerboland commented Dec 6, 2017

jdstrand requested changes Dec 6, 2017

View changes

zyga reviewed Dec 7, 2017

View changes

jdstrand requested changes Dec 7, 2017

View changes

gerboland commented Dec 8, 2017

jdstrand reviewed Dec 8, 2017

View changes

jdstrand requested changes Dec 8, 2017

View changes

gerboland changed the title from interfaces/mir: allow Wayland socket and non-root sockets to WIP: interfaces/mir: allow Wayland socket and non-root sockets Dec 13, 2017

codecov-io commented Dec 13, 2017 •

Edited 3 times

codecov-io
Jan 8, 2018

codecov-io
Jan 4, 2018

codecov-io
Jan 2, 2018

gerboland added some commits Dec 6, 2017

gerboland changed the title from WIP: interfaces/mir: allow Wayland socket and non-root sockets to interfaces/mir: allow Wayland socket and non-root sockets Jan 2, 2018

gerboland commented Jan 2, 2018

jdstrand requested changes Jan 3, 2018

View changes

jdstrand approved these changes Jan 4, 2018

View changes

gerboland commented Jan 5, 2018

snapcore/snapd

Join GitHub today

interfaces/mir: allow Wayland socket and non-root sockets #4365

Conversation

gerboland commented Dec 6, 2017

jdstrand requested changes Dec 6, 2017 View changes

zyga reviewed Dec 7, 2017 View changes

jdstrand requested changes Dec 7, 2017 View changes

gerboland commented Dec 8, 2017

jdstrand reviewed Dec 8, 2017 View changes

jdstrand requested changes Dec 8, 2017 View changes

gerboland changed the title from interfaces/mir: allow Wayland socket and non-root sockets to WIP: interfaces/mir: allow Wayland socket and non-root sockets Dec 13, 2017

codecov-io commented Dec 13, 2017 • edited Edited 3 times codecov-io Jan 8, 2018 codecov-io Jan 4, 2018 codecov-io Jan 2, 2018

Codecov Report

gerboland added some commits Dec 6, 2017

gerboland changed the title from WIP: interfaces/mir: allow Wayland socket and non-root sockets to interfaces/mir: allow Wayland socket and non-root sockets Jan 2, 2018

gerboland commented Jan 2, 2018

jdstrand requested changes Jan 3, 2018 View changes

gerboland Jan 4, 2018 • edited Edited 1 time gerboland Jan 4, 2018

jdstrand Jan 3, 2018 • edited Edited 1 time jdstrand Jan 4, 2018

jdstrand approved these changes Jan 4, 2018 View changes

gerboland commented Jan 5, 2018

jdstrand requested changes Dec 6, 2017

View changes

zyga reviewed Dec 7, 2017

View changes

jdstrand requested changes Dec 7, 2017

View changes

jdstrand reviewed Dec 8, 2017

View changes

jdstrand requested changes Dec 8, 2017

View changes

codecov-io commented Dec 13, 2017 •

Edited 3 times

codecov-io
Jan 8, 2018

codecov-io
Jan 4, 2018

codecov-io
Jan 2, 2018

jdstrand requested changes Jan 3, 2018

View changes

gerboland Jan 4, 2018 •

Edited 1 time

gerboland
Jan 4, 2018

jdstrand Jan 3, 2018 •

Edited 1 time

jdstrand
Jan 4, 2018

jdstrand approved these changes Jan 4, 2018

View changes