@@ -86,20 +86,8 @@ static void setup_private_tmp(const char *snap_instance) {
8686 int base_dir_fd SC_CLEANUP (sc_cleanup_close ) = -1 ;
8787 int tmp_dir_fd SC_CLEANUP (sc_cleanup_close ) = -1 ;
8888
89- // /tmp/snap-private-tmp should have already been created by
90- // systemd-tmpfiles but we can try create it anyway since snapd may have
91- // just been installed in which case the tmpfiles conf would not have got
92- // executed yet. Internally sc_ensure_mkdir() first creates the directory
93- // with 0000 permissions and then updates its ownership and mode to 0700.
94- // Because of this, in case the directory was not present, there is a
95- // possibility for a race with another instance of snap-confine running in
96- // parallel, where one may have only created the directory with 0000
97- // permissions or owned by the user, while another observes the directory
98- // and proceeds with execution up to a point where it asserts mode bits or
99- // ownership.
100- if (sc_ensure_mkdir (SNAP_PRIVATE_TMP_ROOT_DIR , 0700 , 0 , 0 ) != 0 ) {
101- die ("cannot create " SNAP_PRIVATE_TMP_ROOT_DIR );
102- }
89+ // Open the /tmp/snap-private-tmp directory (created by packaging scripts,
90+ // systemd-tmpfiles, or snapd).
10391 private_tmp_root_fd = open (SNAP_PRIVATE_TMP_ROOT_DIR , O_RDONLY | O_DIRECTORY | O_CLOEXEC | O_NOFOLLOW );
10492 if (private_tmp_root_fd < 0 ) {
10593 die ("cannot open %s" , SNAP_PRIVATE_TMP_ROOT_DIR );
@@ -108,35 +96,13 @@ static void setup_private_tmp(const char *snap_instance) {
10896 if (fstat (private_tmp_root_fd , & st ) < 0 ) {
10997 die ("cannot stat %s" , SNAP_PRIVATE_TMP_ROOT_DIR );
11098 }
111- // In case we were racing with another snap-confine instance (as described
112- // earlier), the permissions or ownership we observe here may be a snapshot
113- // of an intermediate state of the directory (before its mode was updated to
114- // 0700, or still owned by the user). Test each property and amend if
115- // possible. Even if racing, another instance of snap-confine tries to reach
116- // the same state. We still have CAP_DAC_OVERRIDE, CAP_FOWNER, so we can
117- // attempt to fix the state.
11899 if (!S_ISDIR (st .st_mode )) {
119- // This we cannot fix
120100 die ("%s has unexpected type" , SNAP_PRIVATE_TMP_ROOT_DIR );
121101 }
122- // We have already verified type, check ownership.
123- if (st .st_uid != 0 || st .st_gid != 0 ) {
124- // May have hit a race, let's fix the ownership.
125- if (fchown (private_tmp_root_fd , 0 , 0 ) != 0 ) {
126- die ("%s has unexpected ownership %d:%d which could not be fixed" , SNAP_PRIVATE_TMP_ROOT_DIR , st .st_uid ,
127- st .st_gid );
128- }
129- }
130- // We have already verified the type and ownership, check mode.
131- if ((st .st_mode & ~S_IFMT ) != 0700 ) {
132- // May have hit a race, let's fix the mode.
133- if (fchmod (private_tmp_root_fd , 0700 ) != 0 ) {
134- die ("%s has unexpected mode 0%o which could not be fixed" , SNAP_PRIVATE_TMP_ROOT_DIR , st .st_mode & ~S_IFMT );
135- }
102+ if (st .st_uid != 0 || st .st_gid != 0 || st .st_mode != (S_IFDIR | 0700 )) {
103+ die ("%s has unexpected ownership / permissions" , SNAP_PRIVATE_TMP_ROOT_DIR );
136104 }
137105 // Create /tmp/snap-private-tmp/snap.$SNAP_INSTANCE_NAME/ 0700 root:root.
138- // Note that the snap is locked at this point, so a race such as when
139- // creating /tmp/snap-private-tmp would not occur.
140106 sc_must_snprintf (base , sizeof (base ), "snap.%s" , snap_instance );
141107 if (sc_ensure_mkdirat (private_tmp_root_fd , base , 0700 , 0 , 0 ) != 0 ) {
142108 die ("cannot create base directory: %s" , base );
@@ -448,7 +414,9 @@ static void sc_replicate_base_rootfs(const char *scratch_dir, const char *rootfs
448414 // Create an empty file which can be used as a mount point, no need
449415 // for 0000, parent directory already owned and writable by root
450416 // only.
451- int fd = open (full_path , O_CREAT | O_TRUNC , 0644 );
417+ // Use O_NOFOLLOW to prevent symlink attacks during the TOCTOU window
418+ // between directory creation and chown.
419+ int fd = open (full_path , O_CREAT | O_TRUNC | O_NOFOLLOW , 0644 );
452420 if (fd < 0 ) {
453421 die ("cannot create mount point for file \"%s\"" , full_path );
454422 }
@@ -506,7 +474,7 @@ static void sc_replicate_base_rootfs(const char *scratch_dir, const char *rootfs
506474 * snaps) or remove them, through the unmount system call.
507475 **/
508476static void sc_bootstrap_mount_namespace (const struct sc_mount_config * config ) {
509- char scratch_dir [] = "/tmp/snap.rootfs_XXXXXX" ;
477+ char scratch_dir [] = "/tmp/snap-private-tmp/snap .rootfs_XXXXXX" ;
510478 char src [PATH_MAX ] = {0 };
511479 char dst [PATH_MAX ] = {0 };
512480 if (mkdtemp (scratch_dir ) == NULL ) {
@@ -765,7 +733,7 @@ static void sc_bootstrap_mount_namespace(const struct sc_mount_config *config) {
765733 // all-snap system where this would-be chroot didn't happen and all the
766734 // rules see / as the root file system _OR_ we are running on top of a
767735 // classic distribution and this chroot has now moved all paths to
768- // /tmp/snap.rootfs_*.
736+ // /tmp/snap-private-tmp/snap .rootfs_*.
769737 //
770738 // Because we are using unshare(2) with CLONE_NEWNS we can essentially use
771739 // pivot_root just like chroot but this makes apparmor unaware of the old
0 commit comments