LinuxLists.cc - [PATCH -next 0/5] landlock: add chmod and chown support

[permalink] [raw]

Subject: [PATCH -next 4/5] landlock/samples: add chmod and chown support

update landlock sample to support the new flags
LANDLOCK_ACCESS_FS_{CHMOD, CHOWN}

Signed-off-by: Xiu Jianfeng <[email protected]>
---
samples/landlock/sandboxer.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 771b6b10d519..8c9da47e8b19 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -77,7 +77,9 @@ static int parse_path(char *env_path, const char ***const path_list)
LANDLOCK_ACCESS_FS_EXECUTE | \
LANDLOCK_ACCESS_FS_WRITE_FILE | \
LANDLOCK_ACCESS_FS_READ_FILE | \
- LANDLOCK_ACCESS_FS_TRUNCATE)
+ LANDLOCK_ACCESS_FS_TRUNCATE | \
+ LANDLOCK_ACCESS_FS_CHMOD | \
+ LANDLOCK_ACCESS_FS_CHOWN)

/* clang-format on */

@@ -162,7 +164,9 @@ static int populate_ruleset(const char *const env_var, const int ruleset_fd,
LANDLOCK_ACCESS_FS_MAKE_BLOCK | \
LANDLOCK_ACCESS_FS_MAKE_SYM | \
LANDLOCK_ACCESS_FS_REFER | \
- LANDLOCK_ACCESS_FS_TRUNCATE)
+ LANDLOCK_ACCESS_FS_TRUNCATE | \
+ LANDLOCK_ACCESS_FS_CHMOD | \
+ LANDLOCK_ACCESS_FS_CHOWN)

/* clang-format on */

@@ -233,6 +237,10 @@ int main(const int argc, char *const argv[], char *const *const envp)
case 2:
/* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
+ case 3:
+ /* Removes LANDLOCK_ACCESS_FS_{CHMOD, CHOWN} for ABI < 4 */
+ ruleset_attr.handled_access_fs &= ~(LANDLOCK_ACCESS_FS_CHMOD |
+ LANDLOCK_ACCESS_FS_CHOWN);
}
access_fs_ro &= ruleset_attr.handled_access_fs;
access_fs_rw &= ruleset_attr.handled_access_fs;
--
2.17.1

2022-08-22 11:57:59

[permalink] [raw]

Subject: [PATCH -next 3/5] landlock/selftests: add selftests for chmod and chown

Add the following simple testcases:
1. chmod/fchmod: remove S_IWUSR and restore S_IWUSR with or without
restriction.
2. chown/fchown: set original uid and gid with or without restriction,
because chown needs CAP_CHOWN and testcase framework don't have this
capability, setting original uid and gid is ok to cover landlock
function.

Signed-off-by: Xiu Jianfeng <[email protected]>
---
tools/testing/selftests/landlock/fs_test.c | 228 +++++++++++++++++++++
1 file changed, 228 insertions(+)

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 5b55b93b5570..f47b4ccd2b26 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -59,6 +59,9 @@ static const char file2_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f2";

static const char dir_s3d1[] = TMP_DIR "/s3d1";
static const char file1_s3d1[] = TMP_DIR "/s3d1/f1";
+static const char file2_s3d1[] = TMP_DIR "/s3d1/f2";
+static const char file3_s3d1[] = TMP_DIR "/s3d1/f3";
+
/* dir_s3d2 is a mount point. */
static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2";
static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
@@ -211,6 +214,8 @@ static void create_layout1(struct __test_metadata *const _metadata)
create_file(_metadata, file2_s2d3);

create_file(_metadata, file1_s3d1);
+ create_file(_metadata, file2_s3d1);
+ create_file(_metadata, file3_s3d1);
create_directory(_metadata, dir_s3d2);
set_cap(_metadata, CAP_SYS_ADMIN);
ASSERT_EQ(0, mount("tmp", dir_s3d2, "tmpfs", 0, "size=4m,mode=700"));
@@ -234,6 +239,8 @@ static void remove_layout1(struct __test_metadata *const _metadata)
EXPECT_EQ(0, remove_path(file1_s2d1));

EXPECT_EQ(0, remove_path(file1_s3d1));
+ EXPECT_EQ(0, remove_path(file2_s3d1));
+ EXPECT_EQ(0, remove_path(file3_s3d1));
EXPECT_EQ(0, remove_path(dir_s3d3));
set_cap(_metadata, CAP_SYS_ADMIN);
umount(dir_s3d2);
@@ -3272,6 +3279,227 @@ TEST_F_FORK(layout1, truncate)
EXPECT_EQ(0, test_creat(file_in_dir_w));
}

+static int test_chmod(const char *path)
+{
+ int ret;
+ struct stat st;
+ mode_t mode;
+
+ ret = stat(path, &st);
+ if (ret < 0)
+ return errno;
+ /* save original mode in order to restore */
+ mode = st.st_mode & 0777;
+ /* remove S_IWUSR */
+ ret = chmod(path, mode & ~0200);
+ if (ret < 0)
+ return errno;
+ ret = stat(path, &st);
+ if (ret < 0)
+ return errno;
+ /* check if still has S_IWUSR */
+ if (st.st_mode & 0200)
+ return -EFAULT;
+ /* restore the original mode */
+ ret = chmod(path, mode);
+ if (ret < 0)
+ return errno;
+ return 0;
+}
+
+static int test_fchmod(const char *path)
+{
+ int ret, fd;
+ struct stat st;
+ mode_t mode;
+
+ ret = stat(path, &st);
+ if (ret < 0)
+ return errno;
+ /* save original mode in order to restore */
+ mode = st.st_mode & 0777;
+
+ fd = openat(AT_FDCWD, path, O_RDWR | O_CLOEXEC);
+ if (fd < 0)
+ return errno;
+ /* remove S_IWUSR */
+ ret = fchmod(fd, mode & ~0200);
+ if (ret < 0)
+ goto err;
+ ret = stat(path, &st);
+ if (ret < 0)
+ goto err;
+ /* check if still has S_IWUSR */
+ if (st.st_mode & 0200) {
+ ret = -1;
+ errno = -EFAULT;
+ goto err;
+ }
+ /* restore the original mode */
+ ret = fchmod(fd, mode);
+err:
+ if (close(fd) < 0)
+ return errno;
+ return ret ? errno : 0;
+}
+
+static int test_chown(const char *path)
+{
+ int ret;
+ struct stat st;
+
+ ret = stat(path, &st);
+ if (ret < 0)
+ return errno;
+ /*
+ * chown needs CAP_CHOWN to modify uid and/or gid, however
+ * there is no such capability when the testcases framework
+ * setup, so just chown to original uid/gid, which can also
+ * cover the function in landlock.
+ */
+ ret = chown(path, st.st_uid, st.st_gid);
+ if (ret < 0)
+ return errno;
+ return 0;
+}
+
+static int test_fchown(const char *path)
+{
+ int ret, fd;
+ struct stat st;
+
+ ret = stat(path, &st);
+ if (ret < 0)
+ return errno;
+ fd = openat(AT_FDCWD, path, O_RDWR | O_CLOEXEC);
+ if (fd < 0)
+ return errno;
+ /*
+ * fchown needs CAP_CHOWN to modify uid and/or gid, however
+ * there is no such capability when the testcases framework
+ * setup, so just fchown to original uid/gid, which can also
+ * cover the function in landlock.
+ */
+ ret = fchown(fd, st.st_uid, st.st_gid);
+ if (close(fd) < 0)
+ return errno;
+ return ret ? errno : 0;
+}
+
+TEST_F_FORK(layout1, unhandled_chmod)
+{
+ const struct rule rules[] = {
+ {
+ .path = file2_s3d1,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_WRITE_FILE,
+ },
+ {
+ .path = file3_s3d1,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_WRITE_FILE,
+ },
+ {},
+ };
+ const int ruleset_fd =
+ create_ruleset(_metadata, ACCESS_RW, rules);
+
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ ASSERT_EQ(0, test_chmod(file2_s3d1));
+ ASSERT_EQ(0, test_fchmod(file2_s3d1));
+ ASSERT_EQ(0, test_chmod(file3_s3d1));
+ ASSERT_EQ(0, test_chmod(dir_s3d1));
+}
+
+TEST_F_FORK(layout1, chmod)
+{
+ const struct rule rules[] = {
+ {
+ .path = file2_s3d1,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_WRITE_FILE |
+ LANDLOCK_ACCESS_FS_CHMOD,
+ },
+ {
+ .path = file3_s3d1,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_WRITE_FILE,
+ },
+ {},
+ };
+ const int ruleset_fd =
+ create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHMOD, rules);
+
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ ASSERT_EQ(0, test_chmod(file2_s3d1));
+ ASSERT_EQ(0, test_fchmod(file2_s3d1));
+ ASSERT_EQ(EACCES, test_chmod(file3_s3d1));
+ ASSERT_EQ(EACCES, test_chmod(dir_s3d1));
+}
+
+TEST_F_FORK(layout1, no_chown)
+{
+ const struct rule rules[] = {
+ {
+ .path = file2_s3d1,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_WRITE_FILE,
+ },
+ {
+ .path = file3_s3d1,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_WRITE_FILE,
+ },
+ {},
+ };
+ const int ruleset_fd =
+ create_ruleset(_metadata, ACCESS_RW, rules);
+
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ ASSERT_EQ(0, test_chown(file2_s3d1));
+ ASSERT_EQ(0, test_fchown(file2_s3d1));
+ ASSERT_EQ(0, test_chown(file3_s3d1));
+ ASSERT_EQ(0, test_chown(dir_s3d1));
+}
+
+TEST_F_FORK(layout1, chown)
+{
+ const struct rule rules[] = {
+ {
+ .path = file2_s3d1,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_WRITE_FILE |
+ LANDLOCK_ACCESS_FS_CHOWN,
+ },
+ {
+ .path = file3_s3d1,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_WRITE_FILE,
+ },
+ {},
+ };
+ const int ruleset_fd =
+ create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHOWN, rules);
+
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ ASSERT_EQ(0, test_chown(file2_s3d1));
+ ASSERT_EQ(0, test_fchown(file2_s3d1));
+ ASSERT_EQ(EACCES, test_chown(file3_s3d1));
+ ASSERT_EQ(EACCES, test_chown(dir_s3d1));
+}
+
/* clang-format off */
FIXTURE(layout1_bind) {};
/* clang-format on */
--
2.17.1

2022-08-22 11:58:04

[permalink] [raw]

Subject: [PATCH -next 2/5] landlock: add chmod and chown support

Add two flags LANDLOCK_ACCESS_FS_CHMOD and LANDLOCK_ACCESS_FS_CHOWN to
support restriction to chmod(2) and chown(2) with landlock.

Also change the landlock ABI version from 3 to 4.

Signed-off-by: Xiu Jianfeng <[email protected]>
---
include/uapi/linux/landlock.h | 8 ++++++--
security/landlock/fs.c | 16 +++++++++++++++-
security/landlock/limits.h | 2 +-
security/landlock/syscalls.c | 2 +-
tools/testing/selftests/landlock/base_test.c | 2 +-
tools/testing/selftests/landlock/fs_test.c | 6 ++++--
6 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 735b1fe8326e..5ce633c92722 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -141,13 +141,15 @@ struct landlock_path_beneath_attr {
* directory) parent. Otherwise, such actions are denied with errno set to
* EACCES. The EACCES errno prevails over EXDEV to let user space
* efficiently deal with an unrecoverable error.
+ * - %LANDLOCK_ACCESS_FS_CHMOD: Change the file mode bits of a file.
+ * - %LANDLOCK_ACCESS_FS_CHOWN: Change the owner and/or group of a file.
*
* .. warning::
*
* It is currently not possible to restrict some file-related actions
* accessible through these syscall families: :manpage:`chdir(2)`,
- * :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
- * :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
+ * :manpage:`stat(2)`, :manpage:`flock(2)`,
+ * :manpage:`setxattr(2)`, :manpage:`utime(2)`,
* :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
* Future Landlock evolutions will enable to restrict them.
*/
@@ -167,6 +169,8 @@ struct landlock_path_beneath_attr {
#define LANDLOCK_ACCESS_FS_MAKE_SYM (1ULL << 12)
#define LANDLOCK_ACCESS_FS_REFER (1ULL << 13)
#define LANDLOCK_ACCESS_FS_TRUNCATE (1ULL << 14)
+#define LANDLOCK_ACCESS_FS_CHMOD (1ULL << 15)
+#define LANDLOCK_ACCESS_FS_CHOWN (1ULL << 16)
/* clang-format on */

#endif /* _UAPI_LINUX_LANDLOCK_H */
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index c57f581a9cd5..c25d5f89c8be 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -147,7 +147,9 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
LANDLOCK_ACCESS_FS_EXECUTE | \
LANDLOCK_ACCESS_FS_WRITE_FILE | \
LANDLOCK_ACCESS_FS_READ_FILE | \
- LANDLOCK_ACCESS_FS_TRUNCATE)
+ LANDLOCK_ACCESS_FS_TRUNCATE | \
+ LANDLOCK_ACCESS_FS_CHMOD | \
+ LANDLOCK_ACCESS_FS_CHOWN)
/* clang-format on */

/*
@@ -1146,6 +1148,16 @@ static int hook_path_truncate(const struct path *const path)
return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
}

+static int hook_path_chmod(const struct path *const dir, umode_t mode)
+{
+ return current_check_access_path(dir, LANDLOCK_ACCESS_FS_CHMOD);
+}
+
+static int hook_path_chown(const struct path *const dir, kuid_t uid, kgid_t gid)
+{
+ return current_check_access_path(dir, LANDLOCK_ACCESS_FS_CHOWN);
+}
+
/* File hooks */

static inline access_mask_t get_file_access(const struct file *const file)
@@ -1199,6 +1211,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(path_unlink, hook_path_unlink),
LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
LSM_HOOK_INIT(path_truncate, hook_path_truncate),
+ LSM_HOOK_INIT(path_chmod, hook_path_chmod),
+ LSM_HOOK_INIT(path_chown, hook_path_chown),

LSM_HOOK_INIT(file_open, hook_file_open),
};
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 82288f0e9e5e..08858da7fb4f 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -18,7 +18,7 @@
#define LANDLOCK_MAX_NUM_LAYERS 16
#define LANDLOCK_MAX_NUM_RULES U32_MAX

-#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_TRUNCATE
+#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_CHOWN
#define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
#define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS)

diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index f4d6fc7ed17f..469e0e11735c 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
.write = fop_dummy_write,
};

