Message-ID: <20250106.ohgh8Zeu6coo@digikod.net>
Date: Mon, 6 Jan 2025 15:51:15 +0100
From: Mickaël Salaün <mic@...ikod.net>
To: Paul Moore <paul@...l-moore.com>
Cc: Eric Paris <eparis@...hat.com>, 
	Günther Noack <gnoack@...gle.com>, "Serge E . Hallyn" <serge@...lyn.com>, 
	Ben Scarlato <akhna@...gle.com>, Casey Schaufler <casey@...aufler-ca.com>, 
	Charles Zaffery <czaffery@...lox.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>, audit@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-security-module@...r.kernel.org
Subject: Re: [PATCH v3 10/23] landlock: Log domain properties and release

On Sat, Jan 04, 2025 at 08:23:51PM -0500, Paul Moore wrote:
> On Nov 22, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@...ikod.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).

As explained in this patch, we don't want to (and cannot realistically)
log domain creations.  That would make the audit support for Landlock
unusable.  Moreover, these information is useless and only add noise
unless there is a denial, hence this asynchronous approach.  However,
users may want to log some syscalls, including landlock_restrict_self(),
and it would make audit logs more consistent using the same timestamp as
the Landlock domain creation time.  I'm wondering why exposing this
timestamp to the kernel would be an issue whereas it is already exposed
to user space.

If you're really opposed to it I can create a new unrelated timestamp
specific to Landlock.

> 
> 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.

The goal of the standalone LL_DOM_INFO record type is to limit useless
log verbosity.  Including this information in LL_DENY would have two
downsides:
- it would increases the length of *all* LL_DENY messages
- it would make it more difficult to extend this new mixed messages with
  access-related informations (e.g. file property) and domain-related
  informations (and associate them with either the object or the
  domain).

I prefer to have clear semantic with distinct and dedicated audit record
types.  Relying on different record type also enable users to
efficiently filter them.

> 
> > Audit event sample for logged domains deletion:
> > 
> >   type=LL_DOM_DROP msg=audit(1732186800.393:45): domain=195ba459b
> > 
> > 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/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.

The specificity of Landlock compared to existing supported systems is
that we cannot log domain creation for the reason I explain before.
We might extend the asynchronous LL_DOM_INFO message with such
information in the future though.

> 
> > 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.

Using a dedicated audit record type enables users to efficiently filter
according to their type, and (in this specific case, sightly) reduce log
size.

> 
> > 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?

I wanted to limit this check but only do it before actually using this
ID.

> 
> > +	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
> 

Powered by blists - more mailing lists