On Thu, Mar 17, 2022 at 8:03 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > On 17/03/2022 02:26, Paul Moore wrote: > > On Mon, Feb 21, 2022 at 4:15 PM 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). > >> > >> See the provided documentation for additional details. > >> > >> New tests are provided with a following commit. > >> > >> Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx> > >> Link: https://lore.kernel.org/r/20220221212522.320243-7-mic@xxxxxxxxxxx > >> --- > >> include/uapi/linux/landlock.h | 27 +- > >> security/landlock/fs.c | 550 ++++++++++++++++--- > >> 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, 516 insertions(+), 70 deletions(-) ... > >> +/* > >> + * Returns true if there is at least one access right different than > >> + * LANDLOCK_ACCESS_FS_REFER. > >> + */ > >> +static inline bool is_eacces( > >> + const layer_mask_t (*const > >> + layer_masks)[LANDLOCK_NUM_ACCESS_FS], > >> const access_mask_t access_request) > >> { > > > > Granted, I don't have as deep of an understanding of Landlock as you > > do, but the function name "is_eacces" seems a little odd given the > > nature of the function. Perhaps "is_fsrefer"? > > Hmm, this helper does multiple things which are necessary to know if we > need to return -EACCES or -EXDEV. Renaming it to is_fsrefer() would > require to inverse the logic and use boolean negations in the callers > (because of ordering). Renaming to something like without_fs_refer() > would not be completely correct because we also check if there is no > layer_masks, which indicated that it doesn't contain an access right > that should return -EACCES. This helper is named as such because the > underlying semantic is to check for such error code, which is a tricky. > I can rename it co contains_eacces() or something, but a longer name > would require to cut the caller lines to fit 80 columns. :| You know the Landlock code better than I do, if you like "is_eacces()", then leave it as it is. > >> - layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {}; > >> - bool allowed = false, has_access = false; > >> + unsigned long access_bit; > >> + /* LANDLOCK_ACCESS_FS_REFER alone must return -EXDEV. */ > >> + const unsigned long access_check = access_request & > >> + ~LANDLOCK_ACCESS_FS_REFER; > >> + > >> + if (!layer_masks) > >> + return false; > >> + > >> + for_each_set_bit(access_bit, &access_check, ARRAY_SIZE(*layer_masks)) { > >> + if ((*layer_masks)[access_bit]) > >> + return true; > >> + } > > > > Is calling for_each_set_bit() overkill here? @access_check should > > only ever have at most one bit set (LANDLOCK_ACCESS_FS_REFER), yes? > > No, it is the contrary ... Gotcha. Thanks for the clarification, I must have missed that when I was looking at it last night. > >> @@ -287,22 +460,20 @@ static int check_access_path(const struct landlock_ruleset *const domain, > >> if (WARN_ON_ONCE(domain->num_layers < 1)) > >> return -EACCES; > >> > >> - /* Saves all layers handling a subset of requested accesses. */ > >> - for (i = 0; i < domain->num_layers; i++) { > >> - const unsigned long access_req = access_request; > >> - unsigned long access_bit; > >> - > >> - for_each_set_bit(access_bit, &access_req, > >> - ARRAY_SIZE(layer_masks)) { > >> - if (domain->fs_access_masks[i] & BIT_ULL(access_bit)) { > >> - layer_masks[access_bit] |= BIT_ULL(i); > >> - has_access = true; > >> - } > >> - } > >> + BUILD_BUG_ON(!layer_masks_dst_parent); > > > > I know the kbuild robot already flagged this, but checking function > > parameters with BUILD_BUG_ON() does seem a bit ... unusual :) > > Yeah, I like such guarantee but it may not work without __always_inline. > I moved this check in the previous WARN_ON_ONCE(). That sounds good to me. > >> @@ -312,11 +483,50 @@ static int check_access_path(const struct landlock_ruleset *const domain, > >> */ > >> while (true) { > >> struct dentry *parent_dentry; > >> + const struct landlock_rule *rule; > >> + > >> + /* > >> + * If at least all accesses allowed on the destination are > >> + * already allowed on the source, respectively if there is at > >> + * least as much as restrictions on the destination than on the > >> + * source, then we can safely refer files from the source to > >> + * the destination without risking a privilege escalation. > >> + * This is crucial for standalone multilayered security > >> + * policies. Furthermore, this helps avoid policy writers to > >> + * shoot themselves in the foot. > >> + */ > >> + if (is_dom_check && is_superset(child_is_directory, > >> + layer_masks_dst_parent, > >> + layer_masks_src_parent, > >> + layer_masks_child)) { > >> + allowed_dst_parent = > >> + scope_to_request(access_request_dst_parent, > >> + layer_masks_dst_parent); > >> + allowed_src_parent = > >> + scope_to_request(access_request_src_parent, > >> + layer_masks_src_parent); > >> + > >> + /* Stops when all accesses are granted. */ > >> + if (allowed_dst_parent && allowed_src_parent) > >> + break; > >> + > >> + /* > >> + * Downgrades checks from domain handled accesses to > >> + * requested accesses. > >> + */ > >> + is_dom_check = false; > >> + access_masked_dst_parent = access_request_dst_parent; > >> + access_masked_src_parent = access_request_src_parent; > >> + } > >> + > >> + rule = find_rule(domain, walker_path.dentry); > >> + allowed_dst_parent = unmask_layers(rule, access_masked_dst_parent, > >> + layer_masks_dst_parent); > >> + allowed_src_parent = unmask_layers(rule, access_masked_src_parent, > >> + layer_masks_src_parent); > >> > >> - allowed = unmask_layers(find_rule(domain, walker_path.dentry), > >> - access_request, &layer_masks); > >> - if (allowed) > >> - /* Stops when a rule from each layer grants access. */ > >> + /* Stops when a rule from each layer grants access. */ > >> + if (allowed_dst_parent && allowed_src_parent) > >> break; > > > > If "(allowed_dst_parent && allowed_src_parent)" is true, you break out > > of the while loop only to do a path_put(), check the two booleans once > > more, and then return zero, yes? Why not just do the path_put() and > > return zero here? > > Correct, that would work, but I prefer not to duplicate the logic of > granting access if it doesn't make the code more complex, which I think > is not the case here, and I'm reluctant to duplicate path_get/put() > calls. This loop break is a small optimization to avoid walking the path > one more step, and writing it this way looks cleaner and less > error-prone from my point of view. I'm a big fan of maintainable code, and since you are the maintainer, if you prefer this approach I say stick with what you have :) -- paul-moore.com
