Mount control

[l0kod]

To avoid filesystem (FS) security policy bypass, a landlocked process with FS restrictions cannot do any FS topology changes (see d722036), which include any mount calls.

Even with FS restrictions, it would be useful for some use cases to be able to safely do new mount and umount.

The main issue I see is that we may want to allow a set of accesses on the newly mount points, independently from the existing path_beneath rules because the new mount point would overlap part of the initial file hierarchy. For this reason, I think we could have a new type of rule dedicated to mount access rights, something like LANDLOCK_RULE_MOUNT. With a dedicated attr struct, probably with landlock_path_beneath_attr's equivalent fields, we'll be able to configure a whole mount point and enforce specific options such as ro and noexec. I guess the current LSM hooks should be enough.

Because a mount would change the file hierarchy, we would also need a dedicated LANDLOCK_ACCESS_FS_MOUNT right to control this change. Everything beneath such mount point will get the source's LANDLOCK_RULE_MOUNT properties/restrictions.

For bind mounts, I think we can follow the same checks as for LANDLOCK_ACCESS_FS_REFER with the LANDLOCK_ACCESS_FS_MOUNT (which could then be used for source and destination).

This approach should also enable to allow a service to do mounts without giving it the right to access them.

I suggest to start with the bind mount case and then incrementally add support for the block device mount case (and the new related rule type).

I now think unmounts should never be denied though, so we may want to add a new flag at the ruleset level to only manage compatibility.

Related chromeOS CL: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5077507

[supersonnic]

This is useful, thanks for the details. I am working on updating that patch to use the refer logic instead of naively allowing a bind mount. I will update as I make progress.

[supersonnic]

Hi @l0kod, I updated the patch to support the same checks as REFER for bind-mounts and wanted to get some feedback on the high-level design. One interesting side-effect is that for the bind-mount to succeed, the source's parent needs to carry the LANDLOCK_ACCESS_FS_MOUNT rule, in addition to the destination directory (i.e. mount point). I think this is OK, as it works the same way for rename and link. For example, if I want to move /parent/child, I need to have the REFER rule applied to /parent. Similarly, to mount /parent/child the MOUNT rule should be applied to /parent. Let me know if the patch is inline with what you envisioned. Thanks.

[l0kod]

This looks good overall.

Thinking more about it, we should probably (re)use LANDLOCK_ACCESS_FS_REFER (instead of LANDLOCK_ACCESS_FS_MOUNT) to check the right to "refer" a file/directory because the related Landlock constraints are the same. This is for the UAPI, but the kernel implementation may differ a bit because of different assumptions (e.g. EXDEV, EACCES, or EPERM error codes).

LANDLOCK_ACCESS_FS_MOUNT should still be required on the destination though, similarly to LANDLOCK_ACCESS_FS_MAKE_*.

I'm worried about a race conditions in hook_sb_mount() regarding dev_name. I think we need a new LSM hook for bind mounts to avoid this kind of issue.

With that in mind, and a clang-formatted code, you can send a first RFC patch series, we'll review it there. Please add at least a few tests for now and propose a set of test scenarios for when we'll be OK with the approach.

You can use https://github.com/landlock-lsm/landlock-test-tools on top of my next branch to test it, or at least on top of b60a173 to avoid the IOCTL patches.

[supersonnic]

LANDLOCK_ACCESS_FS_MOUNT should still be required on the destination though, similarly to LANDLOCK_ACCESS_FS_MAKE_*.

Doesn’t this mean in practice we are also requiring LANDLOCK_ACCESS_FS_MOUNT on the source? If the rule only exists on the destination, the source would gain a new privilege (mount privilege) through the bind mount operation so the operation would fail with EXDEV. If it is missing on the destination, we get EACCES. Is that the behavior we want?

I'm worried about a race conditions in hook_sb_mount() regarding dev_name. I think we need a new LSM hook for bind mounts to avoid this kind of issue.