-#define LANDLOCK_ABI_VERSION 3
+#define LANDLOCK_ABI_VERSION 4

/**
* sys_landlock_create_ruleset - Create a new ruleset
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 72cdae277b02..9f00582f639c 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -75,7 +75,7 @@ TEST(abi_version)
const struct landlock_ruleset_attr ruleset_attr = {
.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
};
- ASSERT_EQ(3, landlock_create_ruleset(NULL, 0,
+ ASSERT_EQ(4, landlock_create_ruleset(NULL, 0,
LANDLOCK_CREATE_RULESET_VERSION));

ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index debe2d9ea6cf..5b55b93b5570 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -404,9 +404,11 @@ TEST_F_FORK(layout1, inval)
LANDLOCK_ACCESS_FS_EXECUTE | \
LANDLOCK_ACCESS_FS_WRITE_FILE | \
LANDLOCK_ACCESS_FS_READ_FILE | \
- LANDLOCK_ACCESS_FS_TRUNCATE)
+ LANDLOCK_ACCESS_FS_TRUNCATE | \
+ LANDLOCK_ACCESS_FS_CHMOD | \
+ LANDLOCK_ACCESS_FS_CHOWN)

-#define ACCESS_LAST LANDLOCK_ACCESS_FS_TRUNCATE
+#define ACCESS_LAST LANDLOCK_ACCESS_FS_CHOWN

#define ACCESS_ALL ( \
ACCESS_FILE | \
--
2.17.1

2022-08-22 11:58:10

[permalink] [raw]

Subject: [PATCH -next 5/5] landlock: update chmod and chown support in document

update LANDLOCK_ACCESS_FS_{CHMOD, CHOWN} support and add abi change
in the document.

Signed-off-by: Xiu Jianfeng <[email protected]>
---
Documentation/userspace-api/landlock.rst | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
index 2509c2fbf98f..05ab338db529 100644
--- a/Documentation/userspace-api/landlock.rst
+++ b/Documentation/userspace-api/landlock.rst
@@ -61,7 +61,9 @@ the need to be explicit about the denied-by-default access rights.
LANDLOCK_ACCESS_FS_MAKE_BLOCK |
LANDLOCK_ACCESS_FS_MAKE_SYM |
LANDLOCK_ACCESS_FS_REFER |
- LANDLOCK_ACCESS_FS_TRUNCATE,
+ LANDLOCK_ACCESS_FS_TRUNCATE |
+ LANDLOCK_ACCESS_FS_CHMOD |
+ LANDLOCK_ACCESS_FS_CHOWN
};

Because we may not know on which kernel version an application will be
@@ -90,6 +92,10 @@ the ABI.
case 2:
/* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;
+ case 3:
+ /* Removes LANDLOCK_ACCESS_FS_{CHMOD, CHOWN} for ABI < 4 */
+ ruleset_attr.handled_access_fs &= ~(LANDLOCK_ACCESS_FS_CHMOD |
+ LANDLOCK_ACCESS_FS_CHOWN);
}

This enables to create an inclusive ruleset that will contain our rules.
--
2.17.1

2022-08-22 12:51:21

[permalink] [raw]

Subject: [PATCH -next 1/5] landlock: expand access_mask_t to u32 type

u16 is not enough to add more types of restritions, so expand it to u32

Signed-off-by: Xiu Jianfeng <[email protected]>
---
security/landlock/ruleset.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index d43231b783e4..607b3dc0ef19 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -19,7 +19,7 @@
#include "limits.h"
#include "object.h"

-typedef u16 access_mask_t;
+typedef u32 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. */
--
2.17.1

2022-08-22 18:39:04

[permalink] [raw]

Subject: Re: [PATCH -next 2/5] landlock: add chmod and chown support

Hi!

Thanks for sending this patch set! :)

On Mon, Aug 22, 2022 at 07:46:58PM +0800, Xiu Jianfeng wrote:
> Add two flags LANDLOCK_ACCESS_FS_CHMOD and LANDLOCK_ACCESS_FS_CHOWN to
> support restriction to chmod(2) and chown(2) with landlock.
>
> Also change the landlock ABI version from 3 to 4.
>
> Signed-off-by: Xiu Jianfeng <[email protected]>
> ---
> include/uapi/linux/landlock.h | 8 ++++++--
> security/landlock/fs.c | 16 +++++++++++++++-
> security/landlock/limits.h | 2 +-
> security/landlock/syscalls.c | 2 +-
> tools/testing/selftests/landlock/base_test.c | 2 +-
> tools/testing/selftests/landlock/fs_test.c | 6 ++++--
> 6 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 735b1fe8326e..5ce633c92722 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -141,13 +141,15 @@ struct landlock_path_beneath_attr {
> * directory) parent. Otherwise, such actions are denied with errno set to
> * EACCES. The EACCES errno prevails over EXDEV to let user space
> * efficiently deal with an unrecoverable error.
> + * - %LANDLOCK_ACCESS_FS_CHMOD: Change the file mode bits of a file.
> + * - %LANDLOCK_ACCESS_FS_CHOWN: Change the owner and/or group of a file.
> *
> * .. warning::
> *
> * It is currently not possible to restrict some file-related actions
> * accessible through these syscall families: :manpage:`chdir(2)`,
> - * :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
> - * :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
> + * :manpage:`stat(2)`, :manpage:`flock(2)`,
> + * :manpage:`setxattr(2)`, :manpage:`utime(2)`,

*formatting nit*
We could fill up the full line width here

> * :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
> * Future Landlock evolutions will enable to restrict them.
> */
> @@ -167,6 +169,8 @@ struct landlock_path_beneath_attr {
> #define LANDLOCK_ACCESS_FS_MAKE_SYM (1ULL << 12)
> #define LANDLOCK_ACCESS_FS_REFER (1ULL << 13)
> #define LANDLOCK_ACCESS_FS_TRUNCATE (1ULL << 14)
> +#define LANDLOCK_ACCESS_FS_CHMOD (1ULL << 15)
> +#define LANDLOCK_ACCESS_FS_CHOWN (1ULL << 16)
> /* clang-format on */
>
> #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index c57f581a9cd5..c25d5f89c8be 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -147,7 +147,9 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
> LANDLOCK_ACCESS_FS_EXECUTE | \
> LANDLOCK_ACCESS_FS_WRITE_FILE | \
> LANDLOCK_ACCESS_FS_READ_FILE | \
> - LANDLOCK_ACCESS_FS_TRUNCATE)
> + LANDLOCK_ACCESS_FS_TRUNCATE | \
> + LANDLOCK_ACCESS_FS_CHMOD | \
> + LANDLOCK_ACCESS_FS_CHOWN)
> /* clang-format on */
>
> /*
> @@ -1146,6 +1148,16 @@ static int hook_path_truncate(const struct path *const path)
> return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
> }
>
> +static int hook_path_chmod(const struct path *const dir, umode_t mode)
> +{
> + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_CHMOD);
> +}
> +
> +static int hook_path_chown(const struct path *const dir, kuid_t uid, kgid_t gid)
> +{
> + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_CHOWN);
> +}

One implication of this approach is that the chown+chmod right on a
directory's contents are always going together with the same rights on
the directory itself.

For example, if you grant chmod+chown access rights for "datadir/",
the command "chmod 0600 datadir/file1" will work, but so will the
command "chmod 0600 datadir". But the approach of checking just the
parent directory's rights is also inflexible if you think through the
kinds of rights you can grant with it. (It would also not be possible
to grant chmod+chown on individual files.)

Do you have any thoughts on how to resolve this if this flexibility
might be needed?

I wonder whether the right way to resolve this would be to give users
a way to make that distinction at the level of landlock_add_rule(),
with an API like this (note the additional flag):

err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
&path_beneath, LANDLOCK_STRICTLY_BENEATH);
^^^^^^^^^^^^^^^^^^^^^^^^^

Multiple calls of landlock_add_rule() on the same file are already
today joining the requested access rights, so it would be possible to
mix-and-match "strict beneath" with "beneath" rights on the same
directory, and it would work in the same way for other access rights
as well.

To be clear: I'm proposing this approach not because I think it should
be part of this patch set, but because it would be good to have a way
forward if that kind of flexibility is needed in the future.

Does that seem reasonable?

> +
> /* File hooks */
>
> static inline access_mask_t get_file_access(const struct file *const file)
> @@ -1199,6 +1211,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(path_unlink, hook_path_unlink),
> LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
> LSM_HOOK_INIT(path_truncate, hook_path_truncate),
> + LSM_HOOK_INIT(path_chmod, hook_path_chmod),
> + LSM_HOOK_INIT(path_chown, hook_path_chown),
>
> LSM_HOOK_INIT(file_open, hook_file_open),
> };
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 82288f0e9e5e..08858da7fb4f 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -18,7 +18,7 @@
> #define LANDLOCK_MAX_NUM_LAYERS 16
> #define LANDLOCK_MAX_NUM_RULES U32_MAX
>
> -#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_TRUNCATE
> +#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_CHOWN
> #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
> #define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index f4d6fc7ed17f..469e0e11735c 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
> .write = fop_dummy_write,
> };
>
> -#define LANDLOCK_ABI_VERSION 3
> +#define LANDLOCK_ABI_VERSION 4

ABI version 3 has not made it into a stable kernel yet; I wonder
whether it wouldn't be easier to just bundle the truncate, chmod and
chown rights as part of ABI version 3 (assuming that the patches make
it into a stable release together)?

Mickaël, do you have an opinion on this?

—Günther

--

2022-08-22 19:22:56

[permalink] [raw]

Subject: Re: [PATCH -next 0/5] landlock: add chmod and chown support

Hi!

Very exciting to see! Thank you for sending this! :)

I'm just throwing in some comments based on the very similar truncate
patch set, in the hope that it helps. (But obviously, Mickaël Salaün
has the last word on this code.)

Slightly higher level question: Should we start to group the
functionality of multiple LSM hooks under one Landlock flag? (Will it
be harder to change the LSM hook interface in the future if we
continue to add one flag per hook? Or is this structure already
exposed to userspace by other LSMs?)

For example, some of the "missing" operations listed on the Landlock
documentation could also be grouped roughly as:

Modifying files:
- truncate

Modifying file metadata:
- chmod
- chown
- setxattr
- utime

Observing files (check presence and file metadata):
- access
- stat
- readlink, following links (can observe symlink presence)
- chdir (can observe dir presence and 'x' attribute)

Ungrouped:
- flock
- ioctl
- fcntl

Do you have opinions on this?

—Günther

On Mon, Aug 22, 2022 at 07:46:56PM +0800, Xiu Jianfeng wrote:
> hi,
> this patchset adds chmod and chown support for landlock
>
> Xiu Jianfeng (5):
> landlock: expand access_mask_t to u32 type
> landlock: add chmod and chown support
> landlock/selftests: add selftests for chmod and chown
> landlock/samples: add chmod and chown support
> landlock: update chmod and chown support in document
>
> Documentation/userspace-api/landlock.rst | 8 +-
> include/uapi/linux/landlock.h | 8 +-
> samples/landlock/sandboxer.c | 12 +-
> security/landlock/fs.c | 16 +-
> security/landlock/limits.h | 2 +-
> security/landlock/ruleset.h | 2 +-
> security/landlock/syscalls.c | 2 +-
> tools/testing/selftests/landlock/base_test.c | 2 +-
> tools/testing/selftests/landlock/fs_test.c | 234 ++++++++++++++++++-
> 9 files changed, 274 insertions(+), 12 deletions(-)
>
> --
> 2.17.1
>

--

2022-08-22 19:37:54

[permalink] [raw]

Subject: Re: [PATCH -next 3/5] landlock/selftests: add selftests for chmod and chown

On Mon, Aug 22, 2022 at 07:46:59PM +0800, Xiu Jianfeng wrote:
> Add the following simple testcases:
> 1. chmod/fchmod: remove S_IWUSR and restore S_IWUSR with or without
> restriction.
> 2. chown/fchown: set original uid and gid with or without restriction,
> because chown needs CAP_CHOWN and testcase framework don't have this
> capability, setting original uid and gid is ok to cover landlock
> function.
>
> Signed-off-by: Xiu Jianfeng <[email protected]>
> ---
> tools/testing/selftests/landlock/fs_test.c | 228 +++++++++++++++++++++
> 1 file changed, 228 insertions(+)
>
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 5b55b93b5570..f47b4ccd2b26 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -59,6 +59,9 @@ static const char file2_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f2";
>
> static const char dir_s3d1[] = TMP_DIR "/s3d1";
> static const char file1_s3d1[] = TMP_DIR "/s3d1/f1";
> +static const char file2_s3d1[] = TMP_DIR "/s3d1/f2";
> +static const char file3_s3d1[] = TMP_DIR "/s3d1/f3";
> +
> /* dir_s3d2 is a mount point. */
> static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2";
> static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
> @@ -211,6 +214,8 @@ static void create_layout1(struct __test_metadata *const _metadata)
> create_file(_metadata, file2_s2d3);
>
> create_file(_metadata, file1_s3d1);
> + create_file(_metadata, file2_s3d1);
> + create_file(_metadata, file3_s3d1);
> create_directory(_metadata, dir_s3d2);
> set_cap(_metadata, CAP_SYS_ADMIN);
> ASSERT_EQ(0, mount("tmp", dir_s3d2, "tmpfs", 0, "size=4m,mode=700"));
> @@ -234,6 +239,8 @@ static void remove_layout1(struct __test_metadata *const _metadata)
> EXPECT_EQ(0, remove_path(file1_s2d1));
>
> EXPECT_EQ(0, remove_path(file1_s3d1));
> + EXPECT_EQ(0, remove_path(file2_s3d1));
> + EXPECT_EQ(0, remove_path(file3_s3d1));
> EXPECT_EQ(0, remove_path(dir_s3d3));
> set_cap(_metadata, CAP_SYS_ADMIN);
> umount(dir_s3d2);
> @@ -3272,6 +3279,227 @@ TEST_F_FORK(layout1, truncate)
> EXPECT_EQ(0, test_creat(file_in_dir_w));
> }
>
> +static int test_chmod(const char *path)

Nitpicks:
- const char *const path
- short documentation? :)

> +{
> + int ret;
> + struct stat st;
> + mode_t mode;
> +
> + ret = stat(path, &st);
> + if (ret < 0)
> + return errno;
> + /* save original mode in order to restore */
> + mode = st.st_mode & 0777;
> + /* remove S_IWUSR */
> + ret = chmod(path, mode & ~0200);
> + if (ret < 0)
> + return errno;
> + ret = stat(path, &st);
> + if (ret < 0)
> + return errno;
> + /* check if still has S_IWUSR */
> + if (st.st_mode & 0200)
> + return -EFAULT;
> + /* restore the original mode */
> + ret = chmod(path, mode);
> + if (ret < 0)
> + return errno;
> + return 0;
> +}

