setpriv: add landlock support #2628

t-8ch · 2023-12-06T21:17:04Z

landlock [0] is a Linux stackable LSM that can be used by unprivileged processes to build a sandbox around them.

With support for landlock in setpriv users can easily construct a sandbox on-the-fly when executing programs.

[0] https://landlock.io/

Note: This is only an RFC state, missing help, completions and docs.

karelzak · 2023-12-07T10:21:21Z

It would be nice to describe how to use the command line option here.

t-8ch · 2023-12-07T11:26:39Z

My hope was that the unittests provide enough examples for the RFC.
I'll add some docs :-)

Note: the CLI makes most sense with an understanding of landlock itself.
Can I assume this understanding in the docs and only document on how the general concepts map to setpriv?

karelzak · 2023-12-07T13:51:27Z

I have read the PR and the CI test, but I am still not sure about --landlock-{access,rule} relation and syntax for the options' arguments ;-)

karelzak · 2023-12-07T13:55:13Z

I'm always a little bit nervous when I see something so complex like: --landlock-access fs:write --landlock-rule path-beneath:write:/dev/zero. It does not mean it's a wrong idea, but I'd like to see what each of the fragments means ;-)

t-8ch · 2023-12-07T19:11:34Z

Some quick docs, I'll write something better on the weekend:

--landlock-access defines for which aspects landlock will be enabled.
For those aspects all actions are forbidden by default, needing a specific --landlock-rule to allow it.
--landlock-access fs disallows all filesystem access. --landlock-access fs:file-write disallows writes to files and --landlock-access fs:file-write,file-read disallows reads and writes.

--landlock-rule path-beneath:write:/dev/zero allows writes to all paths beneath /dev/zero.

At the moment only filesystem access with path-beneath rules are supported.
But this is being extended.

karelzak · 2023-12-08T10:08:49Z

Thanks, now it makes more sense ;-)

The LANDLOCK_ACCESS_* (and --landlock-access) is a strange name if used to deny access. Having --landlock-deny and --landlock-allow would be more readable, but I understand you want to follow kernel.

include/landlock.h

sys-utils/setpriv-landlock.c

sys-utils/setpriv-landlock.h

karelzak · 2023-12-11T09:32:52Z

Ad license, on Fedora we have a licensecheck package; you can use licensecheck --shortname-scheme spdx -r $DIR to get an overview.

landlock [0] is a Linux stackable LSM that can be used by unprivileged processes to build a sandbox around them. With support for landlock in setpriv users can easily construct a sandbox on-the-fly when executing programs. [0] https://landlock.io/ Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>

gnoack · 2023-12-13T10:28:40Z

bash-completion/setpriv

@@ -87,6 +87,16 @@ _setpriv_module()
 			COMPREPLY=( $(compgen -W "profile" -- $cur) )
 			return 0
 			;;
+		'--landlock-access')
+			# FIXME: how to list landlock accesses?


Landlock does not currently provide an API to list all existing Landlock accesses with their names. You need to use the ones from the header.

If this TODO is about listing the accesses that are already enforced, that is also not currently possible.

We can rely on the Landlock ABI version to infer the supported access rights, but we indeed need to have the name mapping. I don't think this is an issue because util-linux would need to know how to use the new access rights anyway, so if they are not part of the code there is no need to use them.

l0kod

Nice work!

My main concern is about potential compatibility issues with older kernels or newer util-linux, see my comments.

l0kod · 2023-12-13T16:26:56Z

sys-utils/setpriv-landlock.c

