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 [off-list ref] wrote:

quoted

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.

quoted

Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 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

...

quoted

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).

quoted

+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).

quoted

+       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.

quoted

+               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)?

quoted

+       audit_log_end(ab);
+}

--
paul-moore.com