Skip to content

Commit

Permalink
landlock: Fix same-layer rule unions
Browse files Browse the repository at this point in the history 
commit 8ba0005 upstream.

The original behavior was to check if the full set of requested accesses
was allowed by at least a rule of every relevant layer.  This didn't
take into account requests for multiple accesses and same-layer rules
allowing the union of these accesses in a complementary way.  As a
result, multiple accesses requested on a file hierarchy matching rules
that, together, allowed these accesses, but without a unique rule
allowing all of them, was illegitimately denied.  This case should be
rare in practice and it can only be triggered by the path_rename or
file_open hook implementations.

For instance, if, for the same layer, a rule allows execution
beneath /a/b and another rule allows read beneath /a, requesting access
to read and execute at the same time for /a/b should be allowed for this
layer.

This was an inconsistency because the union of same-layer rule accesses
was already allowed if requested once at a time anyway.

This fix changes the way allowed accesses are gathered over a path walk.
To take into account all these rule accesses, we store in a matrix all
layer granting the set of requested accesses, according to the handled
accesses.  To avoid heap allocation, we use an array on the stack which
is 2*13 bytes.  A following commit bringing the LANDLOCK_ACCESS_FS_REFER
access right will increase this size to reach 112 bytes (2*14*4) in case
of link or rename actions.

Add a new layout1.layer_rule_unions test to check that accesses from
different rules pertaining to the same layer are ORed in a file
hierarchy.  Also test that it is not the case for rules from different
layers.

Reviewed-by: Paul Moore <paul@paul-moore.com>
Link: https://lore.kernel.org/r/20220506161102.525323-5-mic@digikod.net
Cc: stable@vger.kernel.org
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  • Loading branch information
@l0kod @gregkh
l0kod authored and gregkh committed Jun 9, 2022
1 parent 4055085 commit cd31481
Show file tree
Hide file tree
Showing 3 changed files with 161 additions and 26 deletions.
78 changes: 52 additions & 26 deletions security/landlock/fs.c
View file
Expand Up @@ -207,45 +207,67 @@ find_rule(const struct landlock_ruleset *const domain,
return rule;
}

static inline layer_mask_t unmask_layers(const struct landlock_rule *const rule,
const access_mask_t access_request,
layer_mask_t layer_mask)
/*
* @layer_masks is read and may be updated according to the access request and
* the matching rule.
*
* Returns true if the request is allowed (i.e. relevant layer masks for the
* request are empty).
*/
static inline bool
unmask_layers(const struct landlock_rule *const rule,
const access_mask_t access_request,
layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
{
size_t layer_level;

if (!access_request || !layer_masks)
return true;
if (!rule)
return layer_mask;
return false;

/*
* An access is granted if, for each policy layer, at least one rule
* encountered on the pathwalk grants the requested accesses,
* regardless of their position in the layer stack. We must then check
* encountered on the pathwalk grants the requested access,
* regardless of its position in the layer stack. We must then check
* the remaining layers for each inode, from the first added layer to
* the last one.
* the last one. When there is multiple requested accesses, for each
* policy layer, the full set of requested accesses may not be granted
* by only one rule, but by the union (binary OR) of multiple rules.
* E.g. /a/b <execute> + /a <read> => /a/b <execute + read>
*/
for (layer_level = 0; layer_level < rule->num_layers; layer_level++) {
const struct landlock_layer *const layer =
&rule->layers[layer_level];
const layer_mask_t layer_bit = BIT_ULL(layer->level - 1);
const unsigned long access_req = access_request;
unsigned long access_bit;
bool is_empty;

/* Checks that the layer grants access to the full request. */
if ((layer->access & access_request) == access_request) {
layer_mask &= ~layer_bit;

if (layer_mask == 0)
return layer_mask;
/*
* Records in @layer_masks which layer grants access to each
* requested access.
*/
is_empty = true;
for_each_set_bit(access_bit, &access_req,
ARRAY_SIZE(*layer_masks)) {
if (layer->access & BIT_ULL(access_bit))
(*layer_masks)[access_bit] &= ~layer_bit;
is_empty = is_empty && !(*layer_masks)[access_bit];
}
if (is_empty)
return true;
}
return layer_mask;
return false;
}

static int check_access_path(const struct landlock_ruleset *const domain,
const struct path *const path,
const access_mask_t access_request)
{
bool allowed = false;
layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
bool allowed = false, has_access = false;
struct path walker_path;
layer_mask_t layer_mask;
size_t i;

if (!access_request)
Expand All @@ -265,13 +287,20 @@ static int check_access_path(const struct landlock_ruleset *const domain,
return -EACCES;

/* Saves all layers handling a subset of requested accesses. */
layer_mask = 0;
for (i = 0; i < domain->num_layers; i++) {
if (domain->fs_access_masks[i] & access_request)
layer_mask |= BIT_ULL(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;
}
}
}
/* An access request not handled by the domain is allowed. */
if (layer_mask == 0)
if (!has_access)
return 0;

walker_path = *path;
Expand All @@ -283,14 +312,11 @@ static int check_access_path(const struct landlock_ruleset *const domain,
while (true) {
struct dentry *parent_dentry;

layer_mask =
unmask_layers(find_rule(domain, walker_path.dentry),
access_request, layer_mask);
if (layer_mask == 0) {
allowed = unmask_layers(find_rule(domain, walker_path.dentry),
access_request, &layer_masks);
if (allowed)
/* Stops when a rule from each layer grants access. */
allowed = true;
break;
}

jump_up:
if (walker_path.dentry == walker_path.mnt->mnt_root) {
Expand Down
2 changes: 2 additions & 0 deletions security/landlock/ruleset.h
View file
Expand Up @@ -22,6 +22,8 @@
typedef u16 access_mask_t;
/* Makes sure all filesystem access rights can be stored. */
static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
/* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));

typedef u16 layer_mask_t;
/* Makes sure all layers can be checked. */
Expand Down
107 changes: 107 additions & 0 deletions tools/testing/selftests/landlock/fs_test.c
View file
Expand Up @@ -758,6 +758,113 @@ TEST_F_FORK(layout1, ruleset_overlap)
ASSERT_EQ(0, test_open(dir_s1d3, O_RDONLY | O_DIRECTORY));
}

TEST_F_FORK(layout1, layer_rule_unions)
{
const struct rule layer1[] = {
{
.path = dir_s1d2,
.access = LANDLOCK_ACCESS_FS_READ_FILE,
},
/* dir_s1d3 should allow READ_FILE and WRITE_FILE (O_RDWR). */
{
.path = dir_s1d3,
.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
},
{},
};
const struct rule layer2[] = {
/* Doesn't change anything from layer1. */
{
.path = dir_s1d2,
.access = LANDLOCK_ACCESS_FS_READ_FILE |
LANDLOCK_ACCESS_FS_WRITE_FILE,
},
{},
};
const struct rule layer3[] = {
/* Only allows write (but not read) to dir_s1d3. */
{
.path = dir_s1d2,
.access = LANDLOCK_ACCESS_FS_WRITE_FILE,
},
{},
};
int ruleset_fd = create_ruleset(_metadata, ACCESS_RW, layer1);

ASSERT_LE(0, ruleset_fd);
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));

/* Checks s1d1 hierarchy with layer1. */
ASSERT_EQ(EACCES, test_open(file1_s1d1, O_RDONLY));
ASSERT_EQ(EACCES, test_open(file1_s1d1, O_WRONLY));
ASSERT_EQ(EACCES, test_open(file1_s1d1, O_RDWR));
ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY));

/* Checks s1d2 hierarchy with layer1. */
ASSERT_EQ(0, test_open(file1_s1d2, O_RDONLY));
ASSERT_EQ(EACCES, test_open(file1_s1d2, O_WRONLY));
ASSERT_EQ(EACCES, test_open(file1_s1d2, O_RDWR));
ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY));

