Add Landlock support to Firejail #5315

ChrysoliteAzalea · 2022-08-15T08:46:10Z

Hello everyone!

In Linux kernel 5.13 version, a new LSM was added called "Landlock" that allows unprivileged processes to impose filesystem access self-restrictions (similar to the "unveil" in OpenBSD). It's similar to another Linux kernel feature -- seccomp, that allows unprivileged processes to impose system call access self-restrictions. I propose using this LSM in Firejail, and I've added support for four options (for both command-line and profiles) that create and populate the ruleset. I think it's a feature worth having, if we plan to make Firejail usable without SUID bit.

Landlock support was proposed in issues #3992 and #5269.

Enabling Landlock self-restriction requires a process to have either CAP_SYS_ADMIN in an effective capability set, or the "No New Privileges" enabled.

~~P.S. I'd like to note, that this PR uses my tinyLL library that has to be built before the version of Firejail in this PR, and has to be located in a directory like /usr/lib.~~ Necessary functions have been added to Firejail's code.

…pendency on tinyLL

rusty-snake

My two cents on the cli/profile API:

What about dot namespaces command like we have for caps/seccomp? So instead of landlock-read it would be landlock.read?
write and restricted-write should have the opposite logic. This way users will tend to use write if they want to allow writing most of the time.
- restricted-write -> write
- write -> write-any (or something else, if anybody has a godd idea)

src/zsh_completion/_firejail.in

src/man/firejail.txt

Co-authored-by: rusty-snake <41237666+rusty-snake@users.noreply.github.com>

ChrysoliteAzalea · 2022-08-15T16:23:15Z

What about dot namespaces command like we have for caps/seccomp? So instead of landlock-read it would be landlock.read?

I wanted to do it initially, but I ran into a problem with Bash completion, that it would complete it as "--landlock ", but there is no such option as "--landlock" (unlike --caps and --seccomp). If it isn't a problem, I'd change it back.

rusty-snake · 2022-08-15T16:34:35Z

We could add a --landlock command which would, for example, add a basic restrictions mode (only /usr, /bin, /lib, /sbin, /etc, /home/$USER).

Anyway I know the problem. If you use command-line options other than A-Za-z0-9_- programs with special handling (bash-completion, options lists in reStructuredText, ...) make trouble.

I think it's ok then to leave it as is.

ChrysoliteAzalea · 2022-08-16T07:13:06Z

I've worked on this pull request. What's changed:

landlock-read has been changed to landlock.read, and so on
landlock.restricted-write has been removed, landlock.write now no longer implies permission to create FIFO pipes, Unix-domain sockets and block devices
landlock.special has been added in order to manage the creation of FIFO pipes, Unix-domain sockets and block devices
landlock option has been added in order to populate the ruleset with a basic set of rules
Access to the /proc directory is managed separately -- with the landlock.proc option, which, instead of adding the rule immediately (like other options), schedules the addition of the permitting rule after this directory is set up
If writable-etc is not set, access to the /etc is automatically implied
The file descriptor closing function has been modified so it no longer closes the file descriptor of the Landlock ruleset. It's automatically closed by the landlock_restrict_self wrapper function
The Landlock ruleset self-imposition has been moved and now happens after file descriptors are closed, environment is set, but before seccomp filters are loaded into the kernel
The landlock_restrict_self function now automatically enables the "No New Privileges" restriction

kmk3

Note: I also have comments about some high-level things; this review is mostly
to leave a "request for changes" status.

kmk3 · 2022-08-16T11:49:35Z

src/firejail/landlock.c

+#include <sys/types.h>
+#include <sys/prctl.h>
+#include <linux/prctl.h>
+#include <linux/landlock.h>


This is breaking the build on systems that do not support Landlock (such as on
Ubuntu 20.04, which uses an older kernel), even when Landlock is not enabled.

Partial error log of the codeql analyze job from
https://github.com/netblue30/firejail/runs/7853807421:

gcc -g -O2 -ggdb -O2 -DVERSION='"0.9.71"' -DPREFIX='"/usr"' -DSYSCONFDIR='"/etc/firejail"' -DLIBDIR='"/usr/lib"' -DBINDIR='"/usr/bin"' -DVARDIR='"/var/lib/firejail"' -DHAVE_OUTPUT -DHAVE_X11 -DHAVE_PRIVATE_HOME -DHAVE_USERTMPFS -DHAVE_DBUSPROXY -DHAVE_GLOBALCFG -DHAVE_CHROOT -DHAVE_NETWORK -DHAVE_USERNS -DHAVE_FILE_TRANSFER -DHAVE_SUID -fstack-protector-all -D_FORTIFY_SOURCE=2 -fPIE -Wformat -Wformat-security -fstack-clash-protection -fstack-protector-strong -c landlock.c -o landlock.o landlock.c:11:10: fatal error: linux/landlock.h: No such file or directory 11 | #include <linux/landlock.h> | ^~~~~~~~~~~~~~~~~~ compilation terminated. make[1]: *** [Makefile:8: landlock.o] Error 1 make[1]: Leaving directory '/home/runner/work/firejail/firejail/src/firejail' make: *** [Makefile:31: src/firejail/firejail] Error 2

