Messages in this thread | | | Date | Thu, 21 Dec 2023 19:45:16 +0100 | From | Mickaël Salaün <> | Subject | Re: [RFC PATCH v1 3/7] landlock: Log ruleset creation and release |
| |
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 <mic@digikod.net> 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 <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 > > ... > > > 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
| |