Merge tag 'landlock-6.0-rc4' of git://git.kernel.org/pub/scm/linux/kernel/git/mic...
authorLinus Torvalds <torvalds@linux-foundation.org>
Fri, 2 Sep 2022 22:24:08 +0000 (15:24 -0700)
committerLinus Torvalds <torvalds@linux-foundation.org>
Fri, 2 Sep 2022 22:24:08 +0000 (15:24 -0700)
Pull landlock fix from Mickaël Salaün:
 "This fixes a mis-handling of the LANDLOCK_ACCESS_FS_REFER right when
  multiple rulesets/domains are stacked.

  The expected behaviour was that an additional ruleset can only
  restrict the set of permitted operations, but in this particular case,
  it was potentially possible to re-gain the LANDLOCK_ACCESS_FS_REFER
  right"

* tag 'landlock-6.0-rc4' of git://git.kernel.org/pub/scm/linux/kernel/git/mic/linux:
  landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

security/landlock/fs.c
tools/testing/selftests/landlock/fs_test.c

index ec5a624..a9dbd99 100644 (file)
@@ -149,6 +149,16 @@ retry:
        LANDLOCK_ACCESS_FS_READ_FILE)
 /* clang-format on */
 
+/*
+ * All access rights that are denied by default whether they are handled or not
+ * by a ruleset/layer.  This must be ORed with all ruleset->fs_access_masks[]
+ * entries when we need to get the absolute handled access masks.
+ */
+/* clang-format off */
+#define ACCESS_INITIALLY_DENIED ( \
+       LANDLOCK_ACCESS_FS_REFER)
+/* clang-format on */
+
 /*
  * @path: Should have been checked by get_path_from_fd().
  */
@@ -167,7 +177,9 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
                return -EINVAL;
 
        /* Transforms relative access rights to absolute ones. */
-       access_rights |= LANDLOCK_MASK_ACCESS_FS & ~ruleset->fs_access_masks[0];
+       access_rights |=
+               LANDLOCK_MASK_ACCESS_FS &
+               ~(ruleset->fs_access_masks[0] | ACCESS_INITIALLY_DENIED);
        object = get_inode_object(d_backing_inode(path->dentry));
        if (IS_ERR(object))
                return PTR_ERR(object);
@@ -277,23 +289,12 @@ static inline bool is_nouser_or_private(const struct dentry *dentry)
 static inline access_mask_t
 get_handled_accesses(const struct landlock_ruleset *const domain)
 {
-       access_mask_t access_dom = 0;
-       unsigned long access_bit;
-
-       for (access_bit = 0; access_bit < LANDLOCK_NUM_ACCESS_FS;
-            access_bit++) {
-               size_t layer_level;
+       access_mask_t access_dom = ACCESS_INITIALLY_DENIED;
+       size_t layer_level;
 
-               for (layer_level = 0; layer_level < domain->num_layers;
-                    layer_level++) {
-                       if (domain->fs_access_masks[layer_level] &
-                           BIT_ULL(access_bit)) {
-                               access_dom |= BIT_ULL(access_bit);
-                               break;
-                       }
-               }
-       }
-       return access_dom;
+       for (layer_level = 0; layer_level < domain->num_layers; layer_level++)
+               access_dom |= domain->fs_access_masks[layer_level];
+       return access_dom & LANDLOCK_MASK_ACCESS_FS;
 }
 
 static inline access_mask_t
