fixing --hosts-file privelege check

Currently the code uses the access() call to check if the user has an access to a file that is copied into the root as /etc/hosts. This inevitably adds a race when the user changes the file to a symbolic link pointing to an arbitrary location on the filsystem after the access check is done but before opening the file to copy it. This potentially allows to read any file on the system.

To close this the code adds a utility copy_file_from_user_to_root . It opens the copy destination file as root and then forks/drop privileges. Then as a user the utility opens the source file and do the copy into the destination descriptor that is preserved accross the fork.

3 files changed, 68 insertions, 26 deletions

diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h
index 0f836f1db..7d6e16094 100644
--- a/src/firejail/firejail.h
+++ b/src/firejail/firejail.h
@@ -453,6 +453,7 @@ void logargs(int argc, char **argv) ;
 void logerr(const char *msg);
 int copy_file(const char *srcname, const char *destname, uid_t uid, gid_t gid, mode_t mode);
 void copy_file_as_user(const char *srcname, const char *destname, uid_t uid, gid_t gid, mode_t mode);
+void copy_file_from_user_to_root(const char *srcname, const char *destname, uid_t uid, gid_t gid, mode_t mode);
 void touch_file_as_user(const char *fname, uid_t uid, gid_t gid, mode_t mode);
 int is_dir(const char *fname);
 int is_link(const char *fname);
diff --git a/src/firejail/fs_hostname.c b/src/firejail/fs_hostname.c
index 19af11dd8..3b586b276 100644
--- a/src/firejail/fs_hostname.c
+++ b/src/firejail/fs_hostname.c
@@ -147,7 +147,7 @@ errexit:
 }
 
 void fs_store_hosts_file(void) {
-        copy_file(cfg.hosts_file, RUN_HOSTS_FILE, 0, 0, 0644); // root needed
+        copy_file_from_user_to_root(cfg.hosts_file, RUN_HOSTS_FILE, 0, 0, 0644); // root needed
 }
 
 void fs_mount_hosts_file(void) {
diff --git a/src/firejail/util.c b/src/firejail/util.c
index 10000e912..44891ce2d 100644
--- a/src/firejail/util.c
+++ b/src/firejail/util.c
@@ -169,6 +169,25 @@ void logerr(const char *msg) {
         closelog();
 }
 
+static int copy_file_by_fd(int src, int dst) {
+        assert(src >= 0);
+        assert(dst >= 0);
+
+        ssize_t len;
+        static const int BUFLEN = 1024;
+        unsigned char buf[BUFLEN];
+        while ((len = read(src, buf, BUFLEN)) > 0) {
+                int done = 0;
+                while (done != len) {
+                        int rv = write(dst, buf + done, len - done);
+                        if (rv == -1)
+                                return -1;
+                        done += rv;
+                }
+        }
+        fflush(0);
+        return 0;
+}
 
 // return -1 if error, 0 if no error; if destname already exists, return error
 int copy_file(const char *srcname, const char *destname, uid_t uid, gid_t gid, mode_t mode) {
@@ -190,33 +209,16 @@ int copy_file(const char *srcname, const char *destname, uid_t uid, gid_t gid, m
                 return -1;
         }
 
-        // copy
-        ssize_t len;
-        static const int BUFLEN = 1024;
-        unsigned char buf[BUFLEN];
-        while ((len = read(src, buf, BUFLEN)) > 0) {
-                int done = 0;
-                while (done != len) {
-                        int rv = write(dst, buf + done, len - done);
-                        if (rv == -1) {
-                                close(src);
-                                close(dst);
-                                return -1;
-                        }
-
-                        done += rv;
-                }
+        int errors = copy_file_by_fd(src, dst);
+        if (!errors) {
+                if (fchown(dst, uid, gid) == -1)
+                        errExit("fchown");
+                if (fchmod(dst, mode) == -1)
+                        errExit("fchmod");
         }
-        fflush(0);
-
-        if (fchown(dst, uid, gid) == -1)
-                errExit("fchown");
-        if (fchmod(dst, mode) == -1)
-                errExit("fchmod");
-
         close(src);
         close(dst);
-        return 0;
+        return errors;
 }
 
 // return -1 if error, 0 if no error
@@ -241,6 +243,45 @@ void copy_file_as_user(const char *srcname, const char *destname, uid_t uid, gid
         waitpid(child, NULL, 0);
 }
 
+void copy_file_from_user_to_root(const char *srcname, const char *destname, uid_t uid, gid_t gid, mode_t mode) {
+        // open destination
+        int dst = open(destname, O_CREAT|O_WRONLY|O_TRUNC, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+        if (dst < 0) {
+                fprintf(stderr, "Warning: cannot open destination file %s, file not copied\n", destname);
+                return;
+        }
+
+        pid_t child = fork();
+        if (child < 0)
+                errExit("fork");
+        if (child == 0) {
+                // drop privileges
+                drop_privs(0);
+
+                int src = open(srcname, O_RDONLY);
+                if (src < 0) {
+                        fprintf(stderr, "Warning: cannot open source file %s, file not copied\n", srcname);
+                } else {
+                        if (copy_file_by_fd(src, dst)) {
+                                fprintf(stderr, "Warning: cannot copy %s\n", srcname);
+                        }
+                        close(src);
+                }
+                close(dst);
+#ifdef HAVE_GCOV
+                __gcov_flush();
+#endif
+                _exit(0);
+        }
+        // wait for the child to finish
+        waitpid(child, NULL, 0);
+        if (fchown(dst, uid, gid) == -1)
+                errExit("fchown");
+        if (fchmod(dst, mode) == -1)
+                errExit("fchmod");
+        close(dst);
+}
+
 // return -1 if error, 0 if no error
 void touch_file_as_user(const char *fname, uid_t uid, gid_t gid, mode_t mode) {
         pid_t child = fork();
@@ -864,4 +905,4 @@ errexit:
         close(fd);
         fprintf(stderr, "Error: cannot read %s\n", fname);
         exit(1);
-}
\ No newline at end of file
+}