Skip to content

Commit

Permalink
many: Use /tmp/snap-private-tmp for per-snap private tmps
Browse files Browse the repository at this point in the history 
To avoid unprivileged users being able to interfere with the creation of the
private snap mount namespace, instead of creating this as /tmp/snap.$SNAP_NAME/
we can now use the systemd-tmpfiles configuration to do this for us
at boot with a known fixed name (/tmp/snap-private-tmp/) and then use that as
the base dir for creating per-snap private tmp mount
namespaces (eg. /tmp/snap-private-tmp/snap.$SNAP_INSTANCE/tmp) etc.

Signed-off-by: Alex Murray <alex.murray@canonical.com>
  • Loading branch information
@alexmurray
alexmurray committed Nov 23, 2022
1 parent 6226cdc commit 21ebc51
Show file tree
Hide file tree
Showing 14 changed files with 115 additions and 198 deletions.
40 changes: 0 additions & 40 deletions cmd/snap-confine/mount-support-test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,50 +92,10 @@ static void test_is_subdir(void)
g_assert_false(is_subdir("/", ""));
}

static void test_must_mkdir_and_open_with_perms(void)
{
// make a directory with some contents and check we can
// must_mkdir_and_open_with_perms() to get control of it
GError *error = NULL;
GStatBuf st;
gchar *test_dir = g_dir_make_tmp("test-mkdir-XXXXXX", &error);
g_assert_no_error(error);
g_assert_nonnull(test_dir);
g_assert_cmpint(chmod(test_dir, 0755), ==, 0);
g_assert_true(g_file_test
(test_dir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR));
g_assert_cmpint(g_stat(test_dir, &st), ==, 0);
g_assert_true(st.st_uid == getuid());
g_assert_true(st.st_gid == getgid());
g_assert_true(st.st_mode == (S_IFDIR | 0755));

gchar *test_subdir = g_build_filename(test_dir, "foo", NULL);
g_assert_cmpint(g_mkdir_with_parents(test_dir, 0755), ==, 0);
g_file_test(test_subdir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR);

// take over dir
int fd =
must_mkdir_and_open_with_perms(test_dir, getuid(), getgid(), 0700);
// check can unlink dir itself with no contents successfully and it
// still exists
g_assert_cmpint(fd, >=, 0);
g_assert_false(g_file_test
(test_subdir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR));
g_assert_true(g_file_test
(test_dir, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_DIR));
g_assert_cmpint(g_stat(test_dir, &st), ==, 0);
g_assert_true(st.st_uid == getuid());
g_assert_true(st.st_gid == getgid());
g_assert_true(st.st_mode == (S_IFDIR | 0700));
close(fd);
}

static void __attribute__((constructor)) init(void)
{
g_test_add_func("/mount/get_nextpath/typical",
test_get_nextpath__typical);
g_test_add_func("/mount/get_nextpath/weird", test_get_nextpath__weird);
g_test_add_func("/mount/is_subdir", test_is_subdir);
g_test_add_func("/mount/must_mkdir_and_open_with_perms",
test_must_mkdir_and_open_with_perms);
}
166 changes: 63 additions & 103 deletions cmd/snap-confine/mount-support.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

#include "mount-support.h"

#include <assert.h>
#include <errno.h>
#include <fcntl.h>
#include <libgen.h>
Expand Down Expand Up @@ -49,97 +50,13 @@
#include "mount-support-nvidia.h"

#define MAX_BUF 1000
#define SNAP_PRIVATE_TMP_ROOT_DIR "/tmp/snap-private-tmp"

static void sc_detach_views_of_writable(sc_distro distro, bool normal_mode);

