Re: [PATCH v3 10/23] landlock: Log domain properties and release

[Paul Moore <paul@paul-moore.com>]

On Nov 22, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> 
> TODO: Update sample with audit's creation time and now-removed parent
> hierarchy
> 
> 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_LOGLESS flag).  These records are identified by
> AUDIT_LANDLOCK_DOM_INFO.
> 
> Log domain deletion with the AUDIT_LANDLOCK_DOM_DROP record type when
> a domain was previously logged.  This makes it possible for user space
> to free potential resources when a domain ID will never show again.
> 
> The logged domain properties include:
> - the domain ID
> - creation time
> - its creator's PID
> - its creator's UID
> - its creator's executable path (exe)
> - its creator's command line (comm)
> 
> This require each domain to save some task properties at creation time:
> time, PID, credentials, exe path, and comm.
> 
> It should be noted that we cannot use audit event's serial numbers
> because there may not be any related event.  However, it is still useful
> to use the same potential timestamp instead of a close one.  What really
> matter is how old the related Landlock domain is, not the uniquiness of
> the creation time.  If audit is not enabled, Landlock creates its own
> timestamp.  This timestamp will be exposed to user space with a future
> unprivileged interface.
> 
> Audit event sample for a first denial:
> 
>   type=LL_DENY msg=audit(1732186800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
>   type=LL_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

As mentioned in patch 9/23, I don't want subsystems external to audit
to access the audit timestamp information, so the "creation=" field
in the audit event would need to be removed.  Assuming that the timestamp
was used either to reference the original domain creation and/or simply
provide some additional information for analysis, all of that information
should already be in the audit log, assuming of course that you are
logging domain creation (which you should, at least as an option).

Also, is there a good reason why the LL_DOM_INFO information can't be
recorded in the LL_DENY (or LL_ACCESS) record?  I think that would be
preferable.

> Audit event sample for logged domains deletion:
> 
>   type=LL_DOM_DROP msg=audit(1732186800.393:45): domain=195ba459b
> 
> Cc: Günther Noack <gnoack@google.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20241122143353.59367-11-mic@digikod.net
> ---
> Questions:
> - Should we also log the creator's loginuid?
> - Should we also log the creator's sessionid?

Creation of a Landlock domain can only happen through the Landlock
syscalls, yes?  If so, that information should already be logged in
the associated syscall record (see the "auid=" and "ses=" fields )and
we generally try to avoid duplicating information across records in
the same audit event.

> 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   |  2 +
>  security/landlock/audit.c    | 74 ++++++++++++++++++++++++++++++++++++
>  security/landlock/audit.h    |  7 ++++
>  security/landlock/domain.c   | 41 ++++++++++++++++++++
>  security/landlock/domain.h   | 43 +++++++++++++++++++++
>  security/landlock/ruleset.c  |  7 ++++
>  security/landlock/ruleset.h  |  1 +
>  security/landlock/syscalls.c |  1 +
>  8 files changed, 176 insertions(+)
> 
> 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 */

See my comment about regarding AUDIT_LANDLOCK_DOM_INFO.

Similar to my previous comments regarding AUDIT_LANDLOCK_DENY, it might
be a good idea to change AUDIT_LANDLOCK_DOM_DROP to simply
AUDIT_LANDLOCK_DOM and use an "op=" field to indicate a drop, creation,
or other event.

> diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> index eab6b3a8a231..2d0a96797dd4 100644
> --- a/security/landlock/audit.c
> +++ b/security/landlock/audit.c
> @@ -8,8 +8,11 @@
>  #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"
>  #include "domain.h"
>  #include "ruleset.h"
>  
> @@ -30,6 +33,41 @@ 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);

It seems like you might want to move the WARN_ON assertion up with the
other WARN_ON check?

> +	audit_log_format(ab, "domain=%llx creation=%llu.%03lu pid=%d uid=%u",
> +			 node->id,
> +			 /* See audit_log_start() */
> +			 (unsigned long long)node->creation.tv_sec,
> +			 node->creation.tv_nsec / 1000000, pid_nr(node->pid),
> +			 from_kuid(&init_user_ns, node->cred->uid));
> +	audit_log_d_path(ab, " exe=", &node->exe);
> +	audit_log_format(ab, " comm=");
> +	audit_log_untrustedstring(ab, node->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)
>  {

--
paul-moore.com