I would argue this can be simpler, with the following reasoning:

- Does the file have the right mode after chmod()?

I claim that fs_test should care only about the question of whether
EACCES is returned or not. If fs_test were to also check for the
side effects of these operations, it would eventually contain tests
for the full file system API, not just for Landlock. That seems out
of scope :)

- Undoing the chmod() operation

I'm not sure whether it's worth the effort to restore the exact
state before that function returns. As long as the flags suffice to
remove the test directory at the end, it probably doesn't matter
much what exact mode they have?

I think this could just be

if (chmod(path, mode) < 0)
return errno;
return 0

and it would be a bit simpler to understand :)

The same argument applies also to the other test_...() functions.

> +static int test_fchmod(const char *path)

I initially took the same approach for test_ftruncate() but eventually
settled on using an approach where the file is open()ed before
restricting the thread with Landlock. This eliminates the potential
confusion where test_ftruncate() returns an error but the caller can't
distinguish whether the error is from open() or from ftruncate(). It
also makes fchmod testable even in scenarios where the file cannot be
opened because of missing Landlock rights.

> +{
> + int ret, fd;
> + struct stat st;
> + mode_t mode;
> +
> + ret = stat(path, &st);
> + if (ret < 0)
> + return errno;
> + /* save original mode in order to restore */
> + mode = st.st_mode & 0777;
> +
> + fd = openat(AT_FDCWD, path, O_RDWR | O_CLOEXEC);
> + if (fd < 0)
> + return errno;
> + /* remove S_IWUSR */
> + ret = fchmod(fd, mode & ~0200);
> + if (ret < 0)
> + goto err;
> + ret = stat(path, &st);
> + if (ret < 0)
> + goto err;
> + /* check if still has S_IWUSR */
> + if (st.st_mode & 0200) {
> + ret = -1;
> + errno = -EFAULT;
> + goto err;
> + }
> + /* restore the original mode */
> + ret = fchmod(fd, mode);
> +err:
> + if (close(fd) < 0)
> + return errno;
> + return ret ? errno : 0;
> +}

> +static int test_chown(const char *path)
> +{
> + int ret;
> + struct stat st;
> +
> + ret = stat(path, &st);
> + if (ret < 0)
> + return errno;
> + /*
> + * chown needs CAP_CHOWN to modify uid and/or gid, however
> + * there is no such capability when the testcases framework
> + * setup, so just chown to original uid/gid, which can also
> + * cover the function in landlock.
> + */
> + ret = chown(path, st.st_uid, st.st_gid);
> + if (ret < 0)
> + return errno;
> + return 0;
> +}
> +
> +static int test_fchown(const char *path)
> +{
> + int ret, fd;
> + struct stat st;
> +
> + ret = stat(path, &st);
> + if (ret < 0)
> + return errno;
> + fd = openat(AT_FDCWD, path, O_RDWR | O_CLOEXEC);
> + if (fd < 0)
> + return errno;
> + /*
> + * fchown needs CAP_CHOWN to modify uid and/or gid, however
> + * there is no such capability when the testcases framework
> + * setup, so just fchown to original uid/gid, which can also
> + * cover the function in landlock.
> + */
> + ret = fchown(fd, st.st_uid, st.st_gid);
> + if (close(fd) < 0)
> + return errno;
> + return ret ? errno : 0;
> +}
> +
> +TEST_F_FORK(layout1, unhandled_chmod)
> +{
> + const struct rule rules[] = {
> + {
> + .path = file2_s3d1,
> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_WRITE_FILE,
> + },
> + {
> + .path = file3_s3d1,
> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_WRITE_FILE,
> + },
> + {},
> + };
> + const int ruleset_fd =
> + create_ruleset(_metadata, ACCESS_RW, rules);
> +
> + ASSERT_LE(0, ruleset_fd);
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + ASSERT_EQ(0, test_chmod(file2_s3d1));
> + ASSERT_EQ(0, test_fchmod(file2_s3d1));
> + ASSERT_EQ(0, test_chmod(file3_s3d1));
> + ASSERT_EQ(0, test_chmod(dir_s3d1));

*optional* because the existing tests are already inconsistent about it

These four ASSERT_EQ() calls are independent scenarios and could be
done with EXPECT_EQ(), which would be more in line with the approach
that this test framework takes. (Same for the other tests below)

Compare previous discussion at:
https://lore.kernel.org/all/Yvd3+fy+mDBop+YA@nuc/

> +}
> +
> +TEST_F_FORK(layout1, chmod)
> +{
> + const struct rule rules[] = {
> + {
> + .path = file2_s3d1,
> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_WRITE_FILE |
> + LANDLOCK_ACCESS_FS_CHMOD,
> + },
> + {
> + .path = file3_s3d1,
> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_WRITE_FILE,
> + },
> + {},
> + };
> + const int ruleset_fd =
> + create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHMOD, rules);
> +
> + ASSERT_LE(0, ruleset_fd);
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + ASSERT_EQ(0, test_chmod(file2_s3d1));
> + ASSERT_EQ(0, test_fchmod(file2_s3d1));
> + ASSERT_EQ(EACCES, test_chmod(file3_s3d1));
> + ASSERT_EQ(EACCES, test_chmod(dir_s3d1));
> +}
> +
> +TEST_F_FORK(layout1, no_chown)

"unhandled_chown" to be consistent with the other one above?

> +{
> + const struct rule rules[] = {
> + {
> + .path = file2_s3d1,
> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_WRITE_FILE,
> + },
> + {
> + .path = file3_s3d1,
> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_WRITE_FILE,
> + },
> + {},
> + };
> + const int ruleset_fd =
> + create_ruleset(_metadata, ACCESS_RW, rules);
> +
> + ASSERT_LE(0, ruleset_fd);
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + ASSERT_EQ(0, test_chown(file2_s3d1));
> + ASSERT_EQ(0, test_fchown(file2_s3d1));
> + ASSERT_EQ(0, test_chown(file3_s3d1));
> + ASSERT_EQ(0, test_chown(dir_s3d1));
> +}
> +
> +TEST_F_FORK(layout1, chown)
> +{
> + const struct rule rules[] = {
> + {
> + .path = file2_s3d1,
> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_WRITE_FILE |
> + LANDLOCK_ACCESS_FS_CHOWN,

It might be useful to also check a scenario where the chown right is
granted on a directory (and as a consequence, both the directory
itself as well as its contents can be chowned)? (Same for chmod)

> + },
> + {
> + .path = file3_s3d1,
> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_WRITE_FILE,
> + },
> + {},
> + };
> + const int ruleset_fd =
> + create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHOWN, rules);
> +
> + ASSERT_LE(0, ruleset_fd);
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + ASSERT_EQ(0, test_chown(file2_s3d1));
> + ASSERT_EQ(0, test_fchown(file2_s3d1));
> + ASSERT_EQ(EACCES, test_chown(file3_s3d1));
> + ASSERT_EQ(EACCES, test_chown(dir_s3d1));
> +}
> +
> /* clang-format off */
> FIXTURE(layout1_bind) {};
> /* clang-format on */
> --
> 2.17.1
>

--

2022-08-22 20:09:35

by Casey Schaufler

[permalink] [raw]

Subject: Re: [PATCH -next 0/5] landlock: add chmod and chown support

On 8/22/2022 12:17 PM, Günther Noack wrote:
> Hi!
>
> Very exciting to see! Thank you for sending this! :)
>
> I'm just throwing in some comments based on the very similar truncate
> patch set, in the hope that it helps. (But obviously, Mickaël Salaün
> has the last word on this code.)
>
> Slightly higher level question: Should we start to group the
> functionality of multiple LSM hooks under one Landlock flag? (Will it
> be harder to change the LSM hook interface in the future if we
> continue to add one flag per hook? Or is this structure already
> exposed to userspace by other LSMs?)

I'm not a landlock expert. The question is nonsensical, yet somewhat
frightening nonetheless. Could you put just a touch more context into
what you're asking for?

> For example, some of the "missing" operations listed on the Landlock
> documentation could also be grouped roughly as:
>
> Modifying files:
> - truncate
>
> Modifying file metadata:
> - chmod
> - chown
> - setxattr
> - utime
>
> Observing files (check presence and file metadata):
> - access
> - stat
> - readlink, following links (can observe symlink presence)
> - chdir (can observe dir presence and 'x' attribute)
>
> Ungrouped:
> - flock
> - ioctl
> - fcntl
>
> Do you have opinions on this?
>
> —Günther
>
> On Mon, Aug 22, 2022 at 07:46:56PM +0800, Xiu Jianfeng wrote:
>> hi,
>> this patchset adds chmod and chown support for landlock
>>
>> Xiu Jianfeng (5):
>> landlock: expand access_mask_t to u32 type
>> landlock: add chmod and chown support
>> landlock/selftests: add selftests for chmod and chown
>> landlock/samples: add chmod and chown support
>> landlock: update chmod and chown support in document
>>
>> Documentation/userspace-api/landlock.rst | 8 +-
>> include/uapi/linux/landlock.h | 8 +-
>> samples/landlock/sandboxer.c | 12 +-
>> security/landlock/fs.c | 16 +-
>> security/landlock/limits.h | 2 +-
>> security/landlock/ruleset.h | 2 +-
>> security/landlock/syscalls.c | 2 +-
>> tools/testing/selftests/landlock/base_test.c | 2 +-
>> tools/testing/selftests/landlock/fs_test.c | 234 ++++++++++++++++++-
>> 9 files changed, 274 insertions(+), 12 deletions(-)
>>
>> --
>> 2.17.1
>>
> --

2022-08-22 20:11:15

[permalink] [raw]

Subject: Re: [PATCH -next 3/5] landlock/selftests: add selftests for chmod and chown

On Mon, Aug 22, 2022 at 07:46:59PM +0800, Xiu Jianfeng wrote:
> +TEST_F_FORK(layout1, unhandled_chmod)
> +{
> + const struct rule rules[] = {
> + {
> + .path = file2_s3d1,
> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_WRITE_FILE,
> + },
> + {
> + .path = file3_s3d1,
> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
> + LANDLOCK_ACCESS_FS_WRITE_FILE,
> + },
> + {},
> + };
> + const int ruleset_fd =
> + create_ruleset(_metadata, ACCESS_RW, rules);
> +
> + ASSERT_LE(0, ruleset_fd);
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + ASSERT_EQ(0, test_chmod(file2_s3d1));
> + ASSERT_EQ(0, test_fchmod(file2_s3d1));
> + ASSERT_EQ(0, test_chmod(file3_s3d1));
> + ASSERT_EQ(0, test_chmod(dir_s3d1));
> +}

I missed it in the previous mail:

There are also the chown variants lchown() and fchownat(), as well as
the chmod variant fchmodat(), which might be interesting to test,
especially the symlink scenarios.

fchmodat() has a AT_SYMLINK_NOFOLLOW flag which does the chmod
equivalent to lchown().

--

2022-08-22 21:37:29

[permalink] [raw]

Subject: Re: [PATCH -next 2/5] landlock: add chmod and chown support

On 22/08/2022 20:25, Günther Noack wrote:
> Hi!
>
> Thanks for sending this patch set! :)
>
> On Mon, Aug 22, 2022 at 07:46:58PM +0800, Xiu Jianfeng wrote:
>> Add two flags LANDLOCK_ACCESS_FS_CHMOD and LANDLOCK_ACCESS_FS_CHOWN to
>> support restriction to chmod(2) and chown(2) with landlock.
>>
>> Also change the landlock ABI version from 3 to 4.
>>
>> Signed-off-by: Xiu Jianfeng <[email protected]>
>> ---
>> include/uapi/linux/landlock.h | 8 ++++++--
>> security/landlock/fs.c | 16 +++++++++++++++-
>> security/landlock/limits.h | 2 +-
>> security/landlock/syscalls.c | 2 +-
>> tools/testing/selftests/landlock/base_test.c | 2 +-
>> tools/testing/selftests/landlock/fs_test.c | 6 ++++--
>> 6 files changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>> index 735b1fe8326e..5ce633c92722 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -141,13 +141,15 @@ struct landlock_path_beneath_attr {
>> * directory) parent. Otherwise, such actions are denied with errno set to
>> * EACCES. The EACCES errno prevails over EXDEV to let user space
>> * efficiently deal with an unrecoverable error.
>> + * - %LANDLOCK_ACCESS_FS_CHMOD: Change the file mode bits of a file.
>> + * - %LANDLOCK_ACCESS_FS_CHOWN: Change the owner and/or group of a file.

This section talk about "access rights that only apply to the content of
a directory, not the directory itself", which is not correct (see
LANDLOCK_ACCESS_FS_READ_DIR). I'd like these access rights to remain
here but this kernel patch and the related tests need some changes.

What about a LANDLOCK_ACCESS_FS_CHGRP? I'm not sure if we need to
differentiate these actions or not, but we need arguments to choose.

