From 773073c8484914b31ce68cbd635212253bf96f4c Mon Sep 17 00:00:00 2001 From: "Kelvin M. Klann" Date: Wed, 17 Apr 2024 12:56:06 -0300 Subject: refactor: make rundir lock variables global To enable using them outside of src/firejail/main.c. --- src/firejail/firejail.h | 2 ++ src/firejail/main.c | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h index b8ec4d474..e48591903 100644 --- a/src/firejail/firejail.h +++ b/src/firejail/firejail.h @@ -282,6 +282,8 @@ static inline int any_dhcp(void) { return any_ip_dhcp() || any_ip6_dhcp(); } +extern int lockfd_directory; +extern int lockfd_network; extern int arg_private; // mount private /home extern int arg_private_cache; // private home/.cache extern int arg_debug; // print debug messages diff --git a/src/firejail/main.c b/src/firejail/main.c index 0ce18ab01..34d5b091f 100644 --- a/src/firejail/main.c +++ b/src/firejail/main.c @@ -63,6 +63,8 @@ gid_t firejail_gid = 0; static char child_stack[STACK_SIZE] __attribute__((aligned(STACK_ALIGNMENT))); // space for child's stack Config cfg; // configuration +int lockfd_directory = -1; +int lockfd_network = -1; int arg_private = 0; // mount private /home and /tmp directoryu int arg_private_cache = 0; // mount private home/.cache int arg_debug = 0; // print debug messages @@ -1056,8 +1058,6 @@ static int check_postexec(const char *list) { int main(int argc, char **argv, char **envp) { int i; int prog_index = -1; // index in argv where the program command starts - int lockfd_network = -1; - int lockfd_directory = -1; int custom_profile = 0; // custom profile loaded int arg_caps_cmdline = 0; // caps requested on command line (used to break out of --chroot) char **ptr; -- cgit v1.2.3-54-g00ecf From 53bc6589745a9f09bbc42a12dd14f9e81b6fc93f Mon Sep 17 00:00:00 2001 From: Simo Piiroinen Date: Wed, 17 Apr 2024 17:02:31 -0300 Subject: modif: improve flock handling Changes: * Centralize flock handling in preproc.c * Add debug and error logging * Abort if anything fails Co-authored-by: Kelvin M. Klann --- src/firejail/firejail.h | 4 +++ src/firejail/main.c | 34 ++++---------------- src/firejail/preproc.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 28 deletions(-) diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h index e48591903..273cebd45 100644 --- a/src/firejail/firejail.h +++ b/src/firejail/firejail.h @@ -431,6 +431,10 @@ int net_get_mac(const char *ifname, unsigned char mac[6]); void net_config_interface(const char *dev, uint32_t ip, uint32_t mask, int mtu); // preproc.c +void preproc_lock_firejail_dir(void); +void preproc_unlock_firejail_dir(void); +void preproc_lock_firejail_network_dir(void); +void preproc_unlock_firejail_network_dir(void); void preproc_build_firejail_dir(void); void preproc_mount_mnt_dir(void); void preproc_clean_run(void); diff --git a/src/firejail/main.c b/src/firejail/main.c index 34d5b091f..f00b46640 100644 --- a/src/firejail/main.c +++ b/src/firejail/main.c @@ -1169,15 +1169,9 @@ int main(int argc, char **argv, char **envp) { preproc_build_firejail_dir(); const char *container_name = env_get("container"); if (!container_name || strcmp(container_name, "firejail")) { - lockfd_directory = open(RUN_DIRECTORY_LOCK_FILE, O_WRONLY | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR); - if (lockfd_directory != -1) { - int rv = fchown(lockfd_directory, 0, 0); - (void) rv; - flock(lockfd_directory, LOCK_EX); - } + preproc_lock_firejail_dir(); preproc_clean_run(); - flock(lockfd_directory, LOCK_UN); - close(lockfd_directory); + preproc_unlock_firejail_dir(); } delete_run_files(getpid()); @@ -2990,12 +2984,7 @@ int main(int argc, char **argv, char **envp) { // check and assign an IP address - for macvlan it will be done again in the sandbox! if (any_bridge_configured()) { EUID_ROOT(); - lockfd_network = open(RUN_NETWORK_LOCK_FILE, O_WRONLY | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR); - if (lockfd_network != -1) { - int rv = fchown(lockfd_network, 0, 0); - (void) rv; - flock(lockfd_network, LOCK_EX); - } + preproc_lock_firejail_network_dir(); if (cfg.bridge0.configured && cfg.bridge0.arg_ip_none == 0) check_network(&cfg.bridge0); @@ -3024,21 +3013,13 @@ int main(int argc, char **argv, char **envp) { // set name and x11 run files EUID_ROOT(); - lockfd_directory = open(RUN_DIRECTORY_LOCK_FILE, O_WRONLY | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR); - if (lockfd_directory != -1) { - int rv = fchown(lockfd_directory, 0, 0); - (void) rv; - flock(lockfd_directory, LOCK_EX); - } + preproc_lock_firejail_dir(); if (cfg.name) set_name_run_file(sandbox_pid); int display = x11_display(); if (display > 0) set_x11_run_file(sandbox_pid, display); - if (lockfd_directory != -1) { - flock(lockfd_directory, LOCK_UN); - close(lockfd_directory); - } + preproc_unlock_firejail_dir(); EUID_USER(); #ifdef HAVE_DBUSPROXY @@ -3276,10 +3257,7 @@ int main(int argc, char **argv, char **envp) { close(parent_to_child_fds[1]); EUID_ROOT(); - if (lockfd_network != -1) { - flock(lockfd_network, LOCK_UN); - close(lockfd_network); - } + preproc_unlock_firejail_network_dir(); EUID_USER(); // lock netfilter firewall diff --git a/src/firejail/preproc.c b/src/firejail/preproc.c index 2c7d4264d..335c4b0ba 100644 --- a/src/firejail/preproc.c +++ b/src/firejail/preproc.c @@ -18,13 +18,96 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ #include "firejail.h" +#include #include #include #include #include +#include static int tmpfs_mounted = 0; +static void preproc_lock_file(const char *path, int *lockfd_ptr) { + assert(path != NULL); + assert(lockfd_ptr != NULL); + + long pid = (long)getpid(); + if (arg_debug) + fprintf(stderr, "pid=%ld: locking %s ...\n", pid, path); + + if (*lockfd_ptr != -1) { + if (arg_debug) + fprintf(stderr, "pid=%ld: already locked %s\n", pid, path); + return; + } + + int lockfd = open(path, O_WRONLY | O_CREAT | O_CLOEXEC, S_IRUSR | S_IWUSR); + if (lockfd == -1) { + fprintf(stderr, "Error: cannot create a lockfile at %s\n", path); + errExit("open"); + } + + if (fchown(lockfd, 0, 0) == -1) { + fprintf(stderr, "Error: cannot chown root:root %s\n", path); + errExit("fchown"); + } + + if (flock(lockfd, LOCK_EX) == -1) { + fprintf(stderr, "Error: cannot lock %s\n", path); + errExit("flock"); + } + + *lockfd_ptr = lockfd; + if (arg_debug) + fprintf(stderr, "pid=%ld: locked %s\n", pid, path); +} + +static void preproc_unlock_file(const char *path, int *lockfd_ptr) { + assert(path != NULL); + assert(lockfd_ptr != NULL); + + long pid = (long)getpid(); + if (arg_debug) + fprintf(stderr, "pid=%ld: unlocking %s ...\n", pid, path); + + int lockfd = *lockfd_ptr; + if (lockfd == -1) { + if (arg_debug) + fprintf(stderr, "pid=%ld: already unlocked %s\n", pid, path); + return; + } + + if (flock(lockfd, LOCK_UN) == -1) { + fprintf(stderr, "Error: cannot unlock %s\n", path); + errExit("flock"); + } + + if (close(lockfd) == -1) { + fprintf(stderr, "Error: cannot close %s\n", path); + errExit("close"); + } + + *lockfd_ptr = -1; + if (arg_debug) + fprintf(stderr, "pid=%ld: unlocked %s\n", pid, path); +} + +void preproc_lock_firejail_dir(void) { + preproc_lock_file(RUN_DIRECTORY_LOCK_FILE, &lockfd_directory); +} + +void preproc_unlock_firejail_dir(void) { + preproc_unlock_file(RUN_DIRECTORY_LOCK_FILE, &lockfd_directory); +} + +void preproc_lock_firejail_network_dir(void) { + preproc_lock_file(RUN_NETWORK_LOCK_FILE, &lockfd_network); +} + +void preproc_unlock_firejail_network_dir(void) { + preproc_unlock_file(RUN_NETWORK_LOCK_FILE, &lockfd_network); +} + // build /run/firejail directory void preproc_build_firejail_dir(void) { struct stat s; -- cgit v1.2.3-54-g00ecf From e53b6d66b1e9fb81e19af74946e63babf90a7dd6 Mon Sep 17 00:00:00 2001 From: Simo Piiroinen Date: Thu, 4 Apr 2024 14:02:29 +0300 Subject: modif: populate /run/firejail while holding flock There are reports of firejail sandboxed applications occasionally taking a long time (12 seconds) to start up. When this happens, it affects all sandboxed applications until the device is rebooted. The reason for the slowdown seems to be a timing hazard in the way remounts under /run/firejail are handled. This gets triggered when multiple firejail processes are launched in parallel as part of user session bring up and results in some, dozens, hundreds, or even thousands of stray /run/firejail/xxx mounts. The amount of mount points then affects every mount operation that is done during sandbox filesystem construction. To stop this from happening, arrange it so that only one firejail process at time is inspecting and/or modifying mountpoints under /run/firejail by doing: 1. Create /run/firejail directory (without locking) 2. Create and obtain a lock for /run/firejail/firejail-run.lock 3. Setup files, directories and mounts under /run/firejail 4. Release /run/firejail/firejail-run.lock --- src/firejail/chroot.c | 5 ++++- src/firejail/firejail.h | 3 ++- src/firejail/main.c | 10 +++++----- src/firejail/preproc.c | 13 ++++++++++++- 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/firejail/chroot.c b/src/firejail/chroot.c index ffa6c8b51..67097852e 100644 --- a/src/firejail/chroot.c +++ b/src/firejail/chroot.c @@ -273,7 +273,10 @@ void fs_chroot(const char *rootdir) { errExit("mounting /proc"); // create all other /run/firejail files and directories - preproc_build_firejail_dir(); + preproc_build_firejail_dir_unlocked(); + preproc_lock_firejail_dir(); + preproc_build_firejail_dir_locked(); + preproc_unlock_firejail_dir(); // update /var directory in order to support multiple sandboxes running on the same root directory // if (!arg_private_dev) diff --git a/src/firejail/firejail.h b/src/firejail/firejail.h index 273cebd45..736af018d 100644 --- a/src/firejail/firejail.h +++ b/src/firejail/firejail.h @@ -435,7 +435,8 @@ void preproc_lock_firejail_dir(void); void preproc_unlock_firejail_dir(void); void preproc_lock_firejail_network_dir(void); void preproc_unlock_firejail_network_dir(void); -void preproc_build_firejail_dir(void); +void preproc_build_firejail_dir_unlocked(void); +void preproc_build_firejail_dir_locked(void); void preproc_mount_mnt_dir(void); void preproc_clean_run(void); diff --git a/src/firejail/main.c b/src/firejail/main.c index f00b46640..acbb4bf38 100644 --- a/src/firejail/main.c +++ b/src/firejail/main.c @@ -1166,13 +1166,13 @@ int main(int argc, char **argv, char **envp) { #endif // build /run/firejail directory structure - preproc_build_firejail_dir(); + preproc_build_firejail_dir_unlocked(); + preproc_lock_firejail_dir(); + preproc_build_firejail_dir_locked(); const char *container_name = env_get("container"); - if (!container_name || strcmp(container_name, "firejail")) { - preproc_lock_firejail_dir(); + if (!container_name || strcmp(container_name, "firejail")) preproc_clean_run(); - preproc_unlock_firejail_dir(); - } + preproc_unlock_firejail_dir(); delete_run_files(getpid()); atexit(clear_atexit); diff --git a/src/firejail/preproc.c b/src/firejail/preproc.c index 335c4b0ba..e0ca2141f 100644 --- a/src/firejail/preproc.c +++ b/src/firejail/preproc.c @@ -109,7 +109,10 @@ void preproc_unlock_firejail_network_dir(void) { } // build /run/firejail directory -void preproc_build_firejail_dir(void) { +// +// Note: This creates the base directory of the rundir lockfile; +// it should be called before preproc_lock_firejail_dir(). +void preproc_build_firejail_dir_unlocked(void) { struct stat s; // CentOS 6 doesn't have /run directory @@ -118,6 +121,14 @@ void preproc_build_firejail_dir(void) { } create_empty_dir_as_root(RUN_FIREJAIL_DIR, 0755); +} + +// build directory hierarchy under /run/firejail +// +// Note: Remounts have timing hazards. This function should +// only be called after acquiring the directory lock via +// preproc_lock_firejail_dir(). +void preproc_build_firejail_dir_locked(void) { create_empty_dir_as_root(RUN_FIREJAIL_NETWORK_DIR, 0755); create_empty_dir_as_root(RUN_FIREJAIL_BANDWIDTH_DIR, 0755); create_empty_dir_as_root(RUN_FIREJAIL_NAME_DIR, 0755); -- cgit v1.2.3-54-g00ecf