interfaces: add a steam-support interface #11708

jhenstridge · 2022-04-22T11:43:14Z

This interface is intended to provide some additional permissions needed by the steam snap.

At present, this is primarily AppArmor and seccomp rules to allow Steam to launch pressure-vessel containers, which it uses to provide a consistent runtime environment to some games (at the moment mainly Windows games it runs under Proton/Wine). PV is based on Bubblewrap, as used by Flatpak and various other process sandboxes on GNOME systems.

Related to getting Steam games to run, I've added the futex_waitv syscall to the base template. Although the Ubuntu kernels don't yet support this syscall, we want to let Proton try to call it so it will fall back to the old futex API. As this has essentially the same security concerns as the existing futex syscalls, it seemed sensible to add it to the base template rather than the steam-support interface.

snap-seccomp knows about this syscall as of 15th April, when PR #11674 was merged.

jhenstridge · 2022-04-22T13:59:00Z

The attempt to add support for futex_waitv didn't work, as it looks like snapd is being built against a too old libseccomp. It looks like we'd need libseccomp >= 2.5.4, which was released yesterday. I think it's currently being built with 2.5.3.

setharnold · 2022-04-27T22:28:13Z

interfaces/builtin/steam_support.go

+@{PROC}/@{pid}/gid_map rw,
+@{PROC}/@{pid}/setgroups rw,
+@{PROC}/@{pid}/mounts r,
+@{PROC}/@{pid}/mountinfo r,


Can these @{PROC}/@{pid}/ entries be reduced with an owner prefix? Unfortunately @{pid} doesn't yet mean "only this process's pid", so these rules are perhaps wider than intended.

setharnold · 2022-04-27T22:29:16Z

interfaces/builtin/steam_support.go

+# when performing a bind mount). Ideally we'd write a rule that
+# requires the remount option in combination with any other, but
+# AppArmor doesn't currently support that.
+mount options in (rw, ro, nosuid, nodev, noexec, remount, bind, silent, relatime) -> /newroot/{,**},


Is it possible to write another eleven rules like the above, with smaller subsets of these?

The mount calls this is intended to cover are from this code:

https://github.com/containers/bubblewrap/blob/main/bind-mount.c

It is working around the fact that calling mount() with MS_BIND causes it to ignore all flags passed other than MS_REC. So it is essentially doing the following:

call mount(..., MS_BIND|MS_REC, ...) to perform the mount.

parse /proc/self/mountinfo to discover the new mounts that have been created, and decode the mount flags used for each mount.

for each new mount, bitwise or some additional flags to the existing flags. If the result is different, perform a mount(..., MS_SILENT|MS_BIND|MS_REMOUNT|new_flags,...) call to update the flags.

So the exact combination of flags will depend on the existing mounts on the system, which could include however the user has configured their host sytem, as that gets exposed through /var/lib/snapd/hostfs. So I think it'd be somewhat more than 12 possibilities if avoiding the in syntax.

I think the full enumeration would be the cross product of:

silent, bind, remount, nosuid: always set

nodev or nothing

ro or rw

noexec or nothing

noatime or relatime or nothing

nodiratime or nothing

So I think there are 48 possibilities. Does that sound about right?

Oh, yikes, that code is far more dynamic than I expected. I thought I was going to see an array of a dozen directories plus their flags in the sources... Feel free to ignore this entirely, then. Thanks for the explanation.

metorino

In general LGTM, but could you please add the owner rule qualifier as suggested by @setharnold so we keep the least privileges? Thanks!

…set up pressure-vessel containers

This is a new syscall used to wait on multiple futexes at once, and Wine/Proton will attempt to use it if the kernel supports it. Blocking access prevents it from falling back to the other futex related syscalls.

…ctories

mvo5

This looks good to me, but someone closer to our current interfaces world like @mardy should review it.

mvo5 · 2022-04-28T18:33:56Z

Also the unit tests are failing right now with:

----------------------------------------------------------------------
FAIL: basedeclaration_test.go:881: baseDeclSuite.TestPlugInstallation

basedeclaration_test.go:938:
    c.Check(err, IsNil, comm)
... value *errors.errorString = &errors.errorString{s:"installation not allowed by \"steam-support\" plug rule of interface \"steam-support\""} ("installation not allowed by \"steam-support\" plug rule of interface \"steam-support\"")
... steam-support

mardy

LGTM, thanks! For now please ignore the nitpick comment about generating the rules, it's better to have this merged first. And in any case I'm not sure it would be a win, so we can safely disregard that :-)

mardy · 2022-04-29T05:48:10Z

interfaces/builtin/steam_support.go

+#
+# But that is not supported by AppArmor. So we enumerate the possible
+# combinations of options Bubblewrap might use.
+remount options=(bind, silent, nosuid, rw) /newroot/{,**},


We might save some bytes if we generated this list programmatically, but it's probably not worth the effort (and if even, it should be a follow-up).

mardy · 2022-04-29T06:07:09Z

interfaces/builtin/steam_support.go

+# Pivot from the intermediate root to sandbox root
+mount options in (rw, silent, rprivate) -> /oldroot/,
+umount /oldroot/,
+pivot_root oldroot=/newroot/ /newroot/,


I learnt something new today! Nice! :-)

pedronis

looks not unreasonable, one question

pedronis · 2022-04-29T14:40:58Z

interfaces/builtin/steam_support.go

+/run/pressure-vessel/** mrw,
+/run/host/usr/sbin/ldconfig* ixr,
+/run/host/usr/bin/localedef ixr,
+/var/cache/ldconfig/** rw,


does the snap does a layout on this one, as it comes from the base afaict?

Could we followup with that outside of this PR? I'd like @jhenstridge to respond, but we won't get that as timely as we'd like.

ah, these are in the pivoted root? but is still mount /var from the snap root, so still not entirely sure how that is writable

I added these rules to handle accesses made by the process within the mount namespace created by pressure-vessel/bubblewrap.

Maybe in future this could be separated out into a sub-profile, but that's complicated by the fact that the executables we'd perform the transitions on are downloaded by Steam and may have varying paths.

I'm still a bit confused how that dir gets writable because the underlying dir comes from the base that is read-only

we can land this but I would like to understand how that gets writable by Monday. I'm probably missing something but I'm guessing what happens just looking at the rules here

The root of the bubblewrap sandbox is a tmpfs, so any path not mounted over is potentially writeable. It isn't exposing the whole host system /run or /var, so these locations are writeable (at least when AppArmor allows it).

pedronis · 2022-04-29T18:28:25Z

@jhenstridge I'm merging this but I would still like some better understanding of how that rw cache directory works on Monday.

This interface is intended to provide some additional permissions needed by the steam snap. At present, this is primarily AppArmor and seccomp rules to allow Steam to launch pressure-vessel containers, which it uses to provide a consistent runtime environment to some games (at the moment mainly Windows games it runs under Proton/Wine). PV is based on Bubblewrap, as used by Flatpak and various other process sandboxes on GNOME systems. Related to getting Steam games to run, I've added the futex_waitv syscall to the base template. Although the Ubuntu kernels don't yet support this syscall, we want to let Proton try to call it so it will fall back to the old futex API. As this has essentially the same security concerns as the existing futex syscalls, it seemed sensible to add it to the base template rather than the steam-support interface. snap-seccomp knows about this syscall as of 15th April, when PR #11674 was merged. * interfaces: add a steam-support interface with permissions needed to set up pressure-vessel containers * interfaces/seccomp: add futex_waitv to the base template This is a new syscall used to wait on multiple futexes at once, and Wine/Proton will attempt to use it if the kernel supports it. Blocking access prevents it from falling back to the other futex related syscalls. * tests: add steam-support to policy snap * interfaces: limit proc access to same owner in steam interface * interfaces: lock down the remount AppArmor rules for steam-support * interfaces: allow pressure-vessel to mount tmpfs to mask certain directories * interfaces/policy: add base declaration tests for steam-support

mvo5 added this to the 2.55 milestone Apr 22, 2022

mvo5 added Squash-merge Please squash this PR when merging. Needs security review Can only be merged once security gave a :+1: and removed Squash-merge Please squash this PR when merging. labels Apr 22, 2022

setharnold reviewed Apr 27, 2022

View reviewed changes

metorino approved these changes Apr 28, 2022

View reviewed changes

jhenstridge added 5 commits April 28, 2022 16:14

interfaces: add a steam-support interface with permissions needed to …

d4ebee5

…set up pressure-vessel containers

interfaces/seccomp: add futex_waitv to the base template

bbda62d

This is a new syscall used to wait on multiple futexes at once, and Wine/Proton will attempt to use it if the kernel supports it. Blocking access prevents it from falling back to the other futex related syscalls.

tests: add steam-support to policy snap

bad1e75

interfaces: limit proc access to same owner in steam interface

1cdd307

interfaces: lock down the remount AppArmor rules for steam-support

57cf149

jhenstridge force-pushed the iface-steam-support branch from 3d29f4e to 57cf149 Compare April 28, 2022 09:43

interfaces: allow pressure-vessel to mount tmpfs to mask certain dire…

a1e2623

…ctories

mvo5 approved these changes Apr 28, 2022

View reviewed changes

mardy approved these changes Apr 29, 2022

View reviewed changes

interfaces/policy: add base declaration tests for steam-support

7fe5326

jhenstridge marked this pull request as ready for review April 29, 2022 10:49

pedronis self-requested a review April 29, 2022 12:56

pedronis reviewed Apr 29, 2022

View reviewed changes

mvo5 added the Squash-merge Please squash this PR when merging. label Apr 29, 2022

pedronis self-assigned this Apr 29, 2022

pedronis merged commit eaad8a2 into canonical:master Apr 29, 2022

mvo5 added the cherry-picked label Apr 29, 2022

interfaces: add a steam-support interface #11708

interfaces: add a steam-support interface #11708

Uh oh!

Conversation

jhenstridge commented Apr 22, 2022

Uh oh!

jhenstridge commented Apr 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metorino left a comment

Choose a reason for hiding this comment

Uh oh!

mvo5 left a comment

Choose a reason for hiding this comment

Uh oh!

mvo5 commented Apr 28, 2022

Uh oh!

mardy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedronis commented Apr 29, 2022

Uh oh!

Uh oh!