From 00b6315c91893c3316686fa9d71350690d506b56 Mon Sep 17 00:00:00 2001 From: netblue30 Date: Wed, 31 Jan 2018 09:22:36 -0500 Subject: overlay fixes and additional hardening --- src/firejail/firejail.h | 2 +- src/firejail/fs.c | 5 +++-- src/firejail/main.c | 27 +++++++++++++++++---------- src/firejail/profile.c | 12 ++++++++++++ src/firejail/util.c | 46 ++++++++++++++++++++++++++++++++++++++++------ 5 files changed, 73 insertions(+), 19 deletions(-) (limited to 'src') 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); } -- cgit v1.2.3-54-g00ecf