preserve TMPDIR and HOSTALIASES across snap-confine invocation (LP: #1682308) #3872

mwhudson · Sep 7, 2017

Because snap-confine is setuid, the dynamic loader strips out certain
environment variables when starting the process. Work around this by renaming
them before invoking snap-confine and renaming them back in snap-exec.

mwhudson · Sep 7, 2017

I'm not sure but perhaps this is only something one would want for classic snaps?

codecov-io · Sep 7, 2017

Codecov Report

Merging #3872 into master will decrease coverage by 0.07%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #3872      +/-   ##
==========================================
- Coverage    75.8%   75.72%   -0.08%     
==========================================
  Files         424      428       +4     
  Lines       36580    36689     +109     
==========================================
+ Hits        27729    27784      +55     
- Misses       6905     6951      +46     
- Partials     1946     1954       +8

Impacted Files	Coverage Δ
cmd/snap-exec/main.go	`66.15% <70%> (-0.24%)`	⬇️
snap/snapenv/snapenv.go	`95.12% <86.66%> (-1.98%)`	⬇️
release/release.go	`90.76% <0%> (-9.24%)`	⬇️
cmd/snap/complete.go	`50.7% <0%> (-6.5%)`	⬇️
daemon/api.go	`71.62% <0%> (-0.73%)`	⬇️
snap/info.go	`88.29% <0%> (-0.65%)`	⬇️
interfaces/builtin/unity8.go	`54.16% <0%> (ø)`	⬆️
interfaces/builtin/system_observe.go	`100% <0%> (ø)`	⬆️
cmd/cmd.go	`83.87% <0%> (ø)`	⬆️
interfaces/builtin/mir.go	`80% <0%> (ø)`	⬆️
... and 14 more

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 5e9ea55...2ef8443. Read the comment docs.

zyga · Sep 7, 2017

zyga reviewed Sep 7, 2017

View changes

Two questions, code looks good but not sure about what the motivation is and semantics should be exactly.

If the user sets HOSTALIASES to a directory that is not readable due to the sandbox shoud this be still preserved?

I'm not sure TMPDIR should be preserved. The designated directory may not exist inside the execution environment. What is your motivation for preserving it?

mwhudson · Sep 7, 2017

The motivation is that this keeps confusing users of my Go snap: https://bugs.launchpad.net/snapd/+bug/1682308

I think your questions indicate the answer to my question: this should only be done for a classic snap, and it turns out that that's really easy (pushed).

Here is the full list of variables glibc strips out: https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=sysdeps/generic/unsecvars.h;hb=HEAD -- I guess it would make sense to save some more of these, maybe even all of them?

chipaca · Sep 12, 2017

LGTM, but, should we ask Jamie to review this?

mwhudson · Sep 13, 2017

I fixed the conflicts by amending the second commit to not touch the changelog (it was a mistake to commit it in the first place)

niemeyer · Sep 13, 2017

niemeyer requested changes Sep 13, 2017

View changes

Idea looks good, as long as it's only for classic confinement indeed.

Just some details please:

Sounds good to preserve all of them.

Done

if strings.HasPrefix(kv, PreservedUnsafePrefix) {
        continue
}

It took me a moment to realize why you wanted this so I added a comment too.

mvo5 · Sep 19, 2017

mvo5 reviewed Sep 19, 2017

View changes

nitpick - I'm not a fan of this boolean here. It makes it hard to reason about what the function is doing without looking at the signature. I.e.: what does envMap(env, true) mean? I would prefer a flag argument and a name, maybe something like envMap(env, {0, preserveUnsafeVars]) ?

I'm not 100% what you mean, but see what I pushed?

Yeah, this is what I had in mind, thanks for this! Sorry that I was hard to parse.

mvo5 · Sep 20, 2017

mvo5 approved these changes Sep 20, 2017

View changes

mvo5 · Sep 20, 2017

Code looks nice, thank you! I would love to see some sort of test for this, ideally both unit and spread. Please let us know if you would prefer help with this.

mwhudson · Sep 20, 2017

I've pushed some unit tests and a spread test. I haven't run the spread test locally so if it doesn't work I guess I'd like some help with that :)

mwhudson · Sep 21, 2017

mwhudson reviewed Sep 21, 2017

