linux-kernel - Re: [PATCH v3 8/23] landlock: Log ptrace denials

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250106.wohB1leet2vu@digikod.net>
Date: Mon, 6 Jan 2025 15:45:10 +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 8/23] landlock: Log ptrace denials

On Sat, Jan 04, 2025 at 08:23:49PM -0500, Paul Moore wrote:
> On Nov 22, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@...ikod.net> wrote:
> > 
> > Add audit support to ptrace_access_check and ptrace_traceme hooks.
> > 
> > Add a new AUDIT_LANDLOCK_DENY record type dedicated to any Landlock
> > denials.
> > 
> > Log the domain ID restricting the action, the domain's blockers that are
> > missing to allow the requested access, and the target task.
> > 
> > The blockers are implicit restrictions (e.g. ptrace), or explicit access
> > rights (e.g. filesystem), or explicit scopes (e.g. signal).
> > 
> > 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.
> > 
> > The quick return for non-landlocked tasks is moved from task_ptrace() to
> > each LSM hooks.
> > 
> > Audit event sample:
> > 
> >   type=LL_DENY msg=audit(1732186800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
> >   type=SYSCALL msg=audit(1732186800.349:44): arch=c000003e syscall=101 success=no [...] pid=300 auid=0
> > 
> > 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-9-mic@digikod.net
> > ---
> > 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.
> > 
> > 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  |   2 +-
> >  security/landlock/audit.c   | 137 ++++++++++++++++++++++++++++++++++++
> >  security/landlock/audit.h   |  52 ++++++++++++++
> >  security/landlock/domain.c  |  21 ++++++
> >  security/landlock/domain.h  |  17 +++++
> >  security/landlock/ruleset.c |   3 +
> >  security/landlock/task.c    |  91 ++++++++++++++++++------
> >  8 files changed, 302 insertions(+), 24 deletions(-)
> >  create mode 100644 security/landlock/audit.c
> >  create mode 100644 security/landlock/audit.h
> >  create mode 100644 security/landlock/domain.c
> > 
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 75e21a135483..60c909c396c0 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -33,7 +33,7 @@
> >   * 1100 - 1199 user space trusted application messages
> >   * 1200 - 1299 messages internal to the audit daemon
> >   * 1300 - 1399 audit event messages
> > - * 1400 - 1499 SE Linux use
> > + * 1400 - 1499 access control messages
> 
> Thank you :)
> 
> I'm also reminded once again that the original audit devs stubbornly
> used "SE Linux" instead of "SELinux" :/
> 
> >   * 1500 - 1599 kernel LSPP events
> >   * 1600 - 1699 kernel crypto events
> >   * 1700 - 1799 kernel anomaly records
> > @@ -146,6 +146,7 @@
> >  #define AUDIT_IPE_ACCESS	1420	/* IPE denial or grant */
> >  #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 */
> 
> Generally speaking, we don't really encode denial/allowed verdicts into
> the audit record type, instead we ask that developers use a field like
> "access=" to indicate that an action was allowed or denied.
> 
> How about AUDIT_LANDLOCK_ACCESS ?

A stronger type with the "denied" semantic makes more sense to me,
especially for Landlock which is unprivileged, and it makes it clear
that it should only impact performance and log size (i.e. audit log
creation) for denied actions.  Having dedicated record types enables
users to easily and efficiently filter log with audit rules.
AUDIT_LANDLOCK_DENY is also a clear signal that something unexpected is
ongoing (see the LANDLOCK_RESTRICT_SELF_LOGLESS flag).  The next patch
series will also contain a new kind of audit rule to specifically
identify the origin of the policy that created this denied event, which
should make more sense.

Because of its unprivileged nature, Landlock will never log granted
accesses by default.  In the future, we might want a permissive-like
mode for Landlock, but this will be optional, and I would also strongly
prefer to add new audit record types for new semantics.  That would also
help log parsers to stick to the current deny-only semantic and avoid
misinterpretations or even noise from debug/test log entries.