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

cmd/libsnap-confine-private: pass env real-home to snap-update-ns #13244

Conversation

ernestl
Copy link
Collaborator

@ernestl ernestl commented Oct 2, 2023

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 to snap-update-ns to enable expanding the new ensure-dir mount entries used for the personal-files interface that reference $HOME. It is proposed that $SNAP_UID is also passed to snap-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:

  • Part 1: pass env real-home and uid to snap-update-ns | pull/13244
  • Part 2: many: ensure-dir mounts for personal-files missing dirs | pull/13260
  • Part 3: add snap-update-ns support for ensure-dir mounts | pull/13342

Additional information:

@codecov-commenter
Copy link

Codecov Report

Merging #13244 (51109ba) into master (2725b1f) will increase coverage by 0.01%.
Report is 7 commits behind head on master.
The diff coverage is n/a.

❗ 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     
Flag Coverage Δ
unittests 78.83% <ø> (+0.01%) ⬆️

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

@ernestl ernestl changed the title cmd/libsnap-confine-private: pass env vars real-home and uid to snap-… cmd/libsnap-confine-private: pass env real-home and uid to snap-update-ns Oct 2, 2023
@ernestl ernestl force-pushed the SNAPDENG-8233_provide_snap-update-ns_with_real-home branch from 51109ba to e524e27 Compare October 2, 2023 15:02
@ernestl ernestl requested review from alexmurray and pedronis October 2, 2023 15:08
@ernestl ernestl added the Needs security review Can only be merged once security gave a :+1: label Oct 2, 2023
@ernestl ernestl requested a review from Meulengracht October 2, 2023 15:11
@ernestl ernestl added the Simple 😃 A small PR which can be reviewed quickly label Oct 2, 2023
@ernestl ernestl added the Needs Samuele review Needs a review from Samuele before it can land label Oct 4, 2023
@ernestl ernestl requested a review from miguelpires October 10, 2023 07:15
Copy link
Collaborator

@pedronis pedronis left a 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

Copy link
Contributor

@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.

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.

@ernestl
Copy link
Collaborator Author

ernestl commented Oct 23, 2023

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.
Consuming env variables does seem to present a risk that should perhaps be addressed. At this point I am wondering if we require a short term and longer term solution?

Currently there seems to be a precedent for using env vars in snap-confine e.g.

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 snap-update-ns because for user profiles where $SNAP_REAL_HOME is required, switch_to_privileged_user() also changes effective identity to the real user, and drops supplementary groups while retaining SYS_ADMIN.

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
@alexmurray
@pedronis

@pedronis pedronis self-requested a review October 24, 2023 10:48
@ernestl ernestl requested a review from alexmurray October 24, 2023 12:24
Copy link
Collaborator

@pedronis pedronis left a 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

@ernestl ernestl removed the request for review from Meulengracht October 25, 2023 18:55
Copy link
Contributor

@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 - 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.

@alexmurray alexmurray removed Security-High Needs security review Can only be merged once security gave a :+1: labels Oct 27, 2023
Copy link
Contributor

@ZeyadYasser ZeyadYasser 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! I have question regarding the source of SNAP_REAL_HOME

Copy link
Member

@andrewphelpsj andrewphelpsj 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!

Copy link
Contributor

@ZeyadYasser ZeyadYasser left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you!

@ernestl ernestl added the Squash-merge Please squash this PR when merging. label Dec 13, 2023
@ernestl ernestl changed the title cmd/libsnap-confine-private: pass env real-home and uid to snap-update-ns cmd/libsnap-confine-private: pass env real-home to snap-update-ns Dec 15, 2023
@ernestl
Copy link
Collaborator Author

ernestl commented Dec 15, 2023

Force merging, the spread errors does not relate to the changes.

@ernestl ernestl merged commit f582fcb into canonical:master Dec 15, 2023
fredldotme pushed a commit to fredldotme/snapd that referenced this pull request Mar 2, 2024
…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
fredldotme pushed a commit to fredldotme/snapd that referenced this pull request Jun 4, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Samuele review Needs a review from Samuele before it can land Simple 😃 A small PR which can be reviewed quickly Squash-merge Please squash this PR when merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants