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> > +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. */ ;) -- paul-moore.com
