| Message ID | 20230921061641.273654-4-mic@digikod.net (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | Landlock audit support | expand |
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. > 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 :) > +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 <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
On Thu, Dec 21, 2023 at 1:45 PM Mickaël Salaün <mic@digikod.net> 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 <mic@digikod.net> 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.
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 <mic@digikod.net> 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 <mic@digikod.net> 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 <mic@digikod.net> 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 <mic@digikod.net> 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 <mic@digikod.net> 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 <mic@digikod.net> 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 <mic@digikod.net> 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.
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; }
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 <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