Message-ID: <20250218.uuBaiB2woloo@digikod.net>
Date: Tue, 18 Feb 2025 20:19:36 +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>, 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 v5 9/24] landlock: Add AUDIT_LANDLOCK_ACCESS and log
 ptrace  denials

On Fri, Feb 14, 2025 at 05:52:47PM -0500, Paul Moore wrote:
> On Jan 31, 2025 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@...ikod.net> wrote:
> > 
> > Add a new AUDIT_LANDLOCK_ACCESS record type dedicated to an access
> > request denied by a Landlock domain.  AUDIT_LANDLOCK_ACCESS indicates
> > that something unexpected happened.
> > 
> > For now, only denied access are logged, which means that any
> > AUDIT_LANDLOCK_ACCESS record is always followed by a SYSCALL record with
> > "success=no".  However, log parsers should check this syscall property
> > because this is the only sign that a request was denied.  Indeed, we
> > could have "success=yes" if Landlock would support a "permissive" mode.
> > We could also add a new field for this mode to AUDIT_LANDLOCK_DOMAIN
> > (see following commit).
> >
> > By default, the only logged access requests are those coming from the
> > same executed program that enforced the Landlock restriction on itself.
> > In other words, no audit record are created for a task after it called
> > execve(2).  This is required to avoid log spam because programs may only
> > be aware of their own restrictions, but not the inherited ones.
> > 
> > Following commits will allow to conditionally generate
> > AUDIT_LANDLOCK_ACCESS records according to dedicated
> > landlock_restrict_self(2)'s flags.
> > 
> > The AUDIT_LANDLOCK_ACCESS message contains:
> > - the "domain" ID restricting the action on an object,
> > - the "blockers" that are missing to allow the requested access,
> > - a set of fields identifying the related object (e.g. task identified
> >   with "opid" and "ocomm").
> > 
> > The blockers are implicit restrictions (e.g. ptrace), or explicit access
> > rights (e.g. filesystem), or explicit scopes (e.g. signal).  This field
> > contains a list of at least one element, each separated with a comma.
> > 
> > The initial blocker is "ptrace", which describe all implicit Landlock
> > restrictions related to ptrace (e.g. deny tracing of tasks outside a
> > sandbox).
> > 
> > Add audit support to ptrace_access_check and ptrace_traceme hooks.  For
> > the ptrace_access_check case, we log the current/parent domain and the
> > child task.  For the ptrace_traceme case, we log the parent domain and
> > the parent task.  Indeed, the requester is the current task, but the
> > action would be performed by the parent task.
> > 
> > Audit event sample:
> > 
> >   type=LANDLOCK_ACCESS msg=audit(1729738800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
> >   type=SYSCALL msg=audit(1729738800.349:44): arch=c000003e syscall=101 success=no [...] pid=300 auid=0
> > 
> > A following commit adds user documentation.
> > 
> > Add KUnit tests to check reading of domain ID relative to layer level.
> > 
> > The quick return for non-landlocked tasks is moved from task_ptrace() to
> > each LSM hooks.
> > 
> > Because the landlock_log_denial() function is only called when an access
> > is denied, the compiler should be able to optimize the struct
> > landlock_request initializations.  It is not useful to inline the
> > audit_enabled check because other computation are performed anyway, and
> > by the same landlock_log_denia() code.
> > 
> > Use scoped guards for RCU read-side critical sections.
> > 
> > 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/20250131163059.1139617-10-mic@digikod.net
> > ---
> > Changes since v4:
> > - Rename AUDIT_LANDLOCK_DENY to AUDIT_LANDLOCK_ACCESS, requested by
> >   Paul.
> > - Make landlock_log_denial() get Landlock credential instead of Landlock
> >   domain to be able to filter on the domain_exe variable.
> > - Rebase on top of the migration from struct landlock_ruleset to struct
> >   landlock_cred_security.
> > - Rename landlock_init_current_hierarchy() to
> >   landlock_init_hierarchy_log().
> > - Rebase on top of the scoped guard patches.
> > - By default, do not log denials after an execution.
> > - Use scoped guards for RCU read-side critical sections.
> > 
> > Changes since v3:
> > - Extend commit message.
> > 
> > Changes since v2:
> > - Log domain IDs as hexadecimal number: this is a more compact notation
> >   (i.e. at least one less digit), it improves alignment in logs, and it
> >   makes most IDs start with 1 as leading digit (because of the 2^32
> >   minimal value).  Do not use the "0x" prefix that would add useless
> >   data to logs.
> > - Constify function arguments.
> > - Clean up Makefile entries.
> > 
> > Changes since v1:
> > - Move most audit code to this patch.
> > - Rebase on the TCP patch series.
> > - Don't log missing access right: simplify and make it generic for rule
> >   types.
> > - Don't log errno and then don't wrap the error with
> >   landlock_log_request(), as suggested by Jeff.
> > - Add a WARN_ON_ONCE() check to never dereference null pointers.
> > - Only log when audit is enabled.
> > - Don't log task's PID/TID with log_task() because it would be redundant
> >   with the SYSCALL record.
> > - Move the "op" in front and rename "domain" to "denying_domain" to make
> >   it more consistent with other entries.
> > - Don't update the request with the domain ID but add an helper to get
> >   it from the layer masks (and in a following commit with a struct
> >   file).
> > - Revamp get_domain_id_from_layer_masks() into
> >   get_level_from_layer_masks().
> > - For ptrace_traceme, log the parent domain instead of the current one.
> > - Add documentation.
> > - Rename AUDIT_LANDLOCK_DENIAL to AUDIT_LANDLOCK_DENY.
> > - Only log the domain ID and the target task.
> > - Log "blockers", which are either implicit restrictions (e.g. ptrace)
> >   or explicit access rights (e.g. filesystem), or scopes (e.g. signal).
> > - Don't log LSM hook names/operations.
> > - Pick an audit event ID folling the IPE ones.
> > - Add KUnit tests.
> > ---
> >  include/uapi/linux/audit.h  |   3 +-
> >  security/landlock/Makefile  |   5 +-
> >  security/landlock/audit.c   | 146 ++++++++++++++++++++++++++++++++++++
> >  security/landlock/audit.h   |  53 +++++++++++++
> >  security/landlock/domain.c  |  28 +++++++
> >  security/landlock/domain.h  |  22 ++++++
> >  security/landlock/ruleset.c |   6 ++
> >  security/landlock/task.c    |  96 ++++++++++++++++++------
> >  8 files changed, 334 insertions(+), 25 deletions(-)
> >  create mode 100644 security/landlock/audit.c
> >  create mode 100644 security/landlock/audit.h
> >  create mode 100644 security/landlock/domain.c
> 
> Based on previous discussions I'm under the impression that you are
> planning to add a Landlock "permissive" mode at some point in the
> future and based on the comments above you plan to add a "success="
> field to the _ACCESS record defined here.  There is no problem with
> adding fields to an existing record, but the general guidance is that
> new fields need to be added to the end of the record (limitations due
> the the audit userspace and poor guidance in the early days of audit).
> Assuming you are okay with that there is no need to change anything,
> but if you would prefer the "permissive=" field to occur somewhere
> else in the record you may want to consider adding a "permissive=no"
> now.  Otherwise this looks okay from an audit perspective.
> 
> [P.S. I just got to patch 10/24 and saw the enforcing field there,
>  the comments above still stand, but it looks like you chose to note
>  this in the _DOMAIN record, which is fine.]

The mode is indeed specified in the _DOMAIN record.  I think the
syscall's success field should be enough for users in most cases, no
need to duplicate information.

> 
> Acked-by: Paul Moore <paul@...l-moore.com> (Audit)
> 
> --
> paul-moore.com
> 

Powered by blists - more mailing lists