From b1de742a08cccb5f3ae7e2a8fa851aa0059c92f4 Mon Sep 17 00:00:00 2001 From: Aleksey Manevich Date: Tue, 23 Aug 2016 10:00:31 +0300 Subject: remove unneeded chown --- src/firejail/appimage.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/firejail/appimage.c b/src/firejail/appimage.c index db9382dc3..37e3de5d8 100644 --- a/src/firejail/appimage.c +++ b/src/firejail/appimage.c @@ -39,15 +39,20 @@ void appimage_set(const char *appimage_path) { assert(appimage_path); assert(devloop == NULL); // don't call this twice! EUID_ASSERT(); - + // check appimage_path if (access(appimage_path, R_OK) == -1) { fprintf(stderr, "Error: cannot access AppImage file\n"); exit(1); } - + + // open as user to prevent race condition + int ffd = open(appimage_path, O_RDONLY|O_CLOEXEC); + if (ffd == -1) + errExit("open"); + EUID_ROOT(); - + // find or allocate a free loop device to use int cfd = open("/dev/loop-control", O_RDWR); int devnr = ioctl(cfd, LOOP_CTL_GET_FREE); @@ -59,7 +64,6 @@ void appimage_set(const char *appimage_path) { if (asprintf(&devloop, "/dev/loop%d", devnr) == -1) errExit("asprintf"); - int ffd = open(appimage_path, O_RDONLY|O_CLOEXEC); int lfd = open(devloop, O_RDONLY); if (ioctl(lfd, LOOP_SET_FD, ffd) == -1) { fprintf(stderr, "Error: cannot configure the loopback device\n"); @@ -68,22 +72,21 @@ void appimage_set(const char *appimage_path) { close(lfd); close(ffd); + EUID_USER(); + + // creates directory with perms 0700 char dirname[] = "/tmp/firejail-mnt-XXXXXX"; mntdir = strdup(mkdtemp(dirname)); if (mntdir == NULL) { fprintf(stderr, "Error: cannot create temporary directory\n"); exit(1); } - mkdir(mntdir, 755); - if (chown(mntdir, getuid(), getgid()) == -1) - errExit("chown"); - if (chmod(mntdir, 755) == -1) - errExit("chmod"); char *mode; - if (asprintf(&mode, "mode=755,uid=%d,gid=%d", getuid(), getgid()) == -1) + if (asprintf(&mode, "mode=700,uid=%d,gid=%d", getuid(), getgid()) == -1) errExit("asprintf"); + EUID_ROOT(); if (mount(devloop, mntdir, "iso9660",MS_MGC_VAL|MS_RDONLY, mode) < 0) errExit("mounting appimage"); -- cgit v1.2.3-54-g00ecf From 9e025dab2a228092058d170daa78290a33e626b3 Mon Sep 17 00:00:00 2001 From: Aleksey Manevich Date: Tue, 23 Aug 2016 19:46:43 +0300 Subject: ASSERT_PERMS macros --- src/firejail/appimage.c | 1 + src/firejail/firejail.h | 23 ++++++++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/firejail/appimage.c b/src/firejail/appimage.c index 37e3de5d8..513a5a8a2 100644 --- a/src/firejail/appimage.c +++ b/src/firejail/appimage.c @@ -81,6 +81,7 @@ void appimage_set(const char *appimage_path) { fprintf(stderr, "Error: cannot create temporary directory\n"); exit(1); } + ASSERT_PERMS(mntdir, getuid(), getgid(), 0700); char *mode; if (asprintf(&mode, "mode=700,uid=%d,gid=%d", getuid(), getgid()) == -1) diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h index 8e30e929a..4bc953e24 100644 --- a/src/firejail/firejail.h +++ b/src/firejail/firejail.h @@ -75,6 +75,27 @@ #define DEFAULT_ROOT_PROFILE "server" #define MAX_INCLUDE_LEVEL 6 // include levels in profile files + +#define ASSERT_PERMS(file, uid, gid, mode) \ + do { \ + assert(file);\ + struct stat s;\ + if (stat(file, &s) == -1) errExit("stat");\ + assert(s.st_uid == uid && s.st_gid == gid && (s.st_mode & 07777) == mode);\ + } while (0) +#define ASSERT_PERMS_FD(fd, uid, gid, mode) \ + do { \ + struct stat s;\ + if (stat(fd, &s) == -1) errExit("stat");\ + assert(s.st_uid == uid && s.st_gid == gid && (s.st_mode & 07777) == mode);\ + } while (0) +#define ASSERT_PERMS_STREAM(file, uid, gid, mode) \ + do { \ + int fd = fileno(file);\ + if (fd == -1) errExit("fileno");\ + ASSERT_PERMS_FD(fd, uid, gid, mode);\ + } while (0) + // main.c typedef struct bridge_t { // on the host @@ -386,7 +407,7 @@ void logsignal(int s); void logmsg(const char *msg); void logargs(int argc, char **argv) ; void logerr(const char *msg); -int copy_file(const char *srcname, const char *destname); +int copy_file(const char *srcname, const char *destname, uid_t uid, gid_t gid, mode_t mode); int is_dir(const char *fname); int is_link(const char *fname); char *line_remove_spaces(const char *buf); -- cgit v1.2.3-54-g00ecf From c321020a559a2640799c4144ade6b4e69140e065 Mon Sep 17 00:00:00 2001 From: Aleksey Manevich Date: Wed, 24 Aug 2016 13:14:44 +0300 Subject: tighten secutiry --- src/firejail/fs.c | 120 +++++++++++++--------------------------------- src/firejail/fs_home.c | 25 ++++------ src/firejail/ls.c | 6 +-- src/firejail/pulseaudio.c | 2 +- src/firejail/util.c | 7 ++- src/include/euid_common.h | 6 ++- 6 files changed, 55 insertions(+), 111 deletions(-) diff --git a/src/firejail/fs.c b/src/firejail/fs.c index 6c87df1e9..21ab56bd8 100644 --- a/src/firejail/fs.c +++ b/src/firejail/fs.c @@ -29,16 +29,25 @@ static void fs_rdwr(const char *dir); +static void create_dir_as_root(const char *dir, mode_t mode) { + assert(dir); + if (arg_debug) + printf("Creating %s directory\n", dir); + + if (mkdir(dir, mode) == -1) + errExit("mkdir"); + + ASSERT_PERMS(dir, 0, 0, mode); +} + static void create_empty_dir(void) { struct stat s; if (stat(RUN_RO_DIR, &s)) { /* coverity[toctou] */ - int rv = mkdir(RUN_RO_DIR, S_IRUSR | S_IXUSR); - if (rv == -1) - errExit("mkdir"); - if (chown(RUN_RO_DIR, 0, 0) < 0) - errExit("chown"); + if (mkdir(RUN_RO_DIR, S_IRUSR | S_IXUSR) == -1) + errExit("mkdir"); + ASSERT_PERMS(RUN_RO_DIR, 0, 0, S_IRUSR | S_IXUSR); } } @@ -50,11 +59,16 @@ static void create_empty_file(void) { FILE *fp = fopen(RUN_RO_FILE, "w"); if (!fp) errExit("fopen"); - fclose(fp); - if (chown(RUN_RO_FILE, 0, 0) < 0) + + int fd = fileno(fp); + if (fd == -1) + errExit("fileno"); + if (fchown(fd, 0, 0) < 0) errExit("chown"); - if (chmod(RUN_RO_FILE, S_IRUSR) < 0) + if (fchmod(fd, S_IRUSR) < 0) errExit("chown"); + + fclose(fp); } } @@ -64,16 +78,7 @@ void fs_build_firejail_dir(void) { // CentOS 6 doesn't have /run directory if (stat(RUN_FIREJAIL_BASEDIR, &s)) { - if (arg_debug) - printf("Creating %s directory\n", RUN_FIREJAIL_BASEDIR); - /* coverity[toctou] */ - int rv = mkdir(RUN_FIREJAIL_BASEDIR, 0755); - if (rv == -1) - errExit("mkdir"); - if (chown(RUN_FIREJAIL_BASEDIR, 0, 0) < 0) - errExit("chown"); - if (chmod(RUN_FIREJAIL_BASEDIR, 0755) < 0) - errExit("chmod"); + create_dir_as_root(RUN_FIREJAIL_BASEDIR, 0755); } else { // check /tmp/firejail directory belongs to root end exit if doesn't! if (s.st_uid != 0 || s.st_gid != 0) { @@ -83,61 +88,23 @@ void fs_build_firejail_dir(void) { } if (stat(RUN_FIREJAIL_DIR, &s)) { - if (arg_debug) - printf("Creating %s directory\n", RUN_FIREJAIL_DIR); - /* coverity[toctou] */ - int rv = mkdir(RUN_FIREJAIL_DIR, 0755); - if (rv == -1) - errExit("mkdir"); - if (chown(RUN_FIREJAIL_DIR, 0, 0) < 0) - errExit("chown"); - if (chmod(RUN_FIREJAIL_DIR, 0755) < 0) - errExit("chmod"); + create_dir_as_root(RUN_FIREJAIL_DIR, 0755); } if (stat(RUN_FIREJAIL_NETWORK_DIR, &s)) { - if (arg_debug) - printf("Creating %s directory\n", RUN_FIREJAIL_NETWORK_DIR); - - if (mkdir(RUN_FIREJAIL_NETWORK_DIR, 0755) == -1) - errExit("mkdir"); - if (chown(RUN_FIREJAIL_NETWORK_DIR, 0, 0) < 0) - errExit("chown"); - if (chmod(RUN_FIREJAIL_NETWORK_DIR, 0755) < 0) - errExit("chmod"); + create_dir_as_root(RUN_FIREJAIL_NETWORK_DIR, 0755); } if (stat(RUN_FIREJAIL_BANDWIDTH_DIR, &s)) { - if (arg_debug) - printf("Creating %s directory\n", RUN_FIREJAIL_BANDWIDTH_DIR); - if (mkdir(RUN_FIREJAIL_BANDWIDTH_DIR, 0755) == -1) - errExit("mkdir"); - if (chown(RUN_FIREJAIL_BANDWIDTH_DIR, 0, 0) < 0) - errExit("chown"); - if (chmod(RUN_FIREJAIL_BANDWIDTH_DIR, 0755) < 0) - errExit("chmod"); + create_dir_as_root(RUN_FIREJAIL_BANDWIDTH_DIR, 0755); } if (stat(RUN_FIREJAIL_NAME_DIR, &s)) { - if (arg_debug) - printf("Creating %s directory\n", RUN_FIREJAIL_NAME_DIR); - if (mkdir(RUN_FIREJAIL_NAME_DIR, 0755) == -1) - errExit("mkdir"); - if (chown(RUN_FIREJAIL_NAME_DIR, 0, 0) < 0) - errExit("chown"); - if (chmod(RUN_FIREJAIL_NAME_DIR, 0755) < 0) - errExit("chmod"); + create_dir_as_root(RUN_FIREJAIL_NAME_DIR, 0755); } if (stat(RUN_FIREJAIL_X11_DIR, &s)) { - if (arg_debug) - printf("Creating %s directory\n", RUN_FIREJAIL_X11_DIR); - if (mkdir(RUN_FIREJAIL_X11_DIR, 0755) == -1) - errExit("mkdir"); - if (chown(RUN_FIREJAIL_X11_DIR, 0, 0) < 0) - errExit("chown"); - if (chmod(RUN_FIREJAIL_X11_DIR, 0755) < 0) - errExit("chmod"); + create_dir_as_root(RUN_FIREJAIL_X11_DIR, 0755); } create_empty_dir(); @@ -160,16 +127,7 @@ void fs_build_mnt_dir(void) { // create /run/firejail/mnt directory if (stat(RUN_MNT_DIR, &s)) { - if (arg_debug) - printf("Creating %s directory\n", RUN_MNT_DIR); - /* coverity[toctou] */ - int rv = mkdir(RUN_MNT_DIR, 0755); - if (rv == -1) - errExit("mkdir"); - if (chown(RUN_MNT_DIR, 0, 0) < 0) - errExit("chown"); - if (chmod(RUN_MNT_DIR, 0755) < 0) - errExit("chmod"); + create_dir_as_root(RUN_MNT_DIR, 0755); } // ... and mount tmpfs on top of it @@ -202,16 +160,12 @@ void fs_build_cp_command(void) { fprintf(stderr, "Error: invalid /bin/cp file\n"); exit(1); } - int rv = copy_file(fname, RUN_CP_COMMAND); + int rv = copy_file(fname, RUN_CP_COMMAND, 0, 0, 0755); if (rv) { fprintf(stderr, "Error: cannot access /bin/cp\n"); exit(1); } - /* coverity[toctou] */ - if (chown(RUN_CP_COMMAND, 0, 0)) - errExit("chown"); - if (chmod(RUN_CP_COMMAND, 0755)) - errExit("chmod"); + ASSERT_PERMS(RUN_CP_COMMAND, 0, 0, 0755); free(fname); } @@ -827,10 +781,7 @@ char *fs_check_overlay_dir(const char *subdirname, int allow_reuse) { /* coverity[toctou] */ if (mkdir(dirname, 0700)) errExit("mkdir"); - if (chown(dirname, getuid(), getgid()) < 0) - errExit("chown"); - if (chmod(dirname, 0700) < 0) - errExit("chmod"); + ASSERT_PERMS(dirname, getuid(), getgid(), 0700); } else if (is_link(dirname)) { fprintf(stderr, "Error: invalid ~/.firejail directory\n"); @@ -917,10 +868,7 @@ void fs_overlayfs(void) { errExit("asprintf"); if (mkdir(oroot, 0755)) errExit("mkdir"); - if (chown(oroot, 0, 0) < 0) - errExit("chown"); - if (chmod(oroot, 0755) < 0) - errExit("chmod"); + ASSERT_PERMS(oroot, 0, 0, 0755); struct stat s; char *basedir = RUN_MNT_DIR; @@ -1259,7 +1207,7 @@ void fs_chroot(const char *rootdir) { fprintf(stderr, "Error: invalid %s file\n", fname); exit(1); } - if (copy_file("/etc/resolv.conf", fname) == -1) + if (copy_file("/etc/resolv.conf", fname, 0, 0, 0644) == -1) fprintf(stderr, "Warning: /etc/resolv.conf not initialized\n"); } diff --git a/src/firejail/fs_home.c b/src/firejail/fs_home.c index d328d5f1c..75d69e021 100644 --- a/src/firejail/fs_home.c +++ b/src/firejail/fs_home.c @@ -43,9 +43,7 @@ static void skel(const char *homedir, uid_t u, gid_t g) { if (stat(fname, &s) == 0) return; if (stat("/etc/skel/.zshrc", &s) == 0) { - if (copy_file("/etc/skel/.zshrc", fname) == 0) { - if (chown(fname, u, g) == -1) - errExit("chown"); + if (copy_file("/etc/skel/.zshrc", fname, u, g, 0644) == 0) { fs_logger("clone /etc/skel/.zshrc"); } } @@ -73,9 +71,7 @@ static void skel(const char *homedir, uid_t u, gid_t g) { if (stat(fname, &s) == 0) return; if (stat("/etc/skel/.cshrc", &s) == 0) { - if (copy_file("/etc/skel/.cshrc", fname) == 0) { - if (chown(fname, u, g) == -1) - errExit("chown"); + if (copy_file("/etc/skel/.cshrc", fname, u, g, 0644) == 0) { fs_logger("clone /etc/skel/.cshrc"); } } @@ -104,10 +100,7 @@ static void skel(const char *homedir, uid_t u, gid_t g) { if (stat(fname, &s) == 0) return; if (stat("/etc/skel/.bashrc", &s) == 0) { - if (copy_file("/etc/skel/.bashrc", fname) == 0) { - /* coverity[toctou] */ - if (chown(fname, u, g) == -1) - errExit("chown"); + if (copy_file("/etc/skel/.bashrc", fname, u, g, 0644) == 0) { fs_logger("clone /etc/skel/.bashrc"); } } @@ -131,7 +124,7 @@ static int store_xauthority(void) { exit(1); } - int rv = copy_file(src, dest); + int rv = copy_file(src, dest, -1, -1, 0600); if (rv) { fprintf(stderr, "Warning: cannot transfer .Xauthority in private home directory\n"); return 0; @@ -167,7 +160,7 @@ static int store_asoundrc(void) { free(rp); } - int rv = copy_file(src, dest); + int rv = copy_file(src, dest, -1, -1, -0644); if (rv) { fprintf(stderr, "Warning: cannot transfer .asoundrc in private home directory\n"); return 0; @@ -184,7 +177,7 @@ static void copy_xauthority(void) { char *dest; if (asprintf(&dest, "%s/.Xauthority", cfg.homedir) == -1) errExit("asprintf"); - int rv = copy_file(src, dest); + int rv = copy_file(src, dest, -1, -1, 0600); if (rv) fprintf(stderr, "Warning: cannot transfer .Xauthority in private home directory\n"); else { @@ -207,7 +200,7 @@ static void copy_asoundrc(void) { char *dest; if (asprintf(&dest, "%s/.asoundrc", cfg.homedir) == -1) errExit("asprintf"); - int rv = copy_file(src, dest); + int rv = copy_file(src, dest, -1 , -1, 0644); if (rv) fprintf(stderr, "Warning: cannot transfer .asoundrc in private home directory\n"); else { @@ -360,11 +353,9 @@ int fs_copydir(const char *path, const struct stat *st, int ftype, struct FTW *s return(0); if (stat(path, &s) == 0) { if(ftype == FTW_F) { - if (copy_file(path, dest) == 0) { + if (copy_file(path, dest, u, g, 0644) == 0) { if (arg_debug) printf("copy from %s to %s\n", path, dest); - if (chown(dest, u, g) == -1) - errExit("chown"); fs_logger2("clone", path); } } diff --git a/src/firejail/ls.c b/src/firejail/ls.c index 09577fb0c..495aaf8e2 100644 --- a/src/firejail/ls.c +++ b/src/firejail/ls.c @@ -374,11 +374,7 @@ void sandboxfs(int op, pid_t pid, const char *path) { } // copy file EUID_ROOT(); - copy_file(src_fname, dest_fname); - if (chown(dest_fname, getuid(), getgid()) == -1) - errExit("chown"); - if (chmod(dest_fname, 0644) == -1) - errExit("chmod"); + copy_file(src_fname, dest_fname, getuid(), getgid(), 0644); printf("Transfer complete\n"); EUID_USER(); } diff --git a/src/firejail/pulseaudio.c b/src/firejail/pulseaudio.c index 908ef1d25..dd26d219c 100644 --- a/src/firejail/pulseaudio.c +++ b/src/firejail/pulseaudio.c @@ -114,7 +114,7 @@ void pulseaudio_init(void) { char *pulsecfg = NULL; if (asprintf(&pulsecfg, "%s/client.conf", RUN_PULSE_DIR) == -1) errExit("asprintf"); - if (copy_file("/etc/pulse/client.conf", pulsecfg)) + if (copy_file("/etc/pulse/client.conf", pulsecfg, -1, -1, 0644)) errExit("copy_file"); FILE *fp = fopen(pulsecfg, "a+"); if (!fp) diff --git a/src/firejail/util.c b/src/firejail/util.c index 24bb71e4c..22434e200 100644 --- a/src/firejail/util.c +++ b/src/firejail/util.c @@ -170,7 +170,7 @@ void logerr(const char *msg) { // return -1 if error, 0 if no error -int copy_file(const char *srcname, const char *destname) { +int copy_file(const char *srcname, const char *destname, uid_t uid, gid_t gid, mode_t mode) { assert(srcname); assert(destname); @@ -207,6 +207,11 @@ int copy_file(const char *srcname, const char *destname) { } } + if (fchown(dst, uid, gid) == -1) + errExit("fchown"); + if (fchmod(dst, mode) == -1) + errExit("fchmod"); + close(src); close(dst); return 0; diff --git a/src/include/euid_common.h b/src/include/euid_common.h index b6d341bf4..de5572fb1 100644 --- a/src/include/euid_common.h +++ b/src/include/euid_common.h @@ -37,11 +37,15 @@ extern uid_t firejail_uid; static inline void EUID_ROOT(void) { if (seteuid(0) == -1) fprintf(stderr, "Warning: cannot switch euid to root\n"); + if (setegid(0) == -1) + fprintf(stderr, "Warning: cannot switch egid to root\n"); } static inline void EUID_USER(void) { if (seteuid(firejail_uid) == -1) - fprintf(stderr, "Warning: cannot switch euid to user\n"); + errExit("seteuid"); + if (setegid(firejail_uid) == -1) + errExit("setegid"); } static inline void EUID_PRINT(void) { -- cgit v1.2.3-54-g00ecf