static int must_mkdir_and_open_with_perms(const char *dir, uid_t uid, gid_t gid,
mode_t mode)
{
int retries = 10;
int fd;

mkdir:
if (--retries == 0) {
die("lost race to create dir %s too many times", dir);
}
// Ignore EEXIST since we want to reuse and we will open with
// O_NOFOLLOW, below.
if (mkdir(dir, 0700) < 0 && errno != EEXIST) {
die("cannot create directory %s", dir);
}
fd = open(dir, O_RDONLY | O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW);
if (fd < 0) {
// if is not a directory then remove it and try again
if (errno == ENOTDIR && unlink(dir) == 0) {
goto mkdir;
}
die("cannot open directory %s", dir);
}
// ensure base_dir has the expected permissions since it may have
// already existed
struct stat st;
if (fstat(fd, &st) < 0) {
die("cannot stat base directory %s", dir);
}
if (st.st_uid != uid || st.st_gid != gid
|| st.st_mode != (S_IFDIR | mode)) {
unsigned char random[10] = { 0 };
char random_dir[MAX_BUF] = { 0 };
int offset;
size_t i;

// base_dir isn't what we expect - create a random
// directory name and rename the existing erroneous
// base_dir to this then try recreating it again - NOTE we
// don't use mkdtemp() here since we don't want to actually
// create the directory yet as we want rename() to do that
// for us
#ifdef SYS_getrandom
// use syscall(SYS_getrandom) since getrandom() is
// not available on older glibc
if (syscall(SYS_getrandom, random, sizeof(random), 0) !=
sizeof(random)) {
die("cannot get random bytes");
}
#else
// use /dev/urandom on older systems which don't support
// SYS_getrandom
int rfd = open("/dev/urandom", O_RDONLY);
if (rfd < 0) {
die("cannot open /dev/urandom");
}
if (read(rfd, random, sizeof(random)) != sizeof(random)) {
die("cannot get random bytes");
}
close(rfd);
#endif
offset =
sc_must_snprintf(random_dir, sizeof(random_dir), "%s.",
dir);
for (i = 0; i < sizeof(random); i++) {
offset +=
sc_must_snprintf(random_dir + offset,
sizeof(random_dir) - offset,
"%02x", (unsigned int)random[i]);
}
// try and get dir which we own by renaming it to something
// else then creating it again

// TODO - change this to use renameat2(RENAME_EXCHANGE)
// once we can use a newer version of glibc for snapd
if (rename(dir, random_dir) < 0) {
die("cannot rename base_dir to random_dir '%s'",
random_dir);
}
close(fd);
goto mkdir;
}
return fd;
}

