snap-confine: bind mount /usr/lib/snapd relative to snap-confine #3956

mvo5 · 2017-09-22T08:52:25Z

Use the basedir of snap-confine to bind mount as /usr/lib/snapd
when using bases. This ensures we use the re-execed tools if we
are re-execing. This fixes the bug that busybox-static does not
work with the "bare" snap when using a snapd that re-execs from
e.g. beta.

Use the basedir of snap-confine to bind mount as /usr/lib/snapd when using bases. This ensures we use the re-execed tools if we are re-execing. This fixes the bug that busybox-static does not work with the "bare" snap when using a snapd that re-execs from e.g. beta.

zyga

Looks good. I think we can remove one of the calls to sc_must_snprintf but it is just an optimization. Technically dirname can return the . directory but that would require the attacker to be able to spoof /proc/self/exe already. I don't think we need more defense there. CC @jdstrand

…anks to zyga)

zyga

Thank you for the improvement :)

zyga · 2017-09-22T10:34:18Z

cmd/snap-confine/mount-support.c

 		char self[PATH_MAX + 1] = { 0, };
 		if (readlink("/proc/self/exe", self, sizeof self) > 0) {
-			char *dir = dirname(self);
-			sc_must_snprintf(src, sizeof src, "%s", dir);
+			src = dirname(self);


jdstrand

Comments inline. I think in a follow-up PR it would make sense to look for other instances of readlink() and dirname() in snap-confine and make sure we are being consistent. A small refactor to call a helper function might be in order.

jdstrand · 2017-09-22T14:03:38Z

cmd/snap-confine/mount-support.c

+		// that we are re-execing from
+		char *src = LIBEXECDIR;
+		char self[PATH_MAX + 1] = { 0, };
+		if (readlink("/proc/self/exe", self, sizeof self) > 0) {


Can we use sizeof(self) - 1 here? readlink() doesn't append a NULL byte so if for some reason the kernel returned too much, we are safe (since you initialized 'self' to all NULLs). That seems to be your intent for using PATH_MAX + 1, but let's not trust there are no kernel bugs either.

I might also point out that the not all /proc/*/exes are symlinks, but they will be for /proc/self/exe for snap-confine (ie, kernel processes won't be a symlink). If we are guarding against kernel bugs, we could:

if (strstr(self, "/snap-confine") == NULL) { die(...); }

This is a harder kernel bug to imagine happening, but it's a cheap check. @zyga, I didn't think of this one in the 3621 review. Follow-up PR would be fine there.

@jdstrand ack, will do.

jdstrand · 2017-09-22T14:09:54Z

cmd/snap-confine/mount-support.c

+		char self[PATH_MAX + 1] = { 0, };
+		if (readlink("/proc/self/exe", self, sizeof self) > 0) {
+			src = dirname(self);
+		}


I think we should } else { die(...) }. no? If the kernel is failing to give what we need, we probably don't want to continue using whatever LIBEXECDIR is set to, but maybe I am missing something? Not to mention, we are initializing src to LIBEXECDIR, but then we immediately reassign it to dirname so, barring a kernel bug, we are using extra memory on the stack for no reason.

I was initially thinking that LIBEXECDIR is a valid fallback if for whatever reason we cannot read /proc - however if you think we should fail hard in this case I'm happy to add code for this.

I updated the code to die() if /proc/self/exe cannot be read.

I think it would make the most sense to use char *src = NULL; and require readlink() to succeed and when it does, src = dirname(self).

jdstrand · 2017-09-22T14:14:26Z

cmd/snap-confine/mount-support.c

+		char *src = LIBEXECDIR;
+		char self[PATH_MAX + 1] = { 0, };
+		if (readlink("/proc/self/exe", self, sizeof self) > 0) {
+			src = dirname(self);


Since we are guarding against kernel bugs, and we know that dirname will always return a NULL-terminated string, we can do a cheap check on the first character (we are doing this in PR 3621's bootstrap.c, for example). We should probably check for other occurrences in the snap-confine sources):

// dirname(path) might return '.' depending on path. /proc/self/exe should always point // to an absolute path, but let's guarantee that. if (src[0] != '/') { die(...) }

jdstrand · 2017-09-22T15:06:41Z

cmd/snap-confine/mount-support.c

@@ -363,9 +363,19 @@ static void sc_bootstrap_mount_namespace(const struct sc_mount_config *config)
 		// where $ROOT is either "/" or the "/snap/core/current"
 		// that we are re-execing from
 		char *src = LIBEXECDIR;
-		char self[PATH_MAX + 1] = { 0, };
-		if (readlink("/proc/self/exe", self, sizeof self) > 0) {
+		char self[PATH_MAX] = { 0, };


Note I didn't mean for you to remove PATH_MAX+1 here. Your change for sizeof(self) -1 guards against the issue I was referring to (so this code is safe), but now it means it will fail in the unlikely event that a valid path has length of PATH_MAX.

jdstrand · 2017-09-22T15:31:46Z

cmd/snap-confine/mount-support.c

+		}
+		src = dirname(self);
+		// dirname(path) might return '.' depending on path. /proc/self/exe should always point
+// to an absolute path, but let's guarantee that.


Think you need a go fmt here.

jdstrand

Thanks for the updates! Approving since the code here is hardened and robust, but please consider my follow on comment regarding PATH_MAX+1.

mvo5 · 2017-09-22T16:55:30Z

Thanks! Sorry that I misunderstood the earlier comment. This is fixed now.

zyga · 2017-09-25T12:22:56Z

@mvo5 the test failures look real. I think some apparmor tweak is needed for snap-confine's profile.

When app snap uses a custom base snap we bind-mount the aforementioned directory from the core snap but that bind mount can be safely made to be read-only. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

codecov-io · 2017-09-25T14:30:25Z

Codecov Report

Merging #3956 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3956      +/-   ##
==========================================
+ Coverage   75.93%   75.96%   +0.03%     
==========================================
  Files         420      423       +3     
  Lines       36310    36525     +215     
==========================================
+ Hits        27571    27746     +175     
- Misses       6807     6838      +31     
- Partials     1932     1941       +9

Impacted Files	Coverage Δ
interfaces/apparmor/backend.go	`70.73% <0%> (-1.32%)`	⬇️
cmd/snap-update-ns/main.go	`0% <0%> (ø)`	⬆️
interfaces/builtin/system_observe.go	`100% <0%> (ø)`	⬆️
interfaces/builtin/opengl.go	`100% <0%> (ø)`	⬆️
cmd/snap-repair/trace.go	`81.31% <0%> (ø)`
cmd/snap-repair/cmd_show.go	`82.22% <0%> (ø)`
cmd/snap-repair/cmd_list.go	`87.17% <0%> (ø)`
dirs/dirs.go	`98.09% <0%> (+0.01%)`	⬆️
snap/validate.go	`95.75% <0%> (+0.05%)`	⬆️
cmd/snap-repair/runner.go	`82.59% <0%> (+0.18%)`	⬆️
... and 2 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 f6a3064...11c4a5e. Read the comment docs.

* snap-confine: bind mount /usr/lib/snapd relative to snap-confine Use the basedir of snap-confine to bind mount as /usr/lib/snapd when using bases. This ensures we use the re-execed tools if we are re-execing. This fixes the bug that busybox-static does not work with the "bare" snap when using a snapd that re-execs from e.g. beta.

mvo5 added this to the 2.28 milestone Sep 22, 2017

zyga approved these changes Sep 22, 2017

View reviewed changes

remove extra sc_must_snprintf() in sc_bootstrap_mount_namespace() (th…

cf8f348

…anks to zyga)

mvo5 requested a review from jdstrand September 22, 2017 10:11

zyga reviewed Sep 22, 2017

View reviewed changes

fix apparmor rules

d2b9237

jdstrand suggested changes Sep 22, 2017

View reviewed changes

mvo5 added 2 commits September 22, 2017 16:59

address review feedback (thanks to jdstrand)

7a35e01

die() if /proc/self/exe cannot be read

8b64456

jdstrand reviewed Sep 22, 2017

View reviewed changes

jdstrand approved these changes Sep 22, 2017

View reviewed changes

final set of review comments addressed (thanks jdstrand!)

f65ee91

mvo5 and others added 2 commits September 25, 2017 15:38

fix snap-confine apparmor profile

71093d4

cmd/snap-confine: use read-only bind mount for /usr/lib/snapd

11c4a5e

When app snap uses a custom base snap we bind-mount the aforementioned directory from the core snap but that bind mount can be safely made to be read-only. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

mvo5 merged commit 9c330f8 into canonical:master Sep 25, 2017

jdstrand mentioned this pull request Feb 20, 2020

cmd/snap-confine: fix everything flagged by coverity #8165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

snap-confine: bind mount /usr/lib/snapd relative to snap-confine #3956

snap-confine: bind mount /usr/lib/snapd relative to snap-confine #3956

mvo5 commented Sep 22, 2017

zyga left a comment

zyga left a comment

zyga Sep 22, 2017

jdstrand left a comment

jdstrand Sep 22, 2017

jdstrand Sep 22, 2017

zyga Sep 22, 2017

jdstrand Sep 22, 2017

mvo5 Sep 22, 2017

mvo5 Sep 22, 2017

jdstrand Sep 22, 2017

jdstrand Sep 22, 2017 •

edited

Loading

jdstrand Sep 22, 2017

jdstrand Sep 22, 2017

jdstrand left a comment •

edited

Loading

mvo5 commented Sep 22, 2017

zyga commented Sep 25, 2017 •

edited

Loading

codecov-io commented Sep 25, 2017

snap-confine: bind mount /usr/lib/snapd relative to snap-confine #3956

snap-confine: bind mount /usr/lib/snapd relative to snap-confine #3956

Conversation

mvo5 commented Sep 22, 2017

zyga left a comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand left a comment • edited Loading

Choose a reason for hiding this comment

mvo5 commented Sep 22, 2017

zyga commented Sep 25, 2017 • edited Loading

codecov-io commented Sep 25, 2017

Codecov Report

jdstrand Sep 22, 2017 •

edited

Loading

jdstrand left a comment •

edited

Loading

zyga commented Sep 25, 2017 •

edited

Loading