-
Notifications
You must be signed in to change notification settings - Fork 599
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
cmd/libsnap-confine-private: pass env real-home to snap-update-ns #13244
cmd/libsnap-confine-private: pass env real-home to snap-update-ns #13244
Conversation
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## master #13244 +/- ##
==========================================
+ Coverage 78.82% 78.83% +0.01%
==========================================
Files 1015 1016 +1
Lines 126552 126639 +87
==========================================
+ Hits 99751 99833 +82
- Misses 20555 20558 +3
- Partials 6246 6248 +2
Flags with carried forward coverage won't be shown. Click here to find out more. see 12 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
51109ba
to
e524e27
Compare
e524e27
to
a7ad3d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some paranoia question, but hopefully security hasfurther input on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I am quite wary of this change. The original spec at https://docs.google.com/document/d/1yRqiFcdAIUjwakIQdijtW61vKojcJN2vTjrcwJvLAdo/edit#heading=h.f1vzrc21ib3n says
Currently $SNAP_REAL_HOME is not available within snap-update-ns and would require additional permissions to look up. Avoid additional permission by passing snap environment variable $SNAP_REAL_HOME from snap-confine to snap-update-ns.
My preference actually would be that snap-update-ns / snap-confine would not trust any environment variables which get passed to it and instead it should look these up with whatever additional permissions are required.
Or if we are going to trust values from the environment then these need to be strictly validated.
Thanks for your comments. Currently there seems to be a precedent for using env vars in As Samuele pointed out, this however happens after effective identify is changed to that of the real user, which significantly reduces risk. The same is true for In addition to this, I proposed a simple early check to sanity check $SNAP_REAL_HOME, by opening it. This requires additional apparmor allowance. Such a solution also poses another question: Should snap-update-ns fail for --user-mounts if e.g. there are issues with $SNAP_REAL_HOME. The alternative seems to be to ignore the mount entries, but this does not seem to be an established failure mode. And in addition: #13244 (comment) I added some suggestions to the spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaiu, given https://github.com/snapcore/snapd/blob/master/cmd/snap-update-ns/bootstrap.c#L505, snap-update-ns itself drops to the real user id when used in user ns mode, so this should be ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - snap-confine itself is not using the value from SNAP_REAL_HOME and just passing it through, so it doesn't make a lot of sense for it to validate it. Instead that should be done by snap-update-ns as you have done in the other PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I have question regarding the source of SNAP_REAL_HOME
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thank you!
Force merging, the spread errors does not relate to the changes. |
…nonical#13244) * cmd/libsnap-confine-private: pass env vars real-home and uid to snap-update-ns * cmd/libsnap-confine-private: not required to pass uid
…nonical#13244) * cmd/libsnap-confine-private: pass env vars real-home and uid to snap-update-ns * cmd/libsnap-confine-private: not required to pass uid
Modify snap-confine to pass environment variables
$SNAP_UID
and$SNAP_REAL_HOME
when a non-root user runs a non-classic snap application.$SNAP_REAL_HOME
is passed tosnap-update-ns
to enable expanding the newensure-dir
mount entries used for the personal-files interface that reference$HOME
. It is proposed that$SNAP_UID
is also passed tosnap-update-ns
to allow for a basic sanity check to ensure that$SNAP_REAL_HOME
belongs to the correct uid.Call path:
snap-confine.c | main() -> enter_non_classic_execution_environment() -> snap-confine/mount-support.c | sc_setup_user_mounts() -> libsnap-confine-private/tool.c | sc_call_snap_update_ns_as_user()
This PR is
part 1
of a larger feature to automatically create missing personal-files interface parent directories.There are 3 parts in total:
pass env real-home and uid to snap-update-ns
| pull/13244many: ensure-dir mounts for personal-files missing dirs
| pull/13260add snap-update-ns support for ensure-dir mounts
| pull/13342Additional information: