overlay fixes and additional hardening

5 files changed, 73 insertions, 19 deletions

diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h
index 97e740e6b..e8dc390d4 100644
--- a/src/firejail/firejail.h
+++ b/src/firejail/firejail.h
@@ -513,7 +513,7 @@ const char *gnu_basename(const char *path);
 uid_t pid_get_uid(pid_t pid);
 void invalid_filename(const char *fname, int globbing);
 uid_t get_group_id(const char *group);
-int remove_directory(const char *path);
+int remove_overlay_directory(void);
 void flush_stdin(void);
 void create_empty_dir_as_root(const char *dir, mode_t mode);
 void create_empty_file_as_root(const char *dir, mode_t mode);
diff --git a/src/firejail/fs.c b/src/firejail/fs.c
index ab2958593..25b52f5ce 100644
--- a/src/firejail/fs.c
+++ b/src/firejail/fs.c
@@ -725,7 +725,7 @@ char *fs_check_overlay_dir(const char *subdirname, int allow_reuse) {
                 errExit("asprintf");
 
         if (is_link(dirname)) {
-                fprintf(stderr, "Error: invalid ~/.firejail directory\n");
+                fprintf(stderr, "Error: ~/.firejail directory is a symbolic link\n");
                 exit(1);
         }
         if (stat(dirname, &s) == -1) {
@@ -753,7 +753,7 @@ char *fs_check_overlay_dir(const char *subdirname, int allow_reuse) {
                 }
         }
         else if (s.st_uid != getuid()) {
-                fprintf(stderr, "Error: invalid ~/.firejail directory\n");
+                fprintf(stderr, "Error: ~/.firejail directory is not owned by the current user\n");
                 exit(1);
         }
         free(dirname);
@@ -837,6 +837,7 @@ void fs_overlayfs(void) {
         if (arg_overlay_keep) {
                 // set base for working and diff directories
                 basedir = cfg.overlay_dir;
+                assert(basedir);
 
                 // does the overlay exist?
                 if (stat(basedir, &s) == 0) {
diff --git a/src/firejail/main.c b/src/firejail/main.c
index 00e3729d0..7543c5f4b 100644
--- a/src/firejail/main.c
+++ b/src/firejail/main.c
@@ -314,16 +314,10 @@ static void run_cmd_and_exit(int i, int argc, char **argv) {
 #ifdef HAVE_OVERLAYFS
         else if (strcmp(argv[i], "--overlay-clean") == 0) {
                 if (checkcfg(CFG_OVERLAYFS)) {
-                        char *path;
-                        if (asprintf(&path, "%s/.firejail", cfg.homedir) == -1)
-                                errExit("asprintf");
-                        EUID_ROOT();
-                        if (setreuid(0, 0) < 0 ||
-                            setregid(0, 0) < 0)
-                                errExit("setreuid/setregid");
-                        errno = 0;
-                        if (remove_directory(path))
-                                errExit("remove_directory");
+                        if (remove_overlay_directory()) {
+                                fprintf(stderr, "Error: cannot remove overlay directory\n");
+                                exit(1);
+                        }
                 }
                 else
                         exit_err_feature("overlayfs");
@@ -1429,6 +1423,11 @@ int main(int argc, char **argv) {
 #ifdef HAVE_OVERLAYFS
                 else if (strcmp(argv[i], "--overlay") == 0) {
                         if (checkcfg(CFG_OVERLAYFS)) {
+                                if (arg_overlay) {
+                                        fprintf(stderr, "Error: only one overlay command is allowed\n");
+                                        exit(1);
+                                }
+
                                 if (cfg.chrootdir) {
                                         fprintf(stderr, "Error: --overlay and --chroot options are mutually exclusive\n");
                                         exit(1);
@@ -1453,6 +1452,10 @@ int main(int argc, char **argv) {
                 }
                 else if (strncmp(argv[i], "--overlay-named=", 16) == 0) {
                         if (checkcfg(CFG_OVERLAYFS)) {
+                                if (arg_overlay) {
+                                        fprintf(stderr, "Error: only one overlay command is allowed\n");
+                                        exit(1);
+                                }
                                 if (cfg.chrootdir) {
                                         fprintf(stderr, "Error: --overlay and --chroot options are mutually exclusive\n");
                                         exit(1);
@@ -1485,6 +1488,10 @@ int main(int argc, char **argv) {
                 }
                 else if (strcmp(argv[i], "--overlay-tmpfs") == 0) {
                         if (checkcfg(CFG_OVERLAYFS)) {
+                                if (arg_overlay) {
+                                        fprintf(stderr, "Error: only one overlay command is allowed\n");
+                                        exit(1);
+                                }
                                 if (cfg.chrootdir) {
                                         fprintf(stderr, "Error: --overlay and --chroot options are mutually exclusive\n");
                                         exit(1);
diff --git a/src/firejail/profile.c b/src/firejail/profile.c
index d0c43d13e..77308b7ac 100644
--- a/src/firejail/profile.c
+++ b/src/firejail/profile.c
@@ -920,6 +920,10 @@ int profile_check_line(char *ptr, int lineno, const char *fname) {
 #ifdef HAVE_OVERLAYFS
         if (strncmp(ptr, "overlay-named ", 14) == 0) {
                 if (checkcfg(CFG_OVERLAYFS)) {
+                        if (arg_overlay) {
+                                fprintf(stderr, "Error: only one overlay command is allowed\n");
+                                exit(1);
+                        }
                         if (cfg.chrootdir) {
                                 fprintf(stderr, "Error: --overlay and --chroot options are mutually exclusive\n");
                                 exit(1);
@@ -951,6 +955,10 @@ int profile_check_line(char *ptr, int lineno, const char *fname) {
                 return 0;
         } else if (strcmp(ptr, "overlay-tmpfs") == 0) {
                 if (checkcfg(CFG_OVERLAYFS)) {
+                        if (arg_overlay) {
+                                fprintf(stderr, "Error: only one overlay command is allowed\n");
+                                exit(1);
+                        }
                         if (cfg.chrootdir) {
                                 fprintf(stderr, "Error: --overlay and --chroot options are mutually exclusive\n");
                                 exit(1);
@@ -966,6 +974,10 @@ int profile_check_line(char *ptr, int lineno, const char *fname) {
                 }
         } else if (strcmp(ptr, "overlay") == 0) {
                 if (checkcfg(CFG_OVERLAYFS)) {
+                        if (arg_overlay) {
+                                fprintf(stderr, "Error: only one overlay command is allowed\n");
+                                exit(1);
+                        }
                         if (cfg.chrootdir) {
                                 fprintf(stderr, "Error: --overlay and --chroot options are mutually exclusive\n");
                                 exit(1);
diff --git a/src/firejail/util.c b/src/firejail/util.c
index 5a9f3a6e0..0adca5e33 100644
--- a/src/firejail/util.c
+++ b/src/firejail/util.c
@@ -800,21 +800,55 @@ uid_t get_group_id(const char *group) {
         return gid;
 }
 
-
+static int len_homedir = 0;
 static int remove_callback(const char *fpath, const struct stat *sb, int typeflag, struct FTW *ftwbuf) {
         (void) sb;
         (void) typeflag;
         (void) ftwbuf;
+        assert(fpath);
 
-        int rv = remove(fpath);
-        if (rv)
-                perror(fpath);
+        if (len_homedir == 0)
+                len_homedir = strlen(cfg.homedir);
 
-        return rv;
+        char *rp = realpath(fpath, NULL);       // this should never fail!
+        if (!rp)
+                return 1;
+        if (strncmp(rp, cfg.homedir, len_homedir) != 0)
+                return 1;
+        free(rp);
+
+        if (remove(fpath)) {    // removes the link not the actual file
+                fprintf(stderr, "Error: cannot remove file %s\n", fpath);
+                exit(1);
+        }
+
+        return 0;
 }
 
 
-int remove_directory(const char *path) {
+int remove_overlay_directory(void) {
+        sleep(1);
+
+        char *path;
+        if (asprintf(&path, "%s/.firejail", cfg.homedir) == -1)
+                errExit("asprintf");
+
+        // deal with obvious problems such as symlinks and root ownership
+        if (is_link(path)) {
+                fprintf(stderr, "Error: cannot follow symbolic link\n");
+                exit(1);
+        }
+        if (access(path, R_OK | W_OK | X_OK) == -1) {
+                fprintf(stderr, "Error: cannot access ~/.firejail directory\n");
+                exit(1);
+        }
+
+        EUID_ROOT();
+        if (setreuid(0, 0) < 0 ||
+            setregid(0, 0) < 0)
+                errExit("setreuid/setregid");
+        errno = 0;
+
         // FTW_PHYS - do not follow symbolic links
         return nftw(path, remove_callback, 64, FTW_DEPTH | FTW_PHYS);
 }