See src/include/gcov_wrapper.h and try to create a landlock_wrapper.h in a
similar fashion. That is:

In landlock_wrapper.h, declare dummy functions if HAVE_LANDLOCK is not
defined.

Outside of landlock_wrapper.h, only include "landlock_wrapper.h" instead
of <linux/landlock.h>.

Outside of landlock_wrapper.h, avoid using #ifdef HAVE_LANDLOCK where it
can be helped (just call the landlock functions as if LandLock is always
enabled).

kmk3 · 2022-08-16T11:50:56Z

contrib/vim/syntax/firejail.vim

@@ -52,7 +52,7 @@ syn match fjVar /\v\$\{(CFG|DESKTOP|DOCUMENTS|DOWNLOADS|HOME|MUSIC|PATH|PICTURES

 " Commands grabbed from: src/firejail/profile.c
 " Generate list with: { rg -o 'strn?cmp\(ptr, "([^"]+) "' -r '$1' src/firejail/profile.c; echo private-lib; } | grep -vEx '(include|ignore|caps\.drop|caps\.keep|protocol|restrict-namespaces|seccomp|seccomp\.drop|seccomp\.keep|env|rmenv|net|ip)' | sort -u | tr $'\n' '|' # private-lib is special-cased in the code and doesn't match the regex; grep-ed patterns are handled later with 'syn match nextgroup=' directives (except for include which is special-cased as a fjCommandNoCond keyword)
-syn match fjCommand /\v(apparmor|bind|blacklist|blacklist-nolog|cpu|defaultgw|dns|hostname|hosts-file|ip6|iprange|join-or-start|mac|mkdir|mkfile|mtu|name|netfilter|netfilter6|netmask|nice|noblacklist|noexec|nowhitelist|overlay-named|private|private-bin|private-cwd|private-etc|private-home|private-lib|private-opt|private-srv|read-only|read-write|rlimit-as|rlimit-cpu|rlimit-fsize|rlimit-nofile|rlimit-nproc|rlimit-sigpending|timeout|tmpfs|veth-name|whitelist|xephyr-screen) / skipwhite contained
+syn match fjCommand /\v(apparmor|bind|blacklist|blacklist-nolog|cpu|defaultgw|dns|hostname|hosts-file|ip6|iprange|join-or-start|landlock|landlock.proc|landlock.read|landlock.write|landlock.special|landlock.execute|mac|mkdir|mkfile|mtu|name|netfilter|netfilter6|netmask|nice|noblacklist|noexec|nowhitelist|overlay-named|private|private-bin|private-cwd|private-etc|private-home|private-lib|private-opt|private-srv|read-only|read-write|rlimit-as|rlimit-cpu|rlimit-fsize|rlimit-nofile|rlimit-nproc|rlimit-sigpending|timeout|tmpfs|veth-name|whitelist|xephyr-screen) / skipwhite contained


This should be sorted by name.

kmk3 · 2022-08-16T11:53:10Z

src/firejail/firejail.h

+#ifdef HAVE_LANDLOCK
+
+extern int landlock_create_ruleset(struct landlock_ruleset_attr *rsattr,size_t size,__u32 flags);
+
+extern int landlock_add_rule(int fd,enum landlock_rule_type t,void *attr,__u32 flags);
+
+extern int landlock_restrict_self(int fd,__u32 flags);
+
+extern int create_full_ruleset();
+
+extern int add_read_access_rule_by_path(int rset_fd,char *allowed_path);

+extern int add_write_access_rule_by_path(int rset_fd,char *allowed_path);
+
+extern int add_create_special_rule_by_path(int rset_fd,char *allowed_path);
+
+extern int add_execute_rule_by_path(int rset_fd,char *allowed_path);
+
+#endif


Remove the #ifdef (see the new gcov comment) and extraneous blank lines (see
the rest of the file).

kmk3 · 2022-08-16T11:58:44Z

src/firejail/landlock.c

+	if (result!=0) return result;
+	else {
+		close(fd);
+		return 0;
+	}


Suggested change

if (result!=0) return result;

else {

close(fd);

return 0;

}

if (result != 0)

return result;

else {

close(fd);

return 0;

}

if () foo is done nowhere in the code AFAIK.

The same applies to other if () foo changes in this PR.

kmk3 · 2022-08-16T12:00:42Z

src/firejail/sandbox.c

@@ -1093,9 +1113,12 @@ int sandbox(void* sandbox_arg) {
 	//****************************
 	// rebuild etc directory, set dns
 	//****************************
-	if (!arg_writable_etc)
+	if (!arg_writable_etc){


Suggested change

if (!arg_writable_etc){

if (!arg_writable_etc) {

kmk3 · 2022-08-16T12:11:13Z

src/firejail/landlock.c

+
+int create_full_ruleset() {
+	struct landlock_ruleset_attr attr;
+	attr.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE | LANDLOCK_ACCESS_FS_READ_DIR | LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_REMOVE_FILE | LANDLOCK_ACCESS_FS_REMOVE_DIR | LANDLOCK_ACCESS_FS_MAKE_CHAR | LANDLOCK_ACCESS_FS_MAKE_DIR | LANDLOCK_ACCESS_FS_MAKE_REG | LANDLOCK_ACCESS_FS_MAKE_SOCK | LANDLOCK_ACCESS_FS_MAKE_FIFO | LANDLOCK_ACCESS_FS_MAKE_BLOCK | LANDLOCK_ACCESS_FS_MAKE_SYM | LANDLOCK_ACCESS_FS_EXECUTE;


kmk3 · 2022-08-16T12:15:53Z

src/firejail/firejail.h

+#ifdef HAVE_LANDLOCK
+extern int arg_landlock; // Landlock ruleset file descriptor
+extern int arg_landlock_proc;				// Landlock rule for accessing /proc (0 for no access, 1 for read-only and 2 for read-write)
+#endif


Remove the #ifdef and move them below this line (see the new gcov comment):

extern char *apparmor_profile; // apparmor profile

It was very confusing to see an arg_ variable refer to a data structure,
rather than a flag or something that is actually specified on the command line.

I thought that it meant the path argument that is eventually passed to a
landlock call (such as the PATH in landlock.foo PATH) and only noticed that
that is not the case because argv[i] + N is also passed to the landlock calls
in main.c:

+ else if (strncmp(argv[i], "--landlock.read=", 16) == 0) { + if (arg_landlock == -1) + arg_landlock = create_full_ruleset(); + if (add_read_access_rule_by_path(arg_landlock, argv[i] + 16)) { + fprintf(stderr, "An error has occured while adding a rule to the Landlock ruleset.\n"); + } + }

Suggestion: Avoid being too clever and just use separate variables. Use
arg_landlock to check if --landlock is used (for example, in sandbox.c) and
add a new landlock_ruleset_fd variable to store the file descriptor (note
that this variable name matches the kernel API):

extern int arg_apparmor; // apparmor extern char *apparmor_profile; // apparmor profile +extern int arg_landlock; // Landlock +extern int arg_landlock_proc; // Landlock rule for accessing /proc (0 for no access, 1 for read-only and 2 for read-write) +extern int landlock_ruleset_fd; // Landlock ruleset file descriptor

Example usage on main.c:

else if (strcmp(argv[i], "--landlock") == 0) { + arg_landlock = 1; + - if (arg_landlock == -1) - arg_landlock = create_full_ruleset(); + if (landlock_ruleset_fd < 0) { + landlock_ruleset_fd = create_full_ruleset(); + if (landlock_ruleset_fd < 0) + errExit("create_full_ruleset"); + }

The same applies to the other --landlock arguments (other than maybe setting
arg_landlock = 1).

kmk3 · 2022-08-16T12:18:20Z

src/firejail/landlock.c

+	struct landlock_path_beneath_attr target;
+	target.parent_fd = allowed_fd;
+	target.allowed_access = LANDLOCK_ACCESS_FS_READ_FILE | LANDLOCK_ACCESS_FS_READ_DIR;
+	result = landlock_add_rule(rset_fd,LANDLOCK_RULE_PATH_BENEATH,&target,0);


There are many whitespace issues. See this again:

Add support for custom AppArmor profiles (--apparmor=) #5274 (comment)

rusty-snake · 2022-08-16T15:41:28Z

src/man/firejail.txt

+#ifdef HAVE_LANDLOCK
+.SH LANDLOCK
+.TP
+Landlock is a Linux security module first introduced in the 5.13 version of Linux kernel. It allows unprivileged processes to restrict their access to the filesystem. Once imposed, these restrictions can never be removed, and all child processes created by a Landlock-restricted processes inherit these restrictions. Firejail supports Landlock as an additional sandboxing feature. It can be used to ensure that a sandboxed application can only access files and directories that it was explicitly allowed to access. Firejail supports populating the ruleset with both basic set of rules and with custom set of rules. Basic set of rules allows read-only access to /bin, /dev, /etc, /lib, /opt, /proc, /usr and /var, read-write access to the home directory, and allows execution of binaries located in /bin, /opt and /usr.


Suggested change

Landlock is a Linux security module first introduced in the 5.13 version of Linux kernel. It allows unprivileged processes to restrict their access to the filesystem. Once imposed, these restrictions can never be removed, and all child processes created by a Landlock-restricted processes inherit these restrictions. Firejail supports Landlock as an additional sandboxing feature. It can be used to ensure that a sandboxed application can only access files and directories that it was explicitly allowed to access. Firejail supports populating the ruleset with both basic set of rules and with custom set of rules. Basic set of rules allows read-only access to /bin, /dev, /etc, /lib, /opt, /proc, /usr and /var, read-write access to the home directory, and allows execution of binaries located in /bin, /opt and /usr.

Landlock is a Linux security module first introduced in the 5.13 version of

Linux kernel. It allows unprivileged processes to restrict their access to the

filesystem. Once imposed, these restrictions can never be removed, and all child

processes created by a Landlock-restricted processes inherit these restrictions.

Firejail supports Landlock as an additional sandboxing feature. It can be used

to ensure that a sandboxed application can only access files and directories

that it was explicitly allowed to access. Firejail supports populating the

ruleset with both basic set of rules and with custom set of rules. Basic set of

rules allows read-only access to /bin, /dev, /etc, /lib, /opt, /proc, /usr and

/var, read-write access to the home directory, and allows execution of binaries

located in /bin, /opt and /usr.

rusty-snake · 2022-08-16T15:43:43Z

src/man/firejail.txt

+.br
+
+.br
+- A process can install a Landlock ruleset only if it has either \fBCAP_SYS_ADMIN\fR in its effective capability set, or the "No New Privileges" restriction enabled. Because of this, enabling the Landlock feature will also cause Firejail to enable the "No New Privileges" restriction, regardless of the profile or the \fB\-\-no\-new\-privs\fR command line option.


Do we need this technical background? IMHO a "landlock implies NNP" is enough.

rusty-snake · 2022-08-16T15:46:02Z

src/firejail/profile.c

+		if (arg_landlock == -1) arg_landlock = create_full_ruleset();
+		const char *home_dir = env_get("HOME");
+		int home_fd = open(home_dir,O_PATH | O_CLOEXEC);
+		struct landlock_path_beneath_attr target;
+		target.parent_fd = home_fd;
+		target.allowed_access = LANDLOCK_ACCESS_FS_READ_FILE | LANDLOCK_ACCESS_FS_READ_DIR | LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_REMOVE_FILE | LANDLOCK_ACCESS_FS_REMOVE_DIR | LANDLOCK_ACCESS_FS_MAKE_CHAR | LANDLOCK_ACCESS_FS_MAKE_DIR | LANDLOCK_ACCESS_FS_MAKE_REG | LANDLOCK_ACCESS_FS_MAKE_SYM;
+		if (landlock_add_rule(arg_landlock,LANDLOCK_RULE_PATH_BENEATH,&target,0)) {
+			fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+		}
+		close(home_fd);
+		if (add_read_access_rule_by_path(arg_landlock, "/bin/")) {
+			fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+		}
+		if (add_execute_rule_by_path(arg_landlock, "/bin/")) {
+			fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+		}
+		if (add_read_access_rule_by_path(arg_landlock, "/dev/")) {
+			fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+		}
+		if (add_read_access_rule_by_path(arg_landlock, "/etc/")) {
+			fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+		}
+		if (add_read_access_rule_by_path(arg_landlock, "/lib/")) {
+			fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+		}
+		if (add_execute_rule_by_path(arg_landlock, "/lib/")) {
+			fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+		}
+		if (add_read_access_rule_by_path(arg_landlock, "/opt/")) {
+			fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+		}
+		if (add_execute_rule_by_path(arg_landlock, "/opt/")) {
+			fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+		}
+		if (add_read_access_rule_by_path(arg_landlock, "/usr/")) {
+			fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+		}
+		if (add_execute_rule_by_path(arg_landlock, "/usr/")) {
+			fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+		}
+		if (add_read_access_rule_by_path(arg_landlock, "/var/")) {
+			fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+		}


I think it makes sense to move this into a function to 1. avoid duplication and 2. prevent future change from chaning only commandline/profile implementation.

rusty-snake · 2022-08-16T15:49:57Z

src/firejail/landlock.c

+	int allowed_fd = open(allowed_path,O_PATH | O_CLOEXEC);
+	struct landlock_path_beneath_attr target;
+	target.parent_fd = allowed_fd;
+	target.allowed_access = LANDLOCK_ACCESS_FS_WRITE_FILE | LANDLOCK_ACCESS_FS_REMOVE_FILE | LANDLOCK_ACCESS_FS_REMOVE_DIR | LANDLOCK_ACCESS_FS_MAKE_CHAR | LANDLOCK_ACCESS_FS_MAKE_DIR | LANDLOCK_ACCESS_FS_MAKE_REG | LANDLOCK_ACCESS_FS_MAKE_SYM;


Why did you add LANDLOCK_ACCESS_FS_MAKE_CHAR here instead of special? (I want to understand)

This seems to be a mistake. I plan to move it to special

rusty-snake · 2022-08-16T16:03:24Z

src/firejail/profile.c

+		if (add_read_access_rule_by_path(arg_landlock, "/lib/")) {
+			fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+		}
+		if (add_execute_rule_by_path(arg_landlock, "/lib/")) {


Are there distros which use /lib64 as real dir?

rusty-snake · 2022-08-16T16:11:36Z

src/firejail/profile.c

+			else if (strncmp(ptr+14, "ro", 2) == 0) arg_landlock_proc = 1;
+			else if (strncmp(ptr+14, "rw", 2) == 0) arg_landlock_proc = 2;
+			return 0;
+		}


From my experience with crablock I can tell that --landlock.path_beneath="/proc:READ_FILE" is often enough. Maybe it would be interesting to expose this as a read-only-files-but-no-dirs option.

rusty-snake · 2022-08-16T16:12:41Z

contrib/vim/syntax/firejail.vim

@@ -52,7 +52,7 @@ syn match fjVar /\v\$\{(CFG|DESKTOP|DOCUMENTS|DOWNLOADS|HOME|MUSIC|PATH|PICTURES

 " Commands grabbed from: src/firejail/profile.c
 " Generate list with: { rg -o 'strn?cmp\(ptr, "([^"]+) "' -r '$1' src/firejail/profile.c; echo private-lib; } | grep -vEx '(include|ignore|caps\.drop|caps\.keep|protocol|restrict-namespaces|seccomp|seccomp\.drop|seccomp\.keep|env|rmenv|net|ip)' | sort -u | tr $'\n' '|' # private-lib is special-cased in the code and doesn't match the regex; grep-ed patterns are handled later with 'syn match nextgroup=' directives (except for include which is special-cased as a fjCommandNoCond keyword)
-syn match fjCommand /\v(apparmor|bind|blacklist|blacklist-nolog|cpu|defaultgw|dns|hostname|hosts-file|ip6|iprange|join-or-start|mac|mkdir|mkfile|mtu|name|netfilter|netfilter6|netmask|nice|noblacklist|noexec|nowhitelist|overlay-named|private|private-bin|private-cwd|private-etc|private-home|private-lib|private-opt|private-srv|read-only|read-write|rlimit-as|rlimit-cpu|rlimit-fsize|rlimit-nofile|rlimit-nproc|rlimit-sigpending|timeout|tmpfs|veth-name|whitelist|xephyr-screen) / skipwhite contained
+syn match fjCommand /\v(apparmor|bind|blacklist|blacklist-nolog|cpu|defaultgw|dns|hostname|hosts-file|ip6|iprange|join-or-start|landlock|landlock.proc|landlock.read|landlock.write|landlock.special|landlock.execute|mac|mkdir|mkfile|mtu|name|netfilter|netfilter6|netmask|nice|noblacklist|noexec|nowhitelist|overlay-named|private|private-bin|private-cwd|private-etc|private-home|private-lib|private-opt|private-srv|read-only|read-write|rlimit-as|rlimit-cpu|rlimit-fsize|rlimit-nofile|rlimit-nproc|rlimit-sigpending|timeout|tmpfs|veth-name|whitelist|xephyr-screen) / skipwhite contained


landlock takes no value, right?

rusty-snake · 2022-08-16T16:15:39Z

src/firejail/profile.c

+	// Landlock ruleset paths
+	if (strcmp(ptr, "landlock") == 0) {
+		if (arg_landlock == -1) arg_landlock = create_full_ruleset();
+		const char *home_dir = env_get("HOME");


Does this work with --private?

rusty-snake · 2022-08-16T16:17:24Z

src/man/firejail.txt

+- Access to the /proc directory is managed through the \fB\-\-landlock.proc\fR command line option.
+
+.br
+- Access to the /etc directory is automatically allowed. To override this, use the \fB\-\-writable\-etc\fR command line option. You can also use the \fB\-\-private\-etc\fR option to restrict access to the /etc directory.


Note for me: Does this make sense?

When implementing Landlock support, I ran into a problem, that access rules for /proc (and /etc if --writable-etc wasn't supplied) had no effect when added before sandbox setup (access was denied even when I explicitly allowed it with command-line options), so I had to make these rules a part of sandbox setup. This is why --landlock.proc schedules the rule addition instead of adding it immediately.

rusty-snake · 2022-08-16T16:19:13Z

src/man/firejail.txt

+.br
+
+.TP
+To enable Landlock self-restriction on top of your current Firejail security features, pass \fB\-\-landlock\fR flag to Firejail command line. You can also use \fB\-\-landlock.read\fR, \fB\-\-landlock.write\fR, \fB\-\-landlock.special\fR and \fB\-\-landlock.execute\fR options together with \fB\-\-landlock\fR or instead of it. Example:


Maybe make clear that --landlock.* can only allow additional access kinds/paths but not remove access kinds granted by --landlock.

kmk3

Review 2/?.

kmk3 · 2022-08-16T23:04:59Z

src/firejail/landlock.c

+	}
+}
+
+int create_full_ruleset() {


All functions in this file should probably have a landlock_ prefix to make it
clear on the call site that they are about Landlock (rather than being about
something else, like AppArmor).

kmk3 · 2022-08-16T23:58:48Z

src/firejail/main.c

+#ifdef HAVE_LANDLOCK
+		else if (strcmp(argv[i], "--landlock") == 0) {
+			if (arg_landlock == -1) arg_landlock = create_full_ruleset();
+			const char *home_dir = env_get("HOME");


Use cfg.homedir instead.

The same applies to the other env_get("HOME") calls.

kmk3 · 2022-08-17T00:03:34Z

src/firejail/main.c

+			if (landlock_add_rule(arg_landlock,LANDLOCK_RULE_PATH_BENEATH,&target,0)) {
+				fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+			}
+			close(home_fd);
+			if (add_read_access_rule_by_path(arg_landlock, "/bin/")) {
+				fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");
+			}


Suggested change

if (landlock_add_rule(arg_landlock,LANDLOCK_RULE_PATH_BENEATH,&target,0)) {

fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");

}

close(home_fd);

if (add_read_access_rule_by_path(arg_landlock, "/bin/")) {

fprintf(stderr,"An error has occured while adding a rule to the Landlock ruleset.\n");

}

if (landlock_add_rule(arg_landlock, LANDLOCK_RULE_PATH_BENEATH, &target, 0)) {

fprintf(stderr, "Error: cannot add a landlock rule to %s\n: %s",

cfg.homedir, strerror(errno));

}

if (add_read_access_rule_by_path(arg_landlock, "/bin/")) {

fprintf(stderr, "Error: cannot add a landlock rule to /bin: \n",

strerror(errno));

}

Make the error messages match the rest of the code and make them more
informative.

The same applies to the other fprintf calls.

kmk3 · 2022-08-17T05:31:43Z

src/firejail/sandbox.c

+#ifdef HAVE_LANDLOCK
+		// set Landlock
+		if (arg_landlock >= 0) {
+			if (landlock_restrict_self(arg_landlock,0)) {
+				fprintf(stderr,"An error has occured while enabling Landlock self-restriction. Exiting...\n");
+				exit(1); // it isn't safe to continue if Landlock self-restriction was enabled and the "landlock_restrict_self" syscall has failed
+			}
+		}
+#endif
+


Suggested change

#ifdef HAVE_LANDLOCK

// set Landlock

if (arg_landlock >= 0) {

if (landlock_restrict_self(arg_landlock,0)) {

fprintf(stderr,"An error has occured while enabling Landlock self-restriction. Exiting...\n");

exit(1); // it isn't safe to continue if Landlock self-restriction was enabled and the "landlock_restrict_self" syscall has failed

}

}

#endif

// set Landlock

if (arg_landlock >= 0) {

// It isn't safe to continue if Landlock self-restriction was

// enabled and the "landlock_restrict_self" syscall has failed.

if (landlock_restrict_self(arg_landlock, 0) != 0) {

errExit("landlock_restrict_self");

}

}

Simplify and use errExit (see src/include/common.h).

Also, note that the code indentation usually isn't changed by the presence of
#ifdef.

kmk3 · 2022-08-17T05:42:14Z

src/firejail/profile.c

+#ifdef HAVE_LANDLOCK
+	// Landlock ruleset paths
+	if (strcmp(ptr, "landlock") == 0) {
+		if (arg_landlock == -1) arg_landlock = create_full_ruleset();


Suggested change

if (arg_landlock == -1) arg_landlock = create_full_ruleset();

if (arg_landlock == -1) {

arg_landlock = create_full_ruleset();

if (!arg_landlock) {

fprintf(stderr, "Error: failed to create a landlock full ruleset: %s\n",

strerror(errno));

}

}

Check for errors.

The same applies to any other calls of create_full_ruleset.

Also, shouldn't firejail just exit if something like this fails?

If so, use errExit instead of fprintf:

errExit("create_full_ruleset");

kmk3 · 2022-08-17T05:54:06Z

src/firejail/firejail.h

+#ifdef HAVE_LANDLOCK
+extern int arg_landlock; // Landlock ruleset file descriptor
+extern int arg_landlock_proc;				// Landlock rule for accessing /proc (0 for no access, 1 for read-only and 2 for read-write)
+#endif


It was very confusing to see an arg_ variable refer to a data structure,
rather than a flag or something that is actually specified on the command line.

I thought that it meant the path argument that is eventually passed to a
landlock call (such as the PATH in landlock.foo PATH) and only noticed that
that is not the case because argv[i] + N is also passed to the landlock calls
in main.c:

+ else if (strncmp(argv[i], "--landlock.read=", 16) == 0) { + if (arg_landlock == -1) + arg_landlock = create_full_ruleset(); + if (add_read_access_rule_by_path(arg_landlock, argv[i] + 16)) { + fprintf(stderr, "An error has occured while adding a rule to the Landlock ruleset.\n"); + } + }

Suggestion: Avoid being too clever and just use separate variables. Use
arg_landlock to check if --landlock is used (for example, in sandbox.c) and
add a new landlock_ruleset_fd variable to store the file descriptor (note
that this variable name matches the kernel API):

extern int arg_apparmor; // apparmor extern char *apparmor_profile; // apparmor profile +extern int arg_landlock; // Landlock +extern int arg_landlock_proc; // Landlock rule for accessing /proc (0 for no access, 1 for read-only and 2 for read-write) +extern int landlock_ruleset_fd; // Landlock ruleset file descriptor

Example usage on main.c:

else if (strcmp(argv[i], "--landlock") == 0) { + arg_landlock = 1; + - if (arg_landlock == -1) - arg_landlock = create_full_ruleset(); + if (landlock_ruleset_fd < 0) { + landlock_ruleset_fd = create_full_ruleset(); + if (landlock_ruleset_fd < 0) + errExit("create_full_ruleset"); + }

The same applies to the other --landlock arguments (other than maybe setting
arg_landlock = 1).

netblue30 · 2022-08-29T12:44:47Z

Landlock is going in, thanks @ChrysoliteAzalea!

netblue30 · 2022-08-29T12:49:44Z

The build will break for a while on systems without landlock support in the kernel, I'm bringing in a fix - is fixed for now!

…ock" This reverts commit 54cb3e7, reversing changes made to 97b1e02. There were many issues and requests for changes raised in the pull request (both code-wise and design-wise) and most of them are still unresolved[1]. [1] netblue30#5315

This reverts commit 836ffe3. Relates to netblue30#5315.

…ock" This reverts commit 54cb3e7, reversing changes made to 97b1e02. There were many issues and requests for changes raised in the pull request (both code-wise and design-wise) and most of them are still unresolved[1]. [1] netblue30#5315

ChrysoliteAzalea · 2022-08-30T06:49:20Z

This PR was not ready at the time of its merging. I would like to ask for this pull-request to be re-opened.

rusty-snake · 2022-08-30T06:55:01Z

Can you reopen a merged PR? I think you can only reopen a closed PR.

… file" This reverts commit 6e687c3. Relates to netblue30#5315.

This reverts commit 836ffe3. Relates to netblue30#5315.

…ock" This reverts commit 54cb3e7, reversing changes made to 97b1e02. There were many issues and requests for changes raised in the pull request (both code-wise and design-wise) and most of them are still unresolved[1]. [1] netblue30#5315

Commands used to find and print the relevant commits: $ TZ=UTC0 git log --date='format-local:%Y-%m-%d' \ --pretty='%H # %cd | %s' 60db9f7~1..60db9f7 | grep 'Revert "'; git log --reverse --pretty=%b 60db9f7~1..60db9f7 | sed -E -n 's/^This reverts commit ([0-9a-z]+).*/\1/p' | TZ=UTC0 xargs git show --date='format-local:%Y-%m-%d' \ --pretty='%H # %cd | %s' -s | grep -v 'Merge pull request'; TZ=UTC0 git log --no-merges --date='format-local:%Y-%m-%d' \ --pretty='%H # %cd | %s' 54cb3e7~1..54cb3e7 Explanation: The first `git log` basically takes the revision range from one commit before a given merge commit until the merge commit itself and prints the commits in that range (which should usually mean all commits that were in the branch that was merged). In this case, it's the commits that do the revert. The second `git log` finds the hash of all commits that were reverted and prints them. The `grep -v` and third `git log` are only needed because the merge commit of the original branch was reverted directly (on commit 97874c3), rather than reverting each individual commit on that branch. So these commands are used to print all of the commits in the original branch. Relates to netblue30#5315 netblue30#5347.

ChrysoliteAzalea added 2 commits August 15, 2022 13:32

Landlock support has been added.

61b1544

Landlock functions are added to the code of Firejail, removing the de…

ba828be

…pendency on tinyLL

ChrysoliteAzalea force-pushed the landlock branch from c7db5d2 to ba828be Compare August 15, 2022 09:36

rusty-snake reviewed Aug 15, 2022

View reviewed changes

src/zsh_completion/_firejail.in Outdated Show resolved Hide resolved

src/man/firejail.txt Outdated Show resolved Hide resolved

src/man/firejail.txt Show resolved Hide resolved

rusty-snake assigned rusty-snake and unassigned rusty-snake Aug 15, 2022

rusty-snake marked this pull request as draft August 15, 2022 15:48

Update quotation marks in src/zsh_completion/_firejail.in

877fc99

Co-authored-by: rusty-snake <41237666+rusty-snake@users.noreply.github.com>

Proposed fixes.

460fa7a

ChrysoliteAzalea marked this pull request as ready for review August 16, 2022 07:13

tinyLL has been removed as it's no longer needed

c6d7474

kmk3 requested changes Aug 16, 2022

View reviewed changes

rusty-snake reviewed Aug 16, 2022

View reviewed changes

kmk3 requested changes Aug 17, 2022

View reviewed changes

netblue30 merged commit 54cb3e7 into netblue30:master Aug 29, 2022

kmk3 added a commit to kmk3/firejail that referenced this pull request Aug 29, 2022

Revert "compile fix"

110aa04

This reverts commit 836ffe3. Relates to netblue30#5315.

kmk3 mentioned this pull request Aug 29, 2022

Revert "Add Landlock support to Firejail" #5347

Merged

kmk3 added a commit to kmk3/firejail that referenced this pull request Aug 31, 2022

Revert "tracelog disabled by default in /etc/firejail/firejail.config…

1dd40c8

… file" This reverts commit 6e687c3. Relates to netblue30#5315.

kmk3 added a commit to kmk3/firejail that referenced this pull request Aug 31, 2022

Revert "compile fix"

6f09745

This reverts commit 836ffe3. Relates to netblue30#5315.

rusty-snake mentioned this pull request Sep 1, 2022

landlock: Leftover from #5315 #5354

Open

14 tasks

kmk3 mentioned this pull request Sep 4, 2022

ci: GitLab CI is broken (autoreconf) #5358

Closed

kmk3 added the enhancement New feature request label Sep 6, 2022

kmk3 mentioned this pull request Sep 19, 2022

.git-blame-ignore-revs: change format and add Landlock v1 #5376

Merged

kmk3 mentioned this pull request Nov 4, 2023

feature: add Landlock support #6078

Merged

-		if (arg_landlock == -1) arg_landlock = create_full_ruleset();
+		if (arg_landlock == -1) {
+			arg_landlock = create_full_ruleset();
+			if (!arg_landlock) {
+				fprintf(stderr, "Error: failed to create a landlock full ruleset: %s\n",
+					strerror(errno));
+			}
+		}

Add Landlock support to Firejail #5315

Add Landlock support to Firejail #5315

Conversation

ChrysoliteAzalea commented Aug 15, 2022 • edited Loading

rusty-snake left a comment

Choose a reason for hiding this comment

ChrysoliteAzalea commented Aug 15, 2022

rusty-snake commented Aug 15, 2022

ChrysoliteAzalea commented Aug 16, 2022

kmk3 left a comment

Choose a reason for hiding this comment

kmk3 Aug 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmk3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmk3 Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netblue30 commented Aug 29, 2022

netblue30 commented Aug 29, 2022 • edited Loading

ChrysoliteAzalea commented Aug 30, 2022

rusty-snake commented Aug 30, 2022

ChrysoliteAzalea commented Aug 15, 2022 •

edited

Loading

kmk3 Aug 16, 2022 •

edited

Loading

kmk3 Aug 17, 2022 •

edited

Loading

netblue30 commented Aug 29, 2022 •

edited

Loading