/* Checks s1d3 hierarchy with layer1. */
ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY));
ASSERT_EQ(0, test_open(file1_s1d3, O_WRONLY));
/* dir_s1d3 should allow READ_FILE and WRITE_FILE (O_RDWR). */
ASSERT_EQ(0, test_open(file1_s1d3, O_RDWR));
ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY));

/* Doesn't change anything from layer1. */
ruleset_fd = create_ruleset(_metadata, ACCESS_RW, layer2);
ASSERT_LE(0, ruleset_fd);
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));

/* Checks s1d1 hierarchy with layer2. */
ASSERT_EQ(EACCES, test_open(file1_s1d1, O_RDONLY));
ASSERT_EQ(EACCES, test_open(file1_s1d1, O_WRONLY));
ASSERT_EQ(EACCES, test_open(file1_s1d1, O_RDWR));
ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY));

/* Checks s1d2 hierarchy with layer2. */
ASSERT_EQ(0, test_open(file1_s1d2, O_RDONLY));
ASSERT_EQ(EACCES, test_open(file1_s1d2, O_WRONLY));
ASSERT_EQ(EACCES, test_open(file1_s1d2, O_RDWR));
ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY));

/* Checks s1d3 hierarchy with layer2. */
ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY));
ASSERT_EQ(0, test_open(file1_s1d3, O_WRONLY));
/* dir_s1d3 should allow READ_FILE and WRITE_FILE (O_RDWR). */
ASSERT_EQ(0, test_open(file1_s1d3, O_RDWR));
ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY));

/* Only allows write (but not read) to dir_s1d3. */
ruleset_fd = create_ruleset(_metadata, ACCESS_RW, layer3);
ASSERT_LE(0, ruleset_fd);
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));

/* Checks s1d1 hierarchy with layer3. */
ASSERT_EQ(EACCES, test_open(file1_s1d1, O_RDONLY));
ASSERT_EQ(EACCES, test_open(file1_s1d1, O_WRONLY));
ASSERT_EQ(EACCES, test_open(file1_s1d1, O_RDWR));
ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY));

/* Checks s1d2 hierarchy with layer3. */
ASSERT_EQ(EACCES, test_open(file1_s1d2, O_RDONLY));
ASSERT_EQ(EACCES, test_open(file1_s1d2, O_WRONLY));
ASSERT_EQ(EACCES, test_open(file1_s1d2, O_RDWR));
ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY));

/* Checks s1d3 hierarchy with layer3. */
ASSERT_EQ(EACCES, test_open(file1_s1d3, O_RDONLY));
ASSERT_EQ(0, test_open(file1_s1d3, O_WRONLY));
/* dir_s1d3 should now deny READ_FILE and WRITE_FILE (O_RDWR). */
ASSERT_EQ(EACCES, test_open(file1_s1d3, O_RDWR));
ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY));
}

TEST_F_FORK(layout1, non_overlapping_accesses)
{
const struct rule layer1[] = {
Expand Down

0 comments on commit cd31481

Please sign in to comment.