| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1a169f22750aec9db4d7a377a4f99733@paul-moore.com>
Date: Wed, 15 Jan 2025 18:53:07 -0500
From: Paul Moore <paul@...l-moore.com>
To: Mickaël Salaün <mic@...ikod.net>, Eric Paris <eparis@...hat.com>, Günther Noack <gnoack@...gle.com>, "Serge E . Hallyn" <serge@...lyn.com>
Cc: Mickaël Salaün <mic@...ikod.net>, Ben Scarlato <akhna@...gle.com>, Casey Schaufler <casey@...aufler-ca.com>, Charles Zaffery <czaffery@...lox.com>, Daniel Burgener <dburgener@...ux.microsoft.com>, Francis Laniel <flaniel@...ux.microsoft.com>, James Morris <jmorris@...ei.org>, Jann Horn <jannh@...gle.com>, Jeff Xu <jeffxu@...gle.com>, Jorge Lucangeli Obes <jorgelo@...gle.com>, Kees Cook <kees@...nel.org>, Konstantin Meskhidze <konstantin.meskhidze@...wei.com>, Matt Bobrowski <mattbobrowski@...gle.com>, Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>, Phil Sutter <phil@....cc>, Praveen K Paladugu <prapal@...ux.microsoft.com>, Robert Salvet <robert.salvet@...lox.com>, Shervin Oloumi <enlightened@...gle.com>, Song Liu <song@...nel.org>, Tahera Fahimi <fahimitahera@...il.com>, Tyler Hicks <code@...icks.com>, audit@...r.kernel.org, linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH v4 9/30] landlock: Add AUDIT_LANDLOCK_DOM_{INFO,DROP} and log domain properties
On Jan 8, 2025 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@...ikod.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_DOM_INFO type.
>
> The AUDIT_LANDLOCK_DOM_INFO message contains:
> - the "domain" ID which is described,
> - the "creation" time of this domain,
> - 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").
>
> 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_DOM_INFO record follows the first AUDIT_LANDLOCK_DENY
> 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_DENY msg=audit(1732186800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
> type=LANDLOCK_DOM_INFO msg=audit(1732186800.349:44): domain=195ba459b creation=1732186800.345 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_DENY 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 new AUDIT_LANDLOCK_DOM_DROP record type
> 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 AUDIT_LANDLOCK_DOM_DROP message contains:
> - the "domain" ID which is being freed,
> - the number of "denials" accounted to this domain, which is at least 1.
>
> 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.
Wait, what rate limiting? Landlock shouldn't be adding any audit event
rate limiting beyond the queue management knobs built into the audit
subsystem. If you are comfortable rate limiting the logging of an event
it is a good sign that it probably shouldn't be an audit event.
The audit subsystem is for security releveant events, not diagnostic,
debugging, or other "nice to know" messages.
> Audit event sample for a deletion of a domain that denied something:
>
> type=LANDLOCK_DOM_DROP msg=audit(1732186800.393:46): domain=195ba459b denials=2
As mentioned earlier, I don't like the number of different Landlock
specific audit record types that are being created. I'm going to
suggest combining the LANDLOCK_DOM_INFO and LANDLOCK_DOM_DROP
records into one (LANDLOCK_DOM?) and using an "op=" field to indicate
creation/registration or destruction/unregistration of the domain ID.
> Cc: Günther Noack <gnoack@...gle.com>
> Cc: Paul Moore <paul@...l-moore.com>
> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> Link: https://lore.kernel.org/r/20250108154338.1129069-10-mic@digikod.net
> ---
> Questions about AUDIT_LANDLOCK_DOM_INFO messages (keeping in mind that
> each logged metadata may need to be stored for the lifetime of each
> domain):
> - Should we also log the initially restricted task's loginuid?
> - Should we also log the initially restricted task's sessionid?
...
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 60c909c396c0..a72f7b3403be 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -147,6 +147,8 @@
> #define AUDIT_IPE_CONFIG_CHANGE 1421 /* IPE config change */
> #define AUDIT_IPE_POLICY_LOAD 1422 /* IPE policy load */
> #define AUDIT_LANDLOCK_DENY 1423 /* Landlock denial */
> +#define AUDIT_LANDLOCK_DOM_INFO 1424 /* Landlock domain properties */
> +#define AUDIT_LANDLOCK_DOM_DROP 1425 /* Landlock domain release */
>
> #define AUDIT_FIRST_KERN_ANOM_MSG 1700
> #define AUDIT_LAST_KERN_ANOM_MSG 1799
> diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> index d90680a5026a..ccc591146f8a 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 "domain.h"
> @@ -30,6 +32,43 @@ 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_DOM_INFO);
> + if (!ab)
> + return;
> +
> + WARN_ON_ONCE(node->id == 0);
> + audit_log_format(
> + ab,
> + "domain=%llx creation=%llu.%03lu pid=%d uid=%u exe=", node->id,
> + /* See audit_log_start() */
> + (unsigned long long)node->details->creation.tv_sec,
> + node->details->creation.tv_nsec / 1000000,
> + 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);
I'm still struggling to understand why you need to log the domain's
creation time if you are connecting various Landlock audit events for a
single domain by the domain ID. To be clear, I'm not opposed if you
want to include it, it just seems like there is a disconnect between
how audit is typically used and what you are proposing.
> + /*
> + * 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);
> +}
> +
--
paul-moore.com
Powered by blists - more mailing lists