Merge pull request #4652 from kmk3/fix-toctou-easy

Fix TOCTOU/CodeQL CWE-367 warnings (easy ones + fs.c)

3 files changed, 38 insertions, 30 deletions

diff --git a/src/fids/main.c b/src/fids/main.c
index c899b55e1..8f9bc1ea0 100644
--- a/src/fids/main.c
+++ b/src/fids/main.c
@@ -210,22 +210,29 @@ static void process_config(const char *fname) {
                 exit(1);
         }
 
-        // make sure the file is owned by root
-        struct stat s;
-        if (stat(fname, &s)) {
+        fprintf(stderr, "Opening config file %s\n", fname);
+        int fd = open(fname, O_RDONLY|O_CLOEXEC);
+        if (fd < 0) {
                 if (include_level == 1) {
-                        fprintf(stderr, "Error ids: config file not found\n");
+                        fprintf(stderr, "Error ids: cannot open config file %s\n", fname);
                         exit(1);
                 }
                 return;
         }
+
+        // make sure the file is owned by root
+        struct stat s;
+        if (fstat(fd, &s)) {
+                fprintf(stderr, "Error ids: cannot stat config file %s\n", fname);
+                exit(1);
+        }
         if (s.st_uid || s.st_gid) {
                 fprintf(stderr, "Error ids: config file not owned by root\n");
                 exit(1);
         }
 
-        fprintf(stderr, "Loading %s config file\n", fname);
-        FILE *fp = fopen(fname, "r");
+        fprintf(stderr, "Loading config file %s\n", fname);
+        FILE *fp = fdopen(fd, "r");
         if (!fp) {
                 fprintf(stderr, "Error fids: cannot open config file %s\n", fname);
                 exit(1);
diff --git a/src/firejail/fs.c b/src/firejail/fs.c
index 9c1b889ed..f62e6404e 100644
--- a/src/firejail/fs.c
+++ b/src/firejail/fs.c
@@ -94,16 +94,7 @@ static void disable_file(OPERATION op, const char *filename) {
                 return;
         }
 
-        // if the file is not present, do nothing
         assert(fname);
-        struct stat s;
-        if (stat(fname, &s) < 0) {
-                if (arg_debug)
-                        printf("Warning (blacklisting): cannot access %s: %s\n", fname, strerror(errno));
-                free(fname);
-                return;
-        }
-
         // check for firejail executable
         // we might 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
@@ -113,6 +104,24 @@ static void disable_file(OPERATION op, const char *filename) {
                 return;
         }
 
+        // if the file is not present, do nothing
+        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;
+        }
+
+        struct stat s;
+        if (fstat(fd, &s) < 0) {
+                if (arg_debug)
+                        printf("Warning (blacklisting): cannot stat %s: %s\n", fname, strerror(errno));
+                free(fname);
+                close(fd);
+                return;
+        }
+
         // modify the file
         if (op == BLACKLIST_FILE || op == BLACKLIST_NOLOG) {
                 // some distros put all executables under /usr/bin and make /bin a symbolic link
@@ -136,13 +145,6 @@ 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;
-                        }
                         EUID_ROOT();
                         if (S_ISDIR(s.st_mode)) {
                                 if (bind_mount_path_to_fd(RUN_RO_DIR, fd) < 0)
@@ -153,7 +155,6 @@ static void disable_file(OPERATION op, const char *filename) {
                                         errExit("disable file");
                         }
                         EUID_USER();
-                        close(fd);
 
                         if (op == BLACKLIST_FILE)
                                 fs_logger2("blacklist", fname);
@@ -180,8 +181,7 @@ static void disable_file(OPERATION op, const char *filename) {
         else if (op == MOUNT_TMPFS) {
                 if (!S_ISDIR(s.st_mode)) {
                         fwarning("%s is not a directory; cannot mount a tmpfs on top of it.\n", fname);
-                        free(fname);
-                        return;
+                        goto out;
                 }
 
                 uid_t uid = getuid();
@@ -191,8 +191,7 @@ static void disable_file(OPERATION op, const char *filename) {
                             strncmp(cfg.homedir, fname, strlen(cfg.homedir)) != 0 ||
                             fname[strlen(cfg.homedir)] != '/') {
                                 fwarning("you are not allowed to mount a tmpfs on %s\n", fname);
-                                free(fname);
-                                return;
+                                goto out;
                         }
                 }
 
@@ -202,6 +201,8 @@ static void disable_file(OPERATION op, const char *filename) {
         else
                 assert(0);
 
+out:
+        close(fd);
         free(fname);
 }
 
diff --git a/src/firejail/seccomp.c b/src/firejail/seccomp.c
index 3d9bf9082..e02be29f1 100644
--- a/src/firejail/seccomp.c
+++ b/src/firejail/seccomp.c
@@ -435,11 +435,11 @@ void seccomp_print_filter(pid_t pid) {
         if (asprintf(&fname, "/proc/%d/root%s", pid, RUN_SECCOMP_LIST) == -1)
                 errExit("asprintf");
 
-        struct stat s;
-        if (stat(fname, &s) == -1)
+        int fd = open(fname, O_RDONLY|O_CLOEXEC);
+        if (fd < 0)
                 goto errexit;
 
-        FILE *fp = fopen(fname, "re");
+        FILE *fp = fdopen(fd, "r");
         if (!fp)
                 goto errexit;
         free(fname);