View changes

This fails with "/bin/bash: line 65: SNAPMOUNTDIR: unbound variable" which doesn't make any sense to me. Also the instructions in HACKING.md for running the spread tests don't work. Help?

If you have the a key for linode API access (aka spread key) you can run this test with:

spread -debug -v linode:ubuntu-16.04-64:tests/main/confinement-classic

(substitute target environment if you want other OS)

I would also suggest to cut on the number of workers to just one (see spread.yaml) as otherwise debugging is weird and you waste machines that do nothing useful.

zyga · Sep 29, 2017

zyga reviewed Sep 29, 2017

View changes

Thank you

mvo5 · Oct 4, 2017

This fails in the unit tests:

coverage: 50.0% of statements
ok  	github.com/snapcore/snapd/snap/snapdir	0.004s
# github.com/snapcore/snapd/snap/snapenv
snap/snapenv/snapenv_test.go:195:9: undefined: val
snap/snapenv/snapenv_test.go:197:11: undefined: val

failure looks real.

zyga · Oct 5, 2017

zyga approved these changes Oct 5, 2017

View changes

LGTM, some comments inline for your consideration.

noPreserve and doPreserve differ by one character. With my ageing eyes I find it hard to distinguish easily. How about simply preserve and notPreserve (other ideas welcome)

This would be a nice thing to go generate eventually. No need to do it now but maybe add a comment.

jdstrand · Oct 5, 2017

jdstrand requested changes Oct 5, 2017 • edited

View changes

This looks ok to me overall, but please address my comment on renaming the unit tests. Also, while we test in spread that TMPDIR makes it through to the classic snap, we should also test that TMPDIR doesn't make it through for non-classic.