I think I found a place where we can add a hook for bind mount after the path is evaluated to eliminate the race.

Will work on the tests and preparing for RFC patches.

[l0kod]

LANDLOCK_ACCESS_FS_MOUNT should still be required on the destination though, similarly to LANDLOCK_ACCESS_FS_MAKE_*.

Doesn’t this mean in practice we are also requiring LANDLOCK_ACCESS_FS_MOUNT on the source? If the rule only exists on the destination, the source would gain a new privilege (mount privilege) through the bind mount operation so the operation would fail with EXDEV. If it is missing on the destination, we get EACCES. Is that the behavior we want?

Because of the way mounts propagate by default, we should only allow bind mounts on private mounts if the source doesn't have LANDLOCK_ACCESS_FS_MOUNT. We should then be able to create a new bind mount on the destination directory and mark it as private, and then do another bind mount with the source directory on the destination one (covering the private mount point).

We should also be careful to only create mount point that are not less-restrictive than the source (e.g. removing ro).

For mount operation, no EXDEV error should be returned, only EPERM or EACCES (see mount(2)).

I'm worried about a race conditions in hook_sb_mount() regarding dev_name. I think we need a new LSM hook for bind mounts to avoid this kind of issue.

I think I found a place where we can add a hook for bind mount after the path is evaluated to eliminate the race.

Looking at it again with an up-to-date mount utility, the new mount syscalls (e.g. open_tree, move_mount) are now used and security_move_mount() may be enough.

Will work on the tests and preparing for RFC patches.

Great, we can continue the discussion there.

[l0kod]

Because of the way mounts propagate by default, we should only allow bind mounts on private mounts if the source doesn't have LANDLOCK_ACCESS_FS_MOUNT.

Well, to allow a bind mount on a shared mount point, it's not the source that should have LANDLOCK_ACCESS_FS_MOUNT but the destination (shared) mount point (not only the destination file hierarchy). collect_domain_accesses() may be useful to check that.

However, if the destination mount point is private, we can look for LANDLOCK_ACCESS_FS_MOUNT in its parent mount point and continue looking for these properties in the file hierarchy...

[l0kod]

@supersonnic, what is the status of your work? Do you need some help?

This looks ready for an RFC: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5722641

[supersonnic]

The main issue I'm having right now is that I cannot get LandLock to only accept the bind-mount if the LANDLOCK_ACCESS_FS_MOUNT rule is carried directly by the mount point, and not its parent directories. I tried to achieve this by adding the extra bind-mount check (in fs.c: 1144-1163 of the referenced patch). To do this I pass the parent of the target directory as the mt_root argument for the collect_domain_accesses function, hoping that it stops collecting rules at the parent, effectively only collecting the rules carried by the mount point, but this does not seem to be the case, as the parents' rules keep getting collected also.

In other words, the bug in the patch is that it allows mounts even when one of the mount point parent directories has the rule. This is a problem, as aloowing bind-mounts under an existing bind-mount can open a pathway for policy bypass.

This is an example of a policy bypass that can be achieved if we allow inheritance of the mount point rule. Consider the following directory structure. First we bind-mount /tmp/public to /usr/local/share using the --shared-mount option. This is allowed because /usr/local/share does carry the mount rule and there is no privileges gained (though execution is dropped). Then we bind-mount /bin/exe to /usr/local/share/b. This is allowed because the mount point rule is inherited from b's parent, share and there is no privilege change between /bin/exe and /usr/local/share/b/exe, they both have only RW. However, because the first mount was a shared-mount, the exe mount also propagates under /tmp/public/b through which it can now be executed, hence the privilege escalation!

I considered intercepting the propagate system call, but did not have success with that, so I think the best way to mitigate this is to modify the rule checking behavior for mount point, such that it cannot be inherited, which brings us back the issue I'm having with that patch. I just need a way to collect and check the rules carried by a single directory.