@@ -316,8 +317,13 @@ init_layer_masks(const struct landlock_ruleset *const domain,
 
                for_each_set_bit(access_bit, &access_req,
                                 ARRAY_SIZE(*layer_masks)) {
-                       if (domain->fs_access_masks[layer_level] &
-                           BIT_ULL(access_bit)) {
+                       /*
+                        * Artificially handles all initially denied by default
+                        * access rights.
+                        */
+                       if (BIT_ULL(access_bit) &
+                           (domain->fs_access_masks[layer_level] |
+                            ACCESS_INITIALLY_DENIED)) {
                                (*layer_masks)[access_bit] |=
                                        BIT_ULL(layer_level);
                                handled_accesses |= BIT_ULL(access_bit);
@@ -857,10 +863,6 @@ static int current_check_refer_path(struct dentry *const old_dentry,
                                              NULL, NULL);
        }
 
-       /* Backward compatibility: no reparenting support. */
-       if (!(get_handled_accesses(dom) & LANDLOCK_ACCESS_FS_REFER))
-               return -EXDEV;
-
        access_request_parent1 |= LANDLOCK_ACCESS_FS_REFER;
        access_request_parent2 |= LANDLOCK_ACCESS_FS_REFER;
 
index 21a2ce8..45de42a 100644 (file)
@@ -4,7 +4,7 @@
  *
  * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net>
  * Copyright © 2020 ANSSI
- * Copyright © 2020-2021 Microsoft Corporation
+ * Copyright © 2020-2022 Microsoft Corporation
  */
 
 #define _GNU_SOURCE
@@ -371,6 +371,13 @@ TEST_F_FORK(layout1, inval)
        ASSERT_EQ(EINVAL, errno);
        path_beneath.allowed_access &= ~LANDLOCK_ACCESS_FS_EXECUTE;
 
+       /* Tests with denied-by-default access right. */
+       path_beneath.allowed_access |= LANDLOCK_ACCESS_FS_REFER;
+       ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
+                                       &path_beneath, 0));
+       ASSERT_EQ(EINVAL, errno);
+       path_beneath.allowed_access &= ~LANDLOCK_ACCESS_FS_REFER;
+
        /* Test with unknown (64-bits) value. */
        path_beneath.allowed_access |= (1ULL << 60);
        ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
@@ -1826,6 +1833,20 @@ TEST_F_FORK(layout1, link)
        ASSERT_EQ(0, link(file1_s1d3, file2_s1d3));
 }
 
+static int test_rename(const char *const oldpath, const char *const newpath)
+{
+       if (rename(oldpath, newpath))
+               return errno;
+       return 0;
+}
+
+static int test_exchange(const char *const oldpath, const char *const newpath)
+{
+       if (renameat2(AT_FDCWD, oldpath, AT_FDCWD, newpath, RENAME_EXCHANGE))
+               return errno;
+       return 0;
+}
+
 TEST_F_FORK(layout1, rename_file)
 {
        const struct rule rules[] = {
@@ -1867,10 +1888,10 @@ TEST_F_FORK(layout1, rename_file)
         * to a different directory (which allows file removal).
         */
        ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d3));
-       ASSERT_EQ(EXDEV, errno);
+       ASSERT_EQ(EACCES, errno);
        ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d1, AT_FDCWD, file1_s1d3,
                                RENAME_EXCHANGE));
-       ASSERT_EQ(EXDEV, errno);
+       ASSERT_EQ(EACCES, errno);
        ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_s2d2, AT_FDCWD, file1_s1d3,
                                RENAME_EXCHANGE));
        ASSERT_EQ(EXDEV, errno);
@@ -1894,7 +1915,7 @@ TEST_F_FORK(layout1, rename_file)
        ASSERT_EQ(EXDEV, errno);
        ASSERT_EQ(0, unlink(file1_s1d3));
        ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d3));
-       ASSERT_EQ(EXDEV, errno);
+       ASSERT_EQ(EACCES, errno);
 
        /* Exchanges and renames files with same parent. */
        ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s2d3, AT_FDCWD, file1_s2d3,
@@ -2014,6 +2035,115 @@ TEST_F_FORK(layout1, reparent_refer)
        ASSERT_EQ(0, rename(dir_s1d3, dir_s2d3));
 }
 
