13 files changed, 67 insertions, 126 deletions

diff --git a/src/firejail/appimage.c b/src/firejail/appimage.c
index 40e53f44d..59758bf2d 100644
--- a/src/firejail/appimage.c
+++ b/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(ffd);
         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);
                 }
         }
diff --git a/src/firejail/appimage_size.c b/src/firejail/appimage_size.c
index 4640cb8a5..43ca501da 100644
--- a/src/firejail/appimage_size.c
+++ b/src/firejail/appimage_size.c
@@ -132,22 +132,20 @@ static long unsigned int read_elf64(int fd) {
 
 // return 0 if error
 // return 0 if this is not an appimgage2 file
-long unsigned int appimage2_size(const char *fname) {
+long unsigned int appimage2_size(int fd) {
         ssize_t ret;
-        int fd;
         long unsigned int size = 0;
 
-        fd = open(fname, O_RDONLY);
         if (fd < 0)
                 return 0;
 
         ret = pread(fd, ehdr.e_ident, EI_NIDENT, 0);
         if (ret != EI_NIDENT)
-                goto getout;
+                return 0;
 
         if ((ehdr.e_ident[EI_DATA] != ELFDATA2LSB) &&
              (ehdr.e_ident[EI_DATA] != ELFDATA2MSB))
-                goto getout;
+                return 0;
 
         if(ehdr.e_ident[EI_CLASS] == ELFCLASS32) {
                 size = read_elf32(fd);
@@ -156,23 +154,19 @@ long unsigned int appimage2_size(const char *fname) {
                 size = read_elf64(fd);
         }
         else {
-                goto getout;
+                return 0;
         }
         if (size == 0)
-                goto getout;
+                return 0;
 
 
         // look for a LZMA header at this location
         unsigned char buf[4];
         ret = pread(fd, buf, 4, size);
-        if (ret != 4) {
-                size = 0;
-                goto getout;
-        }
+        if (ret != 4)
+                return 0;
         if (memcmp(buf, "hsqs", 4) != 0)
-                size = 0;
+                return 0;
 
-getout:
-        close(fd);
         return size;
 }
diff --git a/src/firejail/cmdline.c b/src/firejail/cmdline.c
index 0cdcb32bf..f902c4e1c 100644
--- a/src/firejail/cmdline.c
+++ b/src/firejail/cmdline.c
@@ -161,18 +161,16 @@ 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) {
@@ -180,11 +178,6 @@ void build_appimage_cmdline(char **command_line, char **window_title, int argc,
                 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);
 }
diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h
index b21b5bef6..ca4c988fa 100644
--- a/src/firejail/firejail.h
+++ b/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);
+long unsigned int appimage2_size(int fd);
 
 // 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
diff --git a/src/firejail/fs.c b/src/firejail/fs.c
index 8b7e49611..fc67a15f3 100644
--- a/src/firejail/fs.c
+++ b/src/firejail/fs.c
@@ -801,8 +801,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);
 }
 
 
diff --git a/src/firejail/main.c b/src/firejail/main.c
index 9705c2436..b3524fcf5 100644
--- a/src/firejail/main.c
+++ b/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);
diff --git a/src/firejail/sandbox.c b/src/firejail/sandbox.c
index 36a54d6fe..60c097cf2 100644
--- a/src/firejail/sandbox.c
+++ b/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
diff --git a/test/appimage/appimage-args.exp b/test/appimage/appimage-args.exp
index 16ab7f54d..eecb9bf82 100755
--- a/test/appimage/appimage-args.exp
+++ b/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
diff --git a/test/appimage/appimage-trace.exp b/test/appimage/appimage-trace.exp
index 5f05e2846..2f67eb531 100755
--- a/test/appimage/appimage-trace.exp
+++ b/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
 
diff --git a/test/appimage/appimage-v1.exp b/test/appimage/appimage-v1.exp
index d90793bb3..b8b6e0c96 100755
--- a/test/appimage/appimage-v1.exp
+++ b/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
diff --git a/test/appimage/appimage-v2.exp b/test/appimage/appimage-v2.exp
index b606c7a77..243824f75 100755
--- a/test/appimage/appimage-v2.exp
+++ b/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
diff --git a/test/appimage/appimage.sh b/test/appimage/appimage.sh
index 548cd7fdf..e766b1acd 100755
--- a/test/appimage/appimage.sh
+++ b/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
diff --git a/test/appimage/filename.exp b/test/appimage/filename.exp
index 7b64129b7..54d8d722d 100755
--- a/test/appimage/filename.exp
+++ b/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