>> *
>> * .. warning::
>> *
>> * It is currently not possible to restrict some file-related actions
>> * accessible through these syscall families: :manpage:`chdir(2)`,
>> - * :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
>> - * :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
>> + * :manpage:`stat(2)`, :manpage:`flock(2)`,
>> + * :manpage:`setxattr(2)`, :manpage:`utime(2)`,
>
> *formatting nit*
> We could fill up the full line width here
>
>> * :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
>> * Future Landlock evolutions will enable to restrict them.
>> */
>> @@ -167,6 +169,8 @@ struct landlock_path_beneath_attr {
>> #define LANDLOCK_ACCESS_FS_MAKE_SYM (1ULL << 12)
>> #define LANDLOCK_ACCESS_FS_REFER (1ULL << 13)
>> #define LANDLOCK_ACCESS_FS_TRUNCATE (1ULL << 14)
>> +#define LANDLOCK_ACCESS_FS_CHMOD (1ULL << 15)
>> +#define LANDLOCK_ACCESS_FS_CHOWN (1ULL << 16)
>> /* clang-format on */
>>
>> #endif /* _UAPI_LINUX_LANDLOCK_H */
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index c57f581a9cd5..c25d5f89c8be 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>> @@ -147,7 +147,9 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>> LANDLOCK_ACCESS_FS_EXECUTE | \
>> LANDLOCK_ACCESS_FS_WRITE_FILE | \
>> LANDLOCK_ACCESS_FS_READ_FILE | \
>> - LANDLOCK_ACCESS_FS_TRUNCATE)
>> + LANDLOCK_ACCESS_FS_TRUNCATE | \
>> + LANDLOCK_ACCESS_FS_CHMOD | \
>> + LANDLOCK_ACCESS_FS_CHOWN)
>> /* clang-format on */
>>
>> /*
>> @@ -1146,6 +1148,16 @@ static int hook_path_truncate(const struct path *const path)
>> return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
>> }
>>
>> +static int hook_path_chmod(const struct path *const dir, umode_t mode)

This is not a "dir" but a "path".

>> +{
>> + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_CHMOD);
>> +}
>> +
>> +static int hook_path_chown(const struct path *const dir, kuid_t uid, kgid_t gid)

Same here.

>> +{
>> + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_CHOWN);
>> +}
>
> One implication of this approach is that the chown+chmod right on a
> directory's contents are always going together with the same rights on
> the directory itself.
>
> For example, if you grant chmod+chown access rights for "datadir/",
> the command "chmod 0600 datadir/file1" will work, but so will the
> command "chmod 0600 datadir". But the approach of checking just the
> parent directory's rights is also inflexible if you think through the
> kinds of rights you can grant with it. (It would also not be possible
> to grant chmod+chown on individual files.)

Good point. For an initial chmod/chown/chgrp access right, I'd prefer to
be able to set these access rights on a directory but only for its
content, not the directory itself. I think it is much safer and should
be enough for the majority of use cases, but let me know if I'm missing
something. I'm not sure being able to change the root directory access
rights may be a good idea anyway (even for containers). ;)

A path_beneath rule enables to identify a file hierarchy (i.e. the
content of a directory), not to make modifications visible outside of
the directory identifying the hierarchy (hence the "parent_fd" field),
which would be the case with the current chmod/chown access rights.

>
> Do you have any thoughts on how to resolve this if this flexibility
> might be needed?
>
> I wonder whether the right way to resolve this would be to give users
> a way to make that distinction at the level of landlock_add_rule(),
> with an API like this (note the additional flag):
>
> err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
> &path_beneath, LANDLOCK_STRICTLY_BENEATH);
> ^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Multiple calls of landlock_add_rule() on the same file are already
> today joining the requested access rights, so it would be possible to
> mix-and-match "strict beneath" with "beneath" rights on the same
> directory, and it would work in the same way for other access rights
> as well.

This kind of option is interesting. For now, some access rights are kind
of "doubled" to enable to differentiate between a file and a directory
(i.e. READ_DIR/READ_FILE, REMOVE_DIR/REMOVE_FILE, WRITE_FILE/MAKE_*)
when it may be useful, but this is different.

I think this "strictly beneath" behavior should be the default, which is
currently the case.

>
> To be clear: I'm proposing this approach not because I think it should
> be part of this patch set, but because it would be good to have a way
> forward if that kind of flexibility is needed in the future.
>
> Does that seem reasonable?

This is the kind of questions that made such access rights not
appropriate for the initial version of Landlock. But we should talk
about that now.

>
>> +
>> /* File hooks */
>>
>> static inline access_mask_t get_file_access(const struct file *const file)
>> @@ -1199,6 +1211,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
>> LSM_HOOK_INIT(path_unlink, hook_path_unlink),
>> LSM_HOOK_INIT(path_rmdir, hook_path_rmdir),
>> LSM_HOOK_INIT(path_truncate, hook_path_truncate),
>> + LSM_HOOK_INIT(path_chmod, hook_path_chmod),
>> + LSM_HOOK_INIT(path_chown, hook_path_chown),
>>
>> LSM_HOOK_INIT(file_open, hook_file_open),
>> };
>> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
>> index 82288f0e9e5e..08858da7fb4f 100644
>> --- a/security/landlock/limits.h
>> +++ b/security/landlock/limits.h
>> @@ -18,7 +18,7 @@
>> #define LANDLOCK_MAX_NUM_LAYERS 16
>> #define LANDLOCK_MAX_NUM_RULES U32_MAX
>>
>> -#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_TRUNCATE
>> +#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_CHOWN
>> #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>> #define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>>
>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>> index f4d6fc7ed17f..469e0e11735c 100644
>> --- a/security/landlock/syscalls.c
>> +++ b/security/landlock/syscalls.c
>> @@ -129,7 +129,7 @@ static const struct file_operations ruleset_fops = {
>> .write = fop_dummy_write,
>> };
>>
>> -#define LANDLOCK_ABI_VERSION 3
>> +#define LANDLOCK_ABI_VERSION 4
>
> ABI version 3 has not made it into a stable kernel yet; I wonder
> whether it wouldn't be easier to just bundle the truncate, chmod and
> chown rights as part of ABI version 3 (assuming that the patches make
> it into a stable release together)?
>
> Mickaël, do you have an opinion on this?

I'll make sure to only have one ABI version bump per kernel release, but
it is OK to bump it for this patch series in case it is not ready for
the next merge window. I'll change it if required when merging into my
tree. It is easier to change the code to decrease the version, so please
keep it as is. ;)

2022-08-22 21:38:18

[permalink] [raw]

Subject: Re: [PATCH -next 5/5] landlock: update chmod and chown support in document

On 22/08/2022 13:47, Xiu Jianfeng wrote:
> update LANDLOCK_ACCESS_FS_{CHMOD, CHOWN} support and add abi change
> in the document.
>
> Signed-off-by: Xiu Jianfeng <[email protected]>
> ---
> Documentation/userspace-api/landlock.rst | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst
> index 2509c2fbf98f..05ab338db529 100644
> --- a/Documentation/userspace-api/landlock.rst
> +++ b/Documentation/userspace-api/landlock.rst
> @@ -61,7 +61,9 @@ the need to be explicit about the denied-by-default access rights.
> LANDLOCK_ACCESS_FS_MAKE_BLOCK |
> LANDLOCK_ACCESS_FS_MAKE_SYM |
> LANDLOCK_ACCESS_FS_REFER |
> - LANDLOCK_ACCESS_FS_TRUNCATE,
> + LANDLOCK_ACCESS_FS_TRUNCATE |
> + LANDLOCK_ACCESS_FS_CHMOD |
> + LANDLOCK_ACCESS_FS_CHOWN
> };
>
> Because we may not know on which kernel version an application will be
> @@ -90,6 +92,10 @@ the ABI.
> case 2:
> /* Removes LANDLOCK_ACCESS_FS_TRUNCATE for ABI < 3 */
> ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_TRUNCATE;

There is a missing fall-through attribute here.

> + case 3:
> + /* Removes LANDLOCK_ACCESS_FS_{CHMOD, CHOWN} for ABI < 4 */
> + ruleset_attr.handled_access_fs &= ~(LANDLOCK_ACCESS_FS_CHMOD |
> + LANDLOCK_ACCESS_FS_CHOWN);
> }
>
> This enables to create an inclusive ruleset that will contain our rules.

2022-08-22 21:40:37

[permalink] [raw]

Subject: Re: [PATCH -next 0/5] landlock: add chmod and chown support

On 22/08/2022 23:18, Günther Noack wrote:
> On Mon, Aug 22, 2022 at 12:35:18PM -0700, Casey Schaufler wrote:
>> On 8/22/2022 12:17 PM, Günther Noack wrote:
>>> Hi!
>>>
>>> Very exciting to see! Thank you for sending this! :)
>>>
>>> I'm just throwing in some comments based on the very similar truncate
>>> patch set, in the hope that it helps. (But obviously, Mickaël Salaün
>>> has the last word on this code.)
>>>
>>> Slightly higher level question: Should we start to group the
>>> functionality of multiple LSM hooks under one Landlock flag? (Will it
>>> be harder to change the LSM hook interface in the future if we
>>> continue to add one flag per hook? Or is this structure already
>>> exposed to userspace by other LSMs?)
>>
>> I'm not a landlock expert. The question is nonsensical, yet somewhat
>> frightening nonetheless. Could you put just a touch more context into
>> what you're asking for?
>
> By "Landlock flags", I meant the integer that Landlock uses to
> represent the set of possible operations on a file hierarchy:
>
> Landlock's file system access rights (access_mode_t on the kernel
> side) are defined with an integer with flags (LANDLOCK_ACCESS_FS_*)
> for different operations that one might do with files. They get used
> from userspace to control what is permitted on which parts of the file
> system. (Docs: https://docs.kernel.org/userspace-api/landlock.html)
>
> Currently most of the available Landlock flags map pretty closely to
> one of the file- and path-related LSM hooks. (See various hook
> implementations in security/landlock/fs.c)
>
> The file system operations that Landlock doesn't cover yet (as of
> kernel 5.19) are listed below, and there are potentially a few more
> that might be missing. I suspect/hope that there will be more patches
> in the style of the truncate/chmod/chown patches, which will add that
> coverage.
>
> The question is basically:
> When these patches get added, how should the userspace-exposed
> Landlock file system access rights map to the LSM hooks for these
> upcoming Landlock features? Should each of the newly covered
> operations have its own flag, or is it better to group them?
>
> (It's well possible that the right answer is "one flag per feature",
> but I feel it still makes sense to ask this before all these patches
> get written?)

Landlock is not strictly tied to the current LSM hooks, but they fit
well (because they are designed to be flexible enough to be use by
multiple access control systems). In fact, Landlock already uses
orthogonal access rights such as LANDLOCK_ACCESS_FS_REFER (using the
path_link or path_rename hooks), LANDLOCK_ACCESS_FS_MAKE_* (using the
path_mknod and path_mkdir hooks)…

Anyway, the LSM framework is evolving, we can add new hooks and modify
others (e.g. see the security_path_rename hook modification for
FS_REFER) as long as mainline access control systems don't break and
subsystem maintainers are OK with such changes. Like any kernel API, the
LSM API is not stable, but this is not an issue for mainline code.

Landlock's goal is to find the sweet spot between flexibility for
different sandboxing use cases and an understandable/simple-enough
access control system. The access rights should then be meaningful for
users, which are already familiar with the UAPI/syscalls, hence the
current Landlock access rights (which are not very original, and that is
a good thing). This is why I'm wondering if it is worth it to
differentiate between chmod and chgrp (and add a dedicated access right
per action or only one for both).

>
> —Günther
>
>>> For example, some of the "missing" operations listed on the Landlock
>>> documentation could also be grouped roughly as:
>>>
>>> Modifying files:
>>> - truncate
>>>
>>> Modifying file metadata:
>>> - chmod
>>> - chown
>>> - setxattr
>>> - utime
>>>
>>> Observing files (check presence and file metadata):
>>> - access
>>> - stat
>>> - readlink, following links (can observe symlink presence)
>>> - chdir (can observe dir presence and 'x' attribute)
>>>
>>> Ungrouped:
>>> - flock
>>> - ioctl
>>> - fcntl
>>>
>>> Do you have opinions on this?

That could indeed help users identifying currently missing pieces for
their use case.

>>>
>>> —Günther
>>>
>>> On Mon, Aug 22, 2022 at 07:46:56PM +0800, Xiu Jianfeng wrote:
>>>> hi,
>>>> this patchset adds chmod and chown support for landlock
>>>>
>>>> Xiu Jianfeng (5):
>>>> landlock: expand access_mask_t to u32 type
>>>> landlock: add chmod and chown support
>>>> landlock/selftests: add selftests for chmod and chown
>>>> landlock/samples: add chmod and chown support
>>>> landlock: update chmod and chown support in document
>>>>
>>>> Documentation/userspace-api/landlock.rst | 8 +-
>>>> include/uapi/linux/landlock.h | 8 +-
>>>> samples/landlock/sandboxer.c | 12 +-
>>>> security/landlock/fs.c | 16 +-
>>>> security/landlock/limits.h | 2 +-
>>>> security/landlock/ruleset.h | 2 +-
>>>> security/landlock/syscalls.c | 2 +-
>>>> tools/testing/selftests/landlock/base_test.c | 2 +-
>>>> tools/testing/selftests/landlock/fs_test.c | 234 ++++++++++++++++++-
>>>> 9 files changed, 274 insertions(+), 12 deletions(-)
>>>>
>>>> --
>>>> 2.17.1
>>>>
>>> --
>
> --

2022-08-22 21:50:32

[permalink] [raw]

Subject: Re: [PATCH -next 0/5] landlock: add chmod and chown support

On Mon, Aug 22, 2022 at 12:35:18PM -0700, Casey Schaufler wrote:
> On 8/22/2022 12:17 PM, Günther Noack wrote:
> > Hi!
> >
> > Very exciting to see! Thank you for sending this! :)
> >
> > I'm just throwing in some comments based on the very similar truncate
> > patch set, in the hope that it helps. (But obviously, Mickaël Salaün
> > has the last word on this code.)
> >
> > Slightly higher level question: Should we start to group the
> > functionality of multiple LSM hooks under one Landlock flag? (Will it
> > be harder to change the LSM hook interface in the future if we
> > continue to add one flag per hook? Or is this structure already
> > exposed to userspace by other LSMs?)
>
> I'm not a landlock expert. The question is nonsensical, yet somewhat
> frightening nonetheless. Could you put just a touch more context into
> what you're asking for?

By "Landlock flags", I meant the integer that Landlock uses to
represent the set of possible operations on a file hierarchy:

Landlock's file system access rights (access_mode_t on the kernel
side) are defined with an integer with flags (LANDLOCK_ACCESS_FS_*)
for different operations that one might do with files. They get used
from userspace to control what is permitted on which parts of the file
system. (Docs: https://docs.kernel.org/userspace-api/landlock.html)

Currently most of the available Landlock flags map pretty closely to
one of the file- and path-related LSM hooks. (See various hook
implementations in security/landlock/fs.c)

The file system operations that Landlock doesn't cover yet (as of
kernel 5.19) are listed below, and there are potentially a few more
that might be missing. I suspect/hope that there will be more patches
in the style of the truncate/chmod/chown patches, which will add that
coverage.

The question is basically:
When these patches get added, how should the userspace-exposed
Landlock file system access rights map to the LSM hooks for these
upcoming Landlock features? Should each of the newly covered
operations have its own flag, or is it better to group them?

(It's well possible that the right answer is "one flag per feature",
but I feel it still makes sense to ask this before all these patches
get written?)

—Günther

