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

interfaces: allow map and execute permissions for files on removable media #11794

Merged
merged 1 commit into from May 18, 2022

Conversation

jhenstridge
Copy link
Contributor

@jhenstridge jhenstridge commented May 17, 2022

This PR is intended to help address canonical/steam-snap#9 in the Steam snap. Steam allows the user to change the location where it stores games it downloads. While we are not trying to support arbitrary locations with the snap, one common case is to place the library on a volume mounted in /media. We almost support this by plugging the removable-media interface. Unfortunately, the AppArmor restrictions of that interface do not include execute permission.

This PR adds map and execute permissions for files under /mnt/ and /media/*/ to address this. As per @alexmurray's comment, it isn't worth making this configurable since a snap can already execute files found in these locations by first copying them to $SNAP_USER_DATA.

@alexmurray
Copy link
Collaborator

Since as you say - a determined snap will be able to execute binaries found on removable media by first copying them to somewhere in the home directory - why not just add mix to the existing rules in removable-media for all snaps? This doesn't appear to weaken the security IMO (note home is not needed either - if a snap has removable-media it can copy to $SNAP_USER_DATA or similar and execute from there).

@jhenstridge jhenstridge changed the title interfaces: add an allow-exec attribute to the removable-media interface interfaces: allow map and execute permissions for files on removable media May 18, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #11794 (d5e8e04) into master (a9d558e) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #11794   +/-   ##
=======================================
  Coverage   78.22%   78.22%           
=======================================
  Files         954      954           
  Lines      112261   112261           
=======================================
+ Hits        87811    87813    +2     
+ Misses      18945    18943    -2     
  Partials     5505     5505           
Flag Coverage Δ
unittests 78.22% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
interfaces/builtin/removable_media.go 100.00% <ø> (ø)
cmd/snap/cmd_aliases.go 90.14% <0.00%> (-1.41%) ⬇️
overlord/hookstate/hookmgr.go 74.67% <0.00%> (-0.65%) ⬇️
overlord/ifacestate/helpers.go 76.52% <0.00%> (+0.46%) ⬆️
daemon/api_connections.go 93.58% <0.00%> (+0.53%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Collaborator

@alexmurray alexmurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Collaborator

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@bboozzoo bboozzoo added this to the 2.56 milestone May 18, 2022
@mvo5 mvo5 merged commit 71206e0 into snapcore:master May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants