| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 25 Sep 2023 18:26:28 -0700
From: Jeff Xu <jeffxu@...gle.com>
To: Mickaël Salaün <mic@...ikod.net>
Cc: Eric Paris <eparis@...hat.com>, James Morris <jmorris@...ei.org>,
Paul Moore <paul@...l-moore.com>,
"Serge E . Hallyn" <serge@...lyn.com>,
Ben Scarlato <akhna@...gle.com>,
Günther Noack <gnoack@...gle.com>,
Jorge Lucangeli Obes <jorgelo@...gle.com>,
Konstantin Meskhidze <konstantin.meskhidze@...wei.com>,
Shervin Oloumi <enlightened@...gle.com>, audit@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [RFC PATCH v1 5/7] landlock: Log file-related requests
On Wed, Sep 20, 2023 at 11:17 PM Mickaël Salaün <mic@...ikod.net> wrote:
>
> Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate,
> and open requests.
>
> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> ---
> security/landlock/audit.c | 114 ++++++++++++++++++++++++++++++++++++++
> security/landlock/audit.h | 32 +++++++++++
> security/landlock/fs.c | 62 ++++++++++++++++++---
> 3 files changed, 199 insertions(+), 9 deletions(-)
>
> diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> index d9589d07e126..148fc0fafef4 100644
> --- a/security/landlock/audit.c
> +++ b/security/landlock/audit.c
> @@ -14,6 +14,25 @@
>
> atomic64_t ruleset_and_domain_counter = ATOMIC64_INIT(0);
>
> +static const char *op_to_string(enum landlock_operation operation)
> +{
> + const char *const desc[] = {
> + [0] = "",
> + [LANDLOCK_OP_MKDIR] = "mkdir",
> + [LANDLOCK_OP_MKNOD] = "mknod",
> + [LANDLOCK_OP_SYMLINK] = "symlink",
> + [LANDLOCK_OP_UNLINK] = "unlink",
> + [LANDLOCK_OP_RMDIR] = "rmdir",
> + [LANDLOCK_OP_TRUNCATE] = "truncate",
> + [LANDLOCK_OP_OPEN] = "open",
> + };
> +
> + if (WARN_ON_ONCE(operation < 0 || operation > ARRAY_SIZE(desc)))
> + return "unknown";
> +
> + return desc[operation];
> +}
> +
> #define BIT_INDEX(bit) HWEIGHT(bit - 1)
>
> static void log_accesses(struct audit_buffer *const ab,
> @@ -141,3 +160,98 @@ void landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset)
> audit_log_format(ab, "op=release-%s %s=%llu", name, name, id);
> audit_log_end(ab);
> }
> +
> +/* Update request.youngest_domain and request.missing_access */
> +static void
> +update_request(struct landlock_request *const request,
> + const struct landlock_ruleset *const domain,
> + const access_mask_t access_request,
> + const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
> +{
> + const unsigned long access_req = access_request;
> + unsigned long access_bit;
> + long youngest_denied_layer = -1;
> + const struct landlock_hierarchy *node = domain->hierarchy;
> + size_t i;
> +
> + WARN_ON_ONCE(request->youngest_domain);
> + WARN_ON_ONCE(request->missing_access);
> +
> + if (WARN_ON_ONCE(!access_request))
> + return;
> +
> + if (WARN_ON_ONCE(!layer_masks))
> + return;
> +
> + for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(*layer_masks)) {
> + long domain_layer;
> +
> + if (!(*layer_masks)[access_bit])
> + continue;
> +
> + domain_layer = __fls((*layer_masks)[access_bit]);
> +
> + /*
> + * Gets the access rights that are missing from
> + * the youngest (i.e. closest) domain.
> + */
> + if (domain_layer == youngest_denied_layer) {
> + request->missing_access |= BIT_ULL(access_bit);
> + } else if (domain_layer > youngest_denied_layer) {
> + youngest_denied_layer = domain_layer;
> + request->missing_access = BIT_ULL(access_bit);
> + }
> + }
> +
> + WARN_ON_ONCE(!request->missing_access);
> + WARN_ON_ONCE(youngest_denied_layer < 0);
> +
> + /* Gets the nearest domain ID that denies request.missing_access */
> + for (i = domain->num_layers - youngest_denied_layer - 1; i > 0; i--)
> + node = node->parent;
> + request->youngest_domain = node->id;
> +}
> +
> +static void
> +log_request(const int error, struct landlock_request *const request,
> + const struct landlock_ruleset *const domain,
> + const access_mask_t access_request,
> + const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
> +{
> + struct audit_buffer *ab;
> +
> + if (WARN_ON_ONCE(!error))
> + return;
> + if (WARN_ON_ONCE(!request))
> + return;
> + if (WARN_ON_ONCE(!domain || !domain->hierarchy))
> + return;
> +
> + /* Uses GFP_ATOMIC to not sleep. */
> + ab = audit_log_start(audit_context(), GFP_ATOMIC | __GFP_NOWARN,
> + AUDIT_LANDLOCK);
> + if (!ab)
> + return;
> +
> + update_request(request, domain, access_request, layer_masks);
> +
> + log_task(ab);
> + audit_log_format(ab, " domain=%llu op=%s errno=%d missing-fs-accesses=",
> + request->youngest_domain,
> + op_to_string(request->operation), -error);
> + log_accesses(ab, request->missing_access);
> + audit_log_lsm_data(ab, &request->audit);
> + audit_log_end(ab);
> +}
> +
> +// TODO: Make it generic, not FS-centric.
> +int landlock_log_request(
> + const int error, struct landlock_request *const request,
> + const struct landlock_ruleset *const domain,
> + const access_mask_t access_request,
> + const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
> +{
> + /* No need to log the access request, only the missing accesses. */
> + log_request(error, request, domain, access_request, layer_masks);
> + return error;
> +}
> diff --git a/security/landlock/audit.h b/security/landlock/audit.h
> index bc17dc8ca6f1..8edc68b98fca 100644
> --- a/security/landlock/audit.h
> +++ b/security/landlock/audit.h
> @@ -13,6 +13,23 @@
>
> #include "ruleset.h"
>
> +enum landlock_operation {
> + LANDLOCK_OP_MKDIR = 1,
> + LANDLOCK_OP_MKNOD,
> + LANDLOCK_OP_SYMLINK,
> + LANDLOCK_OP_UNLINK,
> + LANDLOCK_OP_RMDIR,
> + LANDLOCK_OP_TRUNCATE,
> + LANDLOCK_OP_OPEN,
> +};
> +
> +struct landlock_request {
> + const enum landlock_operation operation;
> + access_mask_t missing_access;
> + u64 youngest_domain;
> + struct common_audit_data audit;
> +};
> +
> #ifdef CONFIG_AUDIT
>
> void landlock_log_create_ruleset(struct landlock_ruleset *const ruleset);
> @@ -20,6 +37,12 @@ void landlock_log_restrict_self(struct landlock_ruleset *const domain,
> struct landlock_ruleset *const ruleset);
> void landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset);
>
> +int landlock_log_request(
> + const int error, struct landlock_request *const request,
> + const struct landlock_ruleset *const domain,
> + const access_mask_t access_request,
> + const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS]);
> +
> #else /* CONFIG_AUDIT */
>
> static inline void
> @@ -38,6 +61,15 @@ landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset)
> {
> }
>
> +static inline int landlock_log_request(
> + const int error, struct landlock_request *const request,
> + const struct landlock_ruleset *const domain,
> + const access_mask_t access_request,
> + const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
> +{
> + return error;
> +}
> +
> #endif /* CONFIG_AUDIT */
>
> #endif /* _SECURITY_LANDLOCK_AUDIT_H */
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 978e325d8708..104dfb2abc32 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -18,6 +18,7 @@
> #include <linux/kernel.h>
> #include <linux/limits.h>
> #include <linux/list.h>
> +#include <linux/lsm_audit.h>
> #include <linux/lsm_hooks.h>
> #include <linux/mount.h>
> #include <linux/namei.h>
> @@ -30,6 +31,7 @@
> #include <linux/workqueue.h>
> #include <uapi/linux/landlock.h>
>
> +#include "audit.h"
> #include "common.h"
> #include "cred.h"
> #include "fs.h"
> @@ -636,7 +638,8 @@ static bool is_access_to_paths_allowed(
> }
>
> static int current_check_access_path(const struct path *const path,
> - access_mask_t access_request)
> + access_mask_t access_request,
> + struct landlock_request *const request)
> {
> const struct landlock_ruleset *const dom =
> landlock_get_current_domain();
> @@ -650,7 +653,10 @@ static int current_check_access_path(const struct path *const path,
> NULL, 0, NULL, NULL))
> return 0;
>
> - return -EACCES;
> + request->audit.type = LSM_AUDIT_DATA_PATH;
> + request->audit.u.path = *path;
> + return landlock_log_request(-EACCES, request, dom, access_request,
> + &layer_masks);
It might be more readable to let landlock_log_request return void.
Then the code will look like below.
landlock_log_request(-EACCES, request, dom, access_request, &layer_masks);
return -EACCES;
The allow/deny logic will be in this function, i.e. reader
doesn't need to check landlock_log_request's implementation to find
out it never returns 0.
-Jeff
> }
>
> static inline access_mask_t get_mode_access(const umode_t mode)
> @@ -1097,6 +1103,7 @@ static int hook_path_link(struct dentry *const old_dentry,
> const struct path *const new_dir,
> struct dentry *const new_dentry)
> {
> + // TODO: Implement fine-grained audit
> return current_check_refer_path(old_dentry, new_dir, new_dentry, false,
> false);
> }
> @@ -1115,38 +1122,67 @@ static int hook_path_rename(const struct path *const old_dir,
> static int hook_path_mkdir(const struct path *const dir,
> struct dentry *const dentry, const umode_t mode)
> {
> - return current_check_access_path(dir, LANDLOCK_ACCESS_FS_MAKE_DIR);
> + struct landlock_request request = {
> + .operation = LANDLOCK_OP_MKDIR,
> + };
> +
> + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_MAKE_DIR,
> + &request);
> }
>
> static int hook_path_mknod(const struct path *const dir,
> struct dentry *const dentry, const umode_t mode,
> const unsigned int dev)
> {
> - return current_check_access_path(dir, get_mode_access(mode));
> + struct landlock_request request = {
> + .operation = LANDLOCK_OP_MKNOD,
> + };
> +
> + return current_check_access_path(dir, get_mode_access(mode), &request);
> }
>
> static int hook_path_symlink(const struct path *const dir,
> struct dentry *const dentry,
> const char *const old_name)
> {
> - return current_check_access_path(dir, LANDLOCK_ACCESS_FS_MAKE_SYM);
> + struct landlock_request request = {
> + .operation = LANDLOCK_OP_SYMLINK,
> + };
> +
> + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_MAKE_SYM,
> + &request);
> }
>
> static int hook_path_unlink(const struct path *const dir,
> struct dentry *const dentry)
> {
> - return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_FILE);
> + struct landlock_request request = {
> + .operation = LANDLOCK_OP_UNLINK,
> + };
> +
> + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_FILE,
> + &request);
> }
>
> static int hook_path_rmdir(const struct path *const dir,
> struct dentry *const dentry)
> {
> - return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
> + struct landlock_request request = {
> + .operation = LANDLOCK_OP_RMDIR,
> + };
> +
> + return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR,
> + &request);
> }
>
> static int hook_path_truncate(const struct path *const path)
> {
> - return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE);
> + struct landlock_request request = {
> + .operation = LANDLOCK_OP_TRUNCATE,
> + };
> +
> + return current_check_access_path(path, LANDLOCK_ACCESS_FS_TRUNCATE,
> + &request);
> }
>
> /* File hooks */
> @@ -1199,6 +1235,13 @@ static int hook_file_open(struct file *const file)
> const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
> const struct landlock_ruleset *const dom =
> landlock_get_current_domain();
> + struct landlock_request request = {
> + .operation = LANDLOCK_OP_OPEN,
> + .audit = {
> + .type = LSM_AUDIT_DATA_PATH,
> + .u.path = file->f_path,
> + },
> + };
>
> if (!dom)
> return 0;
> @@ -1249,7 +1292,8 @@ static int hook_file_open(struct file *const file)
> if ((open_access_request & allowed_access) == open_access_request)
> return 0;
>
> - return -EACCES;
> + return landlock_log_request(-EACCES, &request, dom, open_access_request,
> + &layer_masks);
> }
>
> static int hook_file_truncate(struct file *const file)
> --
> 2.42.0
>
Powered by blists - more mailing lists