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

tests: add snap-confine privilege test #3428

Merged
merged 4 commits into from
Jun 6, 2017

Conversation

zyga
Copy link
Contributor

@zyga zyga commented Jun 2, 2017

This test ensures that snap confine correctly drops privileges (user and
group identifiers) in various scenarios involving sudo and regular users.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

This test ensures that snap confine correctly drops privileges (user and
grup identifiers) in various scenarios involving sudo and regular users.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

Thanks for this test! I have some small comments but marking this as approved since the tests themselves are fine.

details: |
The openSUSE security team has made a remark about a particular part of
snap-confine's UID/GID handling. The code there was, we believe, correct
but this test is here to demonstrate that and ensure it never regresses.
Copy link

Choose a reason for hiding this comment

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

The tests prove snap-confine has the desired behavior so we don't have to say 'we believe' here.

Security review https://bugzilla.opensuse.org/show_bug.cgi?id=986050
# This test is not executed on a core system simply because of the hassle of
# building the support C program. In the future it might be improved with the
# use of the classic snap where we just use classic to build the helper.
Copy link

Choose a reason for hiding this comment

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

Note, core has python3 and python3 has os.getresuid(). I think it is fine to test on just classic, but if you really want it everywhere, keep that in mind. Also, I think this test is valid on other distros where snap-confine is setuid. On those with fscaps, we'd of course need different tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But python scripts cannot be setuid/setgid as they use an interpreter (bummer).

As for fscaps, that code is not used anymore and I actually removed it in one of my patches today (still pending PR)

Copy link

@jdstrand jdstrand Jun 2, 2017

Choose a reason for hiding this comment

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

re interpreted-- oh, right, duh.

On Debian /snap/bin is not on the secure path. Programs such as sudo and
su reset PATH to a predictable value and this breaks specific test that
wishes to start a snap command as a regular user.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
@codecov-io
Copy link

codecov-io commented Jun 2, 2017

Codecov Report

Merging #3428 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3428   +/-   ##
=======================================
  Coverage   77.56%   77.56%           
=======================================
  Files         371      371           
  Lines       25519    25519           
=======================================
  Hits        19794    19794           
  Misses       3975     3975           
  Partials     1750     1750
Impacted Files Coverage Δ
interfaces/sorting.go 93.33% <0%> (-3.34%) ⬇️
interfaces/builtin/network_manager.go 81.57% <0%> (ø) ⬆️
cmd/snap/cmd_aliases.go 96% <0%> (+2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d65d03...8b9c112. Read the comment docs.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Copy link
Contributor

@morphis morphis left a comment

Choose a reason for hiding this comment

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

LGTM, one minor thing

@@ -0,0 +1,24 @@
#define _GNU_SOURCE
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need a proper copyright header here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected, thank you!

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
@zyga zyga merged commit c856dfd into canonical:master Jun 6, 2017
@zyga zyga deleted the feature/snap-confine-guid-handing branch June 6, 2017 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants