Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discovery for sufficient snapd Permission Scope for Steam #364

Open
1 task done
Tracked by #363
ZoopOTheGoop opened this issue Feb 1, 2024 · 3 comments
Open
1 task done
Tracked by #363

Discovery for sufficient snapd Permission Scope for Steam #364

ZoopOTheGoop opened this issue Feb 1, 2024 · 3 comments
Labels
keep-open Label for the github action. Keeps it open even if issue is abandoned needs-information Issue or PR needs further information before triage needs-scoping Issue/enhancement needs to be scoped type/enhancement New feature or request

Comments

@ZoopOTheGoop
Copy link
Contributor

ZoopOTheGoop commented Feb 1, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Is the feature related to a problem or existing issue?

Yes, most of the issues are ultimately related to this, it is primarily discussed here, but has come up at other points:
canonical/snapd#12794 (comment)

Describe the solution/feature you'd like.

Assume we have near-unlimited ability to widen the AppArmor permissions for Steam, what would that look like? This also includes any ideal changes to the file layout of the environment the Steam Snap observes, even if it's more complex than a simple bind mount for us. Ways to prevent things like the most recent commit referencing the snap, AKA #361, from becoming problems before they're even discovered.

Of the options being discussed, this seems to (slowly) be the route we're currently taking. Though we are largely doing it reactively rather than proactively.

Short of completely unworkable ideas like just handing Steam **/** rwuxml[...] access, what is a reasonable scope for Steam permissions not to just tackle current issues but also ensure any reasonable feature added is compatible (ideally including VR someday)? No need to make an exact list of desired AppArmor profile rules and mount behavior, but a gist. Is such a scope is even reasonable to attain given your observation of our history of security reviews, or is this path's likely endpoint in reality just damage control unlikely to solve the core issue?

I will note that there is an in-progress feature that I do not know the ETA of that will allow at least some level of flexibility by asking the user if they want to allow a Snap certain AppArmor privileges directly, but I'm personally unsure of the scope of it and how much of a solution it will be in practice vs in concept, and am also unclear if there will be any ability (or consent on behalf of snapd/security etc) to give Steam more leeway if needed. Not unsure due to doubt or skepticism, just simple unfamiliarity.

Ignoring that feature, how close are we to approaching this with current proposed changes? Obviously this entire set of issues invalidates if we're extremely close, but I've gotten the impression that these are merely good steps in the right direction. What is the cost-benefit of this path compared to alternative ones from the tracking issue? In general, I'm inclined to agree with the viewpoint expressed in this commit message that fixing anything that requires snapd changes has a long turnaround time (involving at least three separate teams, four if your end finds the issue before us), which makes it somewhat inconvenient for everyone involved.

Ultimately, I'm not sure if my question is even "how close are we" so much as, "if this is the route we take, what's the ultimate likelihood that we limit surprise regressions and day-1 broken features in the Snap environment to a pleasant level?" Are there advantages/disadvantages to this path the other path(s) doesn't have? Are there core snapd changes unrelated to AppArmor profiles and other minor snapd interface tweaks like observed file layouts that would likely be needed for Steam to run at full capacity (as full as can be expected in any sandboxed container) that would be obviated by another solution? Can we do this in a way likely to both pass review and eliminate or minimize the number of Steam-specific snapd PRs?

Describe any alternatives you've considered.

#363 (comment)

Additional context

No response

@ZoopOTheGoop ZoopOTheGoop added the type/enhancement New feature or request label Feb 1, 2024
@ZoopOTheGoop ZoopOTheGoop added needs-scoping Issue/enhancement needs to be scoped needs-information Issue or PR needs further information before triage keep-open Label for the github action. Keeps it open even if issue is abandoned labels Feb 1, 2024
@smcv
Copy link

smcv commented Feb 1, 2024

Assume we have near-unlimited ability to widen the AppArmor permissions for Steam, what would that look like?

One thing that I would very much appreciate is relaxing the profile far enough that I can get a shell inside the sandbox and understand what the filesystem looks like! At the moment, for example ls /usr fails, and that's really unfriendly. I find it hard to see any security justification for why the contents of /usr would be a secret, when the answer seems to be that it's a core22 Snap runtime that is available to the public and can already be downloaded and inspected by any would-be attacker.

@smcv
Copy link

smcv commented Feb 1, 2024

ideally including VR someday

VR, specifically, is going to be a problem, in a way that other gaming isn't.

Steam's use of VR wants elevated privileges and no container, because its compositor strongly benefits from real-time GPU scheduling that is only available to processes that have capabilities in the top-level user namespace. I don't have VR equipment myself, but I'm told that in practice, this priority bump can be the difference between motion sickness and usable VR. As long as it needs to operate this way, this is never going to be possible in a Flatpak sandbox, and probably not in a Snap sandbox either.

I've tried to encourage VR developers to move towards a model where no elevated privileges are needed, but the various constraints involved don't make this at all straightforward: VR developers want guaranteed performance characteristics, and kernel developers don't want unprivileged users to be able to carry out a denial-of-service, but if I understand correctly, the elevated priority that the VR developers want is enough to lock arbitrary amounts of data into RAM (or possibly GPU memory) and stall the whole machine.

At the moment, this is represented by CAP_SYS_NICE in the top-level user namespace, and at least in Flatpak's case, sandbox developers can't let sandboxed processes have capabilities in the top-level user namespace even if we wanted to.

One suggestion was to turn this into an elevated rlimit which can be passed around between cooperating processes with sysadmin permission, but the driver developers didn't consider this to be a valid solution, and I don't understand the graphics driver architecture well enough to describe the alternative they advocated or assess whether it's feasible.

@smcv
Copy link

smcv commented Feb 1, 2024

Some general thoughts about AppArmor:

I think the fact that Snap is based on AppArmor restrictions is always going to be difficult to reconcile with the ability to run nested containers (bubblewrap or equivalent), because AppArmor is path-based and relies on being able to infer the meaning and security characteristics of a file from its path, but the whole point of Steam's use of bubblewrap is that it changes the meaning of a path, making it difficult to reason about whether an AppArmor restriction is safe or whether it can be circumvented.

This makes me cautiously optimistic about #365, because that would (mostly? entirely?) remove the need for Steam to run bubblewrap, at least in the short term - which would mean that each path always means what you think it does.

I personally think the mount-namespace-based model is a more attractive one than AppArmor-based access control, because if you want to be sure that app A can't access resource R, "there is no path within A's namespace that could be used to refer to R" is quite a compelling argument. This is also more compatible with nested containers than AppArmor is: a nested container with the ability to rewrite the mount namespace might be able to arrange for an AppArmor-denied path to reappear at an allowed location, but it can't possibly gain access to files that don't exist in the "outer" container, so making files disappear can be used as a ratcheting mechanism that can only become more restrictive as you go down the hierarchy.

But, if Snap is sufficiently wedded to AppArmor that it will continue to use AppArmor rules for this sort of thing, there are some design principles that I think can be good mitigations (this is partly based on my work with Steam, but also partly based on a previous project with an OS that used AppArmor heavily, and partly on my work with AppArmor profiles in Debian).

I think AppArmor rules are least fragile against implementation details changing when they address relatively large categories of files. For instance, you could apply fine-grained restrictions where one app can read /usr/share/applications and another can read /usr/share/fonts; but it would be more robust to say that all of /usr has essentially the same security characteristics (trusted, "owned" by the sysadmin/package manager, and essentially public because any would-be attacker can download and inspect the same set of .debs out-of-band), and grant blanket access to all of /usr at once.

Similarly, I see that Steam currently spams stderr and the audit log with what appear to be attempts to take out a flock(2) advisory lock on fonts in /usr/share. I don't know why it's doing this, but I also don't really care: if there's no security reason why allowing it to lock files that it can read would be bad, then I don't see any point in denying it (subject to the usual DAC rules of course).

The Steam library directories, **/steamapps/common/**, are another thing that could usefully be handled as a unit, with everything in there treated as essentially equal.

The more I look at AppArmor, the more I think that "magic" transitions as a side-effect of executing a program are something to be concerned about: if the program gets more privileges than its parent, then it's suddenly being placed in a position where, like a setuid or setcap executable, it is expected to distrust all aspects of its execution environment. Experience tells us that not many developers are able to code with sufficient paranoia to achieve that, even if they know it's expected of them, which in this case they probably do not (because the author of the program probably didn't write the AppArmor profile). So I would suggest avoiding px, ux and similar transitions. Cx can be OK when used cautiously, especially if it's a transition from more to less privilege, but if there's no security boundary to be enforced then it's somewhat pointless.

Conversely, if an app is allowed to read an executable file, I don't generally see a good reason why ix (execute, inheriting current profile) shouldn't also be allowed: by definition this can't give you the ability to do anything that the current process couldn't already do. So a broad rmix permission on relatively large units of the filesystem would remove a lot of fragility - for instance, if a program or library starts using a helper subprocess, a Snap app maintainer probably shouldn't need to find out from user bug reports that it has done so.

I can see that some AppArmor profiles use a denylist of known setuid executables to try to stop apps elevating privileges that way, but quite apart from "enumerating badness" being inherently doomed, I think nosuid mount points or PR_SET_NO_NEW_PRIVS are a more robust way to prevent that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep-open Label for the github action. Keeps it open even if issue is abandoned needs-information Issue or PR needs further information before triage needs-scoping Issue/enhancement needs to be scoped type/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants