harden private-home mounting, small improvements

src/firejail/fs_etc.c

@@ -101,7 +101,7 @@ errexit:
 static void duplicate(const char *fname, const char *private_dir, const char *private_run_dir) {
 	assert(fname);
 
-	if (*fname == '~' || *fname == '/' || strstr(fname, "..")) {
+	if (*fname == '~' || strchr(fname, '/') || strcmp(fname, "..") == 0) {
 		fprintf(stderr, "Error: \"%s\" is an invalid filename\n", fname);
 		exit(1);
 	}

src/firejail/fs_home.c

@@ -235,8 +235,29 @@ void fs_private_homedir(void) {
 	// mount bind private_homedir on top of homedir
 	if (arg_debug)
 		printf("Mount-bind %s on top of %s\n", private_homedir, homedir);
-	if (mount(private_homedir, homedir, NULL, MS_NOSUID | MS_NODEV | MS_BIND | MS_REC, NULL) < 0)
+	// get a file descriptor for private_homedir, fails if there is any symlink
+	int fd = safe_fd(private_homedir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC);
+	if (fd == -1)
+		errExit("safe_fd");
+	// check if new home directory is owned by the user
+	struct stat s;
+	if (fstat(fd, &s) == -1)
+		errExit("fstat");
+	if (s.st_uid != getuid()) {
+		fprintf(stderr, "Error: private directory is not owned by the current user\n");
+		exit(1);
+	}
+	if ((S_IRWXU & s.st_mode) != S_IRWXU)
+		fwarning("no full permissions for private directory\n");
+	// mount via the link in /proc/self/fd
+	char *proc;
+	if (asprintf(&proc, "/proc/self/fd/%d", fd) == -1)
+		errExit("asprintf");
+	if (mount(proc, homedir, NULL, MS_NOSUID | MS_NODEV | MS_BIND | MS_REC, NULL) < 0)
 		errExit("mount bind");
+	free(proc);
+	close(fd);
+
 	fs_logger3("mount-bind", private_homedir, cfg.homedir);
 	fs_logger2("whitelist", cfg.homedir);
 // preserve mode and ownership
@@ -339,31 +360,10 @@ void fs_check_private_dir(void) {
 	free(tmp);
 
 	if (!cfg.home_private
-	 || !is_dir(cfg.home_private)
+	 || !is_dir(cfg.home_private)) {
-	 || is_link(cfg.home_private)
-	 || strstr(cfg.home_private, "..")) {
 		fprintf(stderr, "Error: invalid private directory\n");
 		exit(1);
 	}
-
-	// check home directory and chroot home directory have the same owner
-	struct stat s2;
-	int rv = stat(cfg.home_private, &s2);
-	if (rv < 0) {
-		fprintf(stderr, "Error: cannot find %s directory\n", cfg.home_private);
-		exit(1);
-	}
-
-	struct stat s1;
-	rv = stat(cfg.homedir, &s1);
-	if (rv < 0) {
-		fprintf(stderr, "Error: cannot find %s directory, full path name required\n", cfg.homedir);
-		exit(1);
-	}
-	if (s1.st_uid != s2.st_uid) {
-		printf("Error: --private directory should be owned by the current user\n");
-		exit(1);
-	}
 }
 
 //***********************************************************************************
@@ -400,34 +400,33 @@ static char *check_dir_or_file(const char *name) {
 			}
 			return fname;
 		}
-		else {
+		else // dangling link
-			fprintf(stderr, "Error: invalid file %s\n", name);
+			goto errexit;
-			exit(1);
-		}
 	}
 	else {
 		// check the file is in user home directory, a full home directory is not allowed
 		char *rname = realpath(fname, NULL);
 		if (!rname ||
 		    strncmp(rname, cfg.homedir, strlen(cfg.homedir)) != 0 ||
-		    strcmp(rname, cfg.homedir) == 0) {
+		    strcmp(rname, cfg.homedir) == 0)
-			fprintf(stderr, "Error: invalid file %s\n", name);
+			goto errexit;
-			exit(1);
-		}
 
 		// only top files and directories in user home are allowed
 		char *ptr = rname + strlen(cfg.homedir);
-		assert(*ptr != '\0');
+		if (*ptr != '/')
+			goto errexit;
 		ptr = strchr(++ptr, '/');
 		if (ptr) {
-			if (*ptr != '\0') {
 			fprintf(stderr, "Error: only top files and directories in user home are allowed\n");
 			exit(1);
 		}
-		}
 		free(fname);
 		return rname;
 	}
+
+errexit:
+	fprintf(stderr, "Error: invalid file %s\n", name);
+	exit(1);
 }
 
 static void duplicate(char *name) {

src/firejail/main.c

@@ -1633,7 +1633,7 @@ int main(int argc, char **argv) {
 		else if (strncmp(argv[i], "--private-srv=", 14) == 0) {
 			// extract private srv list
 			if (*(argv[i] + 14) == '\0') {
-				fprintf(stderr, "Error: invalid private-etc option\n");
+				fprintf(stderr, "Error: invalid private-srv option\n");
 				exit(1);
 			}
 			if (cfg.srv_private_keep) {