make appimage mounts private to sandbox

src/firejail/appimage.c

@@ -29,7 +29,7 @@
 #include <errno.h>
 
 static char *devloop = NULL;	// device file
-static char *mntdir = NULL;	// mount point in /tmp directory
+static long unsigned size = 0;	// offset into appimage file
 
 #ifdef LOOP_CTL_GET_FREE	// test for older kernels; this definition is found in /usr/include/linux/loop.h
 static void err_loop(void) {
@@ -44,27 +44,27 @@ void appimage_set(const char *appimage) {
 	EUID_ASSERT();
 
 #ifdef LOOP_CTL_GET_FREE
-	// check appimage file
+	// open appimage file
 	invalid_filename(appimage, 0); // no globbing
-	if (access(appimage, R_OK) == -1) {
-		fprintf(stderr, "Error: cannot access AppImage file\n");
+	int ffd = open(appimage, O_RDONLY|O_CLOEXEC);
+	if (ffd == -1) {
+		fprintf(stderr, "Error: cannot read AppImage file\n");
+		exit(1);
+	}
+	struct stat s;
+	if (fstat(ffd, &s) == -1)
+		errExit("fstat");
+	if (!S_ISREG(s.st_mode)) {
+		fprintf(stderr, "Error: invalid AppImage file\n");
 		exit(1);
 	}
 
 	// get appimage type and ELF size
 	// a value of 0 means we are dealing with a type1 appimage
-	long unsigned int size = appimage2_size(appimage);
+	size = appimage2_size(appimage);
 	if (arg_debug)
 		printf("AppImage ELF size %lu\n", size);
 
-	// open appimage file
-	/* coverity[toctou] */
-	int ffd = open(appimage, O_RDONLY|O_CLOEXEC);
-	if (ffd == -1) {
-		fprintf(stderr, "Error: cannot open AppImage file\n");
-		exit(1);
-	}
-
 	// find or allocate a free loop device to use
 	EUID_ROOT();
 	int cfd = open("/dev/loop-control", O_RDWR);
@@ -77,6 +77,7 @@ void appimage_set(const char *appimage) {
 	if (asprintf(&devloop, "/dev/loop%d", devnr) == -1)
 		errExit("asprintf");
 
+	// associate loop device with appimage
 	int lfd = open(devloop, O_RDONLY);
 	if (lfd == -1)
 		err_loop();
@@ -90,64 +91,24 @@ void appimage_set(const char *appimage) {
 		if (ioctl(lfd,  LOOP_SET_STATUS64, &info) == -1)
 			err_loop();
 	}
-
 	close(lfd);
 	close(ffd);
 	EUID_USER();
 
-	// creates appimage mount point perms 0700
-	if (asprintf(&mntdir, "%s/.appimage-%u",  RUN_FIREJAIL_APPIMAGE_DIR, getpid()) == -1)
-		errExit("asprintf");
-	EUID_ROOT();
-	mkdir_attr(mntdir, 0700, getuid(), getgid());
-	EUID_USER();
-
-	// mount
-	char *mode;
-	if (asprintf(&mode, "mode=700,uid=%d,gid=%d", getuid(), getgid()) == -1)
-		errExit("asprintf");
-	unsigned long flags = MS_MGC_VAL|MS_RDONLY;
-	if (getuid())
-		flags |= MS_NODEV|MS_NOSUID;
-
-	EUID_ROOT();
-	if (size == 0) {
-		fmessage("Mounting appimage type 1\n");
-		if (mount(devloop, mntdir, "iso9660", flags, mode) < 0)
-			errExit("mounting appimage");
-	}
-	else {
-		fmessage("Mounting appimage type 2\n");
-		if (mount(devloop, mntdir, "squashfs", flags, NULL) < 0)
-			errExit("mounting appimage");
-	}
-
-	if (arg_debug)
-		printf("appimage mounted on %s\n", mntdir);
-	EUID_USER();
-
+	// set environment
 	char* abspath = realpath(appimage, NULL);
 	if (abspath == NULL)
 		errExit("Failed to obtain absolute path");
-
-	// set environment
 	env_store_name_val("APPIMAGE", abspath, SETENV);
+	free(abspath);
 
-	if (mntdir)
-		env_store_name_val("APPDIR", mntdir, SETENV);
+	env_store_name_val("APPDIR", RUN_FIREJAIL_APPIMAGE_DIR, SETENV);
 
 	if (size != 0)
 		env_store_name_val("ARGV0", appimage, SETENV);
 
 	if (cfg.cwd)
 		env_store_name_val("OWD", cfg.cwd, SETENV);
-
-	// build new command line
-	if (asprintf(&cfg.command_line, "%s/AppRun", mntdir) == -1)
-		errExit("asprintf");
-
-	free(abspath);
-	free(mode);
 #ifdef HAVE_GCOV
 	__gcov_flush();
 #endif
@@ -157,44 +118,38 @@ void appimage_set(const char *appimage) {
 #endif
 }
 
-void appimage_clear(void) {
-	int rv;
+// mount appimage into sandbox file system
+void appimage_mount(void) {
+	if (!devloop)
+		return;
 
-	EUID_ROOT();
-	if (mntdir) {
-		int i;
-		int rv = 0;
-		for (i = 0; i < 5; i++) {
-			rv = umount2(mntdir, MNT_FORCE);
-			if (rv == 0) {
-				fmessage("AppImage unmounted\n");
-
-				break;
-			}
-			if (rv == -1 && errno == EBUSY) {
-				fwarning("EBUSY error trying to unmount %s\n", mntdir);
-				sleep(2);
-				continue;
-			}
-
-			// rv = -1
-			if (!arg_quiet) {
-				fwarning("error trying to unmount %s\n", mntdir);
-				perror("umount");
-			}
-		}
+	unsigned long flags = MS_MGC_VAL|MS_RDONLY;
+	if (getuid())
+		flags |= MS_NODEV|MS_NOSUID;
 
-		if (rv == 0) {
-			rmdir(mntdir);
-			free(mntdir);
-		}
+	if (size == 0) {
+		fmessage("Mounting appimage type 1\n");
+		char *mode;
+		if (asprintf(&mode, "mode=700,uid=%d,gid=%d", getuid(), getgid()) == -1)
+			errExit("asprintf");
+		if (mount(devloop, RUN_FIREJAIL_APPIMAGE_DIR, "iso9660", flags, mode) < 0)
+			errExit("mounting appimage");
+		free(mode);
 	}
+	else {
+		fmessage("Mounting appimage type 2\n");
+		if (mount(devloop, RUN_FIREJAIL_APPIMAGE_DIR, "squashfs", flags, NULL) < 0)
+			errExit("mounting appimage");
+	}
+}
 
+void appimage_clear(void) {
+	EUID_ROOT();
 	if (devloop) {
 		int lfd = open(devloop, O_RDONLY);
 		if (lfd != -1) {
-			rv = ioctl(lfd, LOOP_CLR_FD, 0);
-			(void) rv;
+			if (ioctl(lfd, LOOP_CLR_FD, 0) != -1)
+				fmessage("AppImage detached\n");
 			close(lfd);
 		}
 	}

src/firejail/cmdline.c

@@ -161,30 +161,23 @@ void build_cmdline(char **command_line, char **window_title, int argc, char **ar
 	assert(*window_title);
 }
 
-void build_appimage_cmdline(char **command_line, char **window_title, int argc, char **argv, int index, char *apprun_path) {
+void build_appimage_cmdline(char **command_line, char **window_title, int argc, char **argv, int index) {
 	// index == -1 could happen if we have --shell=none and no program was specified
 	// the program should exit with an error before entering this function
 	assert(index != -1);
 
-	if (arg_debug)
-		printf("Building AppImage command line: %s\n", *command_line);
-
+	char *apprun_path = RUN_FIREJAIL_APPIMAGE_DIR "/AppRun";
 
 	int len1 = cmdline_length(argc, argv, index);  // length of argv w/o changes
 	int len2 = cmdline_length(1, &argv[index], 0); // apptest.AppImage
-	int len3 = cmdline_length(1, &apprun_path, 0); // /run/firejail/appimage/.appimage-23304/AppRun
+	int len3 = cmdline_length(1, &apprun_path, 0); // /run/firejail/appimage/AppRun
 	int len4 = (len1 - len2 + len3) + 1;           // apptest.AppImage is replaced by /path/to/AppRun
 
 	if (len4 > ARG_MAX) {
 		errno = E2BIG;
 		errExit("cmdline_length");
 	}
 
-	// save created apprun in cfg.command_line
-	char *tmp1 = strdup(*command_line);
-	if (!tmp1)
-		errExit("strdup");
-
 	// TODO: deal with extra allocated memory.
 	char *command_line_tmp = malloc(len1 + len3 + 1);
 	if (!command_line_tmp)
@@ -200,13 +193,12 @@ void build_appimage_cmdline(char **command_line, char **window_title, int argc,
 	assert(*window_title);
 
 	// 'fix' command_line now
-	if (asprintf(command_line, "'%s' %s", tmp1, command_line_tmp + len2) == -1)
+	if (asprintf(command_line, "'%s' %s", apprun_path, command_line_tmp + len2) == -1)
 		errExit("asprintf");
 
 	if (arg_debug)
 		printf("AppImage quoted command line: %s\n", *command_line);
 
 	// free strdup
-	free(tmp1);
 	free(command_line_tmp);
 }

src/firejail/firejail.h

@@ -798,15 +798,15 @@ void print_compiletime_support(void);
 
 // appimage.c
 void appimage_set(const char *appimage_path);
+void appimage_mount(void);
 void appimage_clear(void);
-const char *appimage_getdir(void);
 
 // appimage_size.c
 long unsigned int appimage2_size(const char *fname);
 
 // cmdline.c
 void build_cmdline(char **command_line, char **window_title, int argc, char **argv, int index);
-void build_appimage_cmdline(char **command_line, char **window_title, int argc, char **argv, int index, char *apprun_path);
+void build_appimage_cmdline(char **command_line, char **window_title, int argc, char **argv, int index);
 
 // sbox.c
 // programs

src/firejail/fs.c

@@ -800,8 +800,6 @@ void disable_config(void) {
 		disable_file(BLACKLIST_FILE, RUN_FIREJAIL_PROFILE_DIR);
 	if (stat(RUN_FIREJAIL_X11_DIR, &s) == 0)
 		disable_file(BLACKLIST_FILE, RUN_FIREJAIL_X11_DIR);
-	if (!arg_appimage && stat(RUN_FIREJAIL_APPIMAGE_DIR, &s) == 0)
-		disable_file(BLACKLIST_FILE, RUN_FIREJAIL_APPIMAGE_DIR);
 }
 
 

src/firejail/main.c

@@ -2790,7 +2790,7 @@ int main(int argc, char **argv, char **envp) {
 		if (arg_debug)
 			printf("Configuring appimage environment\n");
 		appimage_set(cfg.command_name);
-		build_appimage_cmdline(&cfg.command_line, &cfg.window_title, argc, argv, prog_index, cfg.command_line);
+		build_appimage_cmdline(&cfg.command_line, &cfg.window_title, argc, argv, prog_index);
 	}
 	else {
 		build_cmdline(&cfg.command_line, &cfg.window_title, argc, argv, prog_index);

src/firejail/sandbox.c

@@ -630,6 +630,8 @@ int sandbox(void* sandbox_arg) {
 		errExit("mounting " RUN_FIREJAIL_LIB_DIR);
 	// keep a copy of dhclient executable before the filesystem is modified
 	dhcp_store_exec();
+	// mount appimage before the filesystem is modified
+	appimage_mount();
 
 	//****************************
 	// log sandbox data

test/appimage/appimage-args.exp

@@ -96,7 +96,7 @@ send -- "firejail --shutdown=appimage-test\r"
 set spawn_id $appimage_id
 expect {
 	timeout {puts "shutdown\n";exit}
-	"AppImage unmounted"
+	"AppImage detached"
 }
 
 after 100

test/appimage/appimage-trace.exp

@@ -31,7 +31,7 @@ expect {
 }
 expect {
 	timeout {puts "shutdown\n"}
-	"AppImage unmounted"
+	"AppImage detached"
 }
 sleep 1
 
@@ -58,7 +58,7 @@ expect {
 }
 expect {
 	timeout {puts "shutdown\n"}
-	"AppImage unmounted"
+	"AppImage detached"
 }
 sleep 1
 

test/appimage/appimage-v1.exp

@@ -84,7 +84,7 @@ send -- "firejail --shutdown=appimage-test\r"
 set spawn_id $appimage_id
 expect {
 	timeout {puts "shutdown\n"}
-	"AppImage unmounted"
+	"AppImage detached"
 }
 
 after 100

test/appimage/appimage-v2.exp

@@ -83,7 +83,7 @@ send -- "firejail --shutdown=appimage-test\r"
 set spawn_id $appimage_id
 expect {
 	timeout {puts "shutdown\n"}
-	"AppImage unmounted"
+	"AppImage detached"
 }
 
 after 100

test/appimage/appimage.sh

@@ -20,4 +20,4 @@ echo "TESTING: AppImage argsv1 (test/appimage/appimage-args.exp)"
 ./appimage-args.exp
 
 echo "TESTING: AppImage trace (test/appimage/appimage-trace.exp)"
-./appimage-args.exp
+./appimage-trace.exp

test/appimage/filename.exp

@@ -17,7 +17,7 @@ after 100
 send -- "firejail --appimage /etc/shadow\r"
 expect {
 	timeout {puts "TESTING ERROR 2\n";exit}
-	"cannot access"
+	"cannot read"
 }
 after 100