On 08/04/2022 03:42, Paul Moore wrote:
On Tue, Mar 29, 2022 at 8:51 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
From: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx>
Add a new LANDLOCK_ACCESS_FS_REFER access right to enable policy writers
to allow sandboxed processes to link and rename files from and to a
specific set of file hierarchies. This access right should be composed
with LANDLOCK_ACCESS_FS_MAKE_* for the destination of a link or rename,
and with LANDLOCK_ACCESS_FS_REMOVE_* for a source of a rename. This
lift a Landlock limitation that always denied changing the parent of an
inode.
Renaming or linking to the same directory is still always allowed,
whatever LANDLOCK_ACCESS_FS_REFER is used or not, because it is not
considered a threat to user data.
However, creating multiple links or renaming to a different parent
directory may lead to privilege escalations if not handled properly.
Indeed, we must be sure that the source doesn't gain more privileges by
being accessible from the destination. This is handled by making sure
that the source hierarchy (including the referenced file or directory
itself) restricts at least as much the destination hierarchy. If it is
not the case, an EXDEV error is returned, making it potentially possible
for user space to copy the file hierarchy instead of moving or linking
it.
Instead of creating different access rights for the source and the
destination, we choose to make it simple and consistent for users.
Indeed, considering the previous constraint, it would be weird to
require such destination access right to be also granted to the source
(to make it a superset). Moreover, RENAME_EXCHANGE would also add to
the confusion because of paths being both a source and a destination.
See the provided documentation for additional details.
New tests are provided with a following commit.
Cc: Paul Moore <paul@xxxxxxxxxxxxxx>
Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx>
Link: https://lore.kernel.org/r/20220329125117.1393824-8-mic@xxxxxxxxxxx
---
Changes since v1:
* Update current_check_access_path() to efficiently handle
RENAME_EXCHANGE thanks to the updated LSM hook (see previous patch).
Only one path walk is performed per rename arguments until their
common mount point is reached. Superset of access rights is correctly
checked, including when exchanging a file with a directory. This
requires to store another matrix of layer masks.
* Reorder and rename check_access_path_dual() arguments in a more
generic way: switch from src/dst to 1/2. This makes it easier to
understand the RENAME_EXCHANGE cases alongs with the others. Update
and improve check_access_path_dual() documentation accordingly.
* Clean up the check_access_path_dual() loop: set both allowed_parent*
when reaching internal filesystems and remove a useless one. This
allows potential renames in internal filesystems (like for other
operations).
* Move the function arguments checks from BUILD_BUG_ON() to
WARN_ON_ONCE() to avoid clang build error.
* Rename is_superset() to no_more_access() and make it handle superset
checks of source and destination for simple and exchange cases.
* Move the layer_masks_child* creation from current_check_refer_path()
to check_access_path_dual(): this is simpler and less error-prone,
especially with RENAME_EXCHANGE.
* Remove one optimization in current_check_refer_path() to make the code
simpler, especially with the RENAME_EXCHANGE handling.
* Remove overzealous WARN_ON_ONCE() for !access_request check in
init_layer_masks().
---
include/uapi/linux/landlock.h | 27 +-
security/landlock/fs.c | 607 ++++++++++++++++---
security/landlock/limits.h | 2 +-
security/landlock/syscalls.c | 2 +-
tools/testing/selftests/landlock/base_test.c | 2 +-
tools/testing/selftests/landlock/fs_test.c | 3 +-
6 files changed, 566 insertions(+), 77 deletions(-)
I'm still not going to claim that I'm a Landlock expert, but this
looks sane to me.
Reviewed-by: Paul Moore <paul@xxxxxxxxxxxxxx>
Thanks Paul! I'll send a small update shortly, with some typo fixes,
some unlikely() calls, and rebased on the other Landlock patch series.
+static inline access_mask_t init_layer_masks(
+ const struct landlock_ruleset *const domain,
+ const access_mask_t access_request,
+ layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
+{
+ access_mask_t handled_accesses = 0;
+ size_t layer_level;
+
+ memset(layer_masks, 0, sizeof(*layer_masks));
+ /* An empty access request can happen because of O_WRONLY | O_RDWR. */
;)