// TODO: simplify this, after all it is just a tmpfs
// TODO: fold this into bootstrap
static void setup_private_mount(const char *snap_name)
static void setup_private_tmp(const char *snap_instance)
{
// Create a 0700 base directory. This is the "base" directory that is
// protected from other users. This directory name is NOT randomly
Expand All @@ -162,37 +79,80 @@ static void setup_private_mount(const char *snap_name)
// Because the directories are reused across invocations by distinct users
// and because the directories are trivially guessable, each invocation
// unconditionally chowns/chmods them to appropriate values.
char base_dir[MAX_BUF] = { 0 };
char base[MAX_BUF] = { 0 };
char tmp_dir[MAX_BUF] = { 0 };
int private_tmp_root_fd SC_CLEANUP(sc_cleanup_close) = -1;
int base_dir_fd SC_CLEANUP(sc_cleanup_close) = -1;
int tmp_dir_fd SC_CLEANUP(sc_cleanup_close) = -1;
sc_must_snprintf(base_dir, sizeof(base_dir), "/tmp/snap.%s", snap_name);
sc_must_snprintf(tmp_dir, sizeof(tmp_dir), "%s/tmp", base_dir);

/* Switch to root group so that mkdir and open calls below create filesystem
* elements that are not owned by the user calling into snap-confine. */
/* Switch to root group so that mkdir and open calls below create
* filesystem elements that are not owned by the user calling into
* snap-confine. */
sc_identity old = sc_set_effective_identity(sc_root_group_identity());
// Create /tmp/snap.$SNAP_NAME/ 0700 root.root.
base_dir_fd = must_mkdir_and_open_with_perms(base_dir, 0, 0, 0700);
// Create /tmp/snap.$SNAP_NAME/tmp 01777 root.root Ignore EEXIST since we

// /tmp/snap-private-tmp should have already been created by
// systemd-tmpfiles but we can try create it anyway since snapd may have
// just been installed in which case the tmpfiles conf would not have
// got executed yet
if (mkdir(SNAP_PRIVATE_TMP_ROOT_DIR, 0700) < 0 && errno != EEXIST) {
die("cannot create /tmp/snap-private-tmp");
}
private_tmp_root_fd = open(SNAP_PRIVATE_TMP_ROOT_DIR,
O_RDONLY | O_DIRECTORY | O_CLOEXEC |
O_NOFOLLOW);
if (private_tmp_root_fd < 0) {
die("cannot open %s", SNAP_PRIVATE_TMP_ROOT_DIR);
}
struct stat st;
if (fstat(private_tmp_root_fd, &st) < 0) {
die("cannot stat %s", SNAP_PRIVATE_TMP_ROOT_DIR);
}
if (st.st_uid != 0 || st.st_gid != 0 || st.st_mode != (S_IFDIR | 0700)) {
die("%s has unexpected ownership / permissions",
SNAP_PRIVATE_TMP_ROOT_DIR);
}
// Create /tmp/snap-private-tmp/snap.$SNAP_INSTANCE_NAME/ 0700 root.root.
sc_must_snprintf(base, sizeof(base), "snap.%s", snap_instance);
if (mkdirat(private_tmp_root_fd, base, 0700) < 0 && errno != EEXIST) {
die("cannot create base directory: %s", base);
}
base_dir_fd =
openat(private_tmp_root_fd, base,
O_RDONLY | O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW);
if (base_dir_fd < 0) {
die("cannot open base directory: %s", base);
}
if (fstat(base_dir_fd, &st) < 0) {
die("cannot stat %s/%s", SNAP_PRIVATE_TMP_ROOT_DIR, base);
}
if (st.st_uid != 0 || st.st_gid != 0 || st.st_mode != (S_IFDIR | 0700)) {
die("%s/%s has unexpected ownership / permissions",
SNAP_PRIVATE_TMP_ROOT_DIR, base);
}
// Create /tmp/$PRIVATE/snap.$SNAP_NAME/tmp 01777 root.root Ignore EEXIST since we
// want to reuse and we will open with O_NOFOLLOW, below.
if (mkdirat(base_dir_fd, "tmp", 01777) < 0 && errno != EEXIST) {
die("cannot create private tmp directory %s/tmp", base_dir);
die("cannot create private tmp directory %s/tmp", base);
}
(void)sc_set_effective_identity(old);
tmp_dir_fd = openat(base_dir_fd, "tmp",
O_RDONLY | O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW);
if (tmp_dir_fd < 0) {
die("cannot open private tmp directory %s/tmp", base_dir);
die("cannot open private tmp directory %s/tmp", base);
}
if (fchown(tmp_dir_fd, 0, 0) < 0) {
die("cannot chown private tmp directory %s/tmp to root.root",
base_dir);
if (fstat(tmp_dir_fd, &st) < 0) {
die("cannot stat %s/%s/tmp", SNAP_PRIVATE_TMP_ROOT_DIR, base);
}
if (fchmod(tmp_dir_fd, 01777) < 0) {
die("cannot chmod private tmp directory %s/tmp to 01777",
base_dir);
if (st.st_uid != 0 || st.st_gid != 0 || st.st_mode != (S_IFDIR | 01777)) {
die("%s/%s/tmp has unexpected ownership / permissions",
SNAP_PRIVATE_TMP_ROOT_DIR, base);
}
// use the path to the file-descriptor in proc as the source mount point
// as this is a symlink itself to the real directory at
// /tmp/snap-private-tmp/snap.$SNAP_INSTANCE/tmp but doing it this way
// helps avoid any potential race
sc_must_snprintf(tmp_dir, sizeof(tmp_dir),
"/proc/self/fd/%d", tmp_dir_fd);
sc_do_mount(tmp_dir, "/tmp", NULL, MS_BIND, NULL);
sc_do_mount("none", "/tmp", NULL, MS_PRIVATE, NULL);
}
Expand Down Expand Up @@ -781,7 +741,7 @@ void sc_populate_mount_ns(struct sc_apparmor *apparmor, int snap_update_ns_fd,
}

// TODO: rename this and fold it into bootstrap
setup_private_mount(inv->snap_instance);
setup_private_tmp(inv->snap_instance);
// set up private /dev/pts
// TODO: fold this into bootstrap
setup_private_pts();
Expand Down
8 changes: 5 additions & 3 deletions cmd/snap-confine/snap-confine.apparmor.in
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@
mount options=(rw rshared) -> /var/lib/snapd/snap/,

# boostrapping the mount namespace
/tmp/snap.rootfs_*/ rw,
mount options=(rw rshared) -> /,
mount options=(rw bind) /tmp/snap.rootfs_*/ -> /tmp/snap.rootfs_*/,
mount options=(rw unbindable) -> /tmp/snap.rootfs_*/,
Expand Down Expand Up @@ -313,10 +314,11 @@
# set up snap-specific private /tmp dir
capability chown,
/tmp/ rw,
/tmp/snap.*/ rw,
/tmp/snap.*/tmp/ rw,
/tmp/snap-private-tmp/ rw,
/tmp/snap-private-tmp/snap.*/ rw,
/tmp/snap-private-tmp/snap.*/tmp/ rw,
mount options=(rw private) -> /tmp/,
mount options=(rw bind) /tmp/snap.*/tmp/ -> /tmp/,
mount options=(rw bind) /tmp/snap-private-tmp/snap.*/tmp/ -> /tmp/,
mount fstype=devpts options=(rw) devpts -> /dev/pts/,
mount options=(rw bind) /dev/pts/ptmx -> /dev/ptmx, # for bind mounting
mount options=(rw bind) /dev/pts/ptmx -> /dev/pts/ptmx, # for bind mounting under LXD
Expand Down
6 changes: 3 additions & 3 deletions cmd/snap-update-ns/system.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,12 @@ func (upCtx *SystemProfileUpdateContext) Assumptions() *Assumptions {
// the slot-side snap, as there is no mechanism to convey this information.
// As such, provide write access to all of /tmp.
as.AddUnrestrictedPaths("/var/lib/snapd/hostfs/tmp")
as.AddModeHint("/var/lib/snapd/hostfs/tmp/snap.*", 0700)
as.AddModeHint("/var/lib/snapd/hostfs/tmp/snap.*/tmp", 0777|os.ModeSticky)
as.AddModeHint("/var/lib/snapd/hostfs/tmp/snap-private-tmp/snap.*", 0700)
as.AddModeHint("/var/lib/snapd/hostfs/tmp/snap-private-tmp/snap.*/tmp", 0777|os.ModeSticky)
// This is to ensure that unprivileged users can create the socket. This
// permission only matters if the plug-side app constructs its mount
// namespace before the slot-side app is launched.
as.AddModeHint("/var/lib/snapd/hostfs/tmp/snap.*/tmp/.X11-unix", 0777|os.ModeSticky)
as.AddModeHint("/var/lib/snapd/hostfs/tmp/snap-private-tmp/snap.*/tmp/.X11-unix", 0777|os.ModeSticky)
// This is to ensure private shared-memory directories have
// the right permissions.
as.AddModeHint("/dev/shm/snap.*", 0777|os.ModeSticky)
Expand Down
8 changes: 4 additions & 4 deletions cmd/snap-update-ns/system_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,10 @@ func (s *systemSuite) TestAssumptions(c *C) {
c.Check(as.ModeForPath("/stuff"), Equals, os.FileMode(0755))
c.Check(as.ModeForPath("/tmp"), Equals, os.FileMode(0755))
c.Check(as.ModeForPath("/var/lib/snapd/hostfs/tmp"), Equals, os.FileMode(0755))
c.Check(as.ModeForPath("/var/lib/snapd/hostfs/tmp/snap.x11-server"), Equals, os.FileMode(0700))
c.Check(as.ModeForPath("/var/lib/snapd/hostfs/tmp/snap.x11-server/tmp"), Equals, os.FileMode(0777)|os.ModeSticky)
c.Check(as.ModeForPath("/var/lib/snapd/hostfs/tmp/snap.x11-server/foo"), Equals, os.FileMode(0755))
c.Check(as.ModeForPath("/var/lib/snapd/hostfs/tmp/snap.x11-server/tmp/.X11-unix"), Equals, os.FileMode(0777)|os.ModeSticky)
c.Check(as.ModeForPath("/var/lib/snapd/hostfs/tmp/snap-private-tmp/snap.x11-server"), Equals, os.FileMode(0700))
c.Check(as.ModeForPath("/var/lib/snapd/hostfs/tmp/snap-private-tmp/snap.x11-server/tmp"), Equals, os.FileMode(0777)|os.ModeSticky)
c.Check(as.ModeForPath("/var/lib/snapd/hostfs/tmp/snap-private-tmp/snap.x11-server/foo"), Equals, os.FileMode(0755))
c.Check(as.ModeForPath("/var/lib/snapd/hostfs/tmp/snap-private-tmp/snap.x11-server/tmp/.X11-unix"), Equals, os.FileMode(0777)|os.ModeSticky)
c.Check(as.ModeForPath("/dev/shm/snap.some-snap"), Equals, os.FileMode(0777)|os.ModeSticky)

// Instances can, in addition, access /snap/$SNAP_INSTANCE_NAME
Expand Down
6 changes: 3 additions & 3 deletions interfaces/builtin/x11.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ func (iface *x11Interface) MountConnectedPlug(spec *mount.Specification, plug *i
}
slotSnapName := slot.Snap().InstanceName()
return spec.AddMountEntry(osutil.MountEntry{
Name: fmt.Sprintf("/var/lib/snapd/hostfs/tmp/snap.%s/tmp/.X11-unix", slotSnapName),
Name: fmt.Sprintf("/var/lib/snapd/hostfs/tmp/snap-private-tmp/snap.%s/tmp/.X11-unix", slotSnapName),
Dir: "/tmp/.X11-unix",
Options: []string{"bind", "ro"},
})
Expand All @@ -220,8 +220,8 @@ func (iface *x11Interface) AppArmorConnectedPlug(spec *apparmor.Specification, p
slotSnapName := slot.Snap().InstanceName()
spec.AddUpdateNS(fmt.Sprintf(`
/tmp/.X11-unix/ rw,
/var/lib/snapd/hostfs/tmp/snap.%s/tmp/.X11-unix/ rw,
mount options=(rw, bind) /var/lib/snapd/hostfs/tmp/snap.%s/tmp/.X11-unix/ -> /tmp/.X11-unix/,
/var/lib/snapd/hostfs/tmp/snap-private-tmp/snap.%s/tmp/.X11-unix/ rw,
mount options=(rw, bind) /var/lib/snapd/hostfs/tmp/snap-private-tmp/snap.%s/tmp/.X11-unix/ -> /tmp/.X11-unix/,
mount options=(ro, remount, bind) -> /tmp/.X11-unix/,
mount options=(rslave) -> /tmp/.X11-unix/,
umount /tmp/.X11-unix/,
Expand Down
4 changes: 2 additions & 2 deletions interfaces/builtin/x11_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func (s *X11InterfaceSuite) TestMountSpec(c *C) {
spec = &mount.Specification{}
c.Assert(spec.AddConnectedPlug(s.iface, s.plug, s.coreSlot), IsNil)
c.Assert(spec.MountEntries(), DeepEquals, []osutil.MountEntry{{
Name: "/var/lib/snapd/hostfs/tmp/snap.x11/tmp/.X11-unix",
Name: "/var/lib/snapd/hostfs/tmp/snap-private-tmp/snap.x11/tmp/.X11-unix",
Dir: "/tmp/.X11-unix",
Options: []string{"bind", "ro"},
}})
Expand Down Expand Up @@ -155,7 +155,7 @@ func (s *X11InterfaceSuite) TestAppArmorSpec(c *C) {
c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "fontconfig")
c.Assert(spec.UpdateNS(), HasLen, 1)
c.Assert(spec.UpdateNS()[0], testutil.Contains, `mount options=(rw, bind) /var/lib/snapd/hostfs/tmp/snap.x11/tmp/.X11-unix/ -> /tmp/.X11-unix/,`)
c.Assert(spec.UpdateNS()[0], testutil.Contains, `mount options=(rw, bind) /var/lib/snapd/hostfs/tmp/snap-private-tmp/snap.x11/tmp/.X11-unix/ -> /tmp/.X11-unix/,`)

// Slot side connection permissions
spec = &apparmor.Specification{}
Expand Down
4 changes: 2 additions & 2 deletions tests/lib/reset.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ reset_classic() {
ls -lR "$SNAP_MOUNT_DIR"/ /var/snap/
exit 1
fi
rm -rf /tmp/snap.*
rm -rf /tmp/snap-private-tmp

case "$SPREAD_SYSTEM" in
fedora-*|centos-*)
Expand Down Expand Up @@ -144,7 +144,7 @@ reset_all_snap() {
systemctl stop snapd.service snapd.socket
restore_snapd_state
rm -rf /root/.snap
rm -rf /tmp/snap.*
rm -rf /tmp/snap-private-tmp/snap.*
if [ "$1" != "--keep-stopped" ]; then
systemctl start snapd.service snapd.socket
fi
Expand Down
Loading

0 comments on commit 21ebc51

Please sign in to comment.