GoT Landlock fixes

GoT Landlock fixes

[Mickaël Salaün]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

Hi,

I noticed Omar Polo added support for Landlock to the Linux version of 
Game Of Trees [1]. This is great! However, the handled filesystem access 
is only LANDLOCK_ACCESS_FS_READ_FILE, and it will still be allowed to do 
multiple filesystem-related actions (e.g. write to files, remove 
files…). I don't know much about Game Of Trees but, according to the 
commit message, I think you would like to revoke any (currently 
supported) filesystem access. You should then add the 12 remaining 
access rights [2]. There is also a typo in the errno check, it should be 
EOPNOTSUPP (not ENOTSUP). You'll find a small patch attached. Let me 
know if I can help.

In a nutshell, the ruleset's handled_access_fs is required for backward 
and forward compatibility (i.e. the kernel and user space may not know 
each other's supported restrictions), hence the need to be explicit 
about the denied-by-default access rights.

Regards,
  Mickaël


[1] 
https://git.gameoftrees.org/gitweb/?p=got-portable.git;a=commit;h=97799ccd4b67a81f97039305d4fdd66588da9962
[2] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags

[-- Attachment #2: 0001-portable-extend-support-for-Landlock-and-fix-error-h.patch --]
[-- Type: text/x-patch, Size: 1980 bytes --]

From f2c1e06c218b997f4c686a59d901b5e1948e8001 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net>
Date: Thu, 10 Feb 2022 18:09:52 +0100
Subject: [PATCH] portable: extend support for Landlock and fix error handling
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This add all the remaining currently supported (Linux >= 5.13)
filesystem restrictions: creation, removal, reading, writing and
executing.

Fix the errno check with EOPNOTSUPP in case of kernel with Landlock
support built-in but disabled at boot time.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 compat/landlock.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/compat/landlock.c b/compat/landlock.c
index 47a5209dbfe2..9a637bb0753f 100644
--- a/compat/landlock.c
+++ b/compat/landlock.c
@@ -76,7 +76,19 @@ landlock_no_fs(void)
 		 * rejecting *any* filesystem access, we still have to
 		 * list some "possible actions" here.
 		 */
-		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
+		.handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE | \
+				     LANDLOCK_ACCESS_FS_READ_FILE | \
+				     LANDLOCK_ACCESS_FS_READ_DIR | \
+				     LANDLOCK_ACCESS_FS_WRITE_FILE | \
+				     LANDLOCK_ACCESS_FS_REMOVE_DIR | \
+				     LANDLOCK_ACCESS_FS_REMOVE_FILE | \
+				     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,
 	};
 	int fd, saved_errno;
 
@@ -86,7 +98,7 @@ landlock_no_fs(void)
 	fd = landlock_create_ruleset(&rattr, sizeof(rattr), 0);
 	if (fd == -1) {
 		/* this kernel doesn't have landlock built in */
-		if (errno == ENOSYS || errno == ENOTSUP)
+		if (errno == ENOSYS || errno == EOPNOTSUPP)
 			return 0;
 		return -1;
 	}
-- 
2.34.1

Re: GoT Landlock fixes

[Omar Polo]

Hello,

Mickaël Salaün <mic@digikod.net> writes:

> Hi,
>
> I noticed Omar Polo added support for Landlock to the Linux version of
> Game Of Trees [1]. This is great! However, the handled filesystem
> access is only LANDLOCK_ACCESS_FS_READ_FILE, and it will still be
> allowed to do multiple filesystem-related actions (e.g. write to
> files, remove files…). I don't know much about Game Of Trees but,
> according to the commit message, I think you would like to revoke any
> (currently supported) filesystem access. You should then add the 12
> remaining access rights [2]. There is also a typo in the errno check,
> it should be EOPNOTSUPP (not ENOTSUP). You'll find a small patch
> attached. Let me know if I can help.
>
> In a nutshell, the ruleset's handled_access_fs is required for
> backward and forward compatibility (i.e. the kernel and user space may
> not know each other's supported restrictions), hence the need to be
> explicit about the denied-by-default access rights.

Yes, the original diff had all the actions, but when I picked that up
again before it got committed I got confused and dropped the others (I
was sure the checks not listed there were dropped by default.)  I humbly
apologies to everyone for the incredibly stupid mistake, I don't have
any excuse.

Regarding the ENOTSUP/EOPNOTSUPP mistake, I thought that on linux the
two errno were defined to the same value, but I agree on the change
obviously (this point was raised by Brian too recently.)

Thanks a lot for reviewing the diff and fixing my mistake!

> Regards,
>  Mickaël
>
>
> [1]
> https://git.gameoftrees.org/gitweb/?p=got-portable.git;a=commit;h=97799ccd4b67a81f97039305d4fdd66588da9962
> [2] https://docs.kernel.org/userspace-api/landlock.html#filesystem-flags
>
> [2. text/x-patch; 0001-portable-extend-support-for-Landlock-and-fix-error-h.patch]...

Re: GoT Landlock fixes

[Stefan Sperling]

On Thu, Feb 10, 2022 at 06:34:54PM +0100, Mickaël Salaün wrote:
> Hi,
> 
> I noticed Omar Polo added support for Landlock to the Linux version of Game
> Of Trees [1]. This is great! However, the handled filesystem access is only
> LANDLOCK_ACCESS_FS_READ_FILE, and it will still be allowed to do multiple
> filesystem-related actions (e.g. write to files, remove files…). I don't
> know much about Game Of Trees but, according to the commit message, I think
> you would like to revoke any (currently supported) filesystem access. You
> should then add the 12 remaining access rights [2]. There is also a typo in
> the errno check, it should be EOPNOTSUPP (not ENOTSUP). You'll find a small
> patch attached. Let me know if I can help.

Thank you for looking at this Mickaël, it helps us a great deal!
 
> In a nutshell, the ruleset's handled_access_fs is required for backward and
> forward compatibility (i.e. the kernel and user space may not know each
> other's supported restrictions), hence the need to be explicit about the
> denied-by-default access rights.

I suspect Omar trimmed down the list of fs flags was in response to
a question I asked during code review. The code initially followed
the example given in your docs, but we ended up diverging from it.

If the point you made above was explained in your docs it would be
much easier to understand why there is a long list of access rules
in struct rndlock_ruleset_attr, many of which may not seem relevant
to the application's purposes. Character or block devices will rarely
be needed by applications, for example. The name "handled_access_fs"
does not immediately bring to mind that this could be a default deny list.

Cheers,
Stefan

Re: GoT Landlock fixes

[Mickaël Salaün]

On 11/02/2022 11:39, Stefan Sperling wrote:
> On Thu, Feb 10, 2022 at 06:34:54PM +0100, Mickaël Salaün wrote:
>> Hi,
>>
>> I noticed Omar Polo added support for Landlock to the Linux version of Game
>> Of Trees [1]. This is great! However, the handled filesystem access is only
>> LANDLOCK_ACCESS_FS_READ_FILE, and it will still be allowed to do multiple
>> filesystem-related actions (e.g. write to files, remove files…). I don't
>> know much about Game Of Trees but, according to the commit message, I think
>> you would like to revoke any (currently supported) filesystem access. You
>> should then add the 12 remaining access rights [2]. There is also a typo in
>> the errno check, it should be EOPNOTSUPP (not ENOTSUP). You'll find a small
>> patch attached. Let me know if I can help.
> 
> Thank you for looking at this Mickaël, it helps us a great deal!

You're welcome. Omar did the hardest part, thanks to him!

>   
>> In a nutshell, the ruleset's handled_access_fs is required for backward and
>> forward compatibility (i.e. the kernel and user space may not know each
>> other's supported restrictions), hence the need to be explicit about the
>> denied-by-default access rights.
> 
> I suspect Omar trimmed down the list of fs flags was in response to
> a question I asked during code review. The code initially followed
> the example given in your docs, but we ended up diverging from it.
> 
> If the point you made above was explained in your docs it would be
> much easier to understand why there is a long list of access rules
> in struct rndlock_ruleset_attr, many of which may not seem relevant
> to the application's purposes. Character or block devices will rarely
> be needed by applications, for example. The name "handled_access_fs"
> does not immediately bring to mind that this could be a default deny list.

The documentation about handled_access_fs says: "Bitmask of actions (cf. 
Filesystem flags) that is handled by this ruleset and should then be 
forbidden if no rule explicitly allow them. This is needed for backward 
compatibility reasons."

I guess it is too succinct. What do you think about that:

"[...] if no rule explicitly allow them: it is a deny-by-default list 
that should contain as much Landlock access rights as possible. Indeed, 
all Landlock filesystem access rights that are not part of 
handled_access_fs are allowed. This is needed for [...]"

And before that, in the example, after "The ruleset then needs to handle 
both of these kind of actions", I can add "This is required for backward 
and forward compatibility (i.e. the kernel and user space may not know 
each other's supported restrictions), hence the need to be explicit 
about the denied-by-default access rights."

Re: GoT Landlock fixes

[Stefan Sperling]

On Fri, Feb 11, 2022 at 06:15:43PM +0100, Mickaël Salaün wrote:
> The documentation about handled_access_fs says: "Bitmask of actions (cf.
> Filesystem flags) that is handled by this ruleset and should then be
> forbidden if no rule explicitly allow them. This is needed for backward
> compatibility reasons."
> 
> I guess it is too succinct. What do you think about that:
> 
> "[...] if no rule explicitly allow them: it is a deny-by-default list that
> should contain as much Landlock access rights as possible. Indeed, all
> Landlock filesystem access rights that are not part of handled_access_fs are
> allowed. This is needed for [...]"
> 
> And before that, in the example, after "The ruleset then needs to handle
> both of these kind of actions", I can add "This is required for backward and
> forward compatibility (i.e. the kernel and user space may not know each
> other's supported restrictions), hence the need to be explicit about the
> denied-by-default access rights."
 
Yes, this would make the docs a lot clearer.

I suppose developers using landlock are supposed to keep their applications
up-to-date and expand their bitmask definition whenever new flags are
added to the landlock API?

If so, it might be worth pointing this out in the documentation, to ensure
readers understand that writing this list in their code is not a one-time
action, and their attention will be required when new flags are added.
Something like:
"Future versions of landlock may define additional filesystem flags,
which applications must add to handled_access_fs if a deny-by-default
policy is desired."

Which makes me wonder if applications which prefer to always fail closed
could simply set all the bits. Would this break anything?

struct landlock_ruleset_attr ruleset_attr = {
    .handled_access_fs = 0xffffffffffffffff;
};

Otherwise, how about providing a macro which programs like Got could
use to initialize this mask to all currently known filesystem flags?

#define LANDLOCK_ACCESS_FS_POLICY_DENY_ALL \
	(LANDLOCK_ACCESS_FS_EXECUTE | \
	LANDLOCK_ACCESS_FS_WRITE_FILE | \
	... etc.)
	

struct landlock_ruleset_attr ruleset_attr = {
    .handled_access_fs = LANDLOCK_ACCESS_FS_POLICY_DENY_ALL;
};

With this, only one copy of a default-deny mask would need to be maintained.
Applications could stay in sync and keep failing closed by recompiling them
against new landlock header files. Would this work?

Cheers,
Stefan

Re: GoT Landlock fixes

[Mickaël Salaün]

On 11/02/2022 21:11, Stefan Sperling wrote:
> On Fri, Feb 11, 2022 at 06:15:43PM +0100, Mickaël Salaün wrote:
>> The documentation about handled_access_fs says: "Bitmask of actions (cf.
>> Filesystem flags) that is handled by this ruleset and should then be
>> forbidden if no rule explicitly allow them. This is needed for backward
>> compatibility reasons."
>>
>> I guess it is too succinct. What do you think about that:

(applied to https://docs.kernel.org/userspace-api/landlock.html)

>>
>> "[...] if no rule explicitly allow them: it is a deny-by-default list that
>> should contain as much Landlock access rights as possible. Indeed, all
>> Landlock filesystem access rights that are not part of handled_access_fs are
>> allowed. This is needed for [...]"
>>
>> And before that, in the example, after "The ruleset then needs to handle
>> both of these kind of actions", I can add "This is required for backward and
>> forward compatibility (i.e. the kernel and user space may not know each
>> other's supported restrictions), hence the need to be explicit about the
>> denied-by-default access rights."
>   
> Yes, this would make the docs a lot clearer.
> 
> I suppose developers using landlock are supposed to keep their applications
> up-to-date and expand their bitmask definition whenever new flags are
> added to the landlock API?
> 
> If so, it might be worth pointing this out in the documentation, to ensure
> readers understand that writing this list in their code is not a one-time
> action, and their attention will be required when new flags are added.
> Something like:
> "Future versions of landlock may define additional filesystem flags,
> which applications must add to handled_access_fs if a deny-by-default
> policy is desired."

Right, I'll improve the documentation in this area.

> 
> Which makes me wonder if applications which prefer to always fail closed
> could simply set all the bits. Would this break anything?
> 
> struct landlock_ruleset_attr ruleset_attr = {
>      .handled_access_fs = 0xffffffffffffffff;
> };
> 
> Otherwise, how about providing a macro which programs like Got could
> use to initialize this mask to all currently known filesystem flags?
> 
> #define LANDLOCK_ACCESS_FS_POLICY_DENY_ALL \
> 	(LANDLOCK_ACCESS_FS_EXECUTE | \
> 	LANDLOCK_ACCESS_FS_WRITE_FILE | \
> 	... etc.)
> 	
> 
> struct landlock_ruleset_attr ruleset_attr = {
>      .handled_access_fs = LANDLOCK_ACCESS_FS_POLICY_DENY_ALL;
> };
> 
> With this, only one copy of a default-deny mask would need to be maintained.
> Applications could stay in sync and keep failing closed by recompiling them
> against new landlock header files. Would this work?

The issue with these approaches is that we don't know the future and new 
Landlock accesses could break an application. I don't want this to be 
silent: the same (application) code should produce the same restrictions 
(which would not be the true if an included variable change over time). 
I agree that there is a use case to block all filesystem-related actions 
(even if the frontier with other subsystems might be blurry, e.g. UNIX 
socket), but I think it would be too bug-prone for applications to rely 
on that. As a kernel developer, I cannot assure that applications using 
this API would not suddenly break if built with a new set of 
(kernel-provided) headers. In this case, a conservative approach seems 
better for users but also for (the sanity of) most developers. ;)

One of the goal of the landlock@lists.linux.dev [1] mailing list is for 
developers to keep an eye on Landlock evolution.

[1] https://subspace.kernel.org/lists.linux.dev.html