| Message ID | 20250108154338.1129069-12-mic@digikod.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Landlock audit support | expand |
On Wed, Jan 08, 2025 at 04:43:19PM +0100, Mickaël Salaün wrote: > Fix a logical issue that could have been visible if the source or the > destination of a rename/link action was allowed for either the source or > the destination but not both. However, this logical bug is unreachable > because either: > - the rename/link action is allowed by the access rights tied to the > same mount point (without relying on access rights in a parent mount > point) and the access request is allowed (i.e. allow_parent1 and > allow_parent2 are true in current_check_refer_path), > - or a common rule in a parent mount point updates the access check for > the source and the destination (cf. is_access_to_paths_allowed). > > See the following layout1.refer_part_mount_tree_is_allowed test that > work with and without this fix. > > This fix does not impact current code but it is required for the audit > support. > > Cc: Günther Noack <gnoack@google.com> > Signed-off-by: Mickaël Salaün <mic@digikod.net> > Link: https://lore.kernel.org/r/20250108154338.1129069-12-mic@digikod.net Pushed in my next tree to simplify next patch series. > --- > > Changes since v2: > - New patch. > --- > security/landlock/fs.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > index 171012efb559..ddadc465581e 100644 > --- a/security/landlock/fs.c > +++ b/security/landlock/fs.c > @@ -567,6 +567,12 @@ static void test_no_more_access(struct kunit *const test) > #undef NMA_TRUE > #undef NMA_FALSE > > +static bool is_layer_masks_allowed( > + layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS]) > +{ > + return !memchr_inv(layer_masks, 0, sizeof(*layer_masks)); > +} > + > /* > * Removes @layer_masks accesses that are not requested. > * > @@ -584,7 +590,8 @@ scope_to_request(const access_mask_t access_request, > > for_each_clear_bit(access_bit, &access_req, ARRAY_SIZE(*layer_masks)) > (*layer_masks)[access_bit] = 0; > - return !memchr_inv(layer_masks, 0, sizeof(*layer_masks)); > + > + return is_layer_masks_allowed(layer_masks); > } > > #ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST > @@ -773,9 +780,14 @@ static bool is_access_to_paths_allowed( > if (WARN_ON_ONCE(domain->num_layers < 1 || !layer_masks_parent1)) > return false; > > + allowed_parent1 = is_layer_masks_allowed(layer_masks_parent1); > + > if (unlikely(layer_masks_parent2)) { > if (WARN_ON_ONCE(!dentry_child1)) > return false; > + > + allowed_parent2 = is_layer_masks_allowed(layer_masks_parent2); > + > /* > * For a double request, first check for potential privilege > * escalation by looking at domain handled accesses (which are > -- > 2.47.1 > >
diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 171012efb559..ddadc465581e 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -567,6 +567,12 @@ static void test_no_more_access(struct kunit *const test) #undef NMA_TRUE #undef NMA_FALSE +static bool is_layer_masks_allowed( + layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS]) +{ + return !memchr_inv(layer_masks, 0, sizeof(*layer_masks)); +} + /* * Removes @layer_masks accesses that are not requested. * @@ -584,7 +590,8 @@ scope_to_request(const access_mask_t access_request, for_each_clear_bit(access_bit, &access_req, ARRAY_SIZE(*layer_masks)) (*layer_masks)[access_bit] = 0; - return !memchr_inv(layer_masks, 0, sizeof(*layer_masks)); + + return is_layer_masks_allowed(layer_masks); } #ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST @@ -773,9 +780,14 @@ static bool is_access_to_paths_allowed( if (WARN_ON_ONCE(domain->num_layers < 1 || !layer_masks_parent1)) return false; + allowed_parent1 = is_layer_masks_allowed(layer_masks_parent1); + if (unlikely(layer_masks_parent2)) { if (WARN_ON_ONCE(!dentry_child1)) return false; + + allowed_parent2 = is_layer_masks_allowed(layer_masks_parent2); + /* * For a double request, first check for potential privilege * escalation by looking at domain handled accesses (which are
Fix a logical issue that could have been visible if the source or the destination of a rename/link action was allowed for either the source or the destination but not both. However, this logical bug is unreachable because either: - the rename/link action is allowed by the access rights tied to the same mount point (without relying on access rights in a parent mount point) and the access request is allowed (i.e. allow_parent1 and allow_parent2 are true in current_check_refer_path), - or a common rule in a parent mount point updates the access check for the source and the destination (cf. is_access_to_paths_allowed). See the following layout1.refer_part_mount_tree_is_allowed test that work with and without this fix. This fix does not impact current code but it is required for the audit support. Cc: Günther Noack <gnoack@google.com> Signed-off-by: Mickaël Salaün <mic@digikod.net> Link: https://lore.kernel.org/r/20250108154338.1129069-12-mic@digikod.net --- Changes since v2: - New patch. --- security/landlock/fs.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)