From 34f148031a41bd9d2db3a3bd286d8741a7ed1fe9 Mon Sep 17 00:00:00 2001 From: smitsohu Date: Tue, 28 Aug 2018 16:45:55 +0200 Subject: fix and harden overlay options --- src/faudit/faudit.h | 2 +- src/firejail/fs.c | 222 +++++++++++++++++++++++++++++++-------------------- src/firejail/util.c | 78 ++++++++++++------ src/include/common.h | 2 +- src/man/firejail.txt | 9 ++- 5 files changed, 198 insertions(+), 115 deletions(-) diff --git a/src/faudit/faudit.h b/src/faudit/faudit.h index 180121ec1..e940a9dbf 100644 --- a/src/faudit/faudit.h +++ b/src/faudit/faudit.h @@ -31,7 +31,7 @@ #include #include -#define errExit(msg) do { char msgout[500]; sprintf(msgout, "Error %s:%s(%d)", msg, __FUNCTION__, __LINE__); perror(msgout); exit(1);} while (0) +#define errExit(msg) do { char msgout[500]; snprintf(msgout, 500, "Error %s:%s(%d)", msg, __FUNCTION__, __LINE__); perror(msgout); exit(1);} while (0) // main.c extern char *prog; diff --git a/src/firejail/fs.c b/src/firejail/fs.c index d262d18bf..98efb5e41 100644 --- a/src/firejail/fs.c +++ b/src/firejail/fs.c @@ -779,19 +779,28 @@ void fs_basic_fs(void) { #ifdef HAVE_OVERLAYFS char *fs_check_overlay_dir(const char *subdirname, int allow_reuse) { + assert(subdirname); struct stat s; char *dirname; - // create ~/.firejail directory if (asprintf(&dirname, "%s/.firejail", cfg.homedir) == -1) errExit("asprintf"); - - if (is_link(dirname)) { - fprintf(stderr, "Error: ~/.firejail directory is a symbolic link\n"); - exit(1); + // check if ~/.firejail already exists + if (lstat(dirname, &s) == 0) { + if (!S_ISDIR(s.st_mode)) { + if (S_ISLNK(s.st_mode)) + fprintf(stderr, "Error: ~/.firejail is a symbolic link\n"); + else + fprintf(stderr, "Error: ~/.firejail is not a directory\n"); + exit(1); + } + if (s.st_uid != getuid()) { + fprintf(stderr, "Error: ~/.firejail directory is not owned by the current user\n"); + exit(1); + } } - if (stat(dirname, &s) == -1) { - // create directory + else { + // create ~/.firejail directory pid_t child = fork(); if (child < 0) errExit("fork"); @@ -805,6 +814,9 @@ char *fs_check_overlay_dir(const char *subdirname, int allow_reuse) { if (chmod(dirname, 0700) == -1) errExit("chmod"); ASSERT_PERMS(dirname, getuid(), getgid(), 0700); +#ifdef HAVE_GCOV + __gcov_flush(); +#endif _exit(0); } // wait for the child to finish @@ -813,23 +825,27 @@ char *fs_check_overlay_dir(const char *subdirname, int allow_reuse) { fprintf(stderr, "Error: cannot create ~/.firejail directory\n"); exit(1); } - } - else if (s.st_uid != getuid()) { - fprintf(stderr, "Error: ~/.firejail directory is not owned by the current user\n"); - exit(1); + fs_logger2("create", dirname); } free(dirname); // check overlay directory if (asprintf(&dirname, "%s/.firejail/%s", cfg.homedir, subdirname) == -1) errExit("asprintf"); - if (is_link(dirname)) { - fprintf(stderr, "Error: overlay directory is a symbolic link\n"); - exit(1); - } - if (allow_reuse == 0) { - if (stat(dirname, &s) == 0) { - fprintf(stderr, "Error: overlay directory already exists: %s\n", dirname); + if (lstat(dirname, &s) == 0) { + if (!S_ISDIR(s.st_mode)) { + if (S_ISLNK(s.st_mode)) + fprintf(stderr, "Error: %s is a symbolic link\n", dirname); + else + fprintf(stderr, "Error: %s is not a directory\n", dirname); + exit(1); + } + if (s.st_uid != 0) { + fprintf(stderr, "Error: overlay directory %s is not owned by the root user\n", dirname); + exit(1); + } + if (allow_reuse == 0) { + fprintf(stderr, "Error: overlay directory exists, but reuse is not allowed\n"); exit(1); } } @@ -866,9 +882,11 @@ char *fs_check_overlay_dir(const char *subdirname, int allow_reuse) { // # umount /root/overlay/root -// to do: fix the code below; also, it might work without /dev; impose seccomp/caps filters when not root +// to do: fix the code below; also, it might work without /dev, but consider keeping /dev/shm; add locking mechanism for overlay-clean #include void fs_overlayfs(void) { + struct stat s; + // check kernel version struct utsname u; int rv = uname(&u); @@ -894,50 +912,78 @@ void fs_overlayfs(void) { char *oroot = RUN_OVERLAY_ROOT; mkdir_attr(oroot, 0755, 0, 0); - struct stat s; + // set base for working and diff directories char *basedir = RUN_MNT_DIR; + int basefd = -1; + 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) { - if (arg_overlay_reuse == 0) { - fprintf(stderr, "Error: overlay directory exists, but reuse is not allowed\n"); - exit(1); - } - } - else { - /* coverity[toctou] */ - if (mkdir(basedir, 0755) != 0) { - fprintf(stderr, "Error: cannot create overlay directory\n"); - exit(1); - } + // get a file descriptor for ~/.firejail, fails if there is any symlink + char *firejail; + if (asprintf(&firejail, "%s/.firejail", cfg.homedir) == -1) + errExit("asprintf"); + int fd = safe_fd(firejail, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC); + if (fd == -1) + errExit("safe_fd"); + free(firejail); + // create basedir if it doesn't exist + // the new directory will be owned by root + const char *dirname = gnu_basename(basedir); + if (mkdirat(fd, dirname, 0755) == -1 && errno != EEXIST) { + perror("mkdir"); + fprintf(stderr, "Error: cannot create overlay directory %s\n", basedir); + exit(1); } + // open basedir + basefd = openat(fd, dirname, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC); + close(fd); + } + else { + basefd = open(basedir, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC); + } + if (basefd == -1) { + perror("open"); + fprintf(stderr, "Error: cannot open overlay directory %s\n", basedir); + exit(1); } - char *odiff; - if(asprintf(&odiff, "%s/odiff", basedir) == -1) - errExit("asprintf"); + // confirm once more base is owned by root + if (fstat(basefd, &s) == -1) + errExit("fstat"); + if (s.st_uid != 0) { + fprintf(stderr, "Error: overlay directory %s is not owned by the root user\n", basedir); + exit(1); + } + // confirm permissions of base are 0755 + if (((S_IRWXU|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH) & s.st_mode) != (S_IRWXU|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH)) { + fprintf(stderr, "Error: invalid permissions on overlay directory %s\n", basedir); + exit(1); + } + // create diff and work directories inside base // no need to check arg_overlay_reuse - if (stat(odiff, &s) != 0) { - mkdir_attr(odiff, 0755, 0, 0); + char *odiff; + if (asprintf(&odiff, "%s/odiff", basedir) == -1) + errExit("asprintf"); + // the new directory will be owned by root + if (mkdirat(basefd, "odiff", 0755) == -1 && errno != EEXIST) { + perror("mkdir"); + fprintf(stderr, "Error: cannot create overlay directory %s\n", odiff); + exit(1); } - else if (set_perms(odiff, 0, 0, 0755)) - errExit("set_perms"); + ASSERT_PERMS(odiff, 0, 0, 0755); char *owork; - if(asprintf(&owork, "%s/owork", basedir) == -1) + if (asprintf(&owork, "%s/owork", basedir) == -1) errExit("asprintf"); - - // no need to check arg_overlay_reuse - if (stat(owork, &s) != 0) { - mkdir_attr(owork, 0755, 0, 0); + // the new directory will be owned by root + if (mkdirat(basefd, "owork", 0755) == -1 && errno != EEXIST) { + perror("mkdir"); + fprintf(stderr, "Error: cannot create overlay directory %s\n", owork); + exit(1); } - else if (set_perms(owork, 0, 0, 0755)) - errExit("chown"); + ASSERT_PERMS(owork, 0, 0, 0755); // mount overlayfs if (arg_debug) @@ -977,43 +1023,48 @@ void fs_overlayfs(void) { // BEFORE NEXT, WE NEED TO TEST IF /home has any contents or do we need to mount it? // must create var for oroot/cfg.homedir - if (asprintf(&overlayhome,"%s%s",oroot,cfg.homedir) == -1) + if (asprintf(&overlayhome, "%s%s", oroot, cfg.homedir) == -1) errExit("asprintf"); - if (arg_debug) printf ("DEBUG: overlayhome var holds ##%s##\n",overlayhome); + if (arg_debug) printf ("DEBUG: overlayhome var holds ##%s##\n", overlayhome); // if no homedir in overlay -- create another overlay for /home - if (stat(overlayhome, &s) == -1) { - - if(asprintf(&hroot, "%s/oroot/home", RUN_MNT_DIR) == -1) - errExit("asprintf"); - - if(asprintf(&hdiff, "%s/hdiff", basedir) == -1) - errExit("asprintf"); + if (stat(cfg.homedir, &s) == 0 && stat(overlayhome, &s) == -1) { // no need to check arg_overlay_reuse - if (stat(hdiff, &s) != 0) { - mkdir_attr(hdiff, S_IRWXU | S_IRWXG | S_IRWXO, 0, 0); - } - else if (set_perms(hdiff, 0, 0, S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH)) - errExit("set_perms"); - - if(asprintf(&hwork, "%s/hwork", basedir) == -1) + if (asprintf(&hdiff, "%s/hdiff", basedir) == -1) errExit("asprintf"); + // the new directory will be owned by root + if (mkdirat(basefd, "hdiff", 0755) == -1 && errno != EEXIST) { + perror("mkdir"); + fprintf(stderr, "Error: cannot create overlay directory %s\n", hdiff); + exit(1); + } + ASSERT_PERMS(hdiff, 0, 0, 0755); // no need to check arg_overlay_reuse - if (stat(hwork, &s) != 0) { - mkdir_attr(hwork, S_IRWXU | S_IRWXG | S_IRWXO, 0, 0); + if (asprintf(&hwork, "%s/hwork", basedir) == -1) + errExit("asprintf"); + // the new directory will be owned by root + if (mkdirat(basefd, "hwork", 0755) == -1 && errno != EEXIST) { + perror("mkdir"); + fprintf(stderr, "Error: cannot create overlay directory %s\n", hwork); + exit(1); } - else if (set_perms(hwork, 0, 0, S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH)) - errExit("set_perms"); + ASSERT_PERMS(hwork, 0, 0, 0755); // no homedir in overlay so now mount another overlay for /home + if (asprintf(&hroot, "%s/home", oroot) == -1) + errExit("asprintf"); if (asprintf(&option, "lowerdir=/home,upperdir=%s,workdir=%s", hdiff, hwork) == -1) errExit("asprintf"); if (mount("overlay", hroot, "overlay", MS_MGC_VAL, option) < 0) errExit("mounting overlayfs for mounted home directory"); printf("OverlayFS for /home configured in %s directory\n", basedir); + free(hroot); + free(hdiff); + free(hwork); + } // stat(overlayhome) free(overlayhome); } @@ -1021,7 +1072,9 @@ void fs_overlayfs(void) { //*************************** } fmessage("OverlayFS configured in %s directory\n", basedir); + close(basefd); + // /dev, /run and /tmp are not covered by the overlay // mount-bind dev directory if (arg_debug) printf("Mounting /dev\n"); @@ -1042,19 +1095,15 @@ void fs_overlayfs(void) { errExit("mounting /run"); fs_logger("whitelist /run"); - // mount-bind /tmp/.X11-unix directory - if (stat("/tmp/.X11-unix", &s) == 0) { - if (arg_debug) - printf("Mounting /tmp/.X11-unix\n"); - char *x11; - if (asprintf(&x11, "%s/tmp/.X11-unix", oroot) == -1) - errExit("asprintf"); - if (mount("/tmp/.X11-unix", x11, NULL, MS_BIND|MS_REC, NULL) < 0) - fwarning("cannot mount /tmp/.X11-unix in overlay\n"); - else - fs_logger("whitelist /tmp/.X11-unix"); - free(x11); - } + // mount-bind tmp directory + if (arg_debug) + printf("Mounting /tmp\n"); + char *tmp; + if (asprintf(&tmp, "%s/tmp", oroot) == -1) + errExit("asprintf"); + if (mount("/tmp", tmp, NULL, MS_BIND|MS_REC, NULL) < 0) + errExit("mounting /tmp"); + fs_logger("whitelist /tmp"); // chroot in the new filesystem #ifdef HAVE_GCOV @@ -1068,12 +1117,9 @@ void fs_overlayfs(void) { // fs_dev_shm(); fs_var_lock(); if (!arg_keep_var_tmp) - fs_var_tmp(); + fs_var_tmp(); if (!arg_writable_var_log) fs_var_log(); - else - fs_rdwr("/var/log"); - fs_var_lib(); fs_var_cache(); fs_var_utmp(); @@ -1089,6 +1135,10 @@ void fs_overlayfs(void) { // cleanup and exit free(option); free(odiff); + free(owork); + free(dev); + free(run); + free(tmp); } #endif diff --git a/src/firejail/util.c b/src/firejail/util.c index 050f7534a..ff2bceacd 100644 --- a/src/firejail/util.c +++ b/src/firejail/util.c @@ -828,25 +828,19 @@ 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); - if (len_homedir == 0) - len_homedir = strlen(cfg.homedir); - - 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 (strcmp(fpath, ".") == 0) + return 0; if (remove(fpath)) { // removes the link not the actual file - fprintf(stderr, "Error: cannot remove file %s\n", fpath); + perror("remove"); + fprintf(stderr, "Error: cannot remove file from user .firejail directory: %s\n", fpath); exit(1); } @@ -855,26 +849,64 @@ static int remove_callback(const char *fpath, const struct stat *sb, int typefla int remove_overlay_directory(void) { + EUID_ASSERT(); + struct stat s; 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)) - errLogExit("overlay directory is a symlink\n"); - if (access(path, R_OK | W_OK | X_OK) == -1) - errLogExit("no access to overlay directory\n"); + if (lstat(path, &s) == 0) { + // deal with obvious problems such as symlinks and root ownership + if (!S_ISDIR(s.st_mode)) { + if (S_ISLNK(s.st_mode)) + fprintf(stderr, "Error: %s is a symbolic link\n", path); + else + fprintf(stderr, "Error: %s is not a directory\n", path); + exit(1); + } + if (s.st_uid != getuid()) { + fprintf(stderr, "Error: %s is not owned by the current user\n", path); + exit(1); + } + + pid_t child = fork(); + if (child < 0) + errExit("fork"); + if (child == 0) { + // open ~/.firejail, fails if there is any symlink + int fd = safe_fd(path, O_PATH|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC); + if (fd == -1) + errExit("safe_fd"); + // chdir to ~/.firejail + if (fchdir(fd) == -1) + errExit("fchdir"); + close(fd); - EUID_ROOT(); - if (setreuid(0, 0) < 0 || - setregid(0, 0) < 0) - errExit("setreuid/setregid"); - errno = 0; + EUID_ROOT(); + // FTW_PHYS - do not follow symbolic links + if (nftw(".", remove_callback, 64, FTW_DEPTH | FTW_PHYS) == -1) + errExit("nftw"); - // FTW_PHYS - do not follow symbolic links - return nftw(path, remove_callback, 64, FTW_DEPTH | FTW_PHYS); + EUID_USER(); + // remove ~/.firejail + if (rmdir(path) == -1) + errExit("rmdir"); +#ifdef HAVE_GCOV + __gcov_flush(); +#endif + _exit(0); + } + // wait for the child to finish + waitpid(child, NULL, 0); + // check if ~/.firejail was deleted + if (stat(path, &s) == -1) + return 0; + else + return 1; + } + return 0; } void flush_stdin(void) { diff --git a/src/include/common.h b/src/include/common.h index 413cf98a5..a80ad4688 100644 --- a/src/include/common.h +++ b/src/include/common.h @@ -32,7 +32,7 @@ #include #include -#define errExit(msg) do { char msgout[500]; sprintf(msgout, "Error %s: %s:%d %s", msg, __FILE__, __LINE__, __FUNCTION__); perror(msgout); exit(1);} while (0) +#define errExit(msg) do { char msgout[500]; snprintf(msgout, 500, "Error %s: %s:%d %s", msg, __FILE__, __LINE__, __FUNCTION__); perror(msgout); exit(1);} while (0) // macro to print ip addresses in a printf statement #define PRINT_IP(A) \ diff --git a/src/man/firejail.txt b/src/man/firejail.txt index 7de1bff50..d7e402e31 100644 --- a/src/man/firejail.txt +++ b/src/man/firejail.txt @@ -1267,7 +1267,7 @@ Similar to \-\-output, but stderr is also stored. \fB\-\-overlay Mount a filesystem overlay on top of the current filesystem. Unlike the regular filesystem container, the system directories are mounted read-write. All filesystem modifications go into the overlay. -The overlay is stored in $HOME/.firejail/ directory. +Directories /run, /tmp and /dev are not covered by the overlay. The overlay is stored in $HOME/.firejail/ directory. .br .br @@ -1285,8 +1285,8 @@ $ firejail \-\-overlay firefox \fB\-\-overlay-named=name Mount a filesystem overlay on top of the current filesystem. Unlike the regular filesystem container, the system directories are mounted read-write. All filesystem modifications go into the overlay. -The overlay is stored in $HOME/.firejail/ directory. The created overlay can be reused between multiple -sessions. +Directories /run, /tmp and /dev are not covered by the overlay. The overlay is stored in $HOME/.firejail/ directory. +The created overlay can be reused between multiple sessions. .br .br @@ -1303,7 +1303,8 @@ $ firejail \-\-overlay-named=jail1 firefox .TP \fB\-\-overlay-tmpfs Mount a filesystem overlay on top of the current filesystem. All filesystem modifications -are discarded when the sandbox is closed. +are discarded when the sandbox is closed. Directories /run, /tmp and /dev are not covered by the overlay. + .br .br -- cgit v1.2.3-70-g09d2 From f10fead1c28d1b6cea48836d90b5fe9d05a1c391 Mon Sep 17 00:00:00 2001 From: Vincent43 <31109921+Vincent43@users.noreply.github.com> Date: Tue, 28 Aug 2018 17:37:19 +0100 Subject: spotify.profile: allow /etc/hosts --- etc/spotify.profile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/etc/spotify.profile b/etc/spotify.profile index 4e2718c95..3adf3183c 100644 --- a/etc/spotify.profile +++ b/etc/spotify.profile @@ -45,7 +45,7 @@ tracelog disable-mnt private-bin spotify,bash,sh,zenity private-dev -private-etc fonts,group,ld.so.cache,machine-id,pulse,resolv.conf,ca-certificates,ssl,pki,crypto-policies +private-etc fonts,group,ld.so.cache,machine-id,pulse,resolv.conf,hosts,nsswitch.conf,host.conf,ca-certificates,ssl,pki,crypto-policies private-opt spotify private-tmp -- cgit v1.2.3-70-g09d2