On Jan 31, 2025 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <m...@digikod.net> wrote:
>
> Asynchronously log domain information when it first denies an access.
> This minimize the amount of generated logs, which makes it possible to
> always log denials since they should not happen (except with the new
> LANDLOCK_RESTRICT_SELF_QUIET flag). These records are identified with
> the new AUDIT_LANDLOCK_DOMAIN type.
>
> The AUDIT_LANDLOCK_DOMAIN message contains:
> - the "domain" ID which is described;
> - the "status" which can either be "allocated" or "deallocated";
> - the "mode" which is for now only "enforcing";
> - for the "allocated" status, a minimal set of properties to easily
> identify the task that loaded the domain's policy with
> landlock_restrict_self(2): "pid", "uid", executable path ("exe"), and
> command line ("comm");
> - for the "deallocated" state, the number of "denials" accounted to this
> domain, which is at least 1.
>
> This requires each domain to save these task properties at creation
> time in the new struct landlock_details. A reference to the PID is kept
> for the lifetime of the domain to avoid race conditions when
> investigating the related task. The executable path is resolved and
> stored to not keep a reference to the filesystem and block related
> actions. All these metadata are stored for the lifetime of the related
> domain and should then be minimal. The required memory is not accounted
> to the task calling landlock_restrict_self(2) contrary to most other
> Landlock allocations (see related comment).
>
> The AUDIT_LANDLOCK_DOMAIN record follows the first AUDIT_LANDLOCK_ACCESS
> record for the same domain, which is always followed by AUDIT_SYSCALL
> and AUDIT_PROCTITLE. This is in line with the audit logic to first
> record the cause of an event, and then add context with other types of
> record.
>
> Audit event sample for a first denial:
>
> type=LANDLOCK_ACCESS msg=audit(1732186800.349:44): domain=195ba459b
> blockers=ptrace opid=1 ocomm="systemd"
> type=LANDLOCK_DOMAIN msg=audit(1732186800.349:44): domain=195ba459b
> status=allocated mode=enforcing pid=300 uid=0 exe="/root/sandboxer"
> comm="sandboxer"
> type=SYSCALL msg=audit(1732186800.349:44): arch=c000003e syscall=101
> success=no [...] pid=300 auid=0
>
> Audit event sample for a following denial:
>
> type=LANDLOCK_ACCESS msg=audit(1732186800.372:45): domain=195ba459b
> blockers=ptrace opid=1 ocomm="systemd"
> type=SYSCALL msg=audit(1732186800.372:45): arch=c000003e syscall=101
> success=no [...] pid=300 auid=0
>
> Log domain deletion with the "deallocated" state when a domain was
> previously logged. This makes it possible for log parsers to free
> potential resources when a domain ID will never show again.
>
> The number of denied access requests is useful to easily check how many
> access requests a domain blocked and potentially if some of them are
> missing in logs because of audit rate limiting or audit rules. Rate
> limiting could also drop this record though.
>
> Audit event sample for a deletion of a domain that denied something:
>
> type=LANDLOCK_DOMAIN msg=audit(1732186800.393:46): domain=195ba459b
> status=deallocated denials=2
>
> Cc: Günther Noack <gno...@google.com>
> Cc: Paul Moore <p...@paul-moore.com>
> Signed-off-by: Mickaël Salaün <m...@digikod.net>
> Link: https://lore.kernel.org/r/20250131163059.1139617-11-...@digikod.net
> ---
> Changes since v4:
> - Rename AUDIT_LANDLOCK_DOM_{INFO,DROP} to AUDIT_LANDLOCK_DOMAIN and add
> a "status" field, as requested by Paul.
> - Add a harcoded "mode=enforcing" to leave room for a potential future
> permissive mode, as suggested by Paul.
> - Remove the "creation" timestamp, as suggested by Paul.
> - Move LANDLOCK_PATH_MAX_SIZE to domain.h, check the size of the
> greatest landlock_details at build time, and improve comments.
> - Improve audit check in landlock_log_drop_domain().
> - Add missing headers.
> - Fix typo in comment.
> - Rebase on top of the landlock_log_denial() and subject type changes.
>
> Changes since v3:
> - Log number of denied access requests with AUDIT_LANDLOCK_DOM_DROP
> records, suggested by Tyler.
> - Do not store a struct path pointer but the resolved string instead.
> This enables us to not block unmount of the initially restricted task
> executable's mount point. See the new get_current_info() and
> get_current_exe(). A following patch add tests for this case.
> - Create and allocate a new struct landlock_details for initially
> restricted task's information.
> - Remove audit_get_ctime() call, as requested by Paul. We now always
> have a standalone timestamp per Landlock domain creations.
> - Fix docstring.
>
> Changes since v2:
> - Fix docstring.
> - Fix log_status check in log_hierarchy() to also log
> LANDLOCK_LOG_DISABLED.
> - Add audit's creation time to domain's properties.
> - Use hexadecimal notation for domain IDs.
> - Remove domain's parent records: parent domains are not really useful
> in the logs. They will be available with the upcoming introspection
> feature though.
> - Extend commit message with audit's timestamp explanation.
>
> Changes since v1:
> - Add a ruleset's version for atomic logs.
> - Rebased on the TCP patch series.
> - Rename operation using "_" instead of "-".
> - Rename AUDIT_LANDLOCK to AUDIT_LANDLOCK_RULESET.
> - Only log when audit is enabled, but always set domain IDs.
> - Don't log task's PID/TID with log_task() because it would be redundant
> with the SYSCALL record.
> - Remove race condition when logging ruleset creation and logging
> ruleset modification while the related file descriptor was already
> registered but the ruleset creation not logged yet.
> - Fix domain drop logs.
> - Move the domain drop record from the previous patch into this one.
> - Do not log domain creation but log first domain use instead.
> - Save task's properties that sandbox themselves.
> ---
> include/uapi/linux/audit.h | 1 +
> security/landlock/audit.c | 90 ++++++++++++++++++++++++++++++--
> security/landlock/audit.h | 7 +++
> security/landlock/domain.c | 101 ++++++++++++++++++++++++++++++++++++
> security/landlock/domain.h | 68 ++++++++++++++++++++++++
> security/landlock/ruleset.c | 6 +++
> 6 files changed, 270 insertions(+), 3 deletions(-)
Some minor questions below, but from an audit perspective this is okay.
Acked-by: Paul Moore <p...@paul-moore.com> (Audit)
> diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> index b0dde6bcfb76..a5b055306757 100644
> --- a/security/landlock/audit.c
> +++ b/security/landlock/audit.c
> @@ -8,6 +8,8 @@
> #include <kunit/test.h>
> #include <linux/audit.h>
> #include <linux/lsm_audit.h>
> +#include <linux/pid.h>
> +#include <linux/uidgid.h>
>
> #include "audit.h"
> #include "cred.h"
> @@ -31,6 +33,40 @@ static void log_blockers(struct audit_buffer *const ab,
> audit_log_format(ab, "%s", get_blocker(type));
> }
>
> +static void log_node(struct landlock_hierarchy *const node)
> +{
> + struct audit_buffer *ab;
> +
> + if (WARN_ON_ONCE(!node))
> + return;
> +
> + /* Ignores already logged domains. */
> + if (READ_ONCE(node->log_status) == LANDLOCK_LOG_RECORDED)
> + return;
> +
> + ab = audit_log_start(audit_context(), GFP_ATOMIC,
> + AUDIT_LANDLOCK_DOMAIN);
You use __GFP_NOWARN in the other calls to audit_log_start(), did you
mean to use it here as well?
> + if (!ab)
> + return;
> +
> + WARN_ON_ONCE(node->id == 0);
> + audit_log_format(
> + ab,
> + "domain=%llx status=allocated mode=enforcing pid=%d uid=%u
> exe=",
> + node->id, pid_nr(node->details->pid),
> + from_kuid(&init_user_ns, node->details->cred->uid));
> + audit_log_untrustedstring(ab, node->details->exe_path);
> + audit_log_format(ab, " comm=");
> + audit_log_untrustedstring(ab, node->details->comm);
> + audit_log_end(ab);
> +
> + /*
> + * There may be race condition leading to logging of the same domain
> + * several times but that is OK.
> + */
> + WRITE_ONCE(node->log_status, LANDLOCK_LOG_RECORDED);
> +}
> +
> static struct landlock_hierarchy *
> get_hierarchy(const struct landlock_ruleset *const domain, const size_t
> layer)
> {
> @@ -106,16 +142,24 @@ void landlock_log_denial(const struct
> landlock_cred_security *const subject,
> if (!is_valid_request(request))
> return;
>
> - if (!unlikely(audit_context() && audit_enabled))
> - return;
> -
> youngest_layer = request->layer_plus_one - 1;
> youngest_denied = get_hierarchy(subject->domain, youngest_layer);
>
> + /*
> + * Consistently keeps track of the number of denied access requests
> + * even if audit is currently disabled, if audit rules currently
> + * exclude this record type, or if landlock_restrict_self(2)'s flags
> + * quiet logs.
> + */
> + atomic64_inc(&youngest_denied->num_denials);
> +
> /* Ignores denials after an execution. */
> if (!(subject->domain_exec & (1 << youngest_layer)))
> return;
>
> + if (!unlikely(audit_context() && audit_enabled))
> + return;
> +
Not a big deal either way, but it seems like the check above should
probably be in patch 09/24.
> ab = audit_log_start(audit_context(), GFP_ATOMIC | __GFP_NOWARN,
> AUDIT_LANDLOCK_ACCESS);
> if (!ab)
> @@ -125,6 +169,46 @@ void landlock_log_denial(const struct
> landlock_cred_security *const subject,
> log_blockers(ab, request->type);
> audit_log_lsm_data(ab, &request->audit);
> audit_log_end(ab);
> +
> + /* Logs this domain if it is the first time. */
> + log_node(youngest_denied);
> +}
> +
> +/**
> + * landlock_log_drop_domain - Create an audit record when a domain is deleted
> + *
> + * @domain: The domain being deleted.
> + *
> + * Only domains which previously appeared in the audit logs are logged again.
> + * This is useful to know when a domain will never show again in the audit
> log.
> + *
> + * This record is not directly tied to a syscall entry.
> + *
> + * Called by the cred_free() hook, in an uninterruptible context.
> + */
> +void landlock_log_drop_domain(const struct landlock_ruleset *const domain)
> +{
> + struct audit_buffer *ab;
> +
> + if (WARN_ON_ONCE(!domain->hierarchy))
> + return;
> +
> + if (!unlikely(audit_enabled))
> + return;
I'm guessing you probably also want to check the audit context given
that you are doing it elsewhere?
> + /* Ignores domains that were not logged. */
> + if (READ_ONCE(domain->hierarchy->log_status) != LANDLOCK_LOG_RECORDED)
> + return;
> +
> + ab = audit_log_start(audit_context(), GFP_ATOMIC,
> + AUDIT_LANDLOCK_DOMAIN);
> + if (!ab)
> + return;
> +
> + audit_log_format(ab, "domain=%llx status=deallocated denials=%llu",
> + domain->hierarchy->id,
> + atomic64_read(&domain->hierarchy->num_denials));
> + audit_log_end(ab);
> }
>
> #ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
--
paul-moore.com
