From 0ad36056e3fe9baec2c7d69444f1eaf1648490a5 Mon Sep 17 00:00:00 2001
From: smitsohu <smitsohu@gmail.com>
Date: Tue, 8 Jun 2021 12:27:04 +0200
Subject: refactor mounting

---
 src/firejail/chroot.c         |  40 +++--------
 src/firejail/dbus.c           |  14 +---
 src/firejail/firejail.h       |   4 ++
 src/firejail/fs.c             | 163 ++++++++++++++++++++----------------------
 src/firejail/fs_home.c        |  21 ++----
 src/firejail/fs_lib.c         |   6 +-
 src/firejail/fs_trace.c       |   6 +-
 src/firejail/fs_whitelist.c   |  18 +----
 src/firejail/pulseaudio.c     |   6 +-
 src/firejail/restrict_users.c |  12 +---
 src/firejail/util.c           | 138 ++++++++++++++++++++++++++---------
 src/firejail/x11.c            |  25 ++-----
 12 files changed, 218 insertions(+), 235 deletions(-)

diff --git a/src/firejail/chroot.c b/src/firejail/chroot.c
index 757ffb1f7..4125a4130 100644
--- a/src/firejail/chroot.c
+++ b/src/firejail/chroot.c
@@ -163,12 +163,8 @@ void fs_chroot(const char *rootdir) {
 	int fd = openat(parentfd, "dev", O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
 	if (fd == -1)
 		errExit("open");
-	char *proc;
-	if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
-		errExit("asprintf");
-	if (mount("/dev", proc, NULL, MS_BIND|MS_REC, NULL) < 0)
+	if (bind_mount_path_to_fd("/dev", fd))
 		errExit("mounting /dev");
-	free(proc);
 	close(fd);
 
 #ifdef HAVE_X11
@@ -192,11 +188,8 @@ void fs_chroot(const char *rootdir) {
 		fd = openat(parentfd, "tmp/.X11-unix", O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
 		if (fd == -1)
 			errExit("open");
-		if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
-			errExit("asprintf");
-		if (mount("/tmp/.X11-unix", proc, NULL, MS_BIND|MS_REC, NULL) < 0)
+		if (bind_mount_path_to_fd("/tmp/.X11-unix", fd))
 			errExit("mounting /tmp/.X11-unix");
-		free(proc);
 		close(fd);
 	}
 #endif // HAVE_X11
@@ -225,19 +218,11 @@ void fs_chroot(const char *rootdir) {
 			fprintf(stderr, "Error: cannot open %s\n", pulse);
 			exit(1);
 		}
-		free(pulse);
-
-		char *proc_src, *proc_dst;
-		if (asprintf(&proc_src, "/proc/self/fd/%d", src) == -1)
-			errExit("asprintf");
-		if (asprintf(&proc_dst, "/proc/self/fd/%d", dst) == -1)
-			errExit("asprintf");
-		if (mount(proc_src, proc_dst, NULL, MS_BIND|MS_REC, NULL) < 0)
-			errExit("mount bind");
-		free(proc_src);
-		free(proc_dst);
+		if (bind_mount_by_fd(src, dst))
+			errExit("mounting pulseaudio")
 		close(src);
 		close(dst);
+		free(pulse);
 
 		// update /etc/machine-id in chroot
 		update_file(parentfd, "etc/machine-id");
@@ -256,11 +241,8 @@ void fs_chroot(const char *rootdir) {
 	fd = openat(parentfd, &RUN_FIREJAIL_LIB_DIR[1], O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
 	if (fd == -1)
 		errExit("open");
-	if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
-		errExit("asprintf");
-	if (mount(RUN_FIREJAIL_LIB_DIR, proc, NULL, MS_BIND|MS_REC, NULL) < 0)
+	if (bind_mount_path_to_fd(RUN_FIREJAIL_LIB_DIR, fd))
 		errExit("mount bind");
-	free(proc);
 	close(fd);
 
 	// create /run/firejail/mnt directory in chroot
@@ -271,11 +253,8 @@ void fs_chroot(const char *rootdir) {
 	fd = openat(parentfd, &RUN_MNT_DIR[1], O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
 	if (fd == -1)
 		errExit("open");
-	if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
-		errExit("asprintf");
-	if (mount(RUN_MNT_DIR, proc, NULL, MS_BIND|MS_REC, NULL) < 0)
+	if (bind_mount_path_to_fd(RUN_MNT_DIR, fd))
 		errExit("mount bind");
-	free(proc);
 	close(fd);
 
 	// update chroot resolv.conf
@@ -289,11 +268,8 @@ void fs_chroot(const char *rootdir) {
 	if (mkdir(oroot, 0755) == -1)
 		errExit("mkdir");
 	// mount the chroot dir on top of /run/firejail/mnt/oroot in order to reuse the apparmor rules for overlay
-	if (asprintf(&proc, "/proc/self/fd/%d", parentfd) == -1)
-		errExit("asprintf");
-	if (mount(proc, oroot, NULL, MS_BIND|MS_REC, NULL) < 0)
+	if (bind_mount_fd_to_path(parentfd, oroot))
 		errExit("mounting rootdir oroot");
-	free(proc);
 	close(parentfd);
 	// chroot into the new directory
 	if (arg_debug)
diff --git a/src/firejail/dbus.c b/src/firejail/dbus.c
index b8aa2c974..bfa28fcba 100644
--- a/src/firejail/dbus.c
+++ b/src/firejail/dbus.c
@@ -258,12 +258,8 @@ static char *find_user_socket_by_format(char *format) {
 	if (asprintf(&dbus_user_socket, format, (int) getuid()) == -1)
 		errExit("asprintf");
 	struct stat s;
-	if (stat(dbus_user_socket, &s) == -1) {
-		if (errno == ENOENT)
-			goto fail;
-		return NULL;
-		errExit("stat");
-	}
+	if (lstat(dbus_user_socket, &s) == -1)
+		goto fail;
 	if (!S_ISSOCK(s.st_mode))
 		goto fail;
 	return dbus_user_socket;
@@ -426,12 +422,8 @@ static void socket_overlay(char *socket_path, char *proxy_path) {
 		errno = ENOTSOCK;
 		errExit("mounting DBus proxy socket");
 	}
-	char *proxy_fd_path;
-	if (asprintf(&proxy_fd_path, "/proc/self/fd/%d", fd) == -1)
-		errExit("asprintf");
-	if (mount(proxy_path, socket_path, NULL, MS_BIND | MS_REC, NULL) == -1)
+	if (bind_mount_fd_to_path(fd, socket_path))
 		errExit("mount bind");
-	free(proxy_fd_path);
 	close(fd);
 }
 
diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h
index a5c44739e..ab6e44a8a 100644
--- a/src/firejail/firejail.h
+++ b/src/firejail/firejail.h
@@ -526,6 +526,10 @@ unsigned extract_timeout(const char *str);
 void disable_file_or_dir(const char *fname);
 void disable_file_path(const char *path, const char *file);
 int safer_openat(int dirfd, const char *path, int flags);
+int remount_by_fd(int dst, unsigned long mountflags);
+int bind_mount_by_fd(int src, int dst);
+int bind_mount_path_to_fd(const char *srcname, int dst);
+int bind_mount_fd_to_path(int src, const char *destname);
 int has_handler(pid_t pid, int signal);
 void enter_network_namespace(pid_t pid);
 int read_pid(const char *name, pid_t *pid);
diff --git a/src/firejail/fs.c b/src/firejail/fs.c
index 09de11de9..eaf23e603 100644
--- a/src/firejail/fs.c
+++ b/src/firejail/fs.c
@@ -54,16 +54,9 @@ static char *opstr[] = {
 	[MOUNT_RDWR_NOCHECK] = "read-write",
 };
 
-typedef enum {
-	UNSUCCESSFUL,
-	SUCCESSFUL
-} LAST_DISABLE_OPERATION;
-LAST_DISABLE_OPERATION last_disable = UNSUCCESSFUL;
-
 static void disable_file(OPERATION op, const char *filename) {
 	assert(filename);
 	assert(op <OPERATION_MAX);
-	last_disable = UNSUCCESSFUL;
 
 	// Resolve all symlinks
 	char* fname = realpath(filename, NULL);
@@ -71,20 +64,22 @@ static void disable_file(OPERATION op, const char *filename) {
 		return;
 	}
 	if (fname == NULL && errno == EACCES) {
-		if (arg_debug)
-			printf("Debug: no access to file %s, forcing mount\n", filename);
 		// realpath and stat functions will fail on FUSE filesystems
 		// they don't seem to like a uid of 0
 		// force mounting
-		int rv = mount(RUN_RO_DIR, filename, "none", MS_BIND, "mode=400,gid=0");
-		if (rv == 0)
-			last_disable = SUCCESSFUL;
-		else {
-			rv = mount(RUN_RO_FILE, filename, "none", MS_BIND, "mode=400,gid=0");
-			if (rv == 0)
-				last_disable = SUCCESSFUL;
+		int fd = open(filename, O_PATH|O_CLOEXEC);
+		if (fd < 0) {
+			if (arg_debug)
+				printf("Warning (blacklisting): cannot open %s: %s\n", filename, strerror(errno));
+			return;
 		}
-		if (last_disable == SUCCESSFUL) {
+
+		int err = bind_mount_path_to_fd(RUN_RO_DIR, fd);
+		if (err < 0)
+			err = bind_mount_path_to_fd(RUN_RO_FILE, fd);
+		close(fd);
+
+		if (err == 0) {
 			if (arg_debug)
 				printf("Disable %s\n", filename);
 			if (op == BLACKLIST_FILE)
@@ -92,21 +87,18 @@ static void disable_file(OPERATION op, const char *filename) {
 			else
 				fs_logger2("blacklist-nolog", filename);
 		}
-		else {
-			if (arg_debug)
-				printf("Warning (blacklisting): %s is an invalid file, skipping...\n", filename);
-		}
+		else if (arg_debug)
+			printf("Warning (blacklisting): cannot mount on %s\n", filename);
 
 		return;
 	}
 
 	// if the file is not present, do nothing
+	assert(fname);
 	struct stat s;
-	if (fname == NULL)
-		return;
-	if (stat(fname, &s) == -1) {
+	if (stat(fname, &s) < 0) {
 		if (arg_debug)
-			fwarning("%s does not exist, skipping...\n", fname);
+			printf("Warning (blacklisting): cannot access %s: %s\n", fname, strerror(errno));
 		free(fname);
 		return;
 	}
@@ -115,8 +107,10 @@ static void disable_file(OPERATION op, const char *filename) {
 	// we migth have a file found in ${PATH} pointing to /usr/bin/firejail
 	// blacklisting it here will end up breaking situations like user clicks on a link in Thunderbird
 	//     and expects Firefox to open in the same sandbox
-	if (strcmp(BINDIR "/firejail", fname) == 0)
+	if (strcmp(BINDIR "/firejail", fname) == 0) {
+		free(fname);
 		return;
+	}
 
 	// modify the file
 	if (op == BLACKLIST_FILE || op == BLACKLIST_NOLOG) {
@@ -141,15 +135,23 @@ static void disable_file(OPERATION op, const char *filename) {
 					printf(" - no logging\n");
 			}
 
+			int fd = open(fname, O_PATH|O_CLOEXEC);
+			if (fd < 0) {
+				if (arg_debug)
+					printf("Warning (blacklisting): cannot open %s: %s\n", fname, strerror(errno));
+				free(fname);
+				return;
+			}
 			if (S_ISDIR(s.st_mode)) {
-				if (mount(RUN_RO_DIR, fname, "none", MS_BIND, "mode=400,gid=0") < 0)
+				if (bind_mount_path_to_fd(RUN_RO_DIR, fd) < 0)
 					errExit("disable file");
 			}
 			else {
-				if (mount(RUN_RO_FILE, fname, "none", MS_BIND, "mode=400,gid=0") < 0)
+				if (bind_mount_path_to_fd(RUN_RO_FILE, fd) < 0)
 					errExit("disable file");
 			}
-			last_disable = SUCCESSFUL;
+			close(fd);
+
 			if (op == BLACKLIST_FILE)
 				fs_logger2("blacklist", fname);
 			else
@@ -158,7 +160,6 @@ static void disable_file(OPERATION op, const char *filename) {
 	}
 	else if (op == MOUNT_READONLY || op == MOUNT_RDWR || op == MOUNT_NOEXEC) {
 		fs_remount_rec(fname, op);
-		// todo: last_disable = SUCCESSFUL;
 	}
 	else if (op == MOUNT_TMPFS) {
 		if (S_ISDIR(s.st_mode)) {
@@ -171,7 +172,6 @@ static void disable_file(OPERATION op, const char *filename) {
 			}
 			fs_tmpfs(fname, getuid());
 			selinux_relabel_path(fname, fname);
-			last_disable = SUCCESSFUL;
 		}
 		else
 			fwarning("%s is not a directory; cannot mount a tmpfs on top of it.\n", fname);
@@ -493,35 +493,38 @@ static void fs_remount_simple(const char *path, OPERATION op) {
 	assert(path);
 
 	// open path without following symbolic links
-	int fd1 = safer_openat(-1, path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
-	if (fd1 == -1)
+	int fd = safer_openat(-1, path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+	if (fd < 0)
 		goto out;
-	struct stat s1;
-	if (fstat(fd1, &s1) == -1) {
+
+	struct stat s;
+	if (fstat(fd, &s) < 0) {
 		// fstat can fail with EACCES if path is a FUSE mount,
 		// mounted without 'allow_root' or 'allow_other'
 		if (errno != EACCES)
 			errExit("fstat");
-		close(fd1);
+		close(fd);
 		goto out;
 	}
 	// get mount flags
 	struct statvfs buf;
-	if (fstatvfs(fd1, &buf) == -1)
-		errExit("fstatvfs");
+	if (fstatvfs(fd, &buf) < 0) {
+		close(fd);
+		goto out;
+	}
 	unsigned long flags = buf.f_flag;
 
 	// read-write option
 	if (op == MOUNT_RDWR || op == MOUNT_RDWR_NOCHECK) {
 		// nothing to do if there is no read-only flag
 		if ((flags & MS_RDONLY) == 0) {
-			close(fd1);
+			close(fd);
 			return;
 		}
 		// allow only user owned directories, except the user is root
-		if (op != MOUNT_RDWR_NOCHECK && getuid() != 0 && s1.st_uid != getuid()) {
+		if (op != MOUNT_RDWR_NOCHECK && getuid() != 0 && s.st_uid != getuid()) {
 			fwarning("you are not allowed to change %s to read-write\n", path);
-			close(fd1);
+			close(fd);
 			return;
 		}
 		flags &= ~MS_RDONLY;
@@ -530,7 +533,7 @@ static void fs_remount_simple(const char *path, OPERATION op) {
 	else if (op == MOUNT_NOEXEC) {
 		// nothing to do if path is mounted noexec already
 		if ((flags & (MS_NOEXEC|MS_NODEV|MS_NOSUID)) == (MS_NOEXEC|MS_NODEV|MS_NOSUID)) {
-			close(fd1);
+			close(fd);
 			return;
 		}
 		flags |= MS_NOEXEC|MS_NODEV|MS_NOSUID;
@@ -539,7 +542,7 @@ static void fs_remount_simple(const char *path, OPERATION op) {
 	else if (op == MOUNT_READONLY) {
 		// nothing to do if path is mounted read-only already
 		if ((flags & MS_RDONLY) == MS_RDONLY) {
-			close(fd1);
+			close(fd);
 			return;
 		}
 		flags |= MS_RDONLY;
@@ -549,29 +552,33 @@ static void fs_remount_simple(const char *path, OPERATION op) {
 
 	if (arg_debug)
 		printf("Mounting %s %s\n", opstr[op], path);
+
+	// make path a mount point:
 	// mount --bind path path
-	char *proc;
-	if (asprintf(&proc, "/proc/self/fd/%d", fd1) == -1)
-		errExit("asprintf");
-	if (mount(proc, proc, NULL, MS_BIND|MS_REC, NULL) < 0)
-		errExit("mount");
-	free(proc);
+	int err = bind_mount_by_fd(fd, fd);
+	if (err) {
+		close(fd);
+		goto out;
+	}
 
-	// mount --bind -o remount,ro path
-	// need to open path again without following symbolic links
+	// remount the mount point
+	// need to open path again
 	int fd2 = safer_openat(-1, path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
-	if (fd2 == -1)
-		errExit("open");
+	close(fd); // earliest timepoint to close fd
+	if (fd2 < 0)
+		goto out;
+
+	// device and inode number should be the same
 	struct stat s2;
-	if (fstat(fd2, &s2) == -1)
+	if (fstat(fd2, &s2) < 0)
 		errExit("fstat");
-	// device and inode number should be the same
-	if (s1.st_dev != s2.st_dev || s1.st_ino != s2.st_ino)
+	if (s.st_dev != s2.st_dev || s.st_ino != s2.st_ino)
 		errLogExit("invalid %s mount", opstr[op]);
-	if (asprintf(&proc, "/proc/self/fd/%d", fd2) == -1)
-		errExit("asprintf");
-	if (mount(NULL, proc, NULL, flags|MS_BIND|MS_REMOUNT, NULL) < 0)
-		errExit("mount");
+
+	err = remount_by_fd(fd2, flags);
+	close(fd2);
+	if (err)
+		goto out;
 
 	// run a sanity check on /proc/self/mountinfo and confirm that target of the last
 	// mount operation was path; if there are other mount points contained inside path,
@@ -582,10 +589,8 @@ static void fs_remount_simple(const char *path, OPERATION op) {
 	   (*(mptr->dir + len) != '\0' && *(mptr->dir + len) != '/'))
 	   && strcmp(path, "/") != 0) // support read-only=/
 		errLogExit("invalid %s mount", opstr[op]);
+
 	fs_logger2(opstr[op], path);
-	free(proc);
-	close(fd1);
-	close(fd2);
 	return;
 
 out:
@@ -743,8 +748,6 @@ void fs_proc_sys_dev_boot(void) {
 
 	// disable various ipc sockets in /run/user
 	if (!arg_writable_run_user) {
-		struct stat s;
-
 		char *fname;
 		if (asprintf(&fname, "/run/user/%d", getuid()) == -1)
 			errExit("asprintf");
@@ -755,8 +758,7 @@ void fs_proc_sys_dev_boot(void) {
 				errExit("asprintf");
 			if (create_empty_dir_as_user(fnamegpg, 0700))
 				fs_logger2("create", fnamegpg);
-			if (stat(fnamegpg, &s) == 0)
-				disable_file(BLACKLIST_FILE, fnamegpg);
+			disable_file(BLACKLIST_FILE, fnamegpg);
 			free(fnamegpg);
 
 			// disable /run/user/{uid}/systemd
@@ -765,8 +767,7 @@ void fs_proc_sys_dev_boot(void) {
 				errExit("asprintf");
 			if (create_empty_dir_as_user(fnamesysd, 0755))
 				fs_logger2("create", fnamesysd);
-			if (stat(fnamesysd, &s) == 0)
-				disable_file(BLACKLIST_FILE, fnamesysd);
+			disable_file(BLACKLIST_FILE, fnamesysd);
 			free(fnamesysd);
 		}
 		free(fname);
@@ -781,26 +782,18 @@ void fs_proc_sys_dev_boot(void) {
 
 // disable firejail configuration in ~/.config/firejail
 void disable_config(void) {
-	struct stat s;
-
 	char *fname;
 	if (asprintf(&fname, "%s/.config/firejail", cfg.homedir) == -1)
 		errExit("asprintf");
-	if (stat(fname, &s) == 0)
-		disable_file(BLACKLIST_FILE, fname);
+	disable_file(BLACKLIST_FILE, fname);
 	free(fname);
 
 	// disable run time information
-	if (stat(RUN_FIREJAIL_NETWORK_DIR, &s) == 0)
-		disable_file(BLACKLIST_FILE, RUN_FIREJAIL_NETWORK_DIR);
-	if (stat(RUN_FIREJAIL_BANDWIDTH_DIR, &s) == 0)
-		disable_file(BLACKLIST_FILE, RUN_FIREJAIL_BANDWIDTH_DIR);
-	if (stat(RUN_FIREJAIL_NAME_DIR, &s) == 0)
-		disable_file(BLACKLIST_FILE, RUN_FIREJAIL_NAME_DIR);
-	if (stat(RUN_FIREJAIL_PROFILE_DIR, &s) == 0)
-		disable_file(BLACKLIST_FILE, RUN_FIREJAIL_PROFILE_DIR);
-	if (stat(RUN_FIREJAIL_X11_DIR, &s) == 0)
-		disable_file(BLACKLIST_FILE, RUN_FIREJAIL_X11_DIR);
+	disable_file(BLACKLIST_FILE, RUN_FIREJAIL_NETWORK_DIR);
+	disable_file(BLACKLIST_FILE, RUN_FIREJAIL_BANDWIDTH_DIR);
+	disable_file(BLACKLIST_FILE, RUN_FIREJAIL_NAME_DIR);
+	disable_file(BLACKLIST_FILE, RUN_FIREJAIL_PROFILE_DIR);
+	disable_file(BLACKLIST_FILE, RUN_FIREJAIL_X11_DIR);
 }
 
 
@@ -1241,8 +1234,8 @@ void fs_private_tmp(void) {
 
 	// whitelist x11 directory
 	profile_add("whitelist /tmp/.X11-unix");
-        // read-only x11 directory
-        profile_add("read-only /tmp/.X11-unix");
+	// read-only x11 directory
+	profile_add("read-only /tmp/.X11-unix");
 
 	// whitelist any pulse* file in /tmp directory
 	// some distros use PulseAudio sockets under /tmp instead of the socket in /urn/user
diff --git a/src/firejail/fs_home.c b/src/firejail/fs_home.c
index f61d43c29..ec6dab947 100644
--- a/src/firejail/fs_home.c
+++ b/src/firejail/fs_home.c
@@ -287,17 +287,9 @@ void fs_private_homedir(void) {
 		exit(1);
 	}
 	// mount via the links in /proc/self/fd
-	char *proc_src, *proc_dst;
-	if (asprintf(&proc_src, "/proc/self/fd/%d", src) == -1)
-		errExit("asprintf");
-	if (asprintf(&proc_dst, "/proc/self/fd/%d", dst) == -1)
-		errExit("asprintf");
-	if (mount(proc_src, proc_dst, NULL, MS_NOSUID | MS_NODEV | MS_BIND | MS_REC, NULL) < 0)
+	if (bind_mount_by_fd(src, dst))
 		errExit("mount bind");
-	free(proc_src);
-	free(proc_dst);
-	close(src);
-	close(dst);
+
 	// check /proc/self/mountinfo to confirm the mount is ok
 	MountData *mptr = get_last_mount();
 	size_t len = strlen(homedir);
@@ -305,6 +297,8 @@ void fs_private_homedir(void) {
 	   (*(mptr->dir + len) != '\0' && *(mptr->dir + len) != '/'))
 		errLogExit("invalid private mount");
 
+	close(src);
+	close(dst);
 	fs_logger3("mount-bind", private_homedir, homedir);
 	fs_logger2("whitelist", homedir);
 // preserve mode and ownership
@@ -590,13 +584,10 @@ void fs_private_home_list(void) {
 		exit(1);
 	}
 	// mount using the file descriptor
-	char *proc;
-	if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
-		errExit("asprintf");
-	if (mount(RUN_HOME_DIR, proc, NULL, MS_BIND|MS_REC, NULL) < 0)
+	if (bind_mount_path_to_fd(RUN_HOME_DIR, fd))
 		errExit("mount bind");
-	free(proc);
 	close(fd);
+
 	// check /proc/self/mountinfo to confirm the mount is ok
 	MountData *mptr = get_last_mount();
 	if (strcmp(mptr->dir, homedir) != 0 || strcmp(mptr->fstype, "tmpfs") != 0)
diff --git a/src/firejail/fs_lib.c b/src/firejail/fs_lib.c
index 5df356d04..8369e6259 100644
--- a/src/firejail/fs_lib.c
+++ b/src/firejail/fs_lib.c
@@ -203,7 +203,7 @@ void fslib_mount_libs(const char *full_path, unsigned user) {
 	}
 
 	if (arg_debug || arg_debug_private_lib)
-		printf("    fslib_mount_libs %s (parse as %s)\n", full_path, user ? "user" : "root");
+		printf("    fslib_mount_libs %s\n", full_path);
 	// create an empty RUN_LIB_FILE and allow the user to write to it
 	unlink(RUN_LIB_FILE);			  // in case is there
 	create_empty_file_as_root(RUN_LIB_FILE, 0644);
@@ -212,7 +212,7 @@ void fslib_mount_libs(const char *full_path, unsigned user) {
 
 	// run fldd to extract the list of files
 	if (arg_debug || arg_debug_private_lib)
-		printf("    running fldd %s\n", full_path);
+		printf("    running fldd %s as %s\n", full_path, user ? "user" : "root");
 	unsigned mask;
 	if (user)
 		mask = SBOX_USER;
@@ -298,7 +298,7 @@ static void install_list_entry(const char *lib) {
 //printf("glob %s\n", globbuf.gl_pathv[j]);
 			// GLOB_NOCHECK - no pattern matched returns the original pattern; try to load it anyway
 
-			// foobar/* includes foobar/. and foobar/..
+			// foobar/* expands to foobar/. and foobar/..
 			const char *base = gnu_basename(globbuf.gl_pathv[j]);
 			if (strcmp(base, ".") == 0 || strcmp(base, "..") == 0)
 				continue;
diff --git a/src/firejail/fs_trace.c b/src/firejail/fs_trace.c
index 1fc38361e..475a391ec 100644
--- a/src/firejail/fs_trace.c
+++ b/src/firejail/fs_trace.c
@@ -71,12 +71,8 @@ void fs_tracefile(void) {
 	// mount using the symbolic link in /proc/self/fd
 	if (arg_debug)
 		printf("Bind mount %s to %s\n", arg_tracefile, RUN_TRACE_FILE);
-	char *proc;
-	if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
-		errExit("asprintf");
-	if (mount(proc, RUN_TRACE_FILE, NULL, MS_BIND|MS_REC, NULL) < 0)
+	if (bind_mount_fd_to_path(fd, RUN_TRACE_FILE))
 		errExit("mount bind " RUN_TRACE_FILE);
-	free(proc);
 	close(fd);
 	// now that RUN_TRACE_FILE is user-writable, mount it noexec
 	fs_remount(RUN_TRACE_FILE, MOUNT_NOEXEC, 0);
diff --git a/src/firejail/fs_whitelist.c b/src/firejail/fs_whitelist.c
index 258f023f6..ec3fe4c5b 100644
--- a/src/firejail/fs_whitelist.c
+++ b/src/firejail/fs_whitelist.c
@@ -195,15 +195,7 @@ static void whitelist_file(int dirfd, const char *relpath, const char *path) {
 
 	if (arg_debug || arg_debug_whitelists)
 		printf("Whitelisting %s\n", path);
-
-	// in order to make this mount resilient against symlink attacks, use
-	// magic links in /proc/self/fd instead of mounting the paths directly
-	char *proc_src, *proc_dst;
-	if (asprintf(&proc_src, "/proc/self/fd/%d", fd) == -1)
-		errExit("asprintf");
-	if (asprintf(&proc_dst, "/proc/self/fd/%d", fd3) == -1)
-		errExit("asprintf");
-	if (mount(proc_src, proc_dst, NULL, MS_BIND | MS_REC, NULL) < 0)
+	if (bind_mount_by_fd(fd, fd3))
 		errExit("mount bind");
 	// check the last mount operation
 	MountData *mptr = get_last_mount(); // will do exit(1) if the mount cannot be found
@@ -221,8 +213,6 @@ static void whitelist_file(int dirfd, const char *relpath, const char *path) {
 	//  - there should be more than one '/' char in dest string
 	if (mptr->dir == strrchr(mptr->dir, '/'))
 		errLogExit("invalid whitelist mount");
-	free(proc_src);
-	free(proc_dst);
 	close(fd);
 	close(fd3);
 	fs_logger2("whitelist", path);
@@ -341,12 +331,8 @@ static void tmpfs_topdirs(const TopDir *topdirs) {
 			// restore /run/firejail directory
 			if (mkdir(RUN_FIREJAIL_DIR, 0755) == -1)
 				errExit("mkdir");
-			char *proc;
-			if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
-				errExit("asprintf");
-			if (mount(proc, RUN_FIREJAIL_DIR, NULL, MS_BIND | MS_REC, NULL) < 0)
+			if (bind_mount_fd_to_path(fd, RUN_FIREJAIL_DIR))
 				errExit("mount bind");
-			free(proc);
 			close(fd);
 			fs_logger2("whitelist", RUN_FIREJAIL_DIR);
 
diff --git a/src/firejail/pulseaudio.c b/src/firejail/pulseaudio.c
index 1b01a71c6..be0f5aea6 100644
--- a/src/firejail/pulseaudio.c
+++ b/src/firejail/pulseaudio.c
@@ -158,17 +158,13 @@ void pulseaudio_init(void) {
 	// mount via the link in /proc/self/fd
 	if (arg_debug)
 		printf("Mounting %s on %s\n", RUN_PULSE_DIR, homeusercfg);
-	char *proc;
-	if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
-		errExit("asprintf");
-	if (mount(RUN_PULSE_DIR, proc, "none", MS_BIND, NULL) < 0)
+	if (bind_mount_path_to_fd(RUN_PULSE_DIR, fd))
 		errExit("mount pulseaudio");
 	// check /proc/self/mountinfo to confirm the mount is ok
 	MountData *mptr = get_last_mount();
 	if (strcmp(mptr->dir, homeusercfg) != 0 || strcmp(mptr->fstype, "tmpfs") != 0)
 		errLogExit("invalid pulseaudio mount");
 	fs_logger2("tmpfs", homeusercfg);
-	free(proc);
 	close(fd);
 
 	char *p;
diff --git a/src/firejail/restrict_users.c b/src/firejail/restrict_users.c
index 892244b5f..6f17231a4 100644
--- a/src/firejail/restrict_users.c
+++ b/src/firejail/restrict_users.c
@@ -104,12 +104,8 @@ static void sanitize_home(void) {
 	selinux_relabel_path(cfg.homedir, cfg.homedir);
 
 	// bring back real user home directory
-	char *proc;
-	if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
-		errExit("asprintf");
-	if (mount(proc, cfg.homedir, NULL, MS_BIND|MS_REC, NULL) < 0)
+	if (bind_mount_fd_to_path(fd, cfg.homedir))
 		errExit("mount bind");
-	free(proc);
 	close(fd);
 
 	if (!arg_private)
@@ -154,12 +150,8 @@ static void sanitize_run(void) {
 	selinux_relabel_path(runuser, runuser);
 
 	// bring back real run/user/$UID directory
-	char *proc;
-	if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
-		errExit("asprintf");
-	if (mount(proc, runuser, NULL, MS_BIND|MS_REC, NULL) < 0)
+	if (bind_mount_fd_to_path(fd, runuser))
 		errExit("mount bind");
-	free(proc);
 	close(fd);
 
 	fs_logger2("whitelist", runuser);
diff --git a/src/firejail/util.c b/src/firejail/util.c
index dc9fe2449..44f1cbd02 100644
--- a/src/firejail/util.c
+++ b/src/firejail/util.c
@@ -394,11 +394,11 @@ void touch_file_as_user(const char *fname, mode_t mode) {
 		// drop privileges
 		drop_privs(0);
 
-		FILE *fp = fopen(fname, "wx");
-		if (fp) {
-			fprintf(fp, "\n");
-			SET_PERMS_STREAM(fp, -1, -1, mode);
-			fclose(fp);
+		int fd = open(fname, O_RDONLY|O_CREAT|O_EXCL|O_CLOEXEC, S_IRUSR | S_IWUSR);
+		if (fd > -1) {
+			int err = fchmod(fd, mode);
+			(void) err;
+			close(fd);
 		}
 		else
 			fwarning("cannot create %s\n", fname);
@@ -899,27 +899,29 @@ int remove_overlay_directory(void) {
 		errExit("asprintf");
 
 	if (lstat(path, &s) == 0) {
-		// deal with obvious problems such as symlinks and root ownership
-		if (!S_ISDIR(s.st_mode)) {
-			if (S_ISLNK(s.st_mode))
-				fprintf(stderr, "Error: %s is a symbolic link\n", path);
-			else
-				fprintf(stderr, "Error: %s is not a directory\n", path);
-			exit(1);
-		}
-		if (s.st_uid != getuid()) {
-			fprintf(stderr, "Error: %s is not owned by the current user\n", path);
-			exit(1);
-		}
-
 		pid_t child = fork();
 		if (child < 0)
 			errExit("fork");
 		if (child == 0) {
-			// open ~/.firejail, fails if there is any symlink
-			int fd = safer_openat(-1, path, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
-			if (fd == -1)
-				errExit("safer_openat");
+			// open ~/.firejail
+			int fd = safer_openat(-1, path, O_PATH|O_NOFOLLOW|O_CLOEXEC);
+			if (fd == -1) {
+				fprintf(stderr, "Error: cannot open %s\n", path);
+				_exit(1);
+			}
+			if (fstat(fd, &s) == -1)
+				errExit("fstat");
+			if (!S_ISDIR(s.st_mode)) {
+				if (S_ISLNK(s.st_mode))
+					fprintf(stderr, "Error: %s is a symbolic link\n", path);
+				else
+					fprintf(stderr, "Error: %s is not a directory\n", path);
+				_exit(1);
+			}
+			if (s.st_uid != getuid()) {
+				fprintf(stderr, "Error: %s is not owned by the current user\n", path);
+				_exit(1);
+			}
 			// chdir to ~/.firejail
 			if (fchdir(fd) == -1)
 				errExit("fchdir");
@@ -988,8 +990,8 @@ int create_empty_dir_as_user(const char *dir, mode_t mode) {
 			drop_privs(0);
 
 			if (mkdir(dir, mode) == 0) {
-				if (chmod(dir, mode) == -1)
-					{;} // do nothing
+				int err = chmod(dir, mode);
+				(void) err;
 			}
 			else if (arg_debug)
 				printf("Directory %s not created: %s\n", dir, strerror(errno));
@@ -1110,20 +1112,32 @@ unsigned extract_timeout(const char *str) {
 }
 
 void disable_file_or_dir(const char *fname) {
+	assert(fname);
+
+	int fd = open(fname, O_PATH|O_CLOEXEC);
+	if (fd < 0)
+		return;
+
 	struct stat s;
-	if (stat(fname, &s) != -1) {
-		if (arg_debug)
-			printf("blacklist %s\n", fname);
-		if (is_dir(fname)) {
-			if (mount(RUN_RO_DIR, fname, "none", MS_BIND, "mode=400,gid=0") < 0)
+	if (fstat(fd, &s) < 0) { // FUSE
+		if (errno != EACCES)
+			errExit("fstat");
+		close(fd);
+		return;
+	}
+
+	if (arg_debug)
+		printf("blacklist %s\n", fname);
+	if (S_ISDIR(s.st_mode)) {
+		if (bind_mount_path_to_fd(RUN_RO_DIR, fd) < 0)
 				errExit("disable directory");
-		}
-		else {
-			if (mount(RUN_RO_FILE, fname, "none", MS_BIND, "mode=400,gid=0") < 0)
-				errExit("disable file");
-		}
-		fs_logger2("blacklist", fname);
 	}
+	else {
+		if (bind_mount_path_to_fd(RUN_RO_FILE, fd) < 0)
+			errExit("disable file");
+	}
+	close(fd);
+	fs_logger2("blacklist", fname);
 }
 
 void disable_file_path(const char *path, const char *file) {
@@ -1210,6 +1224,60 @@ int safer_openat(int dirfd, const char *path, int flags) {
 	return fd;
 }
 
+int remount_by_fd(int dst, unsigned long mountflags) {
+	char *proc;
+	if (asprintf(&proc, "/proc/self/fd/%d", dst) < 0)
+		errExit("asprintf");
+
+	int rv = mount(NULL, proc, NULL, mountflags|MS_BIND|MS_REMOUNT, NULL);
+	if (rv < 0 && arg_debug)
+		printf("Failed mount: %s\n", strerror(errno));
+
+	free(proc);
+	return rv;
+}
+
+int bind_mount_by_fd(int src, int dst) {
+	char *proc_src, *proc_dst;
+	if (asprintf(&proc_src, "/proc/self/fd/%d", src) < 0 ||
+	    asprintf(&proc_dst, "/proc/self/fd/%d", dst) < 0)
+		errExit("asprintf");
+
+	int rv = mount(proc_src, proc_dst, NULL, MS_BIND|MS_REC, NULL);
+	if (rv < 0 && arg_debug)
+		printf("Failed mount: %s\n", strerror(errno));
+
+	free(proc_src);
+	free(proc_dst);
+	return rv;
+}
+
+int bind_mount_fd_to_path(int src, const char *destname) {
+	char *proc;
+	if (asprintf(&proc, "/proc/self/fd/%d", src) < 0)
+		errExit("asprintf");
+
+	int rv = mount(proc, destname, NULL, MS_BIND|MS_REC, NULL);
+	if (rv < 0 && arg_debug)
+		printf("Failed mount: %s\n", strerror(errno));
+
+	free(proc);
+	return rv;
+}
+
+int bind_mount_path_to_fd(const char *srcname, int dst) {
+	char *proc;
+	if (asprintf(&proc, "/proc/self/fd/%d", dst) < 0)
+		errExit("asprintf");
+
+	int rv = mount(srcname, proc, NULL, MS_BIND|MS_REC, NULL);
+	if (rv < 0 && arg_debug)
+		printf("Failed mount: %s\n", strerror(errno));
+
+	free(proc);
+	return rv;
+}
+
 int has_handler(pid_t pid, int signal) {
 	if (signal > 0 && signal <= SIGRTMAX) {
 		char *fname;
diff --git a/src/firejail/x11.c b/src/firejail/x11.c
index f4f093138..afe77e246 100644
--- a/src/firejail/x11.c
+++ b/src/firejail/x11.c
@@ -1276,12 +1276,7 @@ void x11_xorg(void) {
 	// mount via the link in /proc/self/fd
 	if (arg_debug)
 		printf("Mounting %s on %s\n", tmpfname, dest);
-	char *proc_src, *proc_dst;
-	if (asprintf(&proc_src, "/proc/self/fd/%d", src) == -1)
-		errExit("asprintf");
-	if (asprintf(&proc_dst, "/proc/self/fd/%d", dst) == -1)
-		errExit("asprintf");
-	if (mount(proc_src, proc_dst, NULL, MS_BIND, NULL) == -1) {
+	if (bind_mount_by_fd(src, dst)) {
 		fprintf(stderr, "Error: cannot mount the new .Xauthority file\n");
 		exit(1);
 	}
@@ -1289,8 +1284,6 @@ void x11_xorg(void) {
 	MountData *mptr = get_last_mount();
 	if (strcmp(mptr->dir, dest) != 0 || strcmp(mptr->fstype, "tmpfs") != 0)
 		errLogExit("invalid .Xauthority mount");
-	free(proc_src);
-	free(proc_dst);
 	close(src);
 	close(dst);
 
@@ -1336,6 +1329,8 @@ void fs_x11(void) {
 		return;
 	}
 
+	// the mount source is under control of the user, so be careful and
+	// mount without following symbolic links, using a file descriptor
 	char *x11file;
 	if (asprintf(&x11file, "/tmp/.X11-unix/X%d", display) == -1)
 		errExit("asprintf");
@@ -1344,10 +1339,10 @@ void fs_x11(void) {
 		free(x11file);
 		return;
 	}
-	struct stat x11stat;
-	if (fstat(src, &x11stat) < 0)
+	struct stat s3;
+	if (fstat(src, &s3) < 0)
 		errExit("fstat");
-	if (!S_ISSOCK(x11stat.st_mode)) {
+	if (!S_ISSOCK(s3.st_mode)) {
 		close(src);
 		free(x11file);
 		return;
@@ -1367,14 +1362,8 @@ void fs_x11(void) {
 	if (dst < 0)
 		errExit("open");
 
-	char *proc_src, *proc_dst;
-	if (asprintf(&proc_src, "/proc/self/fd/%d", src) == -1 ||
-	    asprintf(&proc_dst, "/proc/self/fd/%d", dst) == -1)
-		errExit("asprintf");
-	if (mount(proc_src, proc_dst, NULL, MS_BIND | MS_REC, NULL) < 0)
+	if (bind_mount_by_fd(src, dst))
 		errExit("mount bind");
-	free(proc_src);
-	free(proc_dst);
 	close(src);
 	close(dst);
 	fs_logger2("whitelist", x11file);
-- 
cgit v1.2.3-54-g00ecf