+	size_t i;
+
+	/* without argument, match all */
+	if (list[0] == '\0') {


This might lead to unknown restrictions (from the user point of view) being enforced when updating util-linux with an extended landlock_access_fs definition. For instance, handling a new "ioctl" access right might deny legitimate IOCTL commands without the user noticing. In a nutshell, not specifying any handled access might lead to restricting all util-linux's known access rights, which might lead to an unknown/undeterministic behavior of the sandboxed applications. See the kernel documentation or the Rust library documentation.

Specifying all handled access rights is verbose, and I agree that it would help a lot to be able to not list all of them.

An alternative would be to collect all rules' access rights (which must be specified anyway) and put them here. Access rights that must be denied (and not used in any rule) would still need to be specified though.

An other alternative would be to specify a fixed set of handled access rights, e.g. using a Landlock ABI version.

setpriv is a fairly lowlevel tool. So in my opinion it would be fine to force users to specify explicitly what they want if they need longterm deterministic behavior.
As a default I would like to keep the API as close to the kernel as possible, providing users with the possibility to discover which rules are supported by the kernel and util-linux via exitcodes.
(I see that this also needs to be reworked)

This should be better documented.

Then on top of the base functionality we could add userfriendly extensions.

l0kod · 2023-12-13T16:42:35Z

sys-utils/setpriv-landlock.c

+	size_t i;
+
+	if (strcmp(str, "fs") == 0) {
+		for (i = 0; i < ARRAY_SIZE(landlock_access_fs); i++)


This might also be an issue with new versions of util-linux extending landlock_access_fs.

l0kod · 2023-12-13T16:52:07Z

sys-utils/setpriv-landlock.c

+
+	path++;
+
+	parent_fd = open(path, O_RDONLY | O_PATH | O_CLOEXEC);


It would be nice to be able to specify a file descriptor with a path-beneath argument. Indeed, Landlock's rules supports FDs, and it might be convenient, safer and most importantly race-free for rule building. Maybe something like setpriv --landlock-rule path-beneath:read:fd:0? Relying on /proc/self/fd/* might be another way but maybe a bit hacky (and not possible without /proc). Note that restrictions only applies to newly opened files though.

That's a good idea.
I'm a bit unhappy with path-beneath:read:fd:0 as it would break the structure compared to file paths. So specifying fd:0 as a filename would not be possible directly.

I like the /proc/self/fd paths. I don't expect setpriv to be used without /proc being mounted. Or we could special-case /proc/self/fd/ arguments and parse out the number.

l0kod · 2023-12-13T17:06:35Z

sys-utils/setpriv-landlock.c

+
+		ret = landlock_add_rule(fd, rule->rule_type, &rule->path_beneath_attr, 0);
+		if (ret == -1)
+			err(SETPRIV_EXIT_PRIVERR, _("adding landlock rule failed"));


It would be nice to be able to specify a best-effort approach to still enable partial restriction even if the (old) running kernel doesn't support all util-linux's known access rights. For instance, with the current approach, not specifying --landlock-access might work on the user's system with one version of util-linux but not with a newer version. Likewise, users using an old kernel that only supports a subset of landlock_access_fs would not be able to use Landlock with setpriv unless they specify --landlock-access.

I guess a new CLI flag would fit nicely here.

l0kod · 2023-12-13T17:21:21Z

bash-completion/setpriv

@@ -87,6 +87,16 @@ _setpriv_module()
 			COMPREPLY=( $(compgen -W "profile" -- $cur) )
 			return 0
 			;;
+		'--landlock-access')
+			# FIXME: how to list landlock accesses?


We can rely on the Landlock ABI version to infer the supported access rights, but we indeed need to have the name mapping. I don't think this is an issue because util-linux would need to know how to use the new access rights anyway, so if they are not part of the code there is no need to use them.

l0kod · 2023-12-13T17:25:15Z

Thanks, now it makes more sense ;-)

The LANDLOCK_ACCESS_* (and --landlock-access) is a strange name if used to deny access. Having --landlock-deny and --landlock-allow would be more readable, but I understand you want to follow kernel.

FWIW, the kernel wording is "handled" which can be translated to "denied unless allowed by a rule".

t-8ch force-pushed the setpriv-landlock branch 2 times, most recently from 89e5a5d to 89f195b Compare December 6, 2023 21:28

t-8ch force-pushed the setpriv-landlock branch 3 times, most recently from d0db3eb to 1b786da Compare December 8, 2023 07:23

t-8ch force-pushed the setpriv-landlock branch 3 times, most recently from 2112253 to f3b29d2 Compare December 9, 2023 09:19

t-8ch marked this pull request as ready for review December 9, 2023 14:35

t-8ch changed the title ~~RFC: setpriv: add landlock support~~ setpriv: add landlock support Dec 9, 2023

karelzak reviewed Dec 11, 2023

View reviewed changes

include/landlock.h Outdated Show resolved Hide resolved

karelzak reviewed Dec 11, 2023

View reviewed changes

sys-utils/setpriv-landlock.c Show resolved Hide resolved

karelzak reviewed Dec 11, 2023

View reviewed changes

sys-utils/setpriv-landlock.h Show resolved Hide resolved

t-8ch force-pushed the setpriv-landlock branch from f3b29d2 to 0a9ffa3 Compare December 11, 2023 11:10

t-8ch force-pushed the setpriv-landlock branch from 0a9ffa3 to ae015d7 Compare December 11, 2023 19:52

karelzak merged commit aa49261 into util-linux:master Dec 12, 2023
32 checks passed

gnoack reviewed Dec 13, 2023

View reviewed changes

l0kod reviewed Dec 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

setpriv: add landlock support #2628

setpriv: add landlock support #2628

t-8ch commented Dec 6, 2023

karelzak commented Dec 7, 2023

t-8ch commented Dec 7, 2023

karelzak commented Dec 7, 2023

karelzak commented Dec 7, 2023

t-8ch commented Dec 7, 2023

karelzak commented Dec 8, 2023

karelzak commented Dec 11, 2023

gnoack Dec 13, 2023 •

edited

l0kod Dec 13, 2023

l0kod left a comment

l0kod Dec 13, 2023

t-8ch Dec 15, 2023

l0kod Dec 13, 2023

l0kod Dec 13, 2023

t-8ch Dec 15, 2023

l0kod Dec 13, 2023

t-8ch Dec 15, 2023

l0kod Dec 13, 2023

l0kod commented Dec 13, 2023


		path++;

		parent_fd = open(path, O_RDONLY \| O_PATH \| O_CLOEXEC);

setpriv: add landlock support #2628

setpriv: add landlock support #2628

Conversation

t-8ch commented Dec 6, 2023

karelzak commented Dec 7, 2023

t-8ch commented Dec 7, 2023

karelzak commented Dec 7, 2023

karelzak commented Dec 7, 2023

t-8ch commented Dec 7, 2023

karelzak commented Dec 8, 2023

karelzak commented Dec 11, 2023

gnoack Dec 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0kod left a 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

l0kod commented Dec 13, 2023

gnoack Dec 13, 2023 •

edited