+/* Checks renames beneath dir_s1d1. */
+static void refer_denied_by_default(struct __test_metadata *const _metadata,
+                                   const struct rule layer1[],
+                                   const int layer1_err,
+                                   const struct rule layer2[])
+{
+       int ruleset_fd;
+
+       ASSERT_EQ(0, unlink(file1_s1d2));
+
+       ruleset_fd = create_ruleset(_metadata, layer1[0].access, layer1);
+       ASSERT_LE(0, ruleset_fd);
+       enforce_ruleset(_metadata, ruleset_fd);
+       ASSERT_EQ(0, close(ruleset_fd));
+
+       /*
+        * If the first layer handles LANDLOCK_ACCESS_FS_REFER (according to
+        * layer1_err), then it allows some different-parent renames and links.
+        */
+       ASSERT_EQ(layer1_err, test_rename(file1_s1d1, file1_s1d2));
+       if (layer1_err == 0)
+               ASSERT_EQ(layer1_err, test_rename(file1_s1d2, file1_s1d1));
+       ASSERT_EQ(layer1_err, test_exchange(file2_s1d1, file2_s1d2));
+       ASSERT_EQ(layer1_err, test_exchange(file2_s1d2, file2_s1d1));
+
+       ruleset_fd = create_ruleset(_metadata, layer2[0].access, layer2);
+       ASSERT_LE(0, ruleset_fd);
+       enforce_ruleset(_metadata, ruleset_fd);
+       ASSERT_EQ(0, close(ruleset_fd));
+
+       /*
+        * Now, either the first or the second layer does not handle
+        * LANDLOCK_ACCESS_FS_REFER, which means that any different-parent
+        * renames and links are denied, thus making the layer handling
+        * LANDLOCK_ACCESS_FS_REFER null and void.
+        */
+       ASSERT_EQ(EXDEV, test_rename(file1_s1d1, file1_s1d2));
+       ASSERT_EQ(EXDEV, test_exchange(file2_s1d1, file2_s1d2));
+       ASSERT_EQ(EXDEV, test_exchange(file2_s1d2, file2_s1d1));
+}
+
+const struct rule layer_dir_s1d1_refer[] = {
+       {
+               .path = dir_s1d1,
+               .access = LANDLOCK_ACCESS_FS_REFER,
+       },
+       {},
+};
+
+const struct rule layer_dir_s1d1_execute[] = {
+       {
+               /* Matches a parent directory. */
+               .path = dir_s1d1,
+               .access = LANDLOCK_ACCESS_FS_EXECUTE,
+       },
+       {},
+};
+
+const struct rule layer_dir_s2d1_execute[] = {
+       {
+               /* Does not match a parent directory. */
+               .path = dir_s2d1,
+               .access = LANDLOCK_ACCESS_FS_EXECUTE,
+       },
+       {},
+};
+
+/*
+ * Tests precedence over renames: denied by default for different parent
+ * directories, *with* a rule matching a parent directory, but not directly
+ * denying access (with MAKE_REG nor REMOVE).
+ */
+TEST_F_FORK(layout1, refer_denied_by_default1)
+{
+       refer_denied_by_default(_metadata, layer_dir_s1d1_refer, 0,
+                               layer_dir_s1d1_execute);
+}
+
+/*
+ * Same test but this time turning around the ABI version order: the first
+ * layer does not handle LANDLOCK_ACCESS_FS_REFER.
+ */
+TEST_F_FORK(layout1, refer_denied_by_default2)
+{
+       refer_denied_by_default(_metadata, layer_dir_s1d1_execute, EXDEV,
+                               layer_dir_s1d1_refer);
+}
+
+/*
+ * Tests precedence over renames: denied by default for different parent
+ * directories, *without* a rule matching a parent directory, but not directly
+ * denying access (with MAKE_REG nor REMOVE).
+ */
+TEST_F_FORK(layout1, refer_denied_by_default3)
+{
+       refer_denied_by_default(_metadata, layer_dir_s1d1_refer, 0,
+                               layer_dir_s2d1_execute);
+}
+
+/*
+ * Same test but this time turning around the ABI version order: the first
+ * layer does not handle LANDLOCK_ACCESS_FS_REFER.
+ */
+TEST_F_FORK(layout1, refer_denied_by_default4)
+{
+       refer_denied_by_default(_metadata, layer_dir_s2d1_execute, EXDEV,
+                               layer_dir_s1d1_refer);
+}
+
 TEST_F_FORK(layout1, reparent_link)
 {
        const struct rule layer1[] = {
@@ -2336,11 +2466,12 @@ TEST_F_FORK(layout1, reparent_exdev_layers_rename1)
        ASSERT_EQ(EXDEV, errno);
 
        /*
-        * However, moving the file2_s1d3 file below dir_s2d3 is allowed
-        * because it cannot inherit MAKE_REG nor MAKE_DIR rights (which are
-        * dedicated to directories).
+        * Moving the file2_s1d3 file below dir_s2d3 is denied because the
+        * second layer does not handle REFER, which is always denied by
+        * default.
         */
-       ASSERT_EQ(0, rename(file2_s1d3, file1_s2d3));
+       ASSERT_EQ(-1, rename(file2_s1d3, file1_s2d3));
+       ASSERT_EQ(EXDEV, errno);
 }
 
 TEST_F_FORK(layout1, reparent_exdev_layers_rename2)
@@ -2373,8 +2504,12 @@ TEST_F_FORK(layout1, reparent_exdev_layers_rename2)
        ASSERT_EQ(EACCES, errno);
        ASSERT_EQ(-1, rename(file1_s1d1, file1_s2d3));
        ASSERT_EQ(EXDEV, errno);
-       /* Modify layout! */
-       ASSERT_EQ(0, rename(file2_s1d2, file1_s2d3));
+       /*
+        * Modifying the layout is now denied because the second layer does not
+        * handle REFER, which is always denied by default.
+        */
+       ASSERT_EQ(-1, rename(file2_s1d2, file1_s2d3));
+       ASSERT_EQ(EXDEV, errno);
 
        /* Without REFER source, EACCES wins over EXDEV. */
        ASSERT_EQ(-1, rename(dir_s1d1, file1_s2d2));