> > For example, some of the "missing" operations listed on the Landlock
> > documentation could also be grouped roughly as:
> >
> > Modifying files:
> > - truncate
> >
> > Modifying file metadata:
> > - chmod
> > - chown
> > - setxattr
> > - utime
> >
> > Observing files (check presence and file metadata):
> > - access
> > - stat
> > - readlink, following links (can observe symlink presence)
> > - chdir (can observe dir presence and 'x' attribute)
> >
> > Ungrouped:
> > - flock
> > - ioctl
> > - fcntl
> >
> > Do you have opinions on this?
> >
> > —Günther
> >
> > On Mon, Aug 22, 2022 at 07:46:56PM +0800, Xiu Jianfeng wrote:
> >> hi,
> >> this patchset adds chmod and chown support for landlock
> >>
> >> Xiu Jianfeng (5):
> >> landlock: expand access_mask_t to u32 type
> >> landlock: add chmod and chown support
> >> landlock/selftests: add selftests for chmod and chown
> >> landlock/samples: add chmod and chown support
> >> landlock: update chmod and chown support in document
> >>
> >> Documentation/userspace-api/landlock.rst | 8 +-
> >> include/uapi/linux/landlock.h | 8 +-
> >> samples/landlock/sandboxer.c | 12 +-
> >> security/landlock/fs.c | 16 +-
> >> security/landlock/limits.h | 2 +-
> >> security/landlock/ruleset.h | 2 +-
> >> security/landlock/syscalls.c | 2 +-
> >> tools/testing/selftests/landlock/base_test.c | 2 +-
> >> tools/testing/selftests/landlock/fs_test.c | 234 ++++++++++++++++++-
> >> 9 files changed, 274 insertions(+), 12 deletions(-)
> >>
> >> --
> >> 2.17.1
> >>
> > --

--

2022-08-22 21:55:45

[permalink] [raw]

Subject: Re: [PATCH -next 3/5] landlock/selftests: add selftests for chmod and chown

Agree with Günther's review. :)

On 22/08/2022 20:53, Günther Noack wrote:
> On Mon, Aug 22, 2022 at 07:46:59PM +0800, Xiu Jianfeng wrote:
>> Add the following simple testcases:
>> 1. chmod/fchmod: remove S_IWUSR and restore S_IWUSR with or without
>> restriction.
>> 2. chown/fchown: set original uid and gid with or without restriction,
>> because chown needs CAP_CHOWN and testcase framework don't have this
>> capability, setting original uid and gid is ok to cover landlock
>> function.
>>
>> Signed-off-by: Xiu Jianfeng <[email protected]>
>> ---
>> tools/testing/selftests/landlock/fs_test.c | 228 +++++++++++++++++++++
>> 1 file changed, 228 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
>> index 5b55b93b5570..f47b4ccd2b26 100644
>> --- a/tools/testing/selftests/landlock/fs_test.c
>> +++ b/tools/testing/selftests/landlock/fs_test.c
>> @@ -59,6 +59,9 @@ static const char file2_s2d3[] = TMP_DIR "/s2d1/s2d2/s2d3/f2";
>>
>> static const char dir_s3d1[] = TMP_DIR "/s3d1";
>> static const char file1_s3d1[] = TMP_DIR "/s3d1/f1";
>> +static const char file2_s3d1[] = TMP_DIR "/s3d1/f2";
>> +static const char file3_s3d1[] = TMP_DIR "/s3d1/f3";
>> +
>> /* dir_s3d2 is a mount point. */
>> static const char dir_s3d2[] = TMP_DIR "/s3d1/s3d2";
>> static const char dir_s3d3[] = TMP_DIR "/s3d1/s3d2/s3d3";
>> @@ -211,6 +214,8 @@ static void create_layout1(struct __test_metadata *const _metadata)
>> create_file(_metadata, file2_s2d3);
>>
>> create_file(_metadata, file1_s3d1);
>> + create_file(_metadata, file2_s3d1);
>> + create_file(_metadata, file3_s3d1);
>> create_directory(_metadata, dir_s3d2);
>> set_cap(_metadata, CAP_SYS_ADMIN);
>> ASSERT_EQ(0, mount("tmp", dir_s3d2, "tmpfs", 0, "size=4m,mode=700"));
>> @@ -234,6 +239,8 @@ static void remove_layout1(struct __test_metadata *const _metadata)
>> EXPECT_EQ(0, remove_path(file1_s2d1));
>>
>> EXPECT_EQ(0, remove_path(file1_s3d1));
>> + EXPECT_EQ(0, remove_path(file2_s3d1));
>> + EXPECT_EQ(0, remove_path(file3_s3d1));
>> EXPECT_EQ(0, remove_path(dir_s3d3));
>> set_cap(_metadata, CAP_SYS_ADMIN);
>> umount(dir_s3d2);
>> @@ -3272,6 +3279,227 @@ TEST_F_FORK(layout1, truncate)
>> EXPECT_EQ(0, test_creat(file_in_dir_w));
>> }
>>
>> +static int test_chmod(const char *path)
>
> Nitpicks:
> - const char *const path
> - short documentation? :)
>
>> +{
>> + int ret;
>> + struct stat st;
>> + mode_t mode;
>> +
>> + ret = stat(path, &st);
>> + if (ret < 0)
>> + return errno;
>> + /* save original mode in order to restore */
>> + mode = st.st_mode & 0777;
>> + /* remove S_IWUSR */
>> + ret = chmod(path, mode & ~0200);
>> + if (ret < 0)
>> + return errno;
>> + ret = stat(path, &st);
>> + if (ret < 0)
>> + return errno;
>> + /* check if still has S_IWUSR */
>> + if (st.st_mode & 0200)
>> + return -EFAULT;
>> + /* restore the original mode */
>> + ret = chmod(path, mode);
>> + if (ret < 0)
>> + return errno;
>> + return 0;
>> +}
>
> I would argue this can be simpler, with the following reasoning:
>
> - Does the file have the right mode after chmod()?
>
> I claim that fs_test should care only about the question of whether
> EACCES is returned or not. If fs_test were to also check for the
> side effects of these operations, it would eventually contain tests
> for the full file system API, not just for Landlock. That seems out
> of scope :)
>
> - Undoing the chmod() operation
>
> I'm not sure whether it's worth the effort to restore the exact
> state before that function returns. As long as the flags suffice to
> remove the test directory at the end, it probably doesn't matter
> much what exact mode they have?
>
> I think this could just be
>
> if (chmod(path, mode) < 0)
> return errno;
> return 0
>
> and it would be a bit simpler to understand :)
>
> The same argument applies also to the other test_...() functions.
>
>> +static int test_fchmod(const char *path)
>
> I initially took the same approach for test_ftruncate() but eventually
> settled on using an approach where the file is open()ed before
> restricting the thread with Landlock. This eliminates the potential
> confusion where test_ftruncate() returns an error but the caller can't
> distinguish whether the error is from open() or from ftruncate(). It
> also makes fchmod testable even in scenarios where the file cannot be
> opened because of missing Landlock rights.
>
>> +{
>> + int ret, fd;
>> + struct stat st;
>> + mode_t mode;
>> +
>> + ret = stat(path, &st);
>> + if (ret < 0)
>> + return errno;
>> + /* save original mode in order to restore */
>> + mode = st.st_mode & 0777;
>> +
>> + fd = openat(AT_FDCWD, path, O_RDWR | O_CLOEXEC);
>> + if (fd < 0)
>> + return errno;
>> + /* remove S_IWUSR */
>> + ret = fchmod(fd, mode & ~0200);
>> + if (ret < 0)
>> + goto err;
>> + ret = stat(path, &st);
>> + if (ret < 0)
>> + goto err;
>> + /* check if still has S_IWUSR */
>> + if (st.st_mode & 0200) {
>> + ret = -1;
>> + errno = -EFAULT;
>> + goto err;
>> + }
>> + /* restore the original mode */
>> + ret = fchmod(fd, mode);
>> +err:
>> + if (close(fd) < 0)
>> + return errno;
>> + return ret ? errno : 0;
>> +}
>
>> +static int test_chown(const char *path)
>> +{
>> + int ret;
>> + struct stat st;
>> +
>> + ret = stat(path, &st);
>> + if (ret < 0)
>> + return errno;
>> + /*
>> + * chown needs CAP_CHOWN to modify uid and/or gid, however
>> + * there is no such capability when the testcases framework
>> + * setup, so just chown to original uid/gid, which can also
>> + * cover the function in landlock.
>> + */
>> + ret = chown(path, st.st_uid, st.st_gid);
>> + if (ret < 0)
>> + return errno;
>> + return 0;
>> +}
>> +
>> +static int test_fchown(const char *path)
>> +{
>> + int ret, fd;
>> + struct stat st;
>> +
>> + ret = stat(path, &st);
>> + if (ret < 0)
>> + return errno;
>> + fd = openat(AT_FDCWD, path, O_RDWR | O_CLOEXEC);
>> + if (fd < 0)
>> + return errno;
>> + /*
>> + * fchown needs CAP_CHOWN to modify uid and/or gid, however
>> + * there is no such capability when the testcases framework
>> + * setup, so just fchown to original uid/gid, which can also
>> + * cover the function in landlock.
>> + */
>> + ret = fchown(fd, st.st_uid, st.st_gid);
>> + if (close(fd) < 0)
>> + return errno;
>> + return ret ? errno : 0;
>> +}
>> +
>> +TEST_F_FORK(layout1, unhandled_chmod)
>> +{
>> + const struct rule rules[] = {
>> + {
>> + .path = file2_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {
>> + .path = file3_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {},
>> + };
>> + const int ruleset_fd =
>> + create_ruleset(_metadata, ACCESS_RW, rules);
>> +
>> + ASSERT_LE(0, ruleset_fd);
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + ASSERT_EQ(0, test_chmod(file2_s3d1));
>> + ASSERT_EQ(0, test_fchmod(file2_s3d1));
>> + ASSERT_EQ(0, test_chmod(file3_s3d1));
>> + ASSERT_EQ(0, test_chmod(dir_s3d1));
>
> *optional* because the existing tests are already inconsistent about it
>
> These four ASSERT_EQ() calls are independent scenarios and could be
> done with EXPECT_EQ(), which would be more in line with the approach
> that this test framework takes. (Same for the other tests below)
>
> Compare previous discussion at:
> https://lore.kernel.org/all/Yvd3+fy+mDBop+YA@nuc/
>
>> +}
>> +
>> +TEST_F_FORK(layout1, chmod)
>> +{
>> + const struct rule rules[] = {
>> + {
>> + .path = file2_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE |
>> + LANDLOCK_ACCESS_FS_CHMOD,
>> + },
>> + {
>> + .path = file3_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {},
>> + };
>> + const int ruleset_fd =
>> + create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHMOD, rules);
>> +
>> + ASSERT_LE(0, ruleset_fd);
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + ASSERT_EQ(0, test_chmod(file2_s3d1));
>> + ASSERT_EQ(0, test_fchmod(file2_s3d1));
>> + ASSERT_EQ(EACCES, test_chmod(file3_s3d1));
>> + ASSERT_EQ(EACCES, test_chmod(dir_s3d1));
>> +}
>> +
>> +TEST_F_FORK(layout1, no_chown)
>
> "unhandled_chown" to be consistent with the other one above?
>
>> +{
>> + const struct rule rules[] = {
>> + {
>> + .path = file2_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {
>> + .path = file3_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {},
>> + };
>> + const int ruleset_fd =
>> + create_ruleset(_metadata, ACCESS_RW, rules);
>> +
>> + ASSERT_LE(0, ruleset_fd);
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + ASSERT_EQ(0, test_chown(file2_s3d1));
>> + ASSERT_EQ(0, test_fchown(file2_s3d1));
>> + ASSERT_EQ(0, test_chown(file3_s3d1));
>> + ASSERT_EQ(0, test_chown(dir_s3d1));
>> +}
>> +
>> +TEST_F_FORK(layout1, chown)
>> +{
>> + const struct rule rules[] = {
>> + {
>> + .path = file2_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE |
>> + LANDLOCK_ACCESS_FS_CHOWN,
>
> It might be useful to also check a scenario where the chown right is
> granted on a directory (and as a consequence, both the directory
> itself as well as its contents can be chowned)? (Same for chmod)
>
>> + },
>> + {
>> + .path = file3_s3d1,
>> + .access = LANDLOCK_ACCESS_FS_READ_FILE |
>> + LANDLOCK_ACCESS_FS_WRITE_FILE,
>> + },
>> + {},
>> + };
>> + const int ruleset_fd =
>> + create_ruleset(_metadata, ACCESS_RW | LANDLOCK_ACCESS_FS_CHOWN, rules);
>> +
>> + ASSERT_LE(0, ruleset_fd);
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + ASSERT_EQ(0, test_chown(file2_s3d1));
>> + ASSERT_EQ(0, test_fchown(file2_s3d1));
>> + ASSERT_EQ(EACCES, test_chown(file3_s3d1));
>> + ASSERT_EQ(EACCES, test_chown(dir_s3d1));
>> +}
>> +
>> /* clang-format off */
>> FIXTURE(layout1_bind) {};
>> /* clang-format on */
>> --
>> 2.17.1
>>
>
> --

2022-08-22 22:04:25

by Casey Schaufler

[permalink] [raw]

Subject: Re: [PATCH -next 0/5] landlock: add chmod and chown support

On 8/22/2022 2:21 PM, Mickaël Salaün wrote:
>
>
> On 22/08/2022 23:18, Günther Noack wrote:
>> On Mon, Aug 22, 2022 at 12:35:18PM -0700, Casey Schaufler wrote:
>>> On 8/22/2022 12:17 PM, Günther Noack wrote:
>>>> Hi!
>>>>
>>>> Very exciting to see! Thank you for sending this! :)
>>>>
>>>> I'm just throwing in some comments based on the very similar truncate
>>>> patch set, in the hope that it helps. (But obviously, Mickaël Salaün
>>>> has the last word on this code.)
>>>>
>>>> Slightly higher level question: Should we start to group the
>>>> functionality of multiple LSM hooks under one Landlock flag? (Will it
>>>> be harder to change the LSM hook interface in the future if we
>>>> continue to add one flag per hook? Or is this structure already
>>>> exposed to userspace by other LSMs?)
>>>
>>> I'm not a landlock expert. The question is nonsensical, yet somewhat
>>> frightening nonetheless. Could you put just a touch more context into
>>> what you're asking for?
>>
>> By "Landlock flags", I meant the integer that Landlock uses to
>> represent the set of possible operations on a file hierarchy:
>>
>> Landlock's file system access rights (access_mode_t on the kernel
>> side) are defined with an integer with flags (LANDLOCK_ACCESS_FS_*)
>> for different operations that one might do with files. They get used
>> from userspace to control what is permitted on which parts of the file
>> system. (Docs: https://docs.kernel.org/userspace-api/landlock.html)
>>
>> Currently most of the available Landlock flags map pretty closely to
>> one of the file- and path-related LSM hooks. (See various hook
>> implementations in security/landlock/fs.c)
>>
>> The file system operations that Landlock doesn't cover yet (as of
>> kernel 5.19) are listed below, and there are potentially a few more
>> that might be missing. I suspect/hope that there will be more patches
>> in the style of the truncate/chmod/chown patches, which will add that
>> coverage.
>>
>> The question is basically:
>> When these patches get added, how should the userspace-exposed
>> Landlock file system access rights map to the LSM hooks for these
>> upcoming Landlock features? Should each of the newly covered
>> operations have its own flag, or is it better to group them?
>>
>> (It's well possible that the right answer is "one flag per feature",
>> but I feel it still makes sense to ask this before all these patches
>> get written?)
>
> Landlock is not strictly tied to the current LSM hooks, but they fit
> well (because they are designed to be flexible enough to be use by
> multiple access control systems). In fact, Landlock already uses
> orthogonal access rights such as LANDLOCK_ACCESS_FS_REFER (using the
> path_link or path_rename hooks), LANDLOCK_ACCESS_FS_MAKE_* (using the
> path_mknod and path_mkdir hooks)…
>
> Anyway, the LSM framework is evolving, we can add new hooks and modify
> others (e.g. see the security_path_rename hook modification for
> FS_REFER) as long as mainline access control systems don't break and
> subsystem maintainers are OK with such changes. Like any kernel API,
> the LSM API is not stable, but this is not an issue for mainline code.
>
> Landlock's goal is to find the sweet spot between flexibility for
> different sandboxing use cases and an understandable/simple-enough
> access control system. The access rights should then be meaningful for
> users, which are already familiar with the UAPI/syscalls, hence the
> current Landlock access rights (which are not very original, and that
> is a good thing). This is why I'm wondering if it is worth it to
> differentiate between chmod and chgrp (and add a dedicated access
> right per action or only one for both).

The lesson from capabilities is that differentiating between chmod, chown and chgrp is
pointless, and CAP_DAC_CHMOD, CAP_DAC_CHOWN and CAP_DAC_CHGRP should have just been
CAP_DAC_OVERRIDE. On the other hand, those who argue that SELinux proves the value of
fine granularity would likely have you go with separate rights. What's important is
that you don't tie your rights too tightly to the underlying implementation. That has
the potential to expose details of how the code work that user-space has no business
basing decisions on.

>
>
>>
>> —Günther
>>
>>>> For example, some of the "missing" operations listed on the Landlock
>>>> documentation could also be grouped roughly as:
>>>>
>>>> Modifying files:
>>>> - truncate
>>>>
>>>> Modifying file metadata:
>>>> - chmod
>>>> - chown
>>>> - setxattr
>>>> - utime
>>>>
>>>> Observing files (check presence and file metadata):
>>>> - access
>>>> - stat
>>>> - readlink, following links (can observe symlink presence)
>>>> - chdir (can observe dir presence and 'x' attribute)
>>>>
>>>> Ungrouped:
>>>> - flock
>>>> - ioctl
>>>> - fcntl
>>>>
>>>> Do you have opinions on this?
>
> That could indeed help users identifying currently missing pieces for
> their use case.
>
>
>>>>
>>>> —Günther
>>>>
>>>> On Mon, Aug 22, 2022 at 07:46:56PM +0800, Xiu Jianfeng wrote:
>>>>> hi,
>>>>>    this patchset adds chmod and chown support for landlock
>>>>>
>>>>> Xiu Jianfeng (5):
>>>>>    landlock: expand access_mask_t to u32 type
>>>>>    landlock: add chmod and chown support
>>>>>    landlock/selftests: add selftests for chmod and chown
>>>>>    landlock/samples: add chmod and chown support
>>>>>    landlock: update chmod and chown support in document
>>>>>
>>>>> Documentation/userspace-api/landlock.rst     |   8 +-
>>>>> include/uapi/linux/landlock.h                |   8 +-
>>>>> samples/landlock/sandboxer.c                 | 12 +-
>>>>> security/landlock/fs.c                       | 16 +-
>>>>> security/landlock/limits.h                   |   2 +-
>>>>> security/landlock/ruleset.h                  |   2 +-
>>>>> security/landlock/syscalls.c                 |   2 +-
>>>>> tools/testing/selftests/landlock/base_test.c |   2 +-
>>>>> tools/testing/selftests/landlock/fs_test.c   | 234
>>>>> ++++++++++++++++++-
>>>>> 9 files changed, 274 insertions(+), 12 deletions(-)
>>>>>
>>>>> --
>>>>> 2.17.1
>>>>>
>>>> --
>>
>> --

2022-08-23 19:11:53

[permalink] [raw]

Subject: Re: [PATCH -next 0/5] landlock: add chmod and chown support

On 22/08/2022 23:53, Casey Schaufler wrote:
> On 8/22/2022 2:21 PM, Mickaël Salaün wrote:
>>
>>
>> On 22/08/2022 23:18, Günther Noack wrote:
>>> On Mon, Aug 22, 2022 at 12:35:18PM -0700, Casey Schaufler wrote:
>>>> On 8/22/2022 12:17 PM, Günther Noack wrote:
>>>>> Hi!
>>>>>
>>>>> Very exciting to see! Thank you for sending this! :)
>>>>>
>>>>> I'm just throwing in some comments based on the very similar truncate
>>>>> patch set, in the hope that it helps. (But obviously, Mickaël Salaün
>>>>> has the last word on this code.)
>>>>>
>>>>> Slightly higher level question: Should we start to group the
>>>>> functionality of multiple LSM hooks under one Landlock flag? (Will it
>>>>> be harder to change the LSM hook interface in the future if we
>>>>> continue to add one flag per hook? Or is this structure already
>>>>> exposed to userspace by other LSMs?)
>>>>
>>>> I'm not a landlock expert. The question is nonsensical, yet somewhat
>>>> frightening nonetheless. Could you put just a touch more context into
>>>> what you're asking for?
>>>
>>> By "Landlock flags", I meant the integer that Landlock uses to
>>> represent the set of possible operations on a file hierarchy:
>>>
>>> Landlock's file system access rights (access_mode_t on the kernel
>>> side) are defined with an integer with flags (LANDLOCK_ACCESS_FS_*)
>>> for different operations that one might do with files. They get used
>>> from userspace to control what is permitted on which parts of the file
>>> system. (Docs: https://docs.kernel.org/userspace-api/landlock.html)
>>>
>>> Currently most of the available Landlock flags map pretty closely to
>>> one of the file- and path-related LSM hooks. (See various hook
>>> implementations in security/landlock/fs.c)
>>>
>>> The file system operations that Landlock doesn't cover yet (as of
>>> kernel 5.19) are listed below, and there are potentially a few more
>>> that might be missing. I suspect/hope that there will be more patches
>>> in the style of the truncate/chmod/chown patches, which will add that
>>> coverage.
>>>
>>> The question is basically:
>>> When these patches get added, how should the userspace-exposed
>>> Landlock file system access rights map to the LSM hooks for these
>>> upcoming Landlock features? Should each of the newly covered
>>> operations have its own flag, or is it better to group them?
>>>
>>> (It's well possible that the right answer is "one flag per feature",
>>> but I feel it still makes sense to ask this before all these patches
>>> get written?)
>>
>> Landlock is not strictly tied to the current LSM hooks, but they fit
>> well (because they are designed to be flexible enough to be use by
>> multiple access control systems). In fact, Landlock already uses
>> orthogonal access rights such as LANDLOCK_ACCESS_FS_REFER (using the
>> path_link or path_rename hooks), LANDLOCK_ACCESS_FS_MAKE_* (using the
>> path_mknod and path_mkdir hooks)…
>>
>> Anyway, the LSM framework is evolving, we can add new hooks and modify
>> others (e.g. see the security_path_rename hook modification for
>> FS_REFER) as long as mainline access control systems don't break and
>> subsystem maintainers are OK with such changes. Like any kernel API,
>> the LSM API is not stable, but this is not an issue for mainline code.
>>
>> Landlock's goal is to find the sweet spot between flexibility for
>> different sandboxing use cases and an understandable/simple-enough
>> access control system. The access rights should then be meaningful for
>> users, which are already familiar with the UAPI/syscalls, hence the
>> current Landlock access rights (which are not very original, and that
>> is a good thing). This is why I'm wondering if it is worth it to
>> differentiate between chmod and chgrp (and add a dedicated access
>> right per action or only one for both).
>
> The lesson from capabilities is that differentiating between chmod, chown and chgrp is
> pointless, and CAP_DAC_CHMOD, CAP_DAC_CHOWN and CAP_DAC_CHGRP should have just been
> CAP_DAC_OVERRIDE. On the other hand, those who argue that SELinux proves the value of
> fine granularity would likely have you go with separate rights. What's important is
> that you don't tie your rights too tightly to the underlying implementation. That has
> the potential to expose details of how the code work that user-space has no business
> basing decisions on.

