@@ -82,11 +82,11 @@ static void put_rule(struct landlock_rule *const rule)
/**
* landlock_insert_rule - Insert a rule in a ruleset
*
+ * Intersects access rights of the rule with those of the ruleset.
+ *
* @ruleset: The ruleset to be updated.
* @rule: Read-only payload to be inserted (not owned by this function).
- * @is_merge: If true, intersects access rights and updates the rule's layers
- * (e.g. merge two rulesets), else do a union of access rights and
- * keep the rule's layers (e.g. extend a ruleset)
+ * @is_merge: If true, handle the rule layers.
*
* Assumptions:
*
@@ -117,16 +117,11 @@ int landlock_insert_rule(struct landlock_ruleset *const ruleset,
}
/* If there is a matching rule, updates it. */
- if (is_merge) {
- /* Intersects access rights. */
- this->access &= rule->access;
-
+ if (is_merge)
/* Updates the rule layers with the next one. */
this->layers |= BIT_ULL(ruleset->nb_layers);
- } else {
- /* Extends access rights. */
- this->access |= rule->access;
- }
+ /* Intersects access rights. */
+ this->access &= rule->access;
return 0;
}
@@ -591,14 +591,16 @@ TEST_F(layout1, unhandled_access)
TEST_F(layout1, ruleset_overlap)
{
const struct rule rules[] = {
- /* These rules should be ORed among them. */
+ /* These rules should be ANDed among them. */
{
.path = dir_s1d2,
- .access = LANDLOCK_ACCESS_FS_WRITE_FILE,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_WRITE_FILE,
},
{
.path = dir_s1d2,
- .access = LANDLOCK_ACCESS_FS_READ_DIR,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_READ_DIR,
},
{}
};
@@ -609,24 +611,37 @@ TEST_F(layout1, ruleset_overlap)
enforce_ruleset(_metadata, ruleset_fd);
EXPECT_EQ(0, close(ruleset_fd));
+ /* Checks s1d1 hierarchy. */
+ ASSERT_EQ(-1, open(file1_s1d1, O_RDONLY | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
ASSERT_EQ(-1, open(file1_s1d1, O_WRONLY | O_CLOEXEC));
ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, open(file1_s1d1, O_RDWR | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
ASSERT_EQ(-1, open(dir_s1d1, O_RDONLY | O_DIRECTORY | O_CLOEXEC));
ASSERT_EQ(EACCES, errno);
- open_fd = open(file1_s1d2, O_WRONLY | O_CLOEXEC);
- ASSERT_LE(0, open_fd);
- EXPECT_EQ(0, close(open_fd));
- open_fd = open(dir_s1d2, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+ /* Checks s1d2 hierarchy. */
+ open_fd = open(file1_s1d2, O_RDONLY | O_CLOEXEC);
ASSERT_LE(0, open_fd);
EXPECT_EQ(0, close(open_fd));
+ ASSERT_EQ(-1, open(file1_s1d2, O_WRONLY | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, open(file1_s1d2, O_RDWR | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, open(dir_s1d2, O_RDONLY | O_DIRECTORY | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
- open_fd = open(file1_s1d3, O_WRONLY | O_CLOEXEC);
- ASSERT_LE(0, open_fd);
- EXPECT_EQ(0, close(open_fd));
- open_fd = open(dir_s1d3, O_RDONLY | O_DIRECTORY | O_CLOEXEC);
+ /* Checks s1d3 hierarchy. */
+ open_fd = open(file1_s1d3, O_RDONLY | O_CLOEXEC);
ASSERT_LE(0, open_fd);
EXPECT_EQ(0, close(open_fd));
+ ASSERT_EQ(-1, open(file1_s1d3, O_WRONLY | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, open(file1_s1d3, O_RDWR | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, open(dir_s1d3, O_RDONLY | O_DIRECTORY | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
}
TEST_F(layout1, interleaved_masked_accesses)
@@ -766,8 +781,8 @@ TEST_F(layout1, inherit_subset)
* any new access, only remove some. Once enforced, these rules are
* ANDed with the previous ones.
*/
- add_path_beneath(_metadata, ruleset_fd, LANDLOCK_ACCESS_FS_WRITE_FILE,
- dir_s1d2);
+ add_path_beneath(_metadata, ruleset_fd, rules[0].access |
+ LANDLOCK_ACCESS_FS_WRITE_FILE, dir_s1d2);
/*
* According to ruleset_fd, dir_s1d2 should now have the
* LANDLOCK_ACCESS_FS_READ_FILE and LANDLOCK_ACCESS_FS_WRITE_FILE
Following the previous commit logic, make ruleset updates more consistent by always intersecting access rights (boolean AND) instead of combining them (boolean OR) for the same layer. This defensive approach could also help avoid user space to inadvertently allow multiple access rights for the same object (e.g. write and execute access on a path hierarchy) instead of dealing with such inconsistency. This can happen when there is no deduplication of objects (e.g. paths and underlying inodes) whereas they get different access rights with landlock_add_rule(2). Update layout1.ruleset_overlap and layout1.inherit_subset tests accordingly. Cc: James Morris <jmorris@namei.org> Cc: Jann Horn <jannh@google.com> Cc: Serge E. Hallyn <serge@hallyn.com> Signed-off-by: Mickaël Salaün <mic@digikod.net> --- security/landlock/ruleset.c | 17 ++++----- tools/testing/selftests/landlock/fs_test.c | 41 +++++++++++++++------- 2 files changed, 34 insertions(+), 24 deletions(-)