In terms of the approach, I think it is ok because the actual setuid binary (snap-confine) will still have these variables stripped (ie, we aren't adding to the attack surface for snap-confine). If the classic snap shipped a setuid binary, it would still be stripped at exec().

Furthermore, a classic snap is intended to work more with the system settings so preserving (at least some of these) variables for classic makes sense. That said, it seems to me since we are preserving the various LD_* variables, this PR may add confusion for users and/or may complicate matters related to https://forum.snapcraft.io/t/classic-snaps-failing-on-ubuntu-17-10/2324/7. I've not studied that issue closely so will not block on this point, but please consider it before merging.

For completeness and posterity, it is also worth noting that AppArmor looks at AT_SECURE with the profile transitions for Px, Cx and Ux. This PR will not affect that, since any exec transitions will still have the variables stripped at the time of the exec(). Specifically for snappy, classic snap policy intentionally has 'pix' for profile transitions (ie, to not strip these variables when the snap executes other binaries), and a non-snap application that has an apparmor profile that is executing a classic snap that has a Px/Cx/Ux rule for it will correctly still have these vars cleared on profile transition (just as the local admin specified).

This should be named TestExecEnvNoRenameTMPDIRForNonClassic() and the next test should be named TestExecEnvRenameTMPDIRForClassic(), no? (in this function you are mocking a non-classic snap and the next a classic snap)

mwhudson · Oct 8, 2017

This looks ok to me overall, but please address my comment on renaming the unit tests.

Ack.

Also, while we test in spread that TMPDIR makes it through to the classic snap, we should also test that TMPDIR doesn't make it through for non-classic.

OK, but I think I would also argue that if non-preservation of TMPDIR etc is important, we should probably filter them out ourselves (which would be easy to do in this branch, I guess) rather than relying on the dynamic loader doing it, as e.g. it doesn't look like musl does the same stripping or if for some reason we shipped a statically linked snap-confine there wouldn't be a dynamic loader at all.

Do you think I should change envMap to filter out "unsafe" variables in the non-classic case?

In terms of the approach, I think it is ok because the actual setuid binary (snap-confine) will still have these variables stripped (ie, we aren't adding to the attack surface for snap-confine). If the classic snap shipped a setuid binary, it would still be stripped at exec().

Furthermore, a classic snap is intended to work more with the system settings so preserving (at least some of these) variables for classic makes sense. That said, it seems to me since we are preserving the various LD_* variables, this PR may add confusion for users and/or may complicate matters related to https://forum.snapcraft.io/t/classic-snaps-failing-on-ubuntu-17-10/2324/7. I've not studied that issue closely so will not block on this point, but please consider it before merging.

Although the only variable my users actually seem to care about is TMPDIR, I think I would argue that not allowing these variables through is more confusing than blocking them. My sympathy for people who set LD_LIBRARY_PATH without knowing what they are doing is a bit limited.

For completeness and posterity, it is also worth noting that AppArmor looks at AT_SECURE with the profile transitions for Px, Cx and Ux. This PR will not affect that, since any exec transitions will still have the variables stripped at the time of the exec(). Specifically for snappy, classic snap policy intentionally has 'pix' for profile transitions (ie, to not strip these variables when the snap executes other binaries), and a non-snap application that has an apparmor profile that is executing a classic snap that has a Px/Cx/Ux rule for it will correctly still have these vars cleared on profile transition (just as the local admin specified).

mwhudson · Oct 9, 2017

That said, it seems to me since we are preserving the various LD_* variables, this PR may add confusion for users and/or may complicate matters related to https://forum.snapcraft.io/t/classic-snaps-failing-on-ubuntu-17-10/2324/7. I've not studied that issue closely so will not block on this point, but please consider it before merging.

Although the only variable my users actually seem to care about is TMPDIR, I think I would argue that not allowing these variables through is more confusing than blocking them. My sympathy for people who set LD_LIBRARY_PATH without knowing what they are doing is a bit limited.

In addition, now that I've read that thread, the solution being proposed there in effect means that LD_LIBRARY_PATH will be ignored for binaries in classic snaps anyway.

niemeyer · Oct 17, 2017

niemeyer approved these changes Oct 17, 2017

View changes

Thanks for the changes.

mwhudson changed the title from preserve TMPDIR and HOSTALIASES across snap-confine invocation (LP: #… to preserve TMPDIR and HOSTALIASES across snap-confine invocation (LP: #1682308) Sep 7, 2017

-        -// envMap creates a map from the given environment string list, e.g. the
-        -// list returned from os.Environ()
+        +var unsafeEnv = map[string]bool{
+        +	"HOSTALIASES": true,

-        -// list returned from os.Environ()
+        +var unsafeEnv = map[string]bool{
+        +	"HOSTALIASES": true,
+        +	"TMPDIR":      true,

-        -func envMap(env []string) map[string]string {
+        +var unsafeEnv = map[string]bool{
+        +	"HOSTALIASES": true,
+        +	"TMPDIR":      true,

+        +// rename some variables that will be stripped out by the dynamic
+        +// linker executing the setuid snap-confine by prepending their names
+        +// with PreservedUnsafePrefix.
+        +func envMap(env []string, preserveUnsafeVars bool) map[string]string {
          	envMap := map[string]string{}
          	for _, kv := range env {

+        +// rename variables that will be stripped out by the dynamic linker
+        +// executing the setuid snap-confine by prepending their names with
+        +// PreservedUnsafePrefix.
+        +func envMap(env []string, preserveUnsafeVars bool) map[string]string {

snapcore/snapd

preserve TMPDIR and HOSTALIASES across snap-confine invocation (LP: #1682308) #3872

mwhudson commented Sep 7, 2017 •

Edited 1 time

mwhudson
Sep 7, 2017

mwhudson changed the title from preserve TMPDIR and HOSTALIASES across snap-confine invocation (LP: #… to preserve TMPDIR and HOSTALIASES across snap-confine invocation (LP: #1682308) Sep 7, 2017

mwhudson commented Sep 7, 2017

codecov-io commented Sep 7, 2017 •

Edited 1 time

codecov-io
Sep 13, 2017

zyga reviewed Sep 7, 2017

View changes

mwhudson commented Sep 7, 2017

chipaca commented Sep 12, 2017

mwhudson commented Sep 13, 2017

niemeyer requested changes Sep 13, 2017

View changes

mwhudson added some commits Sep 13, 2017

mvo5 reviewed Sep 19, 2017

View changes

mvo5 approved these changes Sep 20, 2017

View changes

mvo5 commented Sep 20, 2017

mwhudson added some commits Sep 20, 2017

mwhudson commented Sep 20, 2017

mwhudson reviewed Sep 21, 2017

View changes

mwhudson added some commits Sep 29, 2017

zyga reviewed Sep 29, 2017

View changes

mwhudson added some commits Oct 3, 2017

mvo5 commented Oct 4, 2017

zyga approved these changes Oct 5, 2017

View changes

zyga added some commits Oct 5, 2017

jdstrand requested changes Oct 5, 2017 • edited

View changes

mwhudson commented Oct 8, 2017 •

Edited 1 time

mwhudson
Oct 9, 2017

mwhudson added some commits Oct 8, 2017

mwhudson commented Oct 9, 2017

niemeyer approved these changes Oct 17, 2017

View changes

mvo5 merged commit `9b9b6ca` into snapcore:master Oct 18, 2017
6 of 7 checks passed

6 of 7 checks passed

snapcore/snapd

Join GitHub today

preserve TMPDIR and HOSTALIASES across snap-confine invocation (LP: #1682308) #3872

Conversation

mwhudson commented Sep 7, 2017 • edited Edited 1 time mwhudson Sep 7, 2017

All checks have passed

mwhudson changed the title from preserve TMPDIR and HOSTALIASES across snap-confine invocation (LP: #… to preserve TMPDIR and HOSTALIASES across snap-confine invocation (LP: #1682308) Sep 7, 2017

mwhudson commented Sep 7, 2017

codecov-io commented Sep 7, 2017 • edited Edited 1 time codecov-io Sep 13, 2017

Codecov Report

zyga reviewed Sep 7, 2017 View changes

mwhudson commented Sep 7, 2017

chipaca commented Sep 12, 2017

All checks have passed

mwhudson commented Sep 13, 2017

niemeyer requested changes Sep 13, 2017 View changes

mwhudson added some commits Sep 13, 2017

Some checks were not successful

All checks have passed

mvo5 reviewed Sep 19, 2017 View changes

Some checks were not successful

mvo5 approved these changes Sep 20, 2017 View changes

mvo5 commented Sep 20, 2017

mwhudson added some commits Sep 20, 2017

Some checks were not successful

Some checks were not successful

mwhudson commented Sep 20, 2017

mwhudson reviewed Sep 21, 2017 View changes

mwhudson added some commits Sep 29, 2017

All checks have failed

zyga reviewed Sep 29, 2017 View changes

mwhudson added some commits Oct 3, 2017

All checks have failed

All checks have failed

mvo5 commented Oct 4, 2017

All checks have passed

zyga approved these changes Oct 5, 2017 View changes

zyga added some commits Oct 5, 2017

Some checks were not successful

jdstrand requested changes Oct 5, 2017 • edited View changes

jdstrand Oct 5, 2017 • edited Edited 1 time jdstrand Oct 5, 2017

mwhudson commented Oct 8, 2017 • edited Edited 1 time mwhudson Oct 9, 2017

mwhudson added some commits Oct 8, 2017

Some checks were not successful

mwhudson commented Oct 9, 2017

niemeyer approved these changes Oct 17, 2017 View changes

Hide details View details mvo5 merged commit 9b9b6ca into snapcore:master Oct 18, 2017 6 of 7 checks passed

6 of 7 checks passed

mwhudson commented Sep 7, 2017 •

Edited 1 time

mwhudson
Sep 7, 2017

codecov-io commented Sep 7, 2017 •

Edited 1 time

codecov-io
Sep 13, 2017

zyga reviewed Sep 7, 2017

View changes

niemeyer requested changes Sep 13, 2017

View changes

mvo5 reviewed Sep 19, 2017

View changes

mvo5 approved these changes Sep 20, 2017

View changes

mwhudson reviewed Sep 21, 2017

View changes

zyga reviewed Sep 29, 2017

View changes

zyga approved these changes Oct 5, 2017

View changes

jdstrand requested changes Oct 5, 2017 • edited

View changes

jdstrand Oct 5, 2017 •

Edited 1 time

jdstrand
Oct 5, 2017

mwhudson commented Oct 8, 2017 •

Edited 1 time

mwhudson
Oct 9, 2017

niemeyer approved these changes Oct 17, 2017

View changes

mvo5 merged commit `9b9b6ca` into snapcore:master Oct 18, 2017
6 of 7 checks passed