Hi,
This patch series adds basic audit support to Landlock for most actions.
Logging denied requests is useful for different use cases:
* app developers: to ease and speed up sandboxing support
* power users: to understand denials
* sysadmins: to look for users' issues
* tailored distro maintainers: to get usage metrics from their fleet
* security experts: to detect attack attempts
To make logs useful, they need to contain the most relevant Landlock
domain that denied an action, and the reason. This translates to the
latest nested domain and the related missing access rights.
Two "Landlock permissions" are used to describe mandatory restrictions
enforced on all domains:
* fs_layout: change the view of filesystem with mount operations.
* ptrace: tamper with a process.
Here is an example of logs, result of the sandboxer activity:
tid=267 comm="sandboxer" op=create-ruleset ruleset=1 handled_access_fs=execute,write_file,read_file,read_dir,remove_dir,remove_file,make_char,make_dir,make_reg,make_sock,make_fifo,make_block,make_sym,refer,truncate
tid=267 comm="sandboxer" op=restrict-self domain=2 ruleset=1 parent=0
op=release-ruleset ruleset=1
tid=267 comm="bash" domain=2 op=open errno=13 missing-fs-accesses=write_file,read_file missing-permission= path="/dev/tty" dev="devtmpfs" ino=9
tid=268 comm="ls" domain=2 op=open errno=13 missing-fs-accesses=read_dir missing-permission= path="/" dev="vda2" ino=256
tid=269 comm="touch" domain=2 op=mknod errno=13 missing-fs-accesses=make_reg missing-permission= path="/" dev="vda2" ino=256
tid=270 comm="umount" domain=2 op=umount errno=1 missing-fs-accesses= missing-permission=fs_layout name="/" dev="tmpfs" ino=1
tid=271 comm="strace" domain=2 op=ptrace errno=1 missing-fs-accesses= missing-permission=ptrace opid=1 ocomm="systemd"
As highlighted in comments, support for audit is not complete yet with
this series: some actions are not logged (e.g. file reparenting), and
rule additions are not logged neither.
I'm also not sure if we need to have seccomp-like features such as
SECCOMP_FILTER_FLAG_LOG, SECCOMP_RET_LOG, and
/proc/sys/kernel/seccomp/actions_logged
I'd like to get some early feedback on this proposal.
This series is based on v6.6-rc2
Regards,
Mickaël Salaün (7):
lsm: Add audit_log_lsm_data() helper
landlock: Factor out check_access_path()
landlock: Log ruleset creation and release
landlock: Log domain creation and enforcement
landlock: Log file-related requests
landlock: Log mount-related requests
landlock: Log ptrace requests
include/linux/lsm_audit.h | 2 +
include/uapi/linux/audit.h | 1 +
security/landlock/Makefile | 2 +
security/landlock/audit.c | 283 +++++++++++++++++++++++++++++++++++
security/landlock/audit.h | 88 +++++++++++
security/landlock/fs.c | 169 ++++++++++++++++-----
security/landlock/ptrace.c | 47 +++++-
security/landlock/ruleset.c | 6 +
security/landlock/ruleset.h | 10 ++
security/landlock/syscalls.c | 12 ++
security/lsm_audit.c | 26 ++--
11 files changed, 595 insertions(+), 51 deletions(-)
create mode 100644 security/landlock/audit.c
create mode 100644 security/landlock/audit.h
base-commit: ce9ecca0238b140b88f43859b211c9fdfd8e5b70
--
2.42.0
Add audit support for ptrace and ptrace_traceme requests.
Signed-off-by: Mickaël Salaün <[email protected]>
---
security/landlock/audit.c | 2 ++
security/landlock/audit.h | 4 +++-
security/landlock/ptrace.c | 47 ++++++++++++++++++++++++++++++++++----
3 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/security/landlock/audit.c b/security/landlock/audit.c
index 89bd701d124f..2ec2a00822d2 100644
--- a/security/landlock/audit.c
+++ b/security/landlock/audit.c
@@ -18,6 +18,8 @@ static const char *op_to_string(enum landlock_operation operation)
{
const char *const desc[] = {
[0] = "",
+ [LANDLOCK_OP_PTRACE] = "ptrace",
+ [LANDLOCK_OP_PTRACE_TRACEME] = "ptrace_traceme",
[LANDLOCK_OP_MOUNT] = "mount",
[LANDLOCK_OP_MOVE_MOUNT] = "move_mount",
[LANDLOCK_OP_UMOUNT] = "umount",
diff --git a/security/landlock/audit.h b/security/landlock/audit.h
index e559fb6a89dd..b69bba7b908c 100644
--- a/security/landlock/audit.h
+++ b/security/landlock/audit.h
@@ -14,7 +14,9 @@
#include "ruleset.h"
enum landlock_operation {
- LANDLOCK_OP_MOUNT = 1,
+ LANDLOCK_OP_PTRACE = 1,
+ LANDLOCK_OP_PTRACE_TRACEME,
+ LANDLOCK_OP_MOUNT,
LANDLOCK_OP_MOVE_MOUNT,
LANDLOCK_OP_UMOUNT,
LANDLOCK_OP_REMOUNT,
diff --git a/security/landlock/ptrace.c b/security/landlock/ptrace.c
index 8a06d6c492bf..dbe219449a32 100644
--- a/security/landlock/ptrace.c
+++ b/security/landlock/ptrace.c
@@ -10,10 +10,12 @@
#include <linux/cred.h>
#include <linux/errno.h>
#include <linux/kernel.h>
+#include <linux/lsm_audit.h>
#include <linux/lsm_hooks.h>
#include <linux/rcupdate.h>
#include <linux/sched.h>
+#include "audit.h"
#include "common.h"
#include "cred.h"
#include "ptrace.h"
@@ -64,11 +66,9 @@ static bool task_is_scoped(const struct task_struct *const parent,
static int task_ptrace(const struct task_struct *const parent,
const struct task_struct *const child)
{
- /* Quick return for non-landlocked tasks. */
- if (!landlocked(parent))
- return 0;
if (task_is_scoped(parent, child))
return 0;
+
return -EPERM;
}
@@ -88,7 +88,26 @@ static int task_ptrace(const struct task_struct *const parent,
static int hook_ptrace_access_check(struct task_struct *const child,
const unsigned int mode)
{
- return task_ptrace(current, child);
+ const struct landlock_ruleset *const dom =
+ landlock_get_current_domain();
+ struct landlock_request request = {
+ .operation = LANDLOCK_OP_PTRACE,
+ .missing_permission = LANDLOCK_PERM_PTRACE,
+ .audit = {
+ .type = LSM_AUDIT_DATA_TASK,
+ .u.tsk = child,
+ },
+ };
+ int err;
+
+ if (!dom)
+ return 0;
+
+ err = task_ptrace(current, child);
+ if (!err)
+ return 0;
+
+ return landlock_log_request(err, &request, dom, 0, NULL);
}
/**
@@ -105,7 +124,25 @@ static int hook_ptrace_access_check(struct task_struct *const child,
*/
static int hook_ptrace_traceme(struct task_struct *const parent)
{
- return task_ptrace(parent, current);
+ struct landlock_request request = {
+ .operation = LANDLOCK_OP_PTRACE_TRACEME,
+ .missing_permission = LANDLOCK_PERM_PTRACE,
+ .audit = {
+ .type = LSM_AUDIT_DATA_TASK,
+ .u.tsk = parent,
+ },
+ };
+ int err;
+
+ if (!landlock_get_task_domain(parent))
+ return 0;
+
+ err = task_ptrace(parent, current);
+ if (!err)
+ return 0;
+
+ return landlock_log_request(err, &request,
+ landlock_get_current_domain(), 0, NULL);
}
static struct security_hook_list landlock_hooks[] __ro_after_init = {
--
2.42.0
Merge check_access_path() into current_check_access_path() and make
hook_path_mknod() use it. Remove explicit inlining that can be handled
by the compiler.
Signed-off-by: Mickaël Salaün <[email protected]>
---
security/landlock/fs.c | 33 +++++++++++----------------------
1 file changed, 11 insertions(+), 22 deletions(-)
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 1c0c198f6fdb..978e325d8708 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -635,28 +635,22 @@ static bool is_access_to_paths_allowed(
return allowed_parent1 && allowed_parent2;
}
-static inline int check_access_path(const struct landlock_ruleset *const domain,
- const struct path *const path,
- access_mask_t access_request)
-{
- layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
-
- access_request = init_layer_masks(domain, access_request, &layer_masks);
- if (is_access_to_paths_allowed(domain, path, access_request,
- &layer_masks, NULL, 0, NULL, NULL))
- return 0;
- return -EACCES;
-}
-
-static inline int current_check_access_path(const struct path *const path,
- const access_mask_t access_request)
+static int current_check_access_path(const struct path *const path,
+ access_mask_t access_request)
{
const struct landlock_ruleset *const dom =
landlock_get_current_domain();
+ layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
if (!dom)
return 0;
- return check_access_path(dom, path, access_request);
+
+ access_request = init_layer_masks(dom, access_request, &layer_masks);
+ if (is_access_to_paths_allowed(dom, path, access_request, &layer_masks,
+ NULL, 0, NULL, NULL))
+ return 0;
+
+ return -EACCES;
}
static inline access_mask_t get_mode_access(const umode_t mode)
@@ -1128,12 +1122,7 @@ static int hook_path_mknod(const struct path *const dir,
struct dentry *const dentry, const umode_t mode,
const unsigned int dev)
{
- const struct landlock_ruleset *const dom =
- landlock_get_current_domain();
-
- if (!dom)
- return 0;
- return check_access_path(dom, dir, get_mode_access(mode));
+ return current_check_access_path(dir, get_mode_access(mode));
}
static int hook_path_symlink(const struct path *const dir,
--
2.42.0
Add audit support for mount, move_mount, umount, remount, and pivot_root
requests.
Signed-off-by: Mickaël Salaün <[email protected]>
---
security/landlock/audit.c | 26 ++++++++++++-
security/landlock/audit.h | 13 ++++++-
security/landlock/fs.c | 82 ++++++++++++++++++++++++++++++++++-----
3 files changed, 109 insertions(+), 12 deletions(-)
diff --git a/security/landlock/audit.c b/security/landlock/audit.c
index 148fc0fafef4..89bd701d124f 100644
--- a/security/landlock/audit.c
+++ b/security/landlock/audit.c
@@ -18,6 +18,11 @@ static const char *op_to_string(enum landlock_operation operation)
{
const char *const desc[] = {
[0] = "",
+ [LANDLOCK_OP_MOUNT] = "mount",
+ [LANDLOCK_OP_MOVE_MOUNT] = "move_mount",
+ [LANDLOCK_OP_UMOUNT] = "umount",
+ [LANDLOCK_OP_REMOUNT] = "remount",
+ [LANDLOCK_OP_PIVOT_ROOT] = "pivot_root",
[LANDLOCK_OP_MKDIR] = "mkdir",
[LANDLOCK_OP_MKNOD] = "mknod",
[LANDLOCK_OP_SYMLINK] = "symlink",
@@ -33,6 +38,20 @@ static const char *op_to_string(enum landlock_operation operation)
return desc[operation];
}
+static const char *perm_to_string(enum landlock_permission permission)
+{
+ const char *const desc[] = {
+ [0] = "",
+ [LANDLOCK_PERM_PTRACE] = "ptrace",
+ [LANDLOCK_PERM_FS_LAYOUT] = "fs_layout",
+ };
+
+ if (WARN_ON_ONCE(permission < 0 || permission > ARRAY_SIZE(desc)))
+ return "unknown";
+
+ return desc[permission];
+}
+
#define BIT_INDEX(bit) HWEIGHT(bit - 1)
static void log_accesses(struct audit_buffer *const ab,
@@ -177,8 +196,11 @@ update_request(struct landlock_request *const request,
WARN_ON_ONCE(request->youngest_domain);
WARN_ON_ONCE(request->missing_access);
- if (WARN_ON_ONCE(!access_request))
+ if (!access_request) {
+ /* No missing accesses. */
+ request->youngest_domain = node->id;
return;
+ }
if (WARN_ON_ONCE(!layer_masks))
return;
@@ -240,6 +262,8 @@ log_request(const int error, struct landlock_request *const request,
request->youngest_domain,
op_to_string(request->operation), -error);
log_accesses(ab, request->missing_access);
+ audit_log_format(ab, " missing-permission=%s",
+ perm_to_string(request->missing_permission));
audit_log_lsm_data(ab, &request->audit);
audit_log_end(ab);
}
diff --git a/security/landlock/audit.h b/security/landlock/audit.h
index 8edc68b98fca..e559fb6a89dd 100644
--- a/security/landlock/audit.h
+++ b/security/landlock/audit.h
@@ -14,7 +14,12 @@
#include "ruleset.h"
enum landlock_operation {
- LANDLOCK_OP_MKDIR = 1,
+ LANDLOCK_OP_MOUNT = 1,
+ LANDLOCK_OP_MOVE_MOUNT,
+ LANDLOCK_OP_UMOUNT,
+ LANDLOCK_OP_REMOUNT,
+ LANDLOCK_OP_PIVOT_ROOT,
+ LANDLOCK_OP_MKDIR,
LANDLOCK_OP_MKNOD,
LANDLOCK_OP_SYMLINK,
LANDLOCK_OP_UNLINK,
@@ -23,8 +28,14 @@ enum landlock_operation {
LANDLOCK_OP_OPEN,
};
+enum landlock_permission {
+ LANDLOCK_PERM_PTRACE = 1,
+ LANDLOCK_PERM_FS_LAYOUT,
+};
+
struct landlock_request {
const enum landlock_operation operation;
+ const enum landlock_permission missing_permission;
access_mask_t missing_access;
u64 youngest_domain;
struct common_audit_data audit;
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 104dfb2abc32..8600530e304c 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -1050,17 +1050,41 @@ static int hook_sb_mount(const char *const dev_name,
const struct path *const path, const char *const type,
const unsigned long flags, void *const data)
{
- if (!landlock_get_current_domain())
+ const struct landlock_ruleset *const dom =
+ landlock_get_current_domain();
+ struct landlock_request request = {
+ .operation = LANDLOCK_OP_MOUNT,
+ .missing_permission = LANDLOCK_PERM_FS_LAYOUT,
+ .audit = {
+ .type = LSM_AUDIT_DATA_PATH,
+ .u.path = *path,
+ },
+ };
+
+ if (!dom)
return 0;
- return -EPERM;
+
+ return landlock_log_request(-EPERM, &request, dom, 0, NULL);
}
static int hook_move_mount(const struct path *const from_path,
const struct path *const to_path)
{
- if (!landlock_get_current_domain())
+ const struct landlock_ruleset *const dom =
+ landlock_get_current_domain();
+ struct landlock_request request = {
+ .operation = LANDLOCK_OP_MOVE_MOUNT,
+ .missing_permission = LANDLOCK_PERM_FS_LAYOUT,
+ .audit = {
+ .type = LSM_AUDIT_DATA_PATH,
+ .u.path = *to_path,
+ },
+ };
+
+ if (!dom)
return 0;
- return -EPERM;
+
+ return landlock_log_request(-EPERM, &request, dom, 0, NULL);
}
/*
@@ -1069,16 +1093,42 @@ static int hook_move_mount(const struct path *const from_path,
*/
static int hook_sb_umount(struct vfsmount *const mnt, const int flags)
{
- if (!landlock_get_current_domain())
+ const struct landlock_ruleset *const dom =
+ landlock_get_current_domain();
+ struct landlock_request request = {
+ .operation = LANDLOCK_OP_UMOUNT,
+ .missing_permission = LANDLOCK_PERM_FS_LAYOUT,
+ .audit = {
+ // TODO: try to print the mounted path
+ // cf. dentry_path()
+ .type = LSM_AUDIT_DATA_DENTRY,
+ .u.dentry = mnt->mnt_root,
+ },
+ };
+
+ if (!dom)
return 0;
- return -EPERM;
+
+ return landlock_log_request(-EPERM, &request, dom, 0, NULL);
}
static int hook_sb_remount(struct super_block *const sb, void *const mnt_opts)
{
- if (!landlock_get_current_domain())
+ const struct landlock_ruleset *const dom =
+ landlock_get_current_domain();
+ struct landlock_request request = {
+ .operation = LANDLOCK_OP_REMOUNT,
+ .missing_permission = LANDLOCK_PERM_FS_LAYOUT,
+ .audit = {
+ .type = LSM_AUDIT_DATA_DENTRY,
+ .u.dentry = sb->s_root,
+ },
+ };
+
+ if (!dom)
return 0;
- return -EPERM;
+
+ return landlock_log_request(-EPERM, &request, dom, 0, NULL);
}
/*
@@ -1092,9 +1142,21 @@ static int hook_sb_remount(struct super_block *const sb, void *const mnt_opts)
static int hook_sb_pivotroot(const struct path *const old_path,
const struct path *const new_path)
{
- if (!landlock_get_current_domain())
+ const struct landlock_ruleset *const dom =
+ landlock_get_current_domain();
+ struct landlock_request request = {
+ .operation = LANDLOCK_OP_PIVOT_ROOT,
+ .missing_permission = LANDLOCK_PERM_FS_LAYOUT,
+ .audit = {
+ .type = LSM_AUDIT_DATA_PATH,
+ .u.path = *new_path,
+ },
+ };
+
+ if (!dom)
return 0;
- return -EPERM;
+
+ return landlock_log_request(-EPERM, &request, dom, 0, NULL);
}
/* Path hooks */
--
2.42.0
Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate,
and open requests.
Signed-off-by: Mickaël Salaün <[email protected]>
---
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);
}
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
Add audit support for ruleset/domain creation and release. Ruleset and
domain IDs are generated from the same 64-bit counter to avoid confusing
them. There is no need to hide the sequentiality to users that are
already allowed to read logs. In the future, if these IDs were to be
viewable by unprivileged users, then we'll need to scramble them.
Add a new AUDIT_LANDLOCK record type.
Signed-off-by: Mickaël Salaün <[email protected]>
---
include/uapi/linux/audit.h | 1 +
security/landlock/Makefile | 2 +
security/landlock/audit.c | 119 +++++++++++++++++++++++++++++++++++
security/landlock/audit.h | 35 +++++++++++
security/landlock/ruleset.c | 6 ++
security/landlock/ruleset.h | 10 +++
security/landlock/syscalls.c | 8 +++
7 files changed, 181 insertions(+)
create mode 100644 security/landlock/audit.c
create mode 100644 security/landlock/audit.h
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index d676ed2b246e..385e134277b1 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -122,6 +122,7 @@
#define AUDIT_OPENAT2 1337 /* Record showing openat2 how args */
#define AUDIT_DM_CTRL 1338 /* Device Mapper target control */
#define AUDIT_DM_EVENT 1339 /* Device Mapper events */
+#define AUDIT_LANDLOCK 1340 /* Landlock event */
#define AUDIT_AVC 1400 /* SE Linux avc denial or grant */
#define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */
diff --git a/security/landlock/Makefile b/security/landlock/Makefile
index 7bbd2f413b3e..c3e048df7fec 100644
--- a/security/landlock/Makefile
+++ b/security/landlock/Makefile
@@ -2,3 +2,5 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
landlock-y := setup.o syscalls.o object.o ruleset.o \
cred.o ptrace.o fs.o
+
+landlock-$(CONFIG_AUDIT) += audit.o
diff --git a/security/landlock/audit.c b/security/landlock/audit.c
new file mode 100644
index 000000000000..f58bd529784a
--- /dev/null
+++ b/security/landlock/audit.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Landlock LSM - Audit helpers
+ *
+ * Copyright © 2023 Microsoft Corporation
+ */
+
+#include <linux/atomic.h>
+#include <linux/audit.h>
+#include <linux/lsm_audit.h>
+
+#include "audit.h"
+#include "cred.h"
+
+atomic64_t ruleset_and_domain_counter = ATOMIC64_INIT(0);
+
+#define BIT_INDEX(bit) HWEIGHT(bit - 1)
+
+static void log_accesses(struct audit_buffer *const ab,
+ const access_mask_t accesses)
+{
+ const char *const desc[] = {
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = "execute",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)] = "write_file",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_READ_FILE)] = "read_file",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_READ_DIR)] = "read_dir",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_REMOVE_DIR)] = "remove_dir",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_REMOVE_FILE)] = "remove_file",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_CHAR)] = "make_char",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_DIR)] = "make_dir",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_REG)] = "make_reg",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_SOCK)] = "make_sock",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_FIFO)] = "make_fifo",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_BLOCK)] = "make_block",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_SYM)] = "make_sym",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_REFER)] = "refer",
+ [BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE)] = "truncate",
+ };
+ const unsigned long access_mask = accesses;
+ unsigned long access_bit;
+ bool is_first = true;
+
+ BUILD_BUG_ON(ARRAY_SIZE(desc) != LANDLOCK_NUM_ACCESS_FS);
+
+ for_each_set_bit(access_bit, &access_mask, ARRAY_SIZE(desc)) {
+ audit_log_format(ab, "%s%s", is_first ? "" : ",",
+ desc[access_bit]);
+ is_first = false;
+ }
+}
+
+/* Inspired by dump_common_audit_data(). */
+static void log_task(struct audit_buffer *const ab)
+{
+ /* 16 bytes (TASK_COMM_LEN) */
+ char comm[sizeof(current->comm)];
+
+ /*
+ * Uses task_pid_nr() instead of task_tgid_nr() because of how
+ * credentials and Landlock work.
+ */
+ audit_log_format(ab, "tid=%d comm=", task_pid_nr(current));
+ audit_log_untrustedstring(ab,
+ memcpy(comm, current->comm, sizeof(comm)));
+}
+
+void landlock_log_create_ruleset(struct landlock_ruleset *const ruleset)
+{
+ struct audit_buffer *ab;
+
+ WARN_ON_ONCE(ruleset->id);
+
+ ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_LANDLOCK);
+ if (!ab)
+ /* audit_log_lost() call */
+ return;
+
+ ruleset->id = atomic64_inc_return(&ruleset_and_domain_counter);
+ log_task(ab);
+ audit_log_format(ab,
+ " op=create-ruleset ruleset=%llu handled_access_fs=",
+ ruleset->id);
+ log_accesses(ab, ruleset->fs_access_masks[ruleset->num_layers - 1]);
+ audit_log_end(ab);
+}
+
+/*
+ * This is useful to know when a domain or a ruleset will never show again in
+ * the audit log.
+ */
+void landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset)
+{
+ struct audit_buffer *ab;
+ const char *name;
+ u64 id;
+
+ ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_LANDLOCK);
+ if (!ab)
+ /* audit_log_lost() call */
+ return;
+
+ /* It should either be a domain or a ruleset. */
+ if (ruleset->hierarchy) {
+ name = "domain";
+ id = ruleset->hierarchy->id;
+ WARN_ON_ONCE(ruleset->id);
+ } else {
+ name = "ruleset";
+ id = ruleset->id;
+ }
+ WARN_ON_ONCE(!id);
+
+ /*
+ * Because this might be called by kernel threads, logging
+ * related task information with log_task() would be useless.
+ */
+ audit_log_format(ab, "op=release-%s %s=%llu", name, name, id);
+ audit_log_end(ab);
+}
diff --git a/security/landlock/audit.h b/security/landlock/audit.h
new file mode 100644
index 000000000000..2666e9151627
--- /dev/null
+++ b/security/landlock/audit.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Landlock LSM - Audit helpers
+ *
+ * Copyright © 2023 Microsoft Corporation
+ */
+
+#ifndef _SECURITY_LANDLOCK_AUDIT_H
+#define _SECURITY_LANDLOCK_AUDIT_H
+
+#include <linux/audit.h>
+#include <linux/lsm_audit.h>
+
+#include "ruleset.h"
+
+#ifdef CONFIG_AUDIT
+
+void landlock_log_create_ruleset(struct landlock_ruleset *const ruleset);
+void landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset);
+
+#else /* CONFIG_AUDIT */
+
+static inline void
+landlock_log_create_ruleset(struct landlock_ruleset *const ruleset)
+{
+}
+
+static inline void
+landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset)
+{
+}
+
+#endif /* CONFIG_AUDIT */
+
+#endif /* _SECURITY_LANDLOCK_AUDIT_H */
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 996484f98bfd..585ee0f77e67 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -20,6 +20,7 @@
#include <linux/spinlock.h>
#include <linux/workqueue.h>
+#include "audit.h"
#include "limits.h"
#include "object.h"
#include "ruleset.h"
@@ -379,6 +380,11 @@ static void free_ruleset_work(struct work_struct *const work)
struct landlock_ruleset *ruleset;
ruleset = container_of(work, struct landlock_ruleset, work_free);
+
+ /* Only called by hook_cred_free(), hence for a domain. */
+ WARN_ON_ONCE(!ruleset->hierarchy);
+ landlock_log_release_ruleset(ruleset);
+
free_ruleset(ruleset);
}
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index 55b1df8f66a8..c74f1ab60c33 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -74,6 +74,11 @@ struct landlock_rule {
* struct landlock_hierarchy - Node in a ruleset hierarchy
*/
struct landlock_hierarchy {
+#ifdef CONFIG_AUDIT
+ /* domain's ID */
+ u64 id;
+#endif /* CONFIG_AUDIT */
+
/**
* @parent: Pointer to the parent node, or NULL if it is a root
* Landlock domain.
@@ -93,6 +98,11 @@ struct landlock_hierarchy {
* match an object.
*/
struct landlock_ruleset {
+#ifdef CONFIG_AUDIT
+ /* ruleset's ID, must be 0 for a domain */
+ u64 id;
+#endif /* CONFIG_AUDIT */
+
/**
* @root: Root of a red-black tree containing &struct landlock_rule
* nodes. Once a ruleset is tied to a process (i.e. as a domain), this
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 245cc650a4dc..373997a356e7 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -26,6 +26,7 @@
#include <linux/uaccess.h>
#include <uapi/linux/landlock.h>
+#include "audit.h"
#include "cred.h"
#include "fs.h"
#include "limits.h"
@@ -98,6 +99,10 @@ static int fop_ruleset_release(struct inode *const inode,
{
struct landlock_ruleset *ruleset = filp->private_data;
+ /* Only called by ruleset_fops, hence for a ruleset. */
+ WARN_ON_ONCE(ruleset->hierarchy);
+ landlock_log_release_ruleset(ruleset);
+
landlock_put_ruleset(ruleset);
return 0;
}
@@ -198,6 +203,9 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
ruleset, O_RDWR | O_CLOEXEC);
if (ruleset_fd < 0)
landlock_put_ruleset(ruleset);
+ else
+ landlock_log_create_ruleset(ruleset);
+
return ruleset_fd;
}
--
2.42.0
Extract code from common_dump_audit_data() into the audit_log_lsm_data()
helper. This helps reuse common LSM audit data while not abusing
AUDIT_AVC records because of the common_lsm_audit() helper.
Signed-off-by: Mickaël Salaün <[email protected]>
---
include/linux/lsm_audit.h | 2 ++
security/lsm_audit.c | 26 +++++++++++++++++---------
2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
index 97a8b21eb033..5f9a7ed0e7a5 100644
--- a/include/linux/lsm_audit.h
+++ b/include/linux/lsm_audit.h
@@ -122,6 +122,8 @@ int ipv4_skb_to_auditdata(struct sk_buff *skb,
int ipv6_skb_to_auditdata(struct sk_buff *skb,
struct common_audit_data *ad, u8 *proto);
+void audit_log_lsm_data(struct audit_buffer *ab, struct common_audit_data *a);
+
void common_lsm_audit(struct common_audit_data *a,
void (*pre_audit)(struct audit_buffer *, void *),
void (*post_audit)(struct audit_buffer *, void *));
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 849e832719e2..58f9b8bde22a 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -189,16 +189,12 @@ static inline void print_ipv4_addr(struct audit_buffer *ab, __be32 addr,
}
/**
- * dump_common_audit_data - helper to dump common audit data
+ * audit_log_lsm_data - helper to log common LSM audit data
* @ab : the audit buffer
* @a : common audit data
- *
*/
-static void dump_common_audit_data(struct audit_buffer *ab,
- struct common_audit_data *a)
+void audit_log_lsm_data(struct audit_buffer *ab, struct common_audit_data *a)
{
- char comm[sizeof(current->comm)];
-
/*
* To keep stack sizes in check force programmers to notice if they
* start making this union too large! See struct lsm_network_audit
@@ -206,9 +202,6 @@ static void dump_common_audit_data(struct audit_buffer *ab,
*/
BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
- audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
- audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
-
switch (a->type) {
case LSM_AUDIT_DATA_NONE:
return;
@@ -428,6 +421,21 @@ static void dump_common_audit_data(struct audit_buffer *ab,
} /* switch (a->type) */
}
+/**
+ * dump_common_audit_data - helper to dump common audit data
+ * @ab : the audit buffer
+ * @a : common audit data
+ */
+static void dump_common_audit_data(struct audit_buffer *ab,
+ struct common_audit_data *a)
+{
+ char comm[sizeof(current->comm)];
+
+ audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
+ audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
+ audit_log_lsm_data(ab, a);
+}
+
/**
* common_lsm_audit - generic LSM auditing function
* @a: auxiliary audit data
--
2.42.0
On Wed, Sep 20, 2023 at 11:17 PM Mickaël Salaün <[email protected]> wrote:
>
> Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate,
> and open requests.
>
> Signed-off-by: Mickaël Salaün <[email protected]>
> ---
> 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
>
On Mon, Sep 25, 2023 at 06:26:28PM -0700, Jeff Xu wrote:
> On Wed, Sep 20, 2023 at 11:17 PM Mickaël Salaün <[email protected]> wrote:
> >
> > Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate,
> > and open requests.
> >
> > Signed-off-by: Mickaël Salaün <[email protected]>
> > ---
> > security/landlock/audit.c | 114 ++++++++++++++++++++++++++++++++++++++
> > security/landlock/audit.h | 32 +++++++++++
> > security/landlock/fs.c | 62 ++++++++++++++++++---
> > 3 files changed, 199 insertions(+), 9 deletions(-)
> >
> > +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;
> > +}
> > @@ -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.
I did that in an early version of this patch, but I finally choose to write
'return lanlock_log_request();` for mainly two reasons:
* to help not forget to call this function at any non-zero return values
(which can easily be checked with grep),
* to do tail calls.
I guess compiler should be smart enough to do tail calls with a variable
set indirection, but I'd like to check that.
To make it easier to read (and to not forget returning the error), the
landlock_log_request() calls a void log_request() helper, and returns
the error itself. It is then easy to review and know what's happening
without reading log_request().
I'd like the compiler to check itself that every LSM hook returned
values are either 0 or comming from landlock_log_request() but I think
it's not possible right now. Coccinelle might help here though.
BTW, in a next version, we might have landlock_log_request() called even
for allowed requests (i.e. returned value of 0).
On Tue, Sep 26, 2023 at 6:35 AM Mickaël Salaün <[email protected]> wrote:
>
> On Mon, Sep 25, 2023 at 06:26:28PM -0700, Jeff Xu wrote:
> > On Wed, Sep 20, 2023 at 11:17 PM Mickaël Salaün <[email protected]> wrote:
> > >
> > > Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate,
> > > and open requests.
> > >
> > > Signed-off-by: Mickaël Salaün <[email protected]>
> > > ---
> > > security/landlock/audit.c | 114 ++++++++++++++++++++++++++++++++++++++
> > > security/landlock/audit.h | 32 +++++++++++
> > > security/landlock/fs.c | 62 ++++++++++++++++++---
> > > 3 files changed, 199 insertions(+), 9 deletions(-)
> > >
>
> > > +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;
> > > +}
>
> > > @@ -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.
>
> I did that in an early version of this patch, but I finally choose to write
> 'return lanlock_log_request();` for mainly two reasons:
> * to help not forget to call this function at any non-zero return values
> (which can easily be checked with grep),
"grep -A 2 landlock_log_request" would serve the same purpose though.
> * to do tail calls.
>
> I guess compiler should be smart enough to do tail calls with a variable
> set indirection, but I'd like to check that.
>
What are tail calls and what is the benefit of this code pattern ?
i.e. pass the return value into landlock_log_request() and make it a
single point of setting return value for all landlock hooks.
> To make it easier to read (and to not forget returning the error), the
> landlock_log_request() calls a void log_request() helper, and returns
> the error itself. It is then easy to review and know what's happening
> without reading log_request().
>
> I'd like the compiler to check itself that every LSM hook returned
> values are either 0 or comming from landlock_log_request() but I think
> it's not possible right now. Coccinelle might help here though.
>
> BTW, in a next version, we might have landlock_log_request() called even
> for allowed requests (i.e. returned value of 0).
Hi Mickaël!
On Thu, Sep 21, 2023 at 08:16:34AM +0200, Mickaël Salaün wrote:
> This patch series adds basic audit support to Landlock for most actions.
> Logging denied requests is useful for different use cases:
> * app developers: to ease and speed up sandboxing support
> * power users: to understand denials
> * sysadmins: to look for users' issues
> * tailored distro maintainers: to get usage metrics from their fleet
> * security experts: to detect attack attempts
>
> To make logs useful, they need to contain the most relevant Landlock
> domain that denied an action, and the reason. This translates to the
> latest nested domain and the related missing access rights.
Is "domain" always the latest nested domain, or is that the domain which caused
the check to fail because it denied the requested access right? (If it is just
the counter of how many domains are stacked, this could maybe also be queried
through proc instead?)
> Two "Landlock permissions" are used to describe mandatory restrictions
> enforced on all domains:
> * fs_layout: change the view of filesystem with mount operations.
> * ptrace: tamper with a process.
I find the term "access" already a bit overloaded, and the term "permission"
also already appears in other contexts. Maybe we can avoid the additional
terminology by grouping these two together in the log format, and calling them
the "cause" or "reason" for the deny decision? In a sense, the access rights
and the other permissions can already be told apart by their names, so they
might also both appear under the same key without causing additional confusion?
> Here is an example of logs, result of the sandboxer activity:
> tid=267 comm="sandboxer" op=create-ruleset ruleset=1 handled_access_fs=execute,write_file,read_file,read_dir,remove_dir,remove_file,make_char,make_dir,make_reg,make_sock,make_fifo,make_block,make_sym,refer,truncate
> tid=267 comm="sandboxer" op=restrict-self domain=2 ruleset=1 parent=0
> op=release-ruleset ruleset=1
> tid=267 comm="bash" domain=2 op=open errno=13 missing-fs-accesses=write_file,read_file missing-permission= path="/dev/tty" dev="devtmpfs" ino=9
> tid=268 comm="ls" domain=2 op=open errno=13 missing-fs-accesses=read_dir missing-permission= path="/" dev="vda2" ino=256
> tid=269 comm="touch" domain=2 op=mknod errno=13 missing-fs-accesses=make_reg missing-permission= path="/" dev="vda2" ino=256
> tid=270 comm="umount" domain=2 op=umount errno=1 missing-fs-accesses= missing-permission=fs_layout name="/" dev="tmpfs" ino=1
> tid=271 comm="strace" domain=2 op=ptrace errno=1 missing-fs-accesses= missing-permission=ptrace opid=1 ocomm="systemd"
In more complicated cases like "refer" and "open", it is possible that more than
one access right is missing, and presumably they'll both be listed in
missing-fs-accesses=. In this case, it is not clear to me whether the domain=
number is referring to the first or the second of these missing rights.
(Assuming that the domain= is about the domain which caused the denial.)
> As highlighted in comments, support for audit is not complete yet with
> this series: some actions are not logged (e.g. file reparenting), and
> rule additions are not logged neither.
When ftruncate(2) gets denied, it is also not possible to tell which of the
nested domains is responsible, without additional changes to what we carry
around in the file's security blob. (Right now, we calculate the overall
truncation right in advance at open(2) time, and just store that bit with the
newly opened file.)
> I'm also not sure if we need to have seccomp-like features such as
> SECCOMP_FILTER_FLAG_LOG, SECCOMP_RET_LOG, and
> /proc/sys/kernel/seccomp/actions_logged
>
> I'd like to get some early feedback on this proposal.
If you want to have the full feature set as proposed above for other operations
as well, like file reparenting and truncation, it'll complicate the Landlock
logic and increase the amount of data that needs to be kept around just for
logging. I'm not convinced that this is worth it. After all, the simpler the
Landlock implementation is, the easier it'll be to reason about its logic and
its security guarantees.
A possible simplification would be to omit the domain number which is
responsible for a "deny" decision. I feel that for debugging, knowing the fact
that Landlock denied an operation might already be a big step forward, and the
exact domain responsible for it might not be that important?
—Günther
--
Sent using Mutt ???? Woof Woof
Hi Mickaël
On Wed, Sep 20, 2023 at 11:16 PM Mickaël Salaün <[email protected]> wrote:
>
> Hi,
>
> This patch series adds basic audit support to Landlock for most actions.
> Logging denied requests is useful for different use cases:
> * app developers: to ease and speed up sandboxing support
> * power users: to understand denials
> * sysadmins: to look for users' issues
> * tailored distro maintainers: to get usage metrics from their fleet
> * security experts: to detect attack attempts
>
This is a highly desired feature, I think this will save dev's time
when developing Landlock rule sets.
Thanks for adding this patch set!
-Jeff
> To make logs useful, they need to contain the most relevant Landlock
> domain that denied an action, and the reason. This translates to the
> latest nested domain and the related missing access rights.
>
> Two "Landlock permissions" are used to describe mandatory restrictions
> enforced on all domains:
> * fs_layout: change the view of filesystem with mount operations.
> * ptrace: tamper with a process.
>
> Here is an example of logs, result of the sandboxer activity:
> tid=267 comm="sandboxer" op=create-ruleset ruleset=1 handled_access_fs=execute,write_file,read_file,read_dir,remove_dir,remove_file,make_char,make_dir,make_reg,make_sock,make_fifo,make_block,make_sym,refer,truncate
> tid=267 comm="sandboxer" op=restrict-self domain=2 ruleset=1 parent=0
> op=release-ruleset ruleset=1
> tid=267 comm="bash" domain=2 op=open errno=13 missing-fs-accesses=write_file,read_file missing-permission= path="/dev/tty" dev="devtmpfs" ino=9
> tid=268 comm="ls" domain=2 op=open errno=13 missing-fs-accesses=read_dir missing-permission= path="/" dev="vda2" ino=256
> tid=269 comm="touch" domain=2 op=mknod errno=13 missing-fs-accesses=make_reg missing-permission= path="/" dev="vda2" ino=256
> tid=270 comm="umount" domain=2 op=umount errno=1 missing-fs-accesses= missing-permission=fs_layout name="/" dev="tmpfs" ino=1
> tid=271 comm="strace" domain=2 op=ptrace errno=1 missing-fs-accesses= missing-permission=ptrace opid=1 ocomm="systemd"
>
> As highlighted in comments, support for audit is not complete yet with
> this series: some actions are not logged (e.g. file reparenting), and
> rule additions are not logged neither.
>
> I'm also not sure if we need to have seccomp-like features such as
> SECCOMP_FILTER_FLAG_LOG, SECCOMP_RET_LOG, and
> /proc/sys/kernel/seccomp/actions_logged
>
> I'd like to get some early feedback on this proposal.
>
> This series is based on v6.6-rc2
>
> Regards,
>
> Mickaël Salaün (7):
> lsm: Add audit_log_lsm_data() helper
> landlock: Factor out check_access_path()
> landlock: Log ruleset creation and release
> landlock: Log domain creation and enforcement
> landlock: Log file-related requests
> landlock: Log mount-related requests
> landlock: Log ptrace requests
>
> include/linux/lsm_audit.h | 2 +
> include/uapi/linux/audit.h | 1 +
> security/landlock/Makefile | 2 +
> security/landlock/audit.c | 283 +++++++++++++++++++++++++++++++++++
> security/landlock/audit.h | 88 +++++++++++
> security/landlock/fs.c | 169 ++++++++++++++++-----
> security/landlock/ptrace.c | 47 +++++-
> security/landlock/ruleset.c | 6 +
> security/landlock/ruleset.h | 10 ++
> security/landlock/syscalls.c | 12 ++
> security/lsm_audit.c | 26 ++--
> 11 files changed, 595 insertions(+), 51 deletions(-)
> create mode 100644 security/landlock/audit.c
> create mode 100644 security/landlock/audit.h
>
>
> base-commit: ce9ecca0238b140b88f43859b211c9fdfd8e5b70
> --
> 2.42.0
>
On Tue, Sep 26, 2023 at 02:19:51PM -0700, Jeff Xu wrote:
> On Tue, Sep 26, 2023 at 6:35 AM Mickaël Salaün <[email protected]> wrote:
> >
> > On Mon, Sep 25, 2023 at 06:26:28PM -0700, Jeff Xu wrote:
> > > On Wed, Sep 20, 2023 at 11:17 PM Mickaël Salaün <[email protected]> wrote:
> > > >
> > > > Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate,
> > > > and open requests.
> > > >
> > > > Signed-off-by: Mickaël Salaün <[email protected]>
> > > > ---
> > > > security/landlock/audit.c | 114 ++++++++++++++++++++++++++++++++++++++
> > > > security/landlock/audit.h | 32 +++++++++++
> > > > security/landlock/fs.c | 62 ++++++++++++++++++---
> > > > 3 files changed, 199 insertions(+), 9 deletions(-)
> > > >
> >
> > > > +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;
> > > > +}
> >
> > > > @@ -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.
> >
> > I did that in an early version of this patch, but I finally choose to write
> > 'return lanlock_log_request();` for mainly two reasons:
> > * to help not forget to call this function at any non-zero return values
> > (which can easily be checked with grep),
>
> "grep -A 2 landlock_log_request" would serve the same purpose though.
Yes, there is always a way to find a pattern, and the best tool might be
Coccinelle, but I think it's harder to miss with such tail calls.
>
> > * to do tail calls.
> >
> > I guess compiler should be smart enough to do tail calls with a variable
> > set indirection, but I'd like to check that.
> >
>
> What are tail calls and what is the benefit of this code pattern ?
> i.e. pass the return value into landlock_log_request() and make it a
> single point of setting return value for all landlock hooks.
landlock_log_request() should only be called at the end of LSM hooks.
Tail calls is basically when you call a function at the end of the
caller. This enables replacing "call" with "jmp" and save stack space.
landlock_log_request() can fit with this pattern (if not using the
caller's stack, which is not currently the case). See this tail call
optimization example: https://godbolt.org/z/r88ofcW6x
I find it less error prone to not duplicate the error code (once for
landlock_log_request and another for the caller's returned value). I
also don't really see the pro of using a variable only to share this
value. In ptrace.c, an "err" variable is used to check if the error is 0
or not, but that is handled differently for most hooks.
Makeing landlock_log_request() return a value also encourages us (thanks
to compiler warnings) to use this value and keep the error handling
consistent (especially for future code).
Another feature that I'd like to add is to support a "permissive mode",
that would enable to quickly see the impact of a sandbox without
applying it for real. This might change some landlock_log_request()
calls, so we'll see what fits best.
>
> > To make it easier to read (and to not forget returning the error), the
> > landlock_log_request() calls a void log_request() helper, and returns
> > the error itself. It is then easy to review and know what's happening
> > without reading log_request().
> >
> > I'd like the compiler to check itself that every LSM hook returned
> > values are either 0 or comming from landlock_log_request() but I think
> > it's not possible right now. Coccinelle might help here though.
> >
> > BTW, in a next version, we might have landlock_log_request() called even
> > for allowed requests (i.e. returned value of 0).
On Thu, Sep 28, 2023 at 8:16 AM Mickaël Salaün <[email protected]> wrote:
>
> On Tue, Sep 26, 2023 at 02:19:51PM -0700, Jeff Xu wrote:
> > On Tue, Sep 26, 2023 at 6:35 AM Mickaël Salaün <[email protected]> wrote:
> > >
> > > On Mon, Sep 25, 2023 at 06:26:28PM -0700, Jeff Xu wrote:
> > > > On Wed, Sep 20, 2023 at 11:17 PM Mickaël Salaün <[email protected]> wrote:
> > > > >
> > > > > Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate,
> > > > > and open requests.
> > > > >
> > > > > Signed-off-by: Mickaël Salaün <[email protected]>
> > > > > ---
> > > > > security/landlock/audit.c | 114 ++++++++++++++++++++++++++++++++++++++
> > > > > security/landlock/audit.h | 32 +++++++++++
> > > > > security/landlock/fs.c | 62 ++++++++++++++++++---
> > > > > 3 files changed, 199 insertions(+), 9 deletions(-)
> > > > >
> > >
> > > > > +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;
> > > > > +}
> > >
> > > > > @@ -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.
> > >
> > > I did that in an early version of this patch, but I finally choose to write
> > > 'return lanlock_log_request();` for mainly two reasons:
> > > * to help not forget to call this function at any non-zero return values
> > > (which can easily be checked with grep),
> >
> > "grep -A 2 landlock_log_request" would serve the same purpose though.
>
> Yes, there is always a way to find a pattern, and the best tool might be
> Coccinelle, but I think it's harder to miss with such tail calls.
>
> >
> > > * to do tail calls.
> > >
> > > I guess compiler should be smart enough to do tail calls with a variable
> > > set indirection, but I'd like to check that.
> > >
> >
> > What are tail calls and what is the benefit of this code pattern ?
> > i.e. pass the return value into landlock_log_request() and make it a
> > single point of setting return value for all landlock hooks.
>
> landlock_log_request() should only be called at the end of LSM hooks.
> Tail calls is basically when you call a function at the end of the
> caller. This enables replacing "call" with "jmp" and save stack space.
> landlock_log_request() can fit with this pattern (if not using the
> caller's stack, which is not currently the case). See this tail call
> optimization example: https://godbolt.org/z/r88ofcW6x
>
Thanks for giving the context of the tail call.
Compile options are controlled by makefile, and can be customized. In
the past, I have had different projects setting different O levels for
various reasons, including disable optimization completely. Individual
Compiler implementation also matters, gcc vs clang, etc. I think the
tail call is probably not essential to the discussion.
> I find it less error prone to not duplicate the error code (once for
> landlock_log_request and another for the caller's returned value). I
> also don't really see the pro of using a variable only to share this
> value. In ptrace.c, an "err" variable is used to check if the error is 0
> or not, but that is handled differently for most hooks.
>
> Makeing landlock_log_request() return a value also encourages us (thanks
> to compiler warnings) to use this value and keep the error handling
> consistent (especially for future code).
>
One general assumption about logging function is that it is not part
of the main business logic, i.e. if the logging statement is
removed/commented out, it doesn't have side effects to the main
business logic. This is probably why most log functions return void.
> Another feature that I'd like to add is to support a "permissive mode",
> that would enable to quickly see the impact of a sandbox without
> applying it for real. This might change some landlock_log_request()
> calls, so we'll see what fits best.
>
It is an excellent feature to have.
To implement that, I guess there will be a common function as a switch
to allow/deny, and logging the decision, depending on the permissive
setting.
From that point, preparing the code towards that goal makes sense.
> >
> > > To make it easier to read (and to not forget returning the error), the
> > > landlock_log_request() calls a void log_request() helper, and returns
> > > the error itself. It is then easy to review and know what's happening
> > > without reading log_request().
> > >
> > > I'd like the compiler to check itself that every LSM hook returned
> > > values are either 0 or comming from landlock_log_request() but I think
> > > it's not possible right now. Coccinelle might help here though.
> > >
> > > BTW, in a next version, we might have landlock_log_request() called even
> > > for allowed requests (i.e. returned value of 0).
When there is more business logic to landlock_log_request, it is
probably better to rename the function. Most devs might assume the log
function does nothing but logging. Having some meaningful name, e.g.
check_permissive_and_audit_log, will help with readability.
Thanks!
-Jeff
I talked about this patch series at the Kernel Recipes conference, and
you might want to take a look at the future work:
https://landlock.io/talks/2023-09-25_landlock-audit-kr.pdf
In a nutshell, new syscall flags:
* For landlock_create_ruleset() to opt-in for logging ruleset-related
and domain-related use
* For landlock_add_rule() to opt-in for logging this rule if it granted
the requested access
* For landlock_restrict_self() to opt-in for:
* not log anything
* handle a permissive mode to log actions that would have been denied
(very useful to build a sandbox)
On Thu, Sep 21, 2023 at 08:16:34AM +0200, Mickaël Salaün wrote:
> Hi,
>
> This patch series adds basic audit support to Landlock for most actions.
> Logging denied requests is useful for different use cases:
> * app developers: to ease and speed up sandboxing support
> * power users: to understand denials
> * sysadmins: to look for users' issues
> * tailored distro maintainers: to get usage metrics from their fleet
> * security experts: to detect attack attempts
>
> To make logs useful, they need to contain the most relevant Landlock
> domain that denied an action, and the reason. This translates to the
> latest nested domain and the related missing access rights.
>
> Two "Landlock permissions" are used to describe mandatory restrictions
> enforced on all domains:
> * fs_layout: change the view of filesystem with mount operations.
> * ptrace: tamper with a process.
>
> Here is an example of logs, result of the sandboxer activity:
> tid=267 comm="sandboxer" op=create-ruleset ruleset=1 handled_access_fs=execute,write_file,read_file,read_dir,remove_dir,remove_file,make_char,make_dir,make_reg,make_sock,make_fifo,make_block,make_sym,refer,truncate
> tid=267 comm="sandboxer" op=restrict-self domain=2 ruleset=1 parent=0
> op=release-ruleset ruleset=1
> tid=267 comm="bash" domain=2 op=open errno=13 missing-fs-accesses=write_file,read_file missing-permission= path="/dev/tty" dev="devtmpfs" ino=9
> tid=268 comm="ls" domain=2 op=open errno=13 missing-fs-accesses=read_dir missing-permission= path="/" dev="vda2" ino=256
> tid=269 comm="touch" domain=2 op=mknod errno=13 missing-fs-accesses=make_reg missing-permission= path="/" dev="vda2" ino=256
> tid=270 comm="umount" domain=2 op=umount errno=1 missing-fs-accesses= missing-permission=fs_layout name="/" dev="tmpfs" ino=1
> tid=271 comm="strace" domain=2 op=ptrace errno=1 missing-fs-accesses= missing-permission=ptrace opid=1 ocomm="systemd"
>
> As highlighted in comments, support for audit is not complete yet with
> this series: some actions are not logged (e.g. file reparenting), and
> rule additions are not logged neither.
>
> I'm also not sure if we need to have seccomp-like features such as
> SECCOMP_FILTER_FLAG_LOG, SECCOMP_RET_LOG, and
> /proc/sys/kernel/seccomp/actions_logged
>
> I'd like to get some early feedback on this proposal.
>
> This series is based on v6.6-rc2
>
> Regards,
>
> Mickaël Salaün (7):
> lsm: Add audit_log_lsm_data() helper
> landlock: Factor out check_access_path()
> landlock: Log ruleset creation and release
> landlock: Log domain creation and enforcement
> landlock: Log file-related requests
> landlock: Log mount-related requests
> landlock: Log ptrace requests
>
> include/linux/lsm_audit.h | 2 +
> include/uapi/linux/audit.h | 1 +
> security/landlock/Makefile | 2 +
> security/landlock/audit.c | 283 +++++++++++++++++++++++++++++++++++
> security/landlock/audit.h | 88 +++++++++++
> security/landlock/fs.c | 169 ++++++++++++++++-----
> security/landlock/ptrace.c | 47 +++++-
> security/landlock/ruleset.c | 6 +
> security/landlock/ruleset.h | 10 ++
> security/landlock/syscalls.c | 12 ++
> security/lsm_audit.c | 26 ++--
> 11 files changed, 595 insertions(+), 51 deletions(-)
> create mode 100644 security/landlock/audit.c
> create mode 100644 security/landlock/audit.h
>
>
> base-commit: ce9ecca0238b140b88f43859b211c9fdfd8e5b70
> --
> 2.42.0
>
On Thu, Sep 28, 2023 at 01:04:01PM -0700, Jeff Xu wrote:
> On Thu, Sep 28, 2023 at 8:16 AM Mickaël Salaün <[email protected]> wrote:
> >
> > On Tue, Sep 26, 2023 at 02:19:51PM -0700, Jeff Xu wrote:
> > > On Tue, Sep 26, 2023 at 6:35 AM Mickaël Salaün <[email protected]> wrote:
> > > >
> > > > On Mon, Sep 25, 2023 at 06:26:28PM -0700, Jeff Xu wrote:
> > > > > On Wed, Sep 20, 2023 at 11:17 PM Mickaël Salaün <[email protected]> wrote:
> > > > > >
> > > > > > Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate,
> > > > > > and open requests.
> > > > > >
> > > > > > Signed-off-by: Mickaël Salaün <[email protected]>
> > > > > > ---
> > > > > > security/landlock/audit.c | 114 ++++++++++++++++++++++++++++++++++++++
> > > > > > security/landlock/audit.h | 32 +++++++++++
> > > > > > security/landlock/fs.c | 62 ++++++++++++++++++---
> > > > > > 3 files changed, 199 insertions(+), 9 deletions(-)
> > > > > >
> > > >
> > > > > > +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;
> > > > > > +}
> > > >
> > > > > > @@ -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.
> > > >
> > > > I did that in an early version of this patch, but I finally choose to write
> > > > 'return lanlock_log_request();` for mainly two reasons:
> > > > * to help not forget to call this function at any non-zero return values
> > > > (which can easily be checked with grep),
> > >
> > > "grep -A 2 landlock_log_request" would serve the same purpose though.
> >
> > Yes, there is always a way to find a pattern, and the best tool might be
> > Coccinelle, but I think it's harder to miss with such tail calls.
> >
> > >
> > > > * to do tail calls.
> > > >
> > > > I guess compiler should be smart enough to do tail calls with a variable
> > > > set indirection, but I'd like to check that.
> > > >
> > >
> > > What are tail calls and what is the benefit of this code pattern ?
> > > i.e. pass the return value into landlock_log_request() and make it a
> > > single point of setting return value for all landlock hooks.
> >
> > landlock_log_request() should only be called at the end of LSM hooks.
> > Tail calls is basically when you call a function at the end of the
> > caller. This enables replacing "call" with "jmp" and save stack space.
> > landlock_log_request() can fit with this pattern (if not using the
> > caller's stack, which is not currently the case). See this tail call
> > optimization example: https://godbolt.org/z/r88ofcW6x
> >
> Thanks for giving the context of the tail call.
> Compile options are controlled by makefile, and can be customized. In
> the past, I have had different projects setting different O levels for
> various reasons, including disable optimization completely. Individual
> Compiler implementation also matters, gcc vs clang, etc. I think the
> tail call is probably not essential to the discussion.
>
> > I find it less error prone to not duplicate the error code (once for
> > landlock_log_request and another for the caller's returned value). I
> > also don't really see the pro of using a variable only to share this
> > value. In ptrace.c, an "err" variable is used to check if the error is 0
> > or not, but that is handled differently for most hooks.
> >
> > Makeing landlock_log_request() return a value also encourages us (thanks
> > to compiler warnings) to use this value and keep the error handling
> > consistent (especially for future code).
> >
> One general assumption about logging function is that it is not part
> of the main business logic, i.e. if the logging statement is
> removed/commented out, it doesn't have side effects to the main
> business logic. This is probably why most log functions return void.
I get it. We need to be careful not to add blind spots though. If audit
is not compiled, the inline function call will just be removed.
Otherwise, logging or not depends on the audit framework and the runtime
configuration.
Another thing to keep in mind is that for now, if the log failed somehow
(e.g. because of not enough memory), it will not impact the decision
(either accept or deny). However, I guess we may want to be able to
control this behavior is some cases one day, and in this case the log
function needs to return an error.
>
> > Another feature that I'd like to add is to support a "permissive mode",
> > that would enable to quickly see the impact of a sandbox without
> > applying it for real. This might change some landlock_log_request()
> > calls, so we'll see what fits best.
> >
> It is an excellent feature to have.
> To implement that, I guess there will be a common function as a switch
> to allow/deny, and logging the decision, depending on the permissive
> setting.
Permissive mode will be per domain/sandbox, so it will add complexity to
the current logging mechanism, but that is worth it.
> From that point, preparing the code towards that goal makes sense.
>
> > >
> > > > To make it easier to read (and to not forget returning the error), the
> > > > landlock_log_request() calls a void log_request() helper, and returns
> > > > the error itself. It is then easy to review and know what's happening
> > > > without reading log_request().
> > > >
> > > > I'd like the compiler to check itself that every LSM hook returned
> > > > values are either 0 or comming from landlock_log_request() but I think
> > > > it's not possible right now. Coccinelle might help here though.
> > > >
> > > > BTW, in a next version, we might have landlock_log_request() called even
> > > > for allowed requests (i.e. returned value of 0).
> When there is more business logic to landlock_log_request, it is
> probably better to rename the function. Most devs might assume the log
> function does nothing but logging. Having some meaningful name, e.g.
> check_permissive_and_audit_log, will help with readability.
As for the permissive mode, this would be per domain.
I'd like to keep the audit.h functions with the same prefix.
What about landlock_log_result()?
>
> Thanks!
> -Jeff
On Tue, Sep 26, 2023 at 06:24:32PM +0200, Günther Noack wrote:
> Hi Mickaël!
>
> On Thu, Sep 21, 2023 at 08:16:34AM +0200, Mickaël Salaün wrote:
> > This patch series adds basic audit support to Landlock for most actions.
> > Logging denied requests is useful for different use cases:
> > * app developers: to ease and speed up sandboxing support
> > * power users: to understand denials
> > * sysadmins: to look for users' issues
> > * tailored distro maintainers: to get usage metrics from their fleet
> > * security experts: to detect attack attempts
> >
> > To make logs useful, they need to contain the most relevant Landlock
> > domain that denied an action, and the reason. This translates to the
> > latest nested domain and the related missing access rights.
>
> Is "domain" always the latest nested domain, or is that the domain which caused
> the check to fail because it denied the requested access right? (If it is just
> the counter of how many domains are stacked, this could maybe also be queried
> through proc instead?)
The logged domain is the latest nested domain that denied at least one
access request (others might be denied by older domains).
What do you mean to query it through proc?
>
>
> > Two "Landlock permissions" are used to describe mandatory restrictions
> > enforced on all domains:
> > * fs_layout: change the view of filesystem with mount operations.
> > * ptrace: tamper with a process.
>
> I find the term "access" already a bit overloaded, and the term "permission"
> also already appears in other contexts. Maybe we can avoid the additional
> terminology by grouping these two together in the log format, and calling them
> the "cause" or "reason" for the deny decision? In a sense, the access rights
> and the other permissions can already be told apart by their names, so they
> might also both appear under the same key without causing additional confusion?
I choose to have two fields (missing-fs-accesses and missing-permission)
because one is specific to the FS access rights and the other is
generic. The reason of a deny is specifically the "missing FS accesses
or the missing permissions". I though about a generic "missing-accesses"
but in this case we'll need to prefix all rights with "fs_" or
"generic_", which seems too verbose. I think that tying to the access
right types would be less confusing when parsing these logs.
I'm not a fan of the "permission" name neither, but I didn't find a
better name. This comes from EACCES vs. EPERM.
BTW, I should use fs_topology instead of fs_layout.
>
>
> > Here is an example of logs, result of the sandboxer activity:
> > tid=267 comm="sandboxer" op=create-ruleset ruleset=1 handled_access_fs=execute,write_file,read_file,read_dir,remove_dir,remove_file,make_char,make_dir,make_reg,make_sock,make_fifo,make_block,make_sym,refer,truncate
> > tid=267 comm="sandboxer" op=restrict-self domain=2 ruleset=1 parent=0
> > op=release-ruleset ruleset=1
> > tid=267 comm="bash" domain=2 op=open errno=13 missing-fs-accesses=write_file,read_file missing-permission= path="/dev/tty" dev="devtmpfs" ino=9
> > tid=268 comm="ls" domain=2 op=open errno=13 missing-fs-accesses=read_dir missing-permission= path="/" dev="vda2" ino=256
> > tid=269 comm="touch" domain=2 op=mknod errno=13 missing-fs-accesses=make_reg missing-permission= path="/" dev="vda2" ino=256
> > tid=270 comm="umount" domain=2 op=umount errno=1 missing-fs-accesses= missing-permission=fs_layout name="/" dev="tmpfs" ino=1
> > tid=271 comm="strace" domain=2 op=ptrace errno=1 missing-fs-accesses= missing-permission=ptrace opid=1 ocomm="systemd"
>
> In more complicated cases like "refer" and "open", it is possible that more than
> one access right is missing, and presumably they'll both be listed in
> missing-fs-accesses=. In this case, it is not clear to me whether the domain=
> number is referring to the first or the second of these missing rights.
> (Assuming that the domain= is about the domain which caused the denial.)
In the case of "open", only the missing access rigths from the youngest
domain (that denied at least one request) are printed. This enables to
focus on this one, which should be the most common use case and the more
useful when debugging a sandbox. This also means that the logs are not
complete, only the more relevant informations are logged.
It is more complex in the case of "refer" because two paths/objects are
involved. I'm not sure how to log such request yet, but I think an
useful log entry should contain both the source and the destination
paths, which would be new compared to other LSMs. This would require to
extend furthermore audit_log_lsm_data(), probably by adding a prefix to
the "path=" string. I'd like to log only one entry per denial, and then
only one set of missing rights per domain. This should be OK by
extracting the youngest missing access rights from the destination
and/or the source (according to the same domain).
>
>
> > As highlighted in comments, support for audit is not complete yet with
> > this series: some actions are not logged (e.g. file reparenting), and
> > rule additions are not logged neither.
>
> When ftruncate(2) gets denied, it is also not possible to tell which of the
> nested domains is responsible, without additional changes to what we carry
> around in the file's security blob. (Right now, we calculate the overall
> truncation right in advance at open(2) time, and just store that bit with the
> newly opened file.)
Right, one solution would be to add a pointer to the domain that set the
restrictions, but I'd like to avoid that. Instead, we should be able to
identify the struct file at open time (another case for logging a
granted access), and then delegate the complexity of domain tracking and
file lifetime to the (user space) log parser.
>
>
> > I'm also not sure if we need to have seccomp-like features such as
> > SECCOMP_FILTER_FLAG_LOG, SECCOMP_RET_LOG, and
> > /proc/sys/kernel/seccomp/actions_logged
> >
> > I'd like to get some early feedback on this proposal.
>
> If you want to have the full feature set as proposed above for other operations
> as well, like file reparenting and truncation, it'll complicate the Landlock
> logic and increase the amount of data that needs to be kept around just for
> logging. I'm not convinced that this is worth it. After all, the simpler the
> Landlock implementation is, the easier it'll be to reason about its logic and
> its security guarantees.
I'd also like to keep the *enforcement implementation* as simple as
possible, and move most of the logging complexity to the audit.c file
and user space parsers. This patch series doesn't add complexity to the
enforcement logic, only to the audit logic.
The amount of data should only be a 64-bit ID per domain and ruleset,
and maybe the same for landlock_file_security (but I guess there are
other ways to identify a struct file).
Being able to debug Landlock policies is a critical feature for its
adoption.
>
> A possible simplification would be to omit the domain number which is
> responsible for a "deny" decision. I feel that for debugging, knowing the fact
> that Landlock denied an operation might already be a big step forward, and the
> exact domain responsible for it might not be that important?
Debugging a set of nested policies would be very challenging without the
ability to identify the exact domain causing denials. Storing an ID
doesn't look like a significant burden isn't it?
>
> —Günther
>
> --
> Sent using Mutt ???? Woof Woof
On Fri, Sep 29, 2023 at 9:04 AM Mickaël Salaün <[email protected]> wrote:
>
> On Thu, Sep 28, 2023 at 01:04:01PM -0700, Jeff Xu wrote:
> > On Thu, Sep 28, 2023 at 8:16 AM Mickaël Salaün <[email protected]> wrote:
> > >
> > > On Tue, Sep 26, 2023 at 02:19:51PM -0700, Jeff Xu wrote:
> > > > On Tue, Sep 26, 2023 at 6:35 AM Mickaël Salaün <[email protected]> wrote:
> > > > >
> > > > > On Mon, Sep 25, 2023 at 06:26:28PM -0700, Jeff Xu wrote:
> > > > > > On Wed, Sep 20, 2023 at 11:17 PM Mickaël Salaün <[email protected]> wrote:
> > > > > > >
> > > > > > > Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate,
> > > > > > > and open requests.
> > > > > > >
> > > > > > > Signed-off-by: Mickaël Salaün <[email protected]>
> > > > > > > ---
> > > > > > > security/landlock/audit.c | 114 ++++++++++++++++++++++++++++++++++++++
> > > > > > > security/landlock/audit.h | 32 +++++++++++
> > > > > > > security/landlock/fs.c | 62 ++++++++++++++++++---
> > > > > > > 3 files changed, 199 insertions(+), 9 deletions(-)
> > > > > > >
> > > > >
> > > > > > > +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;
> > > > > > > +}
> > > > >
> > > > > > > @@ -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.
> > > > >
> > > > > I did that in an early version of this patch, but I finally choose to write
> > > > > 'return lanlock_log_request();` for mainly two reasons:
> > > > > * to help not forget to call this function at any non-zero return values
> > > > > (which can easily be checked with grep),
> > > >
> > > > "grep -A 2 landlock_log_request" would serve the same purpose though.
> > >
> > > Yes, there is always a way to find a pattern, and the best tool might be
> > > Coccinelle, but I think it's harder to miss with such tail calls.
> > >
> > > >
> > > > > * to do tail calls.
> > > > >
> > > > > I guess compiler should be smart enough to do tail calls with a variable
> > > > > set indirection, but I'd like to check that.
> > > > >
> > > >
> > > > What are tail calls and what is the benefit of this code pattern ?
> > > > i.e. pass the return value into landlock_log_request() and make it a
> > > > single point of setting return value for all landlock hooks.
> > >
> > > landlock_log_request() should only be called at the end of LSM hooks.
> > > Tail calls is basically when you call a function at the end of the
> > > caller. This enables replacing "call" with "jmp" and save stack space.
> > > landlock_log_request() can fit with this pattern (if not using the
> > > caller's stack, which is not currently the case). See this tail call
> > > optimization example: https://godbolt.org/z/r88ofcW6x
> > >
> > Thanks for giving the context of the tail call.
> > Compile options are controlled by makefile, and can be customized. In
> > the past, I have had different projects setting different O levels for
> > various reasons, including disable optimization completely. Individual
> > Compiler implementation also matters, gcc vs clang, etc. I think the
> > tail call is probably not essential to the discussion.
> >
> > > I find it less error prone to not duplicate the error code (once for
> > > landlock_log_request and another for the caller's returned value). I
> > > also don't really see the pro of using a variable only to share this
> > > value. In ptrace.c, an "err" variable is used to check if the error is 0
> > > or not, but that is handled differently for most hooks.
> > >
> > > Makeing landlock_log_request() return a value also encourages us (thanks
> > > to compiler warnings) to use this value and keep the error handling
> > > consistent (especially for future code).
> > >
> > One general assumption about logging function is that it is not part
> > of the main business logic, i.e. if the logging statement is
> > removed/commented out, it doesn't have side effects to the main
> > business logic. This is probably why most log functions return void.
>
> I get it. We need to be careful not to add blind spots though. If audit
> is not compiled, the inline function call will just be removed.
> Otherwise, logging or not depends on the audit framework and the runtime
> configuration.
>
> Another thing to keep in mind is that for now, if the log failed somehow
> (e.g. because of not enough memory), it will not impact the decision
> (either accept or deny). However, I guess we may want to be able to
> control this behavior is some cases one day, and in this case the log
> function needs to return an error.
>
> >
> > > Another feature that I'd like to add is to support a "permissive mode",
> > > that would enable to quickly see the impact of a sandbox without
> > > applying it for real. This might change some landlock_log_request()
> > > calls, so we'll see what fits best.
> > >
> > It is an excellent feature to have.
> > To implement that, I guess there will be a common function as a switch
> > to allow/deny, and logging the decision, depending on the permissive
> > setting.
>
> Permissive mode will be per domain/sandbox, so it will add complexity to
> the current logging mechanism, but that is worth it.
>
> > From that point, preparing the code towards that goal makes sense.
> >
> > > >
> > > > > To make it easier to read (and to not forget returning the error), the
> > > > > landlock_log_request() calls a void log_request() helper, and returns
> > > > > the error itself. It is then easy to review and know what's happening
> > > > > without reading log_request().
> > > > >
> > > > > I'd like the compiler to check itself that every LSM hook returned
> > > > > values are either 0 or comming from landlock_log_request() but I think
> > > > > it's not possible right now. Coccinelle might help here though.
> > > > >
> > > > > BTW, in a next version, we might have landlock_log_request() called even
> > > > > for allowed requests (i.e. returned value of 0).
> > When there is more business logic to landlock_log_request, it is
> > probably better to rename the function. Most devs might assume the log
> > function does nothing but logging. Having some meaningful name, e.g.
> > check_permissive_and_audit_log, will help with readability.
>
> As for the permissive mode, this would be per domain.
>
That is really nice to have.
> I'd like to keep the audit.h functions with the same prefix.
> What about landlock_log_result()?
>
I'm fine with keeping landlock_log_request in this version since it is
only temporary, or landlock_log_result() is better.
In the future version, landlock_log_result() still doesn't quite say
it might override the result, so maybe extract the audit logging out
of that at the time.
> >
> > Thanks!
> > -Jeff
On Thu, Sep 21, 2023 at 2:17 AM Mickaël Salaün <[email protected]> wrote:
>
> Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate,
> and open requests.
>
> Signed-off-by: Mickaël Salaün <[email protected]>
> ---
> 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 you're going to be using a single AUDIT_LANDLOCK record type, do
you want to somehow encode that the above are access/permission
requests in the "op=" field name?
> +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);
> +}
See my previous comments about record format consistency.
--
paul-moore.com
On Thu, Sep 21, 2023 at 2:16 AM Mickaël Salaün <[email protected]> wrote:
>
> Extract code from common_dump_audit_data() into the audit_log_lsm_data()
Did you mean dump_common_audit_data()? Assuming you correct the
function name above this looks fine to me.
Acked-by: Paul Moore <[email protected]>
> helper. This helps reuse common LSM audit data while not abusing
> AUDIT_AVC records because of the common_lsm_audit() helper.
>
> Signed-off-by: Mickaël Salaün <[email protected]>
> ---
> include/linux/lsm_audit.h | 2 ++
> security/lsm_audit.c | 26 +++++++++++++++++---------
> 2 files changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
> index 97a8b21eb033..5f9a7ed0e7a5 100644
> --- a/include/linux/lsm_audit.h
> +++ b/include/linux/lsm_audit.h
> @@ -122,6 +122,8 @@ int ipv4_skb_to_auditdata(struct sk_buff *skb,
> int ipv6_skb_to_auditdata(struct sk_buff *skb,
> struct common_audit_data *ad, u8 *proto);
>
> +void audit_log_lsm_data(struct audit_buffer *ab, struct common_audit_data *a);
> +
> void common_lsm_audit(struct common_audit_data *a,
> void (*pre_audit)(struct audit_buffer *, void *),
> void (*post_audit)(struct audit_buffer *, void *));
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index 849e832719e2..58f9b8bde22a 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -189,16 +189,12 @@ static inline void print_ipv4_addr(struct audit_buffer *ab, __be32 addr,
> }
>
> /**
> - * dump_common_audit_data - helper to dump common audit data
> + * audit_log_lsm_data - helper to log common LSM audit data
> * @ab : the audit buffer
> * @a : common audit data
> - *
> */
> -static void dump_common_audit_data(struct audit_buffer *ab,
> - struct common_audit_data *a)
> +void audit_log_lsm_data(struct audit_buffer *ab, struct common_audit_data *a)
> {
> - char comm[sizeof(current->comm)];
> -
> /*
> * To keep stack sizes in check force programmers to notice if they
> * start making this union too large! See struct lsm_network_audit
> @@ -206,9 +202,6 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> */
> BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
>
> - audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
> - audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
> -
> switch (a->type) {
> case LSM_AUDIT_DATA_NONE:
> return;
> @@ -428,6 +421,21 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> } /* switch (a->type) */
> }
>
> +/**
> + * dump_common_audit_data - helper to dump common audit data
> + * @ab : the audit buffer
> + * @a : common audit data
> + */
> +static void dump_common_audit_data(struct audit_buffer *ab,
> + struct common_audit_data *a)
> +{
> + char comm[sizeof(current->comm)];
> +
> + audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
> + audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
> + audit_log_lsm_data(ab, a);
> +}
> +
> /**
> * common_lsm_audit - generic LSM auditing function
> * @a: auxiliary audit data
> --
> 2.42.0
--
paul-moore.com
On Thu, Sep 21, 2023 at 2:17 AM Mickaël Salaün <[email protected]> wrote:
>
> Add audit support for ruleset/domain creation and release. Ruleset and
> domain IDs are generated from the same 64-bit counter to avoid confusing
> them. There is no need to hide the sequentiality to users that are
> already allowed to read logs. In the future, if these IDs were to be
> viewable by unprivileged users, then we'll need to scramble them.
>
> Add a new AUDIT_LANDLOCK record type.
When adding new audit records we generally ask people to include
examples taken from their testing to the commit description.
> Signed-off-by: Mickaël Salaün <[email protected]>
> ---
> include/uapi/linux/audit.h | 1 +
> security/landlock/Makefile | 2 +
> security/landlock/audit.c | 119 +++++++++++++++++++++++++++++++++++
> security/landlock/audit.h | 35 +++++++++++
> security/landlock/ruleset.c | 6 ++
> security/landlock/ruleset.h | 10 +++
> security/landlock/syscalls.c | 8 +++
> 7 files changed, 181 insertions(+)
> create mode 100644 security/landlock/audit.c
> create mode 100644 security/landlock/audit.h
...
> diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> new file mode 100644
> index 000000000000..f58bd529784a
> --- /dev/null
> +++ b/security/landlock/audit.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Landlock LSM - Audit helpers
> + *
> + * Copyright © 2023 Microsoft Corporation
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/audit.h>
> +#include <linux/lsm_audit.h>
> +
> +#include "audit.h"
> +#include "cred.h"
> +
> +atomic64_t ruleset_and_domain_counter = ATOMIC64_INIT(0);
> +
> +#define BIT_INDEX(bit) HWEIGHT(bit - 1)
> +
> +static void log_accesses(struct audit_buffer *const ab,
> + const access_mask_t accesses)
> +{
> + const char *const desc[] = {
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = "execute",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)] = "write_file",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_READ_FILE)] = "read_file",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_READ_DIR)] = "read_dir",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_REMOVE_DIR)] = "remove_dir",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_REMOVE_FILE)] = "remove_file",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_CHAR)] = "make_char",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_DIR)] = "make_dir",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_REG)] = "make_reg",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_SOCK)] = "make_sock",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_FIFO)] = "make_fifo",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_BLOCK)] = "make_block",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_SYM)] = "make_sym",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_REFER)] = "refer",
> + [BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE)] = "truncate",
> + };
> + const unsigned long access_mask = accesses;
> + unsigned long access_bit;
> + bool is_first = true;
> +
> + BUILD_BUG_ON(ARRAY_SIZE(desc) != LANDLOCK_NUM_ACCESS_FS);
> +
> + for_each_set_bit(access_bit, &access_mask, ARRAY_SIZE(desc)) {
> + audit_log_format(ab, "%s%s", is_first ? "" : ",",
> + desc[access_bit]);
> + is_first = false;
> + }
> +}
> +
> +/* Inspired by dump_common_audit_data(). */
> +static void log_task(struct audit_buffer *const ab)
> +{
> + /* 16 bytes (TASK_COMM_LEN) */
> + char comm[sizeof(current->comm)];
> +
> + /*
> + * Uses task_pid_nr() instead of task_tgid_nr() because of how
> + * credentials and Landlock work.
> + */
> + audit_log_format(ab, "tid=%d comm=", task_pid_nr(current));
> + audit_log_untrustedstring(ab,
> + memcpy(comm, current->comm, sizeof(comm)));
> +}
Depending on how log_task() is used, it may be redundant with respect
to the existing SYSCALL record. Yes, there is already redundancy with
the AVC record, but that is a legacy problem and not something we can
easily fix, but given that the Landlock records are new we have an
opportunity to do things properly :)
> +void landlock_log_create_ruleset(struct landlock_ruleset *const ruleset)
> +{
> + struct audit_buffer *ab;
> +
> + WARN_ON_ONCE(ruleset->id);
> +
> + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_LANDLOCK);
> + if (!ab)
> + /* audit_log_lost() call */
> + return;
> +
> + ruleset->id = atomic64_inc_return(&ruleset_and_domain_counter);
> + log_task(ab);
> + audit_log_format(ab,
> + " op=create-ruleset ruleset=%llu handled_access_fs=",
> + ruleset->id);
"handled_access_fs" seems a bit long for a field name, is there any
reason why it couldn't simply be "access_fs" or something similar?
> + log_accesses(ab, ruleset->fs_access_masks[ruleset->num_layers - 1]);
> + audit_log_end(ab);
> +}
> +
> +/*
> + * This is useful to know when a domain or a ruleset will never show again in
> + * the audit log.
> + */
> +void landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset)
> +{
> + struct audit_buffer *ab;
> + const char *name;
> + u64 id;
> +
> + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_LANDLOCK);
> + if (!ab)
> + /* audit_log_lost() call */
> + return;
> +
> + /* It should either be a domain or a ruleset. */
> + if (ruleset->hierarchy) {
> + name = "domain";
Perhaps I missed it, but I didn't see an audit record with
"op=create-domain", is there one? If there is no audit record for
creating a Landlock domain, why do we care about releasing a Landlock
domain?
[NOTE: I see that domain creation is audited in patch 4, I would
suggest reworking the patchset so the ruleset auditing is in one
patch, domain auditing another ... or just squash the two patches.
Either approach would be preferable to this approach.]
> + id = ruleset->hierarchy->id;
> + WARN_ON_ONCE(ruleset->id);
> + } else {
> + name = "ruleset";
> + id = ruleset->id;
> + }
> + WARN_ON_ONCE(!id);
> +
> + /*
> + * Because this might be called by kernel threads, logging
> + * related task information with log_task() would be useless.
> + */
> + audit_log_format(ab, "op=release-%s %s=%llu", name, name, id);
This starts to get a little tricky. The general guidance is that for
a given audit record type, e.g. AUDIT_LANDLOCK, there should be no
change in presence or ordering of fields, yet in
landlock_log_create_ruleset() we log the permission information and
here in landlock_log_release_ruleset() we do not. The easy fix is to
record the permission information here as well, or simply use a
"handled_access_fs=?" placeholder. Something to keep in mind as you
move forward.
> + audit_log_end(ab);
> +}
--
paul-moore.com
On Wed, Dec 20, 2023 at 04:22:15PM -0500, Paul Moore wrote:
> On Thu, Sep 21, 2023 at 2:17 AM Mickaël Salaün <[email protected]> wrote:
> >
> > Add audit support for ruleset/domain creation and release. Ruleset and
> > domain IDs are generated from the same 64-bit counter to avoid confusing
> > them. There is no need to hide the sequentiality to users that are
> > already allowed to read logs. In the future, if these IDs were to be
> > viewable by unprivileged users, then we'll need to scramble them.
> >
> > Add a new AUDIT_LANDLOCK record type.
>
> When adding new audit records we generally ask people to include
> examples taken from their testing to the commit description.
OK, I'll add that in the next series.
>
> > Signed-off-by: Mickaël Salaün <[email protected]>
> > ---
> > include/uapi/linux/audit.h | 1 +
> > security/landlock/Makefile | 2 +
> > security/landlock/audit.c | 119 +++++++++++++++++++++++++++++++++++
> > security/landlock/audit.h | 35 +++++++++++
> > security/landlock/ruleset.c | 6 ++
> > security/landlock/ruleset.h | 10 +++
> > security/landlock/syscalls.c | 8 +++
> > 7 files changed, 181 insertions(+)
> > create mode 100644 security/landlock/audit.c
> > create mode 100644 security/landlock/audit.h
>
> ...
>
> > diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> > new file mode 100644
> > index 000000000000..f58bd529784a
> > --- /dev/null
> > +++ b/security/landlock/audit.c
> > @@ -0,0 +1,119 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Landlock LSM - Audit helpers
> > + *
> > + * Copyright © 2023 Microsoft Corporation
> > + */
> > +
> > +#include <linux/atomic.h>
> > +#include <linux/audit.h>
> > +#include <linux/lsm_audit.h>
> > +
> > +#include "audit.h"
> > +#include "cred.h"
> > +
> > +atomic64_t ruleset_and_domain_counter = ATOMIC64_INIT(0);
> > +
> > +#define BIT_INDEX(bit) HWEIGHT(bit - 1)
> > +
> > +static void log_accesses(struct audit_buffer *const ab,
> > + const access_mask_t accesses)
> > +{
> > + const char *const desc[] = {
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = "execute",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)] = "write_file",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_READ_FILE)] = "read_file",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_READ_DIR)] = "read_dir",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_REMOVE_DIR)] = "remove_dir",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_REMOVE_FILE)] = "remove_file",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_CHAR)] = "make_char",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_DIR)] = "make_dir",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_REG)] = "make_reg",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_SOCK)] = "make_sock",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_FIFO)] = "make_fifo",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_BLOCK)] = "make_block",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_SYM)] = "make_sym",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_REFER)] = "refer",
> > + [BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE)] = "truncate",
> > + };
> > + const unsigned long access_mask = accesses;
> > + unsigned long access_bit;
> > + bool is_first = true;
> > +
> > + BUILD_BUG_ON(ARRAY_SIZE(desc) != LANDLOCK_NUM_ACCESS_FS);
> > +
> > + for_each_set_bit(access_bit, &access_mask, ARRAY_SIZE(desc)) {
> > + audit_log_format(ab, "%s%s", is_first ? "" : ",",
> > + desc[access_bit]);
> > + is_first = false;
> > + }
> > +}
> > +
> > +/* Inspired by dump_common_audit_data(). */
> > +static void log_task(struct audit_buffer *const ab)
> > +{
> > + /* 16 bytes (TASK_COMM_LEN) */
> > + char comm[sizeof(current->comm)];
> > +
> > + /*
> > + * Uses task_pid_nr() instead of task_tgid_nr() because of how
> > + * credentials and Landlock work.
> > + */
> > + audit_log_format(ab, "tid=%d comm=", task_pid_nr(current));
> > + audit_log_untrustedstring(ab,
> > + memcpy(comm, current->comm, sizeof(comm)));
> > +}
>
> Depending on how log_task() is used, it may be redundant with respect
> to the existing SYSCALL record. Yes, there is already redundancy with
> the AVC record, but that is a legacy problem and not something we can
> easily fix, but given that the Landlock records are new we have an
> opportunity to do things properly :)
Indeed, it would make it simpler too. I wasn't sure how standalone a
record should be, but I guess there tools should be able to look for a
set of related records (e.g. event with a SYSCALL record matching a PID
and a LANDLOCK record).
>
> > +void landlock_log_create_ruleset(struct landlock_ruleset *const ruleset)
> > +{
> > + struct audit_buffer *ab;
> > +
> > + WARN_ON_ONCE(ruleset->id);
> > +
> > + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_LANDLOCK);
> > + if (!ab)
> > + /* audit_log_lost() call */
> > + return;
> > +
> > + ruleset->id = atomic64_inc_return(&ruleset_and_domain_counter);
> > + log_task(ab);
> > + audit_log_format(ab,
> > + " op=create-ruleset ruleset=%llu handled_access_fs=",
> > + ruleset->id);
>
> "handled_access_fs" seems a bit long for a field name, is there any
> reason why it couldn't simply be "access_fs" or something similar?
"handled_access_fs" is from the landlock_create_ruleset(2) API, so I'd
like to use the same name. However, because the types of handled access
rights for a ruleset will expand (e.g. we now have a
handled_access_net), I'm wondering if it would be better to keep this
(growing) one-line record or if we should use several records for a
ruleset creation (i.e. one line per type of handled access righs).
>
> > + log_accesses(ab, ruleset->fs_access_masks[ruleset->num_layers - 1]);
> > + audit_log_end(ab);
> > +}
> > +
> > +/*
> > + * This is useful to know when a domain or a ruleset will never show again in
> > + * the audit log.
> > + */
> > +void landlock_log_release_ruleset(const struct landlock_ruleset *const ruleset)
> > +{
> > + struct audit_buffer *ab;
> > + const char *name;
> > + u64 id;
> > +
> > + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_LANDLOCK);
> > + if (!ab)
> > + /* audit_log_lost() call */
> > + return;
> > +
> > + /* It should either be a domain or a ruleset. */
> > + if (ruleset->hierarchy) {
> > + name = "domain";
>
> Perhaps I missed it, but I didn't see an audit record with
> "op=create-domain", is there one? If there is no audit record for
> creating a Landlock domain, why do we care about releasing a Landlock
> domain?
>
> [NOTE: I see that domain creation is audited in patch 4, I would
> suggest reworking the patchset so the ruleset auditing is in one
> patch, domain auditing another ... or just squash the two patches.
> Either approach would be preferable to this approach.]
Domain creation is indeed recorded with the restrict_self operation from
the next patch. I'll rework and extend these patches to get something
more consistent.
>
> > + id = ruleset->hierarchy->id;
> > + WARN_ON_ONCE(ruleset->id);
> > + } else {
> > + name = "ruleset";
> > + id = ruleset->id;
> > + }
> > + WARN_ON_ONCE(!id);
> > +
> > + /*
> > + * Because this might be called by kernel threads, logging
> > + * related task information with log_task() would be useless.
> > + */
> > + audit_log_format(ab, "op=release-%s %s=%llu", name, name, id);
>
> This starts to get a little tricky. The general guidance is that for
> a given audit record type, e.g. AUDIT_LANDLOCK, there should be no
> change in presence or ordering of fields, yet in
> landlock_log_create_ruleset() we log the permission information and
> here in landlock_log_release_ruleset() we do not. The easy fix is to
> record the permission information here as well, or simply use a
> "handled_access_fs=?" placeholder. Something to keep in mind as you
> move forward.
OK, I used different "op" to specify the related fields, but I should
use a dedicated record type when it makes sense instead. My reasoning
was that it would be easier to filter on one or two record types, but
I like the fixed set of fields per record type.
I plan to add a few record types, something like that:
For a ruleset creation event, several grouped records:
- AUDIT_LANDLOCK_RULESET: "id=[new ruleset ID] op=create"
- AUDIT_LANDLOCK_ACCESS: "type=[fs or net] rights=[bitmask]"
For rule addition, several records per landlock_add_rule(2) call.
Example with a path_beneath rule:
- AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=add_rule"
- AUDIT_LANDLOCK_PATH: "scope=beneath path=[file path] dev= ino="
- AUDIT_LANDLOCK_ACCESS: "type=fs rights=[bitmask]"
Example with a net_port rule:
- AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=add_rule"
- AUDIT_LANDLOCK_PORT: "port="
- AUDIT_LANDLOCK_ACCESS: "type=net rights=[bitmask]"
For domain creation/restriction:
- AUDIT_LANDLOCK_DOMAIN: "id=[new domain ID] op=create"
- AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=use"
For ruleset release:
- AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=release"
For domain release:
- AUDIT_LANDLOCK_DOMAIN: "id=[domain ID] op=release"
For denied FS access:
- AUDIT_LANDLOCK_DENIAL: "id=[domain ID] op=mkdir"
- AUDIT_LANDLOCK_PATH: "scope=exact path=[file path] dev= ino="
For denied net access:
- AUDIT_LANDLOCK_DENIAL: "id=[domain ID] op=connect"
- AUDIT_LANDLOCK_PORT: "port="
It may not be worth it to differenciate between domain and ruleset for
audit though.
What do you think?
I guess it will still be OK to expand a record type with new appended
fields right? For instance, could we append a "flags" field to a
AUDIT_LANDLOCK_RULESET record (because it may not make sense to create a
dedicated record type for that)?
>
> > + audit_log_end(ab);
> > +}
>
> --
> paul-moore.com
On Wed, Dec 20, 2023 at 04:22:33PM -0500, Paul Moore wrote:
> On Thu, Sep 21, 2023 at 2:17 AM Mickaël Salaün <[email protected]> wrote:
> >
> > Add audit support for mkdir, mknod, symlink, unlink, rmdir, truncate,
> > and open requests.
> >
> > Signed-off-by: Mickaël Salaün <[email protected]>
> > ---
> > 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 you're going to be using a single AUDIT_LANDLOCK record type, do
> you want to somehow encode that the above are access/permission
> requests in the "op=" field name?
I'll use several audit record types, one for a denial and others for the
related kernel objects. See my other reply.
>
> > +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);
> > +}
>
> See my previous comments about record format consistency.
right
>
> --
> paul-moore.com
>
On Thu, Dec 21, 2023 at 1:45 PM Mickaël Salaün <[email protected]> wrote:
> On Wed, Dec 20, 2023 at 04:22:15PM -0500, Paul Moore wrote:
> > On Thu, Sep 21, 2023 at 2:17 AM Mickaël Salaün <[email protected]> wrote:
> > >
> > > Add audit support for ruleset/domain creation and release ...
...
> > > +/* Inspired by dump_common_audit_data(). */
> > > +static void log_task(struct audit_buffer *const ab)
> > > +{
> > > + /* 16 bytes (TASK_COMM_LEN) */
> > > + char comm[sizeof(current->comm)];
> > > +
> > > + /*
> > > + * Uses task_pid_nr() instead of task_tgid_nr() because of how
> > > + * credentials and Landlock work.
> > > + */
> > > + audit_log_format(ab, "tid=%d comm=", task_pid_nr(current));
> > > + audit_log_untrustedstring(ab,
> > > + memcpy(comm, current->comm, sizeof(comm)));
> > > +}
> >
> > Depending on how log_task() is used, it may be redundant with respect
> > to the existing SYSCALL record. Yes, there is already redundancy with
> > the AVC record, but that is a legacy problem and not something we can
> > easily fix, but given that the Landlock records are new we have an
> > opportunity to do things properly :)
>
> Indeed, it would make it simpler too. I wasn't sure how standalone a
> record should be, but I guess there tools should be able to look for a
> set of related records (e.g. event with a SYSCALL record matching a PID
> and a LANDLOCK record).
I believe ausearch will output the entire event when it matches on a
field in one of the event's records.
> > > +void landlock_log_create_ruleset(struct landlock_ruleset *const ruleset)
> > > +{
> > > + struct audit_buffer *ab;
> > > +
> > > + WARN_ON_ONCE(ruleset->id);
> > > +
> > > + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_LANDLOCK);
> > > + if (!ab)
> > > + /* audit_log_lost() call */
> > > + return;
> > > +
> > > + ruleset->id = atomic64_inc_return(&ruleset_and_domain_counter);
> > > + log_task(ab);
> > > + audit_log_format(ab,
> > > + " op=create-ruleset ruleset=%llu handled_access_fs=",
> > > + ruleset->id);
> >
> > "handled_access_fs" seems a bit long for a field name, is there any
> > reason why it couldn't simply be "access_fs" or something similar?
>
> "handled_access_fs" is from the landlock_create_ruleset(2) API, so I'd
> like to use the same name.
Okay, that's a reasonable reason.
> However, because the types of handled access
> rights for a ruleset will expand (e.g. we now have a
> handled_access_net), I'm wondering if it would be better to keep this
> (growing) one-line record or if we should use several records for a
> ruleset creation (i.e. one line per type of handled access righs).
I think it would be better to have a single record for rulesets rather
than multiple records all dealing with rulesets.
> > > + id = ruleset->hierarchy->id;
> > > + WARN_ON_ONCE(ruleset->id);
> > > + } else {
> > > + name = "ruleset";
> > > + id = ruleset->id;
> > > + }
> > > + WARN_ON_ONCE(!id);
> > > +
> > > + /*
> > > + * Because this might be called by kernel threads, logging
> > > + * related task information with log_task() would be useless.
> > > + */
> > > + audit_log_format(ab, "op=release-%s %s=%llu", name, name, id);
> >
> > This starts to get a little tricky. The general guidance is that for
> > a given audit record type, e.g. AUDIT_LANDLOCK, there should be no
> > change in presence or ordering of fields, yet in
> > landlock_log_create_ruleset() we log the permission information and
> > here in landlock_log_release_ruleset() we do not. The easy fix is to
> > record the permission information here as well, or simply use a
> > "handled_access_fs=?" placeholder. Something to keep in mind as you
> > move forward.
>
> OK, I used different "op" to specify the related fields, but I should
> use a dedicated record type when it makes sense instead. My reasoning
> was that it would be easier to filter on one or two record types, but
> I like the fixed set of fields per record type.
>
> I plan to add a few record types, something like that:
>
> For a ruleset creation event, several grouped records:
> - AUDIT_LANDLOCK_RULESET: "id=[new ruleset ID] op=create"
> - AUDIT_LANDLOCK_ACCESS: "type=[fs or net] rights=[bitmask]"
I'm guessing that LANDLOCK_RULESET would be for policy changes, and
LANDLOCK_ACCESS would be for individual access grants or denials? If
so, that looks reasonable.
> For rule addition, several records per landlock_add_rule(2) call.
> Example with a path_beneath rule:
> - AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=add_rule"
> - AUDIT_LANDLOCK_PATH: "scope=beneath path=[file path] dev= ino="
> - AUDIT_LANDLOCK_ACCESS: "type=fs rights=[bitmask]"
I worry that LANDLOCK_PATH is too much of a duplicate for the existing
PATH record. Assuming the "scope=" field is important, could it live
in the LANDLOCK_ACCESS record and then you could do away with the
dedicated LANDLOCK_PATH record? Oh, wait ... this is to record the
policy, not a individual access request, gotcha. If that is the case
and RULESET, PATH, ACCESS are all used simply to record the policy
information I might suggest creation of an AUDIT_LANDLOCK_POLICY
record that captures all of the above. If you think that is too
cumbersome, then perhaps you can do the object/access-specific record
type, e.g. AUDIT_LANDLOCK_POLICY_FS and AUDIT_LANDLOCK_POLICY_NET.
You also shouldn't reuse the "type=" field. Steve gets grumpy when
people reuse field names for different things. You can find a
reasonably complete list of fields here:
https://github.com/linux-audit/audit-documentation/blob/main/specs/fields/field-dictionary.csv
> For domain creation/restriction:
> - AUDIT_LANDLOCK_DOMAIN: "id=[new domain ID] op=create"
> - AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=use"
I imagine you could capture this in the policy record type?
> For ruleset release:
> - AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=release"
>
> For domain release:
> - AUDIT_LANDLOCK_DOMAIN: "id=[domain ID] op=release"
Same with the above two.
> For denied FS access:
> - AUDIT_LANDLOCK_DENIAL: "id=[domain ID] op=mkdir"
> - AUDIT_LANDLOCK_PATH: "scope=exact path=[file path] dev= ino="
I would use a single record type, i.e. AUDIT_LANDLOCK_ACCESS, to
capture both access granted and denied events. I'd also omit the
dedicated LANDLOCK_PATH record here in favor of the generic PATH
record (see my comments above).
> For denied net access:
> - AUDIT_LANDLOCK_DENIAL: "id=[domain ID] op=connect"
> - AUDIT_LANDLOCK_PORT: "port="
I would look at the SOCKADDR record type instead of introducing a new
LANDLOCK_PORT type.
> I guess it will still be OK to expand a record type with new appended
> fields right? For instance, could we append a "flags" field to a
> AUDIT_LANDLOCK_RULESET record (because it may not make sense to create a
> dedicated record type for that)?
Of course, one can always add fields to an existing record type with
the understanding that they MUST be added to the end.
--
paul-moore.com
On Fri, Dec 22, 2023 at 05:42:35PM -0500, Paul Moore wrote:
> On Thu, Dec 21, 2023 at 1:45 PM Mickaël Salaün <[email protected]> wrote:
> > On Wed, Dec 20, 2023 at 04:22:15PM -0500, Paul Moore wrote:
> > > On Thu, Sep 21, 2023 at 2:17 AM Mickaël Salaün <[email protected]> wrote:
> > > >
> > > > Add audit support for ruleset/domain creation and release ...
>
> ...
>
> > > > +/* Inspired by dump_common_audit_data(). */
> > > > +static void log_task(struct audit_buffer *const ab)
> > > > +{
> > > > + /* 16 bytes (TASK_COMM_LEN) */
> > > > + char comm[sizeof(current->comm)];
> > > > +
> > > > + /*
> > > > + * Uses task_pid_nr() instead of task_tgid_nr() because of how
> > > > + * credentials and Landlock work.
> > > > + */
> > > > + audit_log_format(ab, "tid=%d comm=", task_pid_nr(current));
> > > > + audit_log_untrustedstring(ab,
> > > > + memcpy(comm, current->comm, sizeof(comm)));
> > > > +}
> > >
> > > Depending on how log_task() is used, it may be redundant with respect
> > > to the existing SYSCALL record. Yes, there is already redundancy with
> > > the AVC record, but that is a legacy problem and not something we can
> > > easily fix, but given that the Landlock records are new we have an
> > > opportunity to do things properly :)
> >
> > Indeed, it would make it simpler too. I wasn't sure how standalone a
> > record should be, but I guess there tools should be able to look for a
> > set of related records (e.g. event with a SYSCALL record matching a PID
> > and a LANDLOCK record).
>
> I believe ausearch will output the entire event when it matches on a
> field in one of the event's records.
Right, and the man page talks about that.
>
> > > > +void landlock_log_create_ruleset(struct landlock_ruleset *const ruleset)
> > > > +{
> > > > + struct audit_buffer *ab;
> > > > +
> > > > + WARN_ON_ONCE(ruleset->id);
> > > > +
> > > > + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_LANDLOCK);
> > > > + if (!ab)
> > > > + /* audit_log_lost() call */
> > > > + return;
> > > > +
> > > > + ruleset->id = atomic64_inc_return(&ruleset_and_domain_counter);
> > > > + log_task(ab);
> > > > + audit_log_format(ab,
> > > > + " op=create-ruleset ruleset=%llu handled_access_fs=",
> > > > + ruleset->id);
> > >
> > > "handled_access_fs" seems a bit long for a field name, is there any
> > > reason why it couldn't simply be "access_fs" or something similar?
> >
> > "handled_access_fs" is from the landlock_create_ruleset(2) API, so I'd
> > like to use the same name.
>
> Okay, that's a reasonable reason.
>
> > However, because the types of handled access
> > rights for a ruleset will expand (e.g. we now have a
> > handled_access_net), I'm wondering if it would be better to keep this
> > (growing) one-line record or if we should use several records for a
> > ruleset creation (i.e. one line per type of handled access righs).
>
> I think it would be better to have a single record for rulesets rather
> than multiple records all dealing with rulesets.
I guess you mean to not create multiple record types specific to ruleset
creation. Reusing existing record types (e.g. path) should be OK even
for a ruleset creation. However, as proposed below, we might still want
a LANDLOCK_ACCESS record type (that can be reused for denied accesses).
>
> > > > + id = ruleset->hierarchy->id;
> > > > + WARN_ON_ONCE(ruleset->id);
> > > > + } else {
> > > > + name = "ruleset";
> > > > + id = ruleset->id;
> > > > + }
> > > > + WARN_ON_ONCE(!id);
> > > > +
> > > > + /*
> > > > + * Because this might be called by kernel threads, logging
> > > > + * related task information with log_task() would be useless.
> > > > + */
> > > > + audit_log_format(ab, "op=release-%s %s=%llu", name, name, id);
> > >
> > > This starts to get a little tricky. The general guidance is that for
> > > a given audit record type, e.g. AUDIT_LANDLOCK, there should be no
> > > change in presence or ordering of fields, yet in
> > > landlock_log_create_ruleset() we log the permission information and
> > > here in landlock_log_release_ruleset() we do not. The easy fix is to
> > > record the permission information here as well, or simply use a
> > > "handled_access_fs=?" placeholder. Something to keep in mind as you
> > > move forward.
> >
> > OK, I used different "op" to specify the related fields, but I should
> > use a dedicated record type when it makes sense instead. My reasoning
> > was that it would be easier to filter on one or two record types, but
> > I like the fixed set of fields per record type.
> >
> > I plan to add a few record types, something like that:
> >
> > For a ruleset creation event, several grouped records:
> > - AUDIT_LANDLOCK_RULESET: "id=[new ruleset ID] op=create"
> > - AUDIT_LANDLOCK_ACCESS: "type=[fs or net] rights=[bitmask]"
>
> I'm guessing that LANDLOCK_RULESET would be for policy changes, and
> LANDLOCK_ACCESS would be for individual access grants or denials? If
> so, that looks reasonable.
I was thinking about using LANDLOCK_ACCESS for both ruleset creation and
denied accesses. That would mkae a ruleset creation event easier to
parse and more flexible. Does that look good?
Otherwise, we can use this instead:
- AUDIT_LANDLOCK_RULESET: "ruleset=[new ruleset ID]
handled_access_fs=[bitmask] handled_access_net=[bitmask]"
>
> > For rule addition, several records per landlock_add_rule(2) call.
> > Example with a path_beneath rule:
> > - AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=add_rule"
> > - AUDIT_LANDLOCK_PATH: "scope=beneath path=[file path] dev= ino="
> > - AUDIT_LANDLOCK_ACCESS: "type=fs rights=[bitmask]"
>
> I worry that LANDLOCK_PATH is too much of a duplicate for the existing
> PATH record. Assuming the "scope=" field is important, could it live
> in the LANDLOCK_ACCESS record and then you could do away with the
> dedicated LANDLOCK_PATH record? Oh, wait ... this is to record the
> policy, not a individual access request, gotcha. If that is the case
> and RULESET, PATH, ACCESS are all used simply to record the policy
> information I might suggest creation of an AUDIT_LANDLOCK_POLICY
> record that captures all of the above. If you think that is too
> cumbersome, then perhaps you can do the object/access-specific record
> type, e.g. AUDIT_LANDLOCK_POLICY_FS and AUDIT_LANDLOCK_POLICY_NET.
OK, what about this records for *one* rule addition event?
- AUDIT_LANDLOCK_RULE: "ruleset=[ruleset ID] rule_type=path_beneath
allowed_access=[bitmask]"
- AUDIT_PATH: "path=[file path] dev= ino= ..."
However, because struct landlock_path_beneath_attr can evolve and get
new fields which might be differents than the landlock_net_port_attr's
ones, wouldn't it be wiser to use a dedicated AUDIT_LANDLOCK_RULE_FS or
AUDIT_LANDLOCK_RULE_PATH_BENEATH record type? These names are getting a
bit long though, but types match the UAPI.
>
> You also shouldn't reuse the "type=" field. Steve gets grumpy when
> people reuse field names for different things. You can find a
> reasonably complete list of fields here:
> https://github.com/linux-audit/audit-documentation/blob/main/specs/fields/field-dictionary.csv
OK
>
> > For domain creation/restriction:
> > - AUDIT_LANDLOCK_DOMAIN: "id=[new domain ID] op=create"
> > - AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=use"
>
> I imagine you could capture this in the policy record type?
What about this?
- AUDIT_LANDLOCK_RESTRICT: "ruleset=[ruleset ID] domain=[new domain ID]
restrict_type=self"
>
> > For ruleset release:
> > - AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=release"
> >
> > For domain release:
> > - AUDIT_LANDLOCK_DOMAIN: "id=[domain ID] op=release"
>
> Same with the above two.
- AUDIT_LANDLOCK_RELEASE: "id=[ruleset or domain ID]
release_type=[ruleset or domain]"
The issue with this record is that the "id" field is not the same as for
AUDIT_LANDLOCK_{RESTRICT,RULE}... To have "domain" or "ruleset" fields,
a dedicated record type would be cleaner:
AUDIT_LANDLOCK_RELEASE_{RULESET,DOMAIN}.
>
> > For denied FS access:
> > - AUDIT_LANDLOCK_DENIAL: "id=[domain ID] op=mkdir"
> > - AUDIT_LANDLOCK_PATH: "scope=exact path=[file path] dev= ino="
>
> I would use a single record type, i.e. AUDIT_LANDLOCK_ACCESS, to
> capture both access granted and denied events. I'd also omit the
> dedicated LANDLOCK_PATH record here in favor of the generic PATH
> record (see my comments above).
Makes sense for the generic PATH record. We would get this:
- AUDIT_LANDLOCK_ACCESS: "domain=[domain ID] op=mkdir result=denied"
- AUDIT_PATH: "path=[file path] dev= ino= ..."
>
> > For denied net access:
> > - AUDIT_LANDLOCK_DENIAL: "id=[domain ID] op=connect"
> > - AUDIT_LANDLOCK_PORT: "port="
>
> I would look at the SOCKADDR record type instead of introducing a new
> LANDLOCK_PORT type.
Good, this is already filled so I don't have to do anything except the
AUDIT_LANDLOCK_ACCESS record.
However, I'm wondering if it would be OK to create a synthetic sockaddr
struct to generate a sockaddr audit record when adding a new net_port
rule. In this case, we'd have to fill the fill the source and
destination addresses with fake values (zeros?) and the source and
destination ports with the rule's port. The pros is that it would not
add a new record type but the cons is that it will probably not work
with future net_port rule properties. It would also be inconsistent with
AUDIT_LANDLOCK_ACCESS.
What about this instead?
- AUDIT_LANDLOCK_RULE: "ruleset=[ruleset ID] rule_type=net_port
allowed_access=[bitmask]"
- AUDIT_LANDLOCK_PORT: "port=[port number]"
>
> > I guess it will still be OK to expand a record type with new appended
> > fields right? For instance, could we append a "flags" field to a
> > AUDIT_LANDLOCK_RULESET record (because it may not make sense to create a
> > dedicated record type for that)?
>
> Of course, one can always add fields to an existing record type with
> the understanding that they MUST be added to the end.
>
> --
> paul-moore.com
>
On Fri, Dec 29, 2023 at 06:42:10PM +0100, Mickaël Salaün wrote:
> On Fri, Dec 22, 2023 at 05:42:35PM -0500, Paul Moore wrote:
> > On Thu, Dec 21, 2023 at 1:45 PM Mickaël Salaün <[email protected]> wrote:
> > > On Wed, Dec 20, 2023 at 04:22:15PM -0500, Paul Moore wrote:
> > > > On Thu, Sep 21, 2023 at 2:17 AM Mickaël Salaün <[email protected]> wrote:
> > > > >
> > > > > Add audit support for ruleset/domain creation and release ...
> >
> > ...
> > > For rule addition, several records per landlock_add_rule(2) call.
> > > Example with a path_beneath rule:
> > > - AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=add_rule"
> > > - AUDIT_LANDLOCK_PATH: "scope=beneath path=[file path] dev= ino="
> > > - AUDIT_LANDLOCK_ACCESS: "type=fs rights=[bitmask]"
> >
> > I worry that LANDLOCK_PATH is too much of a duplicate for the existing
> > PATH record. Assuming the "scope=" field is important, could it live
> > in the LANDLOCK_ACCESS record and then you could do away with the
> > dedicated LANDLOCK_PATH record? Oh, wait ... this is to record the
> > policy, not a individual access request, gotcha. If that is the case
> > and RULESET, PATH, ACCESS are all used simply to record the policy
> > information I might suggest creation of an AUDIT_LANDLOCK_POLICY
> > record that captures all of the above. If you think that is too
> > cumbersome, then perhaps you can do the object/access-specific record
> > type, e.g. AUDIT_LANDLOCK_POLICY_FS and AUDIT_LANDLOCK_POLICY_NET.
>
> OK, what about this records for *one* rule addition event?
>
> - AUDIT_LANDLOCK_RULE: "ruleset=[ruleset ID] rule_type=path_beneath
> allowed_access=[bitmask]"
> - AUDIT_PATH: "path=[file path] dev= ino= ..."
>
> However, because struct landlock_path_beneath_attr can evolve and get
> new fields which might be differents than the landlock_net_port_attr's
> ones, wouldn't it be wiser to use a dedicated AUDIT_LANDLOCK_RULE_FS or
> AUDIT_LANDLOCK_RULE_PATH_BENEATH record type? These names are getting a
> bit long though, but types match the UAPI.
Hmm, AUDIT_PATH is used when a syscall's argument is a path, but in the
case of Landlock, the arguments are file descriptors.
I can still export audit_copy_inode() to create a synthetic audit_names
struct, and export/call audit_log_name() to create an AUDIT_PATH entry
but I'm not sure it is the best approach. What would you prefer?
Should I use AUDIT_TYPE_NORMAL or create a new one?
[...]
> > > For denied FS access:
> > > - AUDIT_LANDLOCK_DENIAL: "id=[domain ID] op=mkdir"
> > > - AUDIT_LANDLOCK_PATH: "scope=exact path=[file path] dev= ino="
> >
> > I would use a single record type, i.e. AUDIT_LANDLOCK_ACCESS, to
> > capture both access granted and denied events. I'd also omit the
> > dedicated LANDLOCK_PATH record here in favor of the generic PATH
> > record (see my comments above).
>
> Makes sense for the generic PATH record. We would get this:
>
> - AUDIT_LANDLOCK_ACCESS: "domain=[domain ID] op=mkdir result=denied"
> - AUDIT_PATH: "path=[file path] dev= ino= ..."
On Fri, Dec 29, 2023 at 12:42 PM Mickaël Salaün <[email protected]> wrote:
> On Fri, Dec 22, 2023 at 05:42:35PM -0500, Paul Moore wrote:
> > On Thu, Dec 21, 2023 at 1:45 PM Mickaël Salaün <[email protected]> wrote:
> > > On Wed, Dec 20, 2023 at 04:22:15PM -0500, Paul Moore wrote:
> > > > On Thu, Sep 21, 2023 at 2:17 AM Mickaël Salaün <[email protected]> wrote:
> > > > >
> > > > > Add audit support for ruleset/domain creation and release ...
...
> > > > > +void landlock_log_create_ruleset(struct landlock_ruleset *const ruleset)
> > > > > +{
> > > > > + struct audit_buffer *ab;
> > > > > +
> > > > > + WARN_ON_ONCE(ruleset->id);
> > > > > +
> > > > > + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_LANDLOCK);
> > > > > + if (!ab)
> > > > > + /* audit_log_lost() call */
> > > > > + return;
> > > > > +
> > > > > + ruleset->id = atomic64_inc_return(&ruleset_and_domain_counter);
> > > > > + log_task(ab);
> > > > > + audit_log_format(ab,
> > > > > + " op=create-ruleset ruleset=%llu handled_access_fs=",
> > > > > + ruleset->id);
> > > >
> > > > "handled_access_fs" seems a bit long for a field name, is there any
> > > > reason why it couldn't simply be "access_fs" or something similar?
> > >
> > > "handled_access_fs" is from the landlock_create_ruleset(2) API, so I'd
> > > like to use the same name.
> >
> > Okay, that's a reasonable reason.
> >
> > > However, because the types of handled access
> > > rights for a ruleset will expand (e.g. we now have a
> > > handled_access_net), I'm wondering if it would be better to keep this
> > > (growing) one-line record or if we should use several records for a
> > > ruleset creation (i.e. one line per type of handled access righs).
> >
> > I think it would be better to have a single record for rulesets rather
> > than multiple records all dealing with rulesets.
>
> I guess you mean to not create multiple record types specific to ruleset
> creation.
Yes.
> Reusing existing record types (e.g. path) should be OK even
> for a ruleset creation. However, as proposed below, we might still want
> a LANDLOCK_ACCESS record type (that can be reused for denied accesses).
I would want to see the code to make sure we are not misunderstanding
each other, but I believe you are on the right track.
> > > > > + id = ruleset->hierarchy->id;
> > > > > + WARN_ON_ONCE(ruleset->id);
> > > > > + } else {
> > > > > + name = "ruleset";
> > > > > + id = ruleset->id;
> > > > > + }
> > > > > + WARN_ON_ONCE(!id);
> > > > > +
> > > > > + /*
> > > > > + * Because this might be called by kernel threads, logging
> > > > > + * related task information with log_task() would be useless.
> > > > > + */
> > > > > + audit_log_format(ab, "op=release-%s %s=%llu", name, name, id);
> > > >
> > > > This starts to get a little tricky. The general guidance is that for
> > > > a given audit record type, e.g. AUDIT_LANDLOCK, there should be no
> > > > change in presence or ordering of fields, yet in
> > > > landlock_log_create_ruleset() we log the permission information and
> > > > here in landlock_log_release_ruleset() we do not. The easy fix is to
> > > > record the permission information here as well, or simply use a
> > > > "handled_access_fs=?" placeholder. Something to keep in mind as you
> > > > move forward.
> > >
> > > OK, I used different "op" to specify the related fields, but I should
> > > use a dedicated record type when it makes sense instead. My reasoning
> > > was that it would be easier to filter on one or two record types, but
> > > I like the fixed set of fields per record type.
> > >
> > > I plan to add a few record types, something like that:
> > >
> > > For a ruleset creation event, several grouped records:
> > > - AUDIT_LANDLOCK_RULESET: "id=[new ruleset ID] op=create"
> > > - AUDIT_LANDLOCK_ACCESS: "type=[fs or net] rights=[bitmask]"
> >
> > I'm guessing that LANDLOCK_RULESET would be for policy changes, and
> > LANDLOCK_ACCESS would be for individual access grants or denials? If
> > so, that looks reasonable.
>
> I was thinking about using LANDLOCK_ACCESS for both ruleset creation and
> denied accesses. That would mkae a ruleset creation event easier to
> parse and more flexible. Does that look good?
In general, configuration events like ruleset creation use one record
type, while individual access requests use a different record type.
> Otherwise, we can use this instead:
>
> - AUDIT_LANDLOCK_RULESET: "ruleset=[new ruleset ID]
> handled_access_fs=[bitmask] handled_access_net=[bitmask]"
>
> > > For rule addition, several records per landlock_add_rule(2) call.
> > > Example with a path_beneath rule:
> > > - AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=add_rule"
> > > - AUDIT_LANDLOCK_PATH: "scope=beneath path=[file path] dev= ino="
> > > - AUDIT_LANDLOCK_ACCESS: "type=fs rights=[bitmask]"
> >
> > I worry that LANDLOCK_PATH is too much of a duplicate for the existing
> > PATH record. Assuming the "scope=" field is important, could it live
> > in the LANDLOCK_ACCESS record and then you could do away with the
> > dedicated LANDLOCK_PATH record? Oh, wait ... this is to record the
> > policy, not a individual access request, gotcha. If that is the case
> > and RULESET, PATH, ACCESS are all used simply to record the policy
> > information I might suggest creation of an AUDIT_LANDLOCK_POLICY
> > record that captures all of the above. If you think that is too
> > cumbersome, then perhaps you can do the object/access-specific record
> > type, e.g. AUDIT_LANDLOCK_POLICY_FS and AUDIT_LANDLOCK_POLICY_NET.
>
> OK, what about this records for *one* rule addition event?
>
> - AUDIT_LANDLOCK_RULE: "ruleset=[ruleset ID] rule_type=path_beneath
> allowed_access=[bitmask]"
> - AUDIT_PATH: "path=[file path] dev= ino= ..."
If the pathname above is for the landlock rule, it should be separate
from the existing AUDIT_PATH record. See my previous comments above,
when I was talking about using the existing AUDIT_PATH record I was
confusing the rule creation event with the permission check event.
> However, because struct landlock_path_beneath_attr can evolve and get
> new fields which might be differents than the landlock_net_port_attr's
> ones, wouldn't it be wiser to use a dedicated AUDIT_LANDLOCK_RULE_FS or
> AUDIT_LANDLOCK_RULE_PATH_BENEATH record type? These names are getting a
> bit long though, but types match the UAPI.
I believe we were thinking similarly, see my previous comments above
about AUDIT_LANDLOCK_POLICY_FS and AUDIT_LANDLOCK_POLICY_NET, etc.
> > You also shouldn't reuse the "type=" field. Steve gets grumpy when
> > people reuse field names for different things. You can find a
> > reasonably complete list of fields here:
> > https://github.com/linux-audit/audit-documentation/blob/main/specs/fields/field-dictionary.csv
>
> OK
>
> >
> > > For domain creation/restriction:
> > > - AUDIT_LANDLOCK_DOMAIN: "id=[new domain ID] op=create"
> > > - AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=use"
> >
> > I imagine you could capture this in the policy record type?
>
> What about this?
>
> - AUDIT_LANDLOCK_RESTRICT: "ruleset=[ruleset ID] domain=[new domain ID]
> restrict_type=self"
>
> >
> > > For ruleset release:
> > > - AUDIT_LANDLOCK_RULESET: "id=[ruleset ID] op=release"
> > >
> > > For domain release:
> > > - AUDIT_LANDLOCK_DOMAIN: "id=[domain ID] op=release"
> >
> > Same with the above two.
>
> - AUDIT_LANDLOCK_RELEASE: "id=[ruleset or domain ID]
> release_type=[ruleset or domain]"
>
> The issue with this record is that the "id" field is not the same as for
> AUDIT_LANDLOCK_{RESTRICT,RULE}... To have "domain" or "ruleset" fields,
> a dedicated record type would be cleaner:
> AUDIT_LANDLOCK_RELEASE_{RULESET,DOMAIN}.
If you need separate record types, you need separate record types.
Regardless of the types used, we need to make sure an administrator
can match up a creation event with a destruction event.
> > > For denied FS access:
> > > - AUDIT_LANDLOCK_DENIAL: "id=[domain ID] op=mkdir"
> > > - AUDIT_LANDLOCK_PATH: "scope=exact path=[file path] dev= ino="
> >
> > I would use a single record type, i.e. AUDIT_LANDLOCK_ACCESS, to
> > capture both access granted and denied events. I'd also omit the
> > dedicated LANDLOCK_PATH record here in favor of the generic PATH
> > record (see my comments above).
>
> Makes sense for the generic PATH record. We would get this:
>
> - AUDIT_LANDLOCK_ACCESS: "domain=[domain ID] op=mkdir result=denied"
> - AUDIT_PATH: "path=[file path] dev= ino= ..."
>
> >
> > > For denied net access:
> > > - AUDIT_LANDLOCK_DENIAL: "id=[domain ID] op=connect"
> > > - AUDIT_LANDLOCK_PORT: "port="
> >
> > I would look at the SOCKADDR record type instead of introducing a new
> > LANDLOCK_PORT type.
>
> Good, this is already filled so I don't have to do anything except the
> AUDIT_LANDLOCK_ACCESS record.
>
> However, I'm wondering if it would be OK to create a synthetic sockaddr
> struct to generate a sockaddr audit record when adding a new net_port
> rule. In this case, we'd have to fill the fill the source and
> destination addresses with fake values (zeros?) and the source and
> destination ports with the rule's port. The pros is that it would not
> add a new record type but the cons is that it will probably not work
> with future net_port rule properties. It would also be inconsistent with
> AUDIT_LANDLOCK_ACCESS.
>
> What about this instead?
>
> - AUDIT_LANDLOCK_RULE: "ruleset=[ruleset ID] rule_type=net_port
> allowed_access=[bitmask]"
> - AUDIT_LANDLOCK_PORT: "port=[port number]"
Just as we probably don't want to reuse the AUDIT_PATH record, we
probably shouldn't reuse the sockaddr record.
--
paul-moore.com