Permalink
Browse files

Re-associate with pid-1 mount namespace if required

This patch changes the initialization process of mount namespace
handling code. If the snap-confine process is itself in a namespace
other than that of pid-1 (which happens when snaps with confinement
other than classic are trying to invoke other snaps) then snap-confine
will move itself to the mount namespace of pid-1 before attempting any
other actions.

This allows snap-confine to get access to /run/snapd/ns directory that
has private sharing and thus is not shared with any mount peer groups.

Fixes: https://bugs.launchpad.net/snap-confine/+bug/1644439
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
  • Loading branch information...
1 parent 19a7ebe commit b36fe6c334ca70166a7def06ba4418a764af492e @zyga zyga committed Dec 2, 2016
Showing with 47 additions and 0 deletions.
  1. +35 −0 src/ns-support.c
  2. +11 −0 src/ns-support.h
  3. +1 −0 src/snap-confine.c
View
@@ -169,6 +169,41 @@ static bool sc_is_ns_group_dir_private()
return false;
}
+void sc_reassociate_with_pid1_mount_ns()
+{
+ int init_mnt_fd __attribute__ ((cleanup(sc_cleanup_close))) = -1;
+ int self_mnt_fd __attribute__ ((cleanup(sc_cleanup_close))) = -1;
+ char init_buf[128], self_buf[128];
+
+ init_mnt_fd =
+ open("/proc/1/ns/mnt", O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_PATH);
+ if (init_mnt_fd < 0) {
+ die("cannot open mount namespace of the init process");
+ }
+ self_mnt_fd =
+ open("/proc/self/ns/mnt",
+ O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_PATH);
@jdstrand

jdstrand Dec 2, 2016

Contributor

O_RDONLY is ignored when specifying O_PATH.

@zyga

zyga Dec 2, 2016

Collaborator

I started with just readlink but then realized readlinkat with empty string and O_PATH dirfd. I'll remove both O_RDONLYs

+ if (self_mnt_fd < 0) {
+ die("cannot open mount namespace of the current process");
+ }
+
+ memset(init_buf, 0, sizeof init_buf);
+ if (readlinkat(init_mnt_fd, "", init_buf, sizeof init_buf) < 0) {
+ die("cannot perform readlinkat() on the mount namespace file descriptor of the init process");
+ }
@jdstrand

jdstrand Dec 2, 2016

Contributor

Off by one here. If for some weird reason readlinkat() returns something 128 characters, you'll have a non-nul terminated string. Please use:

if (readlinkat(init_mnt_fd, "", init_buf, sizeof init_buf - 1) < 0) {
...
@zyga

zyga Dec 2, 2016

Collaborator

Oh, nice catch. Thank you!

+ memset(self_buf, 0, sizeof self_buf);
+ if (readlinkat(self_mnt_fd, "", self_buf, sizeof self_buf) < 0) {
+ die("cannot perform readlinkat() on the mount namespace file descriptor of the current process");
+ }
@jdstrand

jdstrand Dec 2, 2016

Contributor

Same here. Please use:

if (readlinkat(self_mnt_fd, "", self_buf, sizeof self_buf - 1) < 0) {
+ if (memcmp(init_buf, self_buf, sizeof init_buf) != 0) {
@jdstrand

jdstrand Dec 2, 2016

Contributor

I'd prefer that you use a #define for 128 above and reference it here in case future code changes result in sizeof(init_buf) != sizeof(self_buf).

@jdstrand

jdstrand Dec 2, 2016

Contributor

Also note that your use of memcmp means that the above off-by-ones aren't a problem atm but a future code change could expose the issue.

@zyga

zyga Dec 2, 2016

Collaborator

Yes, that's a fair point.

+ debug
+ ("the current process does not share mount namespace with the init process, re-association required");
+ if (setns(init_mnt_fd, CLONE_NEWNS) < 0) {
+ die("cannot re-associate the mount namespace with the init process");
+ }
+ }
+}
+
void sc_initialize_ns_groups()
{
debug("creating namespace group directory %s", sc_ns_dir);
View
@@ -22,6 +22,17 @@
#include "apparmor-support.h"
+/**
+ * Re-associate the current process with the mount namespace of pid 1.
+ *
+ * This function inspects the mount namespace of the current process and that
+ * of pid 1. In case they differ the current process is re-associated with the
+ * mount namespace of pid 1.
+ *
+ * This function should be called before sc_initialize_ns_groups().
+ **/
+void sc_reassociate_with_pid1_mount_ns();
+
/**
* Initialize namespace sharing.
*
View
@@ -90,6 +90,7 @@ int main(int argc, char **argv)
if (group_name == NULL) {
die("SNAP_NAME is not set");
}
+ sc_reassociate_with_pid1_mount_ns();
sc_initialize_ns_groups();
struct sc_ns_group *group = NULL;
group = sc_open_ns_group(group_name, 0);

1 comment on commit b36fe6c

Contributor

jdstrand commented on b36fe6c Dec 2, 2016

The approach is fine, just a couple of small tweaks.

Please sign in to comment.