Hi,
This patch series fixes some issues and makes the Landlock filesystem
access-control more consistent and deterministic when stacking multiple
rulesets. This is checked by current and new tests. I also extended
documentation and example to help users.
This series can be applied on top of
https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git/log/?h=landlock_lsm
Regards,
Mickaël Salaün (9):
landlock: Fix memory allocation error handling
landlock: Cosmetic fixes for filesystem management
landlock: Enforce deterministic interleaved path rules
landlock: Always intersect access rights
landlock: Add extra checks when inserting a rule
selftests/landlock: Extend layout1.inherit_superset
landlock: Clean up get_ruleset_from_fd()
landlock: Add help to enable Landlock as a stacked LSM
landlock: Extend documentation about limitations
Documentation/userspace-api/landlock.rst | 17 +++
samples/landlock/sandboxer.c | 21 +++-
security/landlock/Kconfig | 4 +-
security/landlock/fs.c | 67 +++++-----
security/landlock/object.c | 5 +-
security/landlock/ruleset.c | 34 ++---
security/landlock/syscall.c | 24 ++--
tools/testing/selftests/landlock/fs_test.c | 140 +++++++++++++++++++--
8 files changed, 239 insertions(+), 73 deletions(-)
base-commit: 96b3198c4025c11347651700b77e45a686d78553
--
2.29.2
Handle memory allocation errors in landlock_create_object() call. This
prevent to inadvertently hold an inode. Also, make get_inode_object()
more readable.
Cc: James Morris <[email protected]>
Cc: Jann Horn <[email protected]>
Cc: Serge E. Hallyn <[email protected]>
Signed-off-by: Mickaël Salaün <[email protected]>
---
security/landlock/fs.c | 5 +++++
security/landlock/object.c | 5 +++--
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index d8c5d19ac2af..b67c821bb40b 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -9,6 +9,7 @@
#include <linux/atomic.h>
#include <linux/compiler_types.h>
#include <linux/dcache.h>
+#include <linux/err.h>
#include <linux/fs.h>
#include <linux/init.h>
#include <linux/kernel.h>
@@ -98,6 +99,8 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
* holding any locks).
*/
new_object = landlock_create_object(&landlock_fs_underops, inode);
+ if (IS_ERR(new_object))
+ return new_object;
spin_lock(&inode->i_lock);
object = rcu_dereference_protected(inode_sec->object,
@@ -145,6 +148,8 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
access_rights |= _LANDLOCK_ACCESS_FS_MASK & ~ruleset->fs_access_mask;
rule.access = access_rights;
rule.object = get_inode_object(d_backing_inode(path->dentry));
+ if (IS_ERR(rule.object))
+ return PTR_ERR(rule.object);
mutex_lock(&ruleset->lock);
err = landlock_insert_rule(ruleset, &rule, false);
mutex_unlock(&ruleset->lock);
diff --git a/security/landlock/object.c b/security/landlock/object.c
index a71644ee72a7..54ba0327002a 100644
--- a/security/landlock/object.c
+++ b/security/landlock/object.c
@@ -8,6 +8,7 @@
#include <linux/bug.h>
#include <linux/compiler_types.h>
+#include <linux/err.h>
#include <linux/kernel.h>
#include <linux/rcupdate.h>
#include <linux/refcount.h>
@@ -23,10 +24,10 @@ struct landlock_object *landlock_create_object(
struct landlock_object *new_object;
if (WARN_ON_ONCE(!underops || !underobj))
- return NULL;
+ return ERR_PTR(-ENOENT);
new_object = kzalloc(sizeof(*new_object), GFP_KERNEL_ACCOUNT);
if (!new_object)
- return NULL;
+ return ERR_PTR(-ENOMEM);
refcount_set(&new_object->usage, 1);
spin_lock_init(&new_object->lock);
new_object->underops = underops;
--
2.29.2
To have consistent layered rules, granting access to a path implies that
all accesses tied to inodes, from the requested file to the real root,
must be checked. Otherwise, stacked rules may result to overzealous
restrictions. By excluding the ability to add exceptions in the same
layer (e.g. /a allowed, /a/b denied, and /a/b/c allowed), we get
deterministic interleaved path rules. This removes an optimization
which could be replaced by a proper cache mechanism. This also further
simplifies and explain check_access_path_continue().
Add a layout1.interleaved_masked_accesses test to check corner-case
layered rule combinations.
Cc: James Morris <[email protected]>
Cc: Jann Horn <[email protected]>
Cc: Serge E. Hallyn <[email protected]>
Signed-off-by: Mickaël Salaün <[email protected]>
---
security/landlock/fs.c | 38 +++++----
tools/testing/selftests/landlock/fs_test.c | 95 ++++++++++++++++++++++
2 files changed, 115 insertions(+), 18 deletions(-)
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 33fc7ae17c7f..2ca4dce1e9ed 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -180,16 +180,24 @@ static bool check_access_path_continue(
rcu_dereference(landlock_inode(inode)->object));
rcu_read_unlock();
- /* Checks for matching layers. */
- if (rule && (rule->layers | *layer_mask)) {
- if ((rule->access & access_request) == access_request) {
- *layer_mask &= ~rule->layers;
- return true;
- } else {
- return false;
- }
+ if (!rule)
+ /* Continues to walk if there is no rule for this inode. */
+ return true;
+ /*
+ * We must check all layers for each inode because we may encounter
+ * multiple different accesses from the same layer in a walk. Each
+ * layer must at least allow the access request one time (i.e. with one
+ * inode). This enables to have a deterministic behavior whatever
+ * inode is tagged within interleaved layers.
+ */
+ if ((rule->access & access_request) == access_request) {
+ /* Validates layers for which all accesses are allowed. */
+ *layer_mask &= ~rule->layers;
+ /* Continues to walk until all layers are validated. */
+ return true;
}
- return true;
+ /* Stops if a rule in the path don't allow all requested access. */
+ return false;
}
static int check_access_path(const struct landlock_ruleset *const domain,
@@ -231,12 +239,6 @@ static int check_access_path(const struct landlock_ruleset *const domain,
&layer_mask)) {
struct dentry *parent_dentry;
- /* Stops when a rule from each layer granted access. */
- if (layer_mask == 0) {
- allowed = true;
- break;
- }
-
jump_up:
/*
* Does not work with orphaned/private mounts like overlayfs
@@ -248,10 +250,10 @@ static int check_access_path(const struct landlock_ruleset *const domain,
goto jump_up;
} else {
/*
- * Stops at the real root. Denies access
- * because not all layers have granted access.
+ * Stops at the real root. Denies access if
+ * not all layers granted access.
*/
- allowed = false;
+ allowed = (layer_mask == 0);
break;
}
}
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 8aed28081ec8..ade0ad8728d8 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -629,6 +629,101 @@ TEST_F(layout1, ruleset_overlap)
EXPECT_EQ(0, close(open_fd));
}
+TEST_F(layout1, interleaved_masked_accesses)
+{
+ /*
+ * Checks overly restrictive rules:
+ * layer 1: allows s1d1/s1d2/s1d3/file1
+ * layer 2: allows s1d1/s1d2/s1d3
+ * denies s1d1/s1d2
+ * layer 3: allows s1d1
+ * layer 4: allows s1d1/s1d2
+ */
+ const struct rule layer1[] = {
+ /* Allows access to file1_s1d3 with the first layer. */
+ {
+ .path = file1_s1d3,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ {}
+ };
+ const struct rule layer2[] = {
+ /* Start by granting access to file1_s1d3 with this rule... */
+ {
+ .path = dir_s1d3,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ /* ...but finally denies access to file1_s1d3. */
+ {
+ .path = dir_s1d2,
+ .access = 0,
+ },
+ {}
+ };
+ const struct rule layer3[] = {
+ /* Try to allows access to file1_s1d3. */
+ {
+ .path = dir_s1d1,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ {}
+ };
+ const struct rule layer4[] = {
+ /* Try to bypass layer2. */
+ {
+ .path = dir_s1d2,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ {}
+ };
+ int open_fd, ruleset_fd;
+
+ ruleset_fd = create_ruleset(_metadata, ACCESS_RO, layer1);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+
+ /* Checks that access is granted for file1_s1d3. */
+ open_fd = open(file1_s1d3, O_RDONLY | O_CLOEXEC);
+ ASSERT_LE(0, open_fd);
+ EXPECT_EQ(0, close(open_fd));
+ ASSERT_EQ(-1, open(file2_s1d3, O_RDONLY | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
+
+ ruleset_fd = create_ruleset(_metadata, ACCESS_RO, layer2);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+
+ /* Now, checks that access is denied for file1_s1d3. */
+ ASSERT_EQ(-1, open(file1_s1d3, O_RDONLY | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, open(file2_s1d3, O_RDONLY | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
+
+ ruleset_fd = create_ruleset(_metadata, ACCESS_RO, layer3);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+
+ /* Checks that access rights are unchanged with layer 3. */
+ ASSERT_EQ(-1, open(file1_s1d3, O_RDONLY | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, open(file2_s1d3, O_RDONLY | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
+
+ ruleset_fd = create_ruleset(_metadata, ACCESS_RO, layer4);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+
+ /* Checks that access rights are unchanged with layer 4. */
+ ASSERT_EQ(-1, open(file1_s1d3, O_RDONLY | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, open(file2_s1d3, O_RDONLY | O_CLOEXEC));
+ ASSERT_EQ(EACCES, errno);
+}
+
TEST_F(layout1, inherit_subset)
{
const struct rule rules[] = {
--
2.29.2
On Wed, 11 Nov 2020, Mickaël Salaün wrote:
> Hi,
>
> This patch series fixes some issues and makes the Landlock filesystem
> access-control more consistent and deterministic when stacking multiple
> rulesets. This is checked by current and new tests. I also extended
> documentation and example to help users.
>
> This series can be applied on top of
> https://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git/log/?h=landlock_lsm
Actually, given the number of fixes here, please respin so we get a
cleaner initial PR for Linus.
>
> Regards,
>
> Mickaël Salaün (9):
> landlock: Fix memory allocation error handling
> landlock: Cosmetic fixes for filesystem management
> landlock: Enforce deterministic interleaved path rules
> landlock: Always intersect access rights
> landlock: Add extra checks when inserting a rule
> selftests/landlock: Extend layout1.inherit_superset
> landlock: Clean up get_ruleset_from_fd()
> landlock: Add help to enable Landlock as a stacked LSM
> landlock: Extend documentation about limitations
>
> Documentation/userspace-api/landlock.rst | 17 +++
> samples/landlock/sandboxer.c | 21 +++-
> security/landlock/Kconfig | 4 +-
> security/landlock/fs.c | 67 +++++-----
> security/landlock/object.c | 5 +-
> security/landlock/ruleset.c | 34 ++---
> security/landlock/syscall.c | 24 ++--
> tools/testing/selftests/landlock/fs_test.c | 140 +++++++++++++++++++--
> 8 files changed, 239 insertions(+), 73 deletions(-)
>
>
> base-commit: 96b3198c4025c11347651700b77e45a686d78553
>
--
James Morris
<[email protected]>