Indeed, for a sandboxing feature like Landlock, it may not be useful to
duplicate other access rights. From a user point of view, I think it
would make sense to split the file metadata modification into
potentially-security related or not. That would means three access rights:
- modify user/informative metadata (e.g. dates, user.* xattr);
- modify security-related metadata (e.g. chown, chmod, chgrp, any other
xattr);
- read any metadata.

This require some LSM hook changes to handle paths instead of inodes
(e.g. security_inode_setattr, security_inode_setxattr…).

2022-08-24 12:34:04

[permalink] [raw]

Subject: Re: [PATCH -next 2/5] landlock: add chmod and chown support

On 23/08/2022 14:50, xiujianfeng wrote:
>
>
> 在 2022/8/23 5:07, Mickaël Salaün 写道:
>>
>> On 22/08/2022 20:25, Günther Noack wrote:
>>> Hi!
>>>
>>> Thanks for sending this patch set! :)
>>>
>>> On Mon, Aug 22, 2022 at 07:46:58PM +0800, Xiu Jianfeng wrote:
>>>> Add two flags LANDLOCK_ACCESS_FS_CHMOD and LANDLOCK_ACCESS_FS_CHOWN to
>>>> support restriction to chmod(2) and chown(2) with landlock.
>>>>
>>>> Also change the landlock ABI version from 3 to 4.
>>>>
>>>> Signed-off-by: Xiu Jianfeng <[email protected]>
>>>> ---
>>>> include/uapi/linux/landlock.h                | 8 ++++++--
>>>> security/landlock/fs.c                       | 16 +++++++++++++++-
>>>> security/landlock/limits.h                   | 2 +-
>>>> security/landlock/syscalls.c                 | 2 +-
>>>> tools/testing/selftests/landlock/base_test.c | 2 +-
>>>> tools/testing/selftests/landlock/fs_test.c   | 6 ++++--
>>>> 6 files changed, 28 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/include/uapi/linux/landlock.h
>>>> b/include/uapi/linux/landlock.h
>>>> index 735b1fe8326e..5ce633c92722 100644
>>>> --- a/include/uapi/linux/landlock.h
>>>> +++ b/include/uapi/linux/landlock.h
>>>> @@ -141,13 +141,15 @@ struct landlock_path_beneath_attr {
>>>>    *   directory) parent. Otherwise, such actions are denied with
>>>> errno set to
>>>>    *   EACCES. The EACCES errno prevails over EXDEV to let user space
>>>>    *   efficiently deal with an unrecoverable error.
>>>> + * - %LANDLOCK_ACCESS_FS_CHMOD: Change the file mode bits of a file.
>>>> + * - %LANDLOCK_ACCESS_FS_CHOWN: Change the owner and/or group of a
>>>> file.
>>
>> This section talk about "access rights that only apply to the content of
>> a directory, not the directory itself", which is not correct (see
>> LANDLOCK_ACCESS_FS_READ_DIR). I'd like these access rights to remain
>> here but this kernel patch and the related tests need some changes.
>>
>> What about a LANDLOCK_ACCESS_FS_CHGRP? I'm not sure if we need to
>> differentiate these actions or not, but we need arguments to choose.
>>
>>
>>>>    *
>>>>    * .. warning::
>>>>    *
>>>>    *   It is currently not possible to restrict some file-related
>>>> actions
>>>>    *   accessible through these syscall families: :manpage:`chdir(2)`,
>>>> - *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
>>>> - *   :manpage:`chown(2)`, :manpage:`setxattr(2)`, :manpage:`utime(2)`,
>>>> + *   :manpage:`stat(2)`, :manpage:`flock(2)`,
>>>> + *   :manpage:`setxattr(2)`, :manpage:`utime(2)`,
>>>
>>> *formatting nit*
>>> We could fill up the full line width here
>>>
>>>>    *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`, :manpage:`access(2)`.
>>>>    *   Future Landlock evolutions will enable to restrict them.
>>>>    */
>>>> @@ -167,6 +169,8 @@ struct landlock_path_beneath_attr {
>>>> #define LANDLOCK_ACCESS_FS_MAKE_SYM            (1ULL << 12)
>>>> #define LANDLOCK_ACCESS_FS_REFER            (1ULL << 13)
>>>> #define LANDLOCK_ACCESS_FS_TRUNCATE            (1ULL << 14)
>>>> +#define LANDLOCK_ACCESS_FS_CHMOD            (1ULL << 15)
>>>> +#define LANDLOCK_ACCESS_FS_CHOWN            (1ULL << 16)
>>>> /* clang-format on */
>>>>
>>>> #endif /* _UAPI_LINUX_LANDLOCK_H */
>>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>>> index c57f581a9cd5..c25d5f89c8be 100644
>>>> --- a/security/landlock/fs.c
>>>> +++ b/security/landlock/fs.c
>>>> @@ -147,7 +147,9 @@ static struct landlock_object
>>>> *get_inode_object(struct inode *const inode)
>>>>       LANDLOCK_ACCESS_FS_EXECUTE | \
>>>>       LANDLOCK_ACCESS_FS_WRITE_FILE | \
>>>>       LANDLOCK_ACCESS_FS_READ_FILE | \
>>>> -    LANDLOCK_ACCESS_FS_TRUNCATE)
>>>> +    LANDLOCK_ACCESS_FS_TRUNCATE | \
>>>> +    LANDLOCK_ACCESS_FS_CHMOD | \
>>>> +    LANDLOCK_ACCESS_FS_CHOWN)
>>>> /* clang-format on */
>>>>
>>>> /*
>>>> @@ -1146,6 +1148,16 @@ static int hook_path_truncate(const struct
>>>> path *const path)
>>>>       return current_check_access_path(path,
>>>> LANDLOCK_ACCESS_FS_TRUNCATE);
>>>> }
>>>>
>>>> +static int hook_path_chmod(const struct path *const dir, umode_t mode)
>>
>> This is not a "dir" but a "path".
>>
>>
>>>> +{
>>>> +    return current_check_access_path(dir, LANDLOCK_ACCESS_FS_CHMOD);
>>>> +}
>>>> +
>>>> +static int hook_path_chown(const struct path *const dir, kuid_t uid,
>>>> kgid_t gid)
>>
>> Same here.
>>
>>
>>>> +{
>>>> +    return current_check_access_path(dir, LANDLOCK_ACCESS_FS_CHOWN);
>>>> +}
>>>
>>> One implication of this approach is that the chown+chmod right on a
>>> directory's contents are always going together with the same rights on
>>> the directory itself.
>>>
>>> For example, if you grant chmod+chown access rights for "datadir/",
>>> the command "chmod 0600 datadir/file1" will work, but so will the
>>> command "chmod 0600 datadir". But the approach of checking just the
>>> parent directory's rights is also inflexible if you think through the
>>> kinds of rights you can grant with it. (It would also not be possible
>>> to grant chmod+chown on individual files.)
>>
>> Good point. For an initial chmod/chown/chgrp access right, I'd prefer to
>> be able to set these access rights on a directory but only for its
>> content, not the directory itself. I think it is much safer and should
>> be enough for the majority of use cases, but let me know if I'm missing
>> something. I'm not sure being able to change the root directory access
>> rights may be a good idea anyway (even for containers). ;)
>>
>> A path_beneath rule enables to identify a file hierarchy (i.e. the
>> content of a directory), not to make modifications visible outside of
>> the directory identifying the hierarchy (hence the "parent_fd" field),
>> which would be the case with the current chmod/chown access rights.
>>
>>
>>>
>>> Do you have any thoughts on how to resolve this if this flexibility
>>> might be needed?
>>>
>>> I wonder whether the right way to resolve this would be to give users
>>> a way to make that distinction at the level of landlock_add_rule(),
>>> with an API like this (note the additional flag):
>>>
>>>    err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
>>>                            &path_beneath, LANDLOCK_STRICTLY_BENEATH);
>>>                                           ^^^^^^^^^^^^^^^^^^^^^^^^^
>>>
>>> Multiple calls of landlock_add_rule() on the same file are already
>>> today joining the requested access rights, so it would be possible to
>>> mix-and-match "strict beneath" with "beneath" rights on the same
>>> directory, and it would work in the same way for other access rights
>>> as well.
>>
>> This kind of option is interesting. For now, some access rights are kind
>> of "doubled" to enable to differentiate between a file and a directory
>> (i.e. READ_DIR/READ_FILE, REMOVE_DIR/REMOVE_FILE, WRITE_FILE/MAKE_*)
>> when it may be useful, but this is different.
>>
>> I think this "strictly beneath" behavior should be the default, which is
>> currently the case.
>>
>>
>>>
>>> To be clear: I'm proposing this approach not because I think it should
>>> be part of this patch set, but because it would be good to have a way
>>> forward if that kind of flexibility is needed in the future.
>>>
>>> Does that seem reasonable?
>>
>> This is the kind of questions that made such access rights not
>> appropriate for the initial version of Landlock. But we should talk
>> about that now.
>
> Hi Günther and Mickaël,
>
> Thanks for your comments, so I think the conclusion here is that we have
> to make sure that in this patchset chown/chmod access rights can be set
> on a directory only for its content, not the directory itself, right?
> any good idea about how to implement this? :)

In such hook code, you need to get the parent directory of the path
argument. This require to use and refactor the
check_access_path_dual/jump_up part in a dedicated helper (and take care
of all the corner cases).

2022-08-26 09:54:20

[permalink] [raw]

Subject: Re: [PATCH -next 2/5] landlock: add chmod and chown support

On 26/08/2022 10:36, xiujianfeng wrote:
> Hi,
>
> 在 2022/8/24 19:44, Mickaël Salaün 写道:
>>
>> On 23/08/2022 14:50, xiujianfeng wrote:
>>>
>>>
>>> 在 2022/8/23 5:07, Mickaël Salaün 写道:
>>>>
>>>> On 22/08/2022 20:25, Günther Noack wrote:
>>>>> Hi!
>>>>>
>>>>> Thanks for sending this patch set! :)
>>>>>
>>>>> On Mon, Aug 22, 2022 at 07:46:58PM +0800, Xiu Jianfeng wrote:
>>>>>> Add two flags LANDLOCK_ACCESS_FS_CHMOD and LANDLOCK_ACCESS_FS_CHOWN to
>>>>>> support restriction to chmod(2) and chown(2) with landlock.
>>>>>>
>>>>>> Also change the landlock ABI version from 3 to 4.
>>>>>>
>>>>>> Signed-off-by: Xiu Jianfeng <[email protected]>
>>>>>> ---
>>>>>>    include/uapi/linux/landlock.h                | 8 ++++++--
>>>>>>    security/landlock/fs.c                       | 16 +++++++++++++++-
>>>>>>    security/landlock/limits.h                   | 2 +-
>>>>>>    security/landlock/syscalls.c                 | 2 +-
>>>>>>    tools/testing/selftests/landlock/base_test.c | 2 +-
>>>>>>    tools/testing/selftests/landlock/fs_test.c   | 6 ++++--
>>>>>>    6 files changed, 28 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/include/uapi/linux/landlock.h
>>>>>> b/include/uapi/linux/landlock.h
>>>>>> index 735b1fe8326e..5ce633c92722 100644
>>>>>> --- a/include/uapi/linux/landlock.h
>>>>>> +++ b/include/uapi/linux/landlock.h
>>>>>> @@ -141,13 +141,15 @@ struct landlock_path_beneath_attr {
>>>>>>     *   directory) parent. Otherwise, such actions are denied with
>>>>>> errno set to
>>>>>>     *   EACCES. The EACCES errno prevails over EXDEV to let user
>>>>>> space
>>>>>>     *   efficiently deal with an unrecoverable error.
>>>>>> + * - %LANDLOCK_ACCESS_FS_CHMOD: Change the file mode bits of a file.
>>>>>> + * - %LANDLOCK_ACCESS_FS_CHOWN: Change the owner and/or group of a
>>>>>> file.
>>>>
>>>> This section talk about "access rights that only apply to the content of
>>>> a directory, not the directory itself", which is not correct (see
>>>> LANDLOCK_ACCESS_FS_READ_DIR). I'd like these access rights to remain
>>>> here but this kernel patch and the related tests need some changes.
>>>>
>>>> What about a LANDLOCK_ACCESS_FS_CHGRP? I'm not sure if we need to
>>>> differentiate these actions or not, but we need arguments to choose.
>>>>
>>>>
>>>>>>     *
>>>>>>     * .. warning::
>>>>>>     *
>>>>>>     *   It is currently not possible to restrict some file-related
>>>>>> actions
>>>>>>     *   accessible through these syscall families:
>>>>>> :manpage:`chdir(2)`,
>>>>>> - *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
>>>>>> - *   :manpage:`chown(2)`, :manpage:`setxattr(2)`,
>>>>>> :manpage:`utime(2)`,
>>>>>> + *   :manpage:`stat(2)`, :manpage:`flock(2)`,
>>>>>> + *   :manpage:`setxattr(2)`, :manpage:`utime(2)`,
>>>>>
>>>>> *formatting nit*
>>>>> We could fill up the full line width here
>>>>>
>>>>>>     *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`,
>>>>>> :manpage:`access(2)`.
>>>>>>     *   Future Landlock evolutions will enable to restrict them.
>>>>>>     */
>>>>>> @@ -167,6 +169,8 @@ struct landlock_path_beneath_attr {
>>>>>>    #define LANDLOCK_ACCESS_FS_MAKE_SYM            (1ULL << 12)
>>>>>>    #define LANDLOCK_ACCESS_FS_REFER            (1ULL << 13)
>>>>>>    #define LANDLOCK_ACCESS_FS_TRUNCATE            (1ULL << 14)
>>>>>> +#define LANDLOCK_ACCESS_FS_CHMOD            (1ULL << 15)
>>>>>> +#define LANDLOCK_ACCESS_FS_CHOWN            (1ULL << 16)
>>>>>>    /* clang-format on */
>>>>>>
>>>>>>    #endif /* _UAPI_LINUX_LANDLOCK_H */
>>>>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>>>>> index c57f581a9cd5..c25d5f89c8be 100644
>>>>>> --- a/security/landlock/fs.c
>>>>>> +++ b/security/landlock/fs.c
>>>>>> @@ -147,7 +147,9 @@ static struct landlock_object
>>>>>> *get_inode_object(struct inode *const inode)
>>>>>>        LANDLOCK_ACCESS_FS_EXECUTE | \
>>>>>>        LANDLOCK_ACCESS_FS_WRITE_FILE | \
>>>>>>        LANDLOCK_ACCESS_FS_READ_FILE | \
>>>>>> -    LANDLOCK_ACCESS_FS_TRUNCATE)
>>>>>> +    LANDLOCK_ACCESS_FS_TRUNCATE | \
>>>>>> +    LANDLOCK_ACCESS_FS_CHMOD | \
>>>>>> +    LANDLOCK_ACCESS_FS_CHOWN)
>>>>>>    /* clang-format on */
>>>>>>
>>>>>>    /*
>>>>>> @@ -1146,6 +1148,16 @@ static int hook_path_truncate(const struct
>>>>>> path *const path)
>>>>>>        return current_check_access_path(path,
>>>>>> LANDLOCK_ACCESS_FS_TRUNCATE);
>>>>>>    }
>>>>>>
>>>>>> +static int hook_path_chmod(const struct path *const dir, umode_t
>>>>>> mode)
>>>>
>>>> This is not a "dir" but a "path".
>>>>
>>>>
>>>>>> +{
>>>>>> +    return current_check_access_path(dir, LANDLOCK_ACCESS_FS_CHMOD);
>>>>>> +}
>>>>>> +
>>>>>> +static int hook_path_chown(const struct path *const dir, kuid_t uid,
>>>>>> kgid_t gid)
>>>>
>>>> Same here.
>>>>
>>>>
>>>>>> +{
>>>>>> +    return current_check_access_path(dir, LANDLOCK_ACCESS_FS_CHOWN);
>>>>>> +}
>>>>>
>>>>> One implication of this approach is that the chown+chmod right on a
>>>>> directory's contents are always going together with the same rights on
>>>>> the directory itself.
>>>>>
>>>>> For example, if you grant chmod+chown access rights for "datadir/",
>>>>> the command "chmod 0600 datadir/file1" will work, but so will the
>>>>> command "chmod 0600 datadir". But the approach of checking just the
>>>>> parent directory's rights is also inflexible if you think through the
>>>>> kinds of rights you can grant with it. (It would also not be possible
>>>>> to grant chmod+chown on individual files.)
>>>>
>>>> Good point. For an initial chmod/chown/chgrp access right, I'd prefer to
>>>> be able to set these access rights on a directory but only for its
>>>> content, not the directory itself. I think it is much safer and should
>>>> be enough for the majority of use cases, but let me know if I'm missing
>>>> something. I'm not sure being able to change the root directory access
>>>> rights may be a good idea anyway (even for containers). ;)
>>>>
>>>> A path_beneath rule enables to identify a file hierarchy (i.e. the
>>>> content of a directory), not to make modifications visible outside of
>>>> the directory identifying the hierarchy (hence the "parent_fd" field),
>>>> which would be the case with the current chmod/chown access rights.
>>>>
>>>>
>>>>>
>>>>> Do you have any thoughts on how to resolve this if this flexibility
>>>>> might be needed?
>>>>>
>>>>> I wonder whether the right way to resolve this would be to give users
>>>>> a way to make that distinction at the level of landlock_add_rule(),
>>>>> with an API like this (note the additional flag):
>>>>>
>>>>>     err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
>>>>>                             &path_beneath, LANDLOCK_STRICTLY_BENEATH);
>>>>>                                            ^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>>
>>>>> Multiple calls of landlock_add_rule() on the same file are already
>>>>> today joining the requested access rights, so it would be possible to
>>>>> mix-and-match "strict beneath" with "beneath" rights on the same
>>>>> directory, and it would work in the same way for other access rights
>>>>> as well.
>>>>
>>>> This kind of option is interesting. For now, some access rights are kind
>>>> of "doubled" to enable to differentiate between a file and a directory
>>>> (i.e. READ_DIR/READ_FILE, REMOVE_DIR/REMOVE_FILE, WRITE_FILE/MAKE_*)
>>>> when it may be useful, but this is different.
>>>>
>>>> I think this "strictly beneath" behavior should be the default, which is
>>>> currently the case.
>>>>
>>>>
>>>>>
>>>>> To be clear: I'm proposing this approach not because I think it should
>>>>> be part of this patch set, but because it would be good to have a way
>>>>> forward if that kind of flexibility is needed in the future.
>>>>>
>>>>> Does that seem reasonable?
>>>>
>>>> This is the kind of questions that made such access rights not
>>>> appropriate for the initial version of Landlock. But we should talk
>>>> about that now.
>>>
>>> Hi Günther and Mickaël,
>>>
>>> Thanks for your comments, so I think the conclusion here is that we have
>>> to make sure that in this patchset chown/chmod access rights can be set
>>> on a directory only for its content, not the directory itself, right?
>>> any good idea about how to implement this? :)
>>
>> In such hook code, you need to get the parent directory of the path
>> argument. This require to use and refactor the
>> check_access_path_dual/jump_up part in a dedicated helper (and take care
>> of all the corner cases).
>> .
>
> Sorry, I don't quite understand what you mean, but I have another idea,
> how about this?
>
> static int hook_path_chown(const struct path *const path, kuid_t uid,
> kgid_t gid)
> {
> int ret;
> struct dentry *parent_dentry;
> struct path eff_path;
>
> eff_path = *path;
> path_get(&eff_path);
> if (d_is_dir(eff_path.dentry)) {
> parent_dentry = dget_parent(eff_path.dentry);
> dput(eff_path.dentry);
> eff_path.dentry = parent_dentry;
> }
> ret = current_check_access_path(&eff_path,
> LANDLOCK_ACCESS_FS_CHGRP);
> path_put(&eff_path);
>
> return ret;
> }

This is close but it ignores mount points (e.g. path being used multiple
time as a mount point). This is why we need to use follow_up(), hence my
previous comment. This is the kind of corner case that require tests.

This helper could look like this:
enum walk_result walk_to_visible_parent(struct path *path)
It could then return either WALK_CONTINUE, WALK_DENIED, or WALK_ALLOWED.

2022-08-26 11:56:52

[permalink] [raw]

Subject: Re: [PATCH -next 2/5] landlock: add chmod and chown support

On 26/08/2022 13:14, xiujianfeng wrote:
> Hi,
>
> 在 2022/8/26 17:36, Mickaël Salaün 写道:
>>
>> On 26/08/2022 10:36, xiujianfeng wrote:
>>> Hi,
>>>
>>> 在 2022/8/24 19:44, Mickaël Salaün 写道:
>>>>
>>>> On 23/08/2022 14:50, xiujianfeng wrote:
>>>>>
>>>>>
>>>>> 在 2022/8/23 5:07, Mickaël Salaün 写道:
>>>>>>
>>>>>> On 22/08/2022 20:25, Günther Noack wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>> Thanks for sending this patch set! :)
>>>>>>>
>>>>>>> On Mon, Aug 22, 2022 at 07:46:58PM +0800, Xiu Jianfeng wrote:
>>>>>>>> Add two flags LANDLOCK_ACCESS_FS_CHMOD and
>>>>>>>> LANDLOCK_ACCESS_FS_CHOWN to
>>>>>>>> support restriction to chmod(2) and chown(2) with landlock.
>>>>>>>>
>>>>>>>> Also change the landlock ABI version from 3 to 4.
>>>>>>>>
>>>>>>>> Signed-off-by: Xiu Jianfeng <[email protected]>
>>>>>>>> ---
>>>>>>>>     include/uapi/linux/landlock.h                | 8 ++++++--
>>>>>>>>     security/landlock/fs.c                       | 16
>>>>>>>> +++++++++++++++-
>>>>>>>>     security/landlock/limits.h                   | 2 +-
>>>>>>>>     security/landlock/syscalls.c                 | 2 +-
>>>>>>>>     tools/testing/selftests/landlock/base_test.c | 2 +-
>>>>>>>>     tools/testing/selftests/landlock/fs_test.c   | 6 ++++--
>>>>>>>>     6 files changed, 28 insertions(+), 8 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/uapi/linux/landlock.h
>>>>>>>> b/include/uapi/linux/landlock.h
>>>>>>>> index 735b1fe8326e..5ce633c92722 100644
>>>>>>>> --- a/include/uapi/linux/landlock.h
>>>>>>>> +++ b/include/uapi/linux/landlock.h
>>>>>>>> @@ -141,13 +141,15 @@ struct landlock_path_beneath_attr {
>>>>>>>>      *   directory) parent. Otherwise, such actions are denied with
>>>>>>>> errno set to
>>>>>>>>      *   EACCES. The EACCES errno prevails over EXDEV to let user
>>>>>>>> space
>>>>>>>>      *   efficiently deal with an unrecoverable error.
>>>>>>>> + * - %LANDLOCK_ACCESS_FS_CHMOD: Change the file mode bits of a
>>>>>>>> file.
>>>>>>>> + * - %LANDLOCK_ACCESS_FS_CHOWN: Change the owner and/or group of a
>>>>>>>> file.
>>>>>>
>>>>>> This section talk about "access rights that only apply to the
>>>>>> content of
>>>>>> a directory, not the directory itself", which is not correct (see
>>>>>> LANDLOCK_ACCESS_FS_READ_DIR). I'd like these access rights to remain
>>>>>> here but this kernel patch and the related tests need some changes.
>>>>>>
>>>>>> What about a LANDLOCK_ACCESS_FS_CHGRP? I'm not sure if we need to
>>>>>> differentiate these actions or not, but we need arguments to choose.
>>>>>>
>>>>>>
>>>>>>>>      *
>>>>>>>>      * .. warning::
>>>>>>>>      *
>>>>>>>>      *   It is currently not possible to restrict some file-related
>>>>>>>> actions
>>>>>>>>      *   accessible through these syscall families:
>>>>>>>> :manpage:`chdir(2)`,
>>>>>>>> - *   :manpage:`stat(2)`, :manpage:`flock(2)`, :manpage:`chmod(2)`,
>>>>>>>> - *   :manpage:`chown(2)`, :manpage:`setxattr(2)`,
>>>>>>>> :manpage:`utime(2)`,
>>>>>>>> + *   :manpage:`stat(2)`, :manpage:`flock(2)`,
>>>>>>>> + *   :manpage:`setxattr(2)`, :manpage:`utime(2)`,
>>>>>>>
>>>>>>> *formatting nit*
>>>>>>> We could fill up the full line width here
>>>>>>>
>>>>>>>>      *   :manpage:`ioctl(2)`, :manpage:`fcntl(2)`,
>>>>>>>> :manpage:`access(2)`.
>>>>>>>>      *   Future Landlock evolutions will enable to restrict them.
>>>>>>>>      */
>>>>>>>> @@ -167,6 +169,8 @@ struct landlock_path_beneath_attr {
>>>>>>>>     #define LANDLOCK_ACCESS_FS_MAKE_SYM            (1ULL << 12)
>>>>>>>>     #define LANDLOCK_ACCESS_FS_REFER            (1ULL << 13)
>>>>>>>>     #define LANDLOCK_ACCESS_FS_TRUNCATE            (1ULL << 14)
>>>>>>>> +#define LANDLOCK_ACCESS_FS_CHMOD            (1ULL << 15)
>>>>>>>> +#define LANDLOCK_ACCESS_FS_CHOWN            (1ULL << 16)
>>>>>>>>     /* clang-format on */
>>>>>>>>
>>>>>>>>     #endif /* _UAPI_LINUX_LANDLOCK_H */
>>>>>>>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>>>>>>>> index c57f581a9cd5..c25d5f89c8be 100644
>>>>>>>> --- a/security/landlock/fs.c
>>>>>>>> +++ b/security/landlock/fs.c
>>>>>>>> @@ -147,7 +147,9 @@ static struct landlock_object
>>>>>>>> *get_inode_object(struct inode *const inode)
>>>>>>>>         LANDLOCK_ACCESS_FS_EXECUTE | \
>>>>>>>>         LANDLOCK_ACCESS_FS_WRITE_FILE | \
>>>>>>>>         LANDLOCK_ACCESS_FS_READ_FILE | \
>>>>>>>> -    LANDLOCK_ACCESS_FS_TRUNCATE)
>>>>>>>> +    LANDLOCK_ACCESS_FS_TRUNCATE | \
>>>>>>>> +    LANDLOCK_ACCESS_FS_CHMOD | \
>>>>>>>> +    LANDLOCK_ACCESS_FS_CHOWN)
>>>>>>>>     /* clang-format on */
>>>>>>>>
>>>>>>>>     /*
>>>>>>>> @@ -1146,6 +1148,16 @@ static int hook_path_truncate(const struct
>>>>>>>> path *const path)
>>>>>>>>         return current_check_access_path(path,
>>>>>>>> LANDLOCK_ACCESS_FS_TRUNCATE);
>>>>>>>>     }
>>>>>>>>
>>>>>>>> +static int hook_path_chmod(const struct path *const dir, umode_t
>>>>>>>> mode)
>>>>>>
>>>>>> This is not a "dir" but a "path".
>>>>>>
>>>>>>
>>>>>>>> +{
>>>>>>>> +    return current_check_access_path(dir,
>>>>>>>> LANDLOCK_ACCESS_FS_CHMOD);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int hook_path_chown(const struct path *const dir, kuid_t
>>>>>>>> uid,
>>>>>>>> kgid_t gid)
>>>>>>
>>>>>> Same here.
>>>>>>
>>>>>>
>>>>>>>> +{
>>>>>>>> +    return current_check_access_path(dir,
>>>>>>>> LANDLOCK_ACCESS_FS_CHOWN);
>>>>>>>> +}
>>>>>>>
>>>>>>> One implication of this approach is that the chown+chmod right on a
>>>>>>> directory's contents are always going together with the same
>>>>>>> rights on
>>>>>>> the directory itself.
>>>>>>>
>>>>>>> For example, if you grant chmod+chown access rights for "datadir/",
>>>>>>> the command "chmod 0600 datadir/file1" will work, but so will the
>>>>>>> command "chmod 0600 datadir". But the approach of checking just the
>>>>>>> parent directory's rights is also inflexible if you think through the
>>>>>>> kinds of rights you can grant with it. (It would also not be possible
>>>>>>> to grant chmod+chown on individual files.)
>>>>>>
>>>>>> Good point. For an initial chmod/chown/chgrp access right, I'd
>>>>>> prefer to
>>>>>> be able to set these access rights on a directory but only for its
>>>>>> content, not the directory itself. I think it is much safer and should
>>>>>> be enough for the majority of use cases, but let me know if I'm
>>>>>> missing
>>>>>> something. I'm not sure being able to change the root directory access
>>>>>> rights may be a good idea anyway (even for containers). ;)
>>>>>>
>>>>>> A path_beneath rule enables to identify a file hierarchy (i.e. the
>>>>>> content of a directory), not to make modifications visible outside of
>>>>>> the directory identifying the hierarchy (hence the "parent_fd" field),
>>>>>> which would be the case with the current chmod/chown access rights.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Do you have any thoughts on how to resolve this if this flexibility
>>>>>>> might be needed?
>>>>>>>
>>>>>>> I wonder whether the right way to resolve this would be to give users
>>>>>>> a way to make that distinction at the level of landlock_add_rule(),
>>>>>>> with an API like this (note the additional flag):
>>>>>>>
>>>>>>>      err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
>>>>>>>                              &path_beneath,
>>>>>>> LANDLOCK_STRICTLY_BENEATH);
>>>>>>>                                             ^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>>>>
>>>>>>> Multiple calls of landlock_add_rule() on the same file are already
>>>>>>> today joining the requested access rights, so it would be possible to
>>>>>>> mix-and-match "strict beneath" with "beneath" rights on the same
>>>>>>> directory, and it would work in the same way for other access rights
>>>>>>> as well.
>>>>>>
>>>>>> This kind of option is interesting. For now, some access rights are
>>>>>> kind
>>>>>> of "doubled" to enable to differentiate between a file and a directory
>>>>>> (i.e. READ_DIR/READ_FILE, REMOVE_DIR/REMOVE_FILE, WRITE_FILE/MAKE_*)
>>>>>> when it may be useful, but this is different.
>>>>>>
>>>>>> I think this "strictly beneath" behavior should be the default,
>>>>>> which is
>>>>>> currently the case.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> To be clear: I'm proposing this approach not because I think it
>>>>>>> should
>>>>>>> be part of this patch set, but because it would be good to have a way
>>>>>>> forward if that kind of flexibility is needed in the future.
>>>>>>>
>>>>>>> Does that seem reasonable?
>>>>>>
>>>>>> This is the kind of questions that made such access rights not
>>>>>> appropriate for the initial version of Landlock. But we should talk
>>>>>> about that now.
>>>>>
>>>>> Hi Günther and Mickaël,
>>>>>
>>>>> Thanks for your comments, so I think the conclusion here is that we
>>>>> have
>>>>> to make sure that in this patchset chown/chmod access rights can be set
>>>>> on a directory only for its content, not the directory itself, right?
>>>>> any good idea about how to implement this? :)
>>>>
>>>> In such hook code, you need to get the parent directory of the path
>>>> argument. This require to use and refactor the
>>>> check_access_path_dual/jump_up part in a dedicated helper (and take care
>>>> of all the corner cases).
>>>> .
>>>
>>> Sorry, I don't quite understand what you mean, but I have another idea,
>>> how about this?
>>>
>>> static int hook_path_chown(const struct path *const path, kuid_t uid,
>>> kgid_t gid)
>>> {
>>>           int ret;
>>>           struct dentry *parent_dentry;
>>>           struct path eff_path;
>>>
>>>           eff_path = *path;
>>>           path_get(&eff_path);
>>>           if (d_is_dir(eff_path.dentry)) {
>>>                   parent_dentry = dget_parent(eff_path.dentry);
>>>                   dput(eff_path.dentry);
>>>                   eff_path.dentry = parent_dentry;
>>>           }
>>>           ret = current_check_access_path(&eff_path,
>>> LANDLOCK_ACCESS_FS_CHGRP);
>>>           path_put(&eff_path);
>>>
>>>           return ret;
>>> }
>>
>> This is close but it ignores mount points (e.g. path being used multiple
>> time as a mount point). This is why we need to use follow_up(), hence my
>> previous comment. This is the kind of corner case that require tests.
>>
>> This helper could look like this:
>> enum walk_result walk_to_visible_parent(struct path *path)
>> It could then return either WALK_CONTINUE, WALK_DENIED, or WALK_ALLOWED.
>> .
>
> Thanks, It's more clear now, except the return type, I think void type
> like follows maybe ok:

The enum return type is required to use this helper in
check_access_path_dual(), and to handles the same cases (e.g. internal
mount point).

>
> static void walk_to_visible_parent(struct path *path)
> {
> struct dentry *parent_dentry;
>
> path_get(path);
> /* don't need to follow_up if not dir */
> if (!d_is_dir(path->dentry))

This check should be in hook_path_chown(), to know if it makes sense to
call walk_to_visible_parent().

> return;
>
> jump_up:
> if (path->dentry == path->mnt->mnt_root) {
> if (follow_up(path)) {
> /* Ignores hidden mount points. */
> goto jump_up;
> } else {
> /*Stops at the real root. */
> return;
> }
> }
> parent_dentry = dget_parent(path->dentry);
> dput(path->dentry);
> path->dentry = parent_dentry;
> }
>
> static void walk_to_visible_parent_end(struct path *path)

This function is not useful, we could just explicitly call path_put()
and document that requirement. To make it easier to understand and more
consistent, we should not call path_get() in walk_to_visible_parent()
but before to make it explicit. Something like this:

if (d_is_dir(path->dentry)) {
path_get(path);
switch (walk_to_visible_parent(path)) {
…
}
path_put(path);
…
}

> {
> path_put(path);
> }
>
> static int hook_path_chown(const struct path *const path, kuid_t uid,
> kgid_t gid)
> {
> int ret;
> struct path eff_path;

All Landlock hooks must first check that a process is tied to a domain
and return immediately if it is not the case.

>
> eff_path = *path;
> walk_to_visible_parent(&eff_path);
> ret = current_check_access_path(&eff_path,
> LANDLOCK_ACCESS_FS_CHGRP);
> walk_to_visible_parent_end(&eff_path);
>
> return ret;
> }
>

2022-08-27 17:07:49