Message-ID: <715d3d6c220bfd818bf50175cf14fd3c@paul-moore.com>
Date: Fri, 14 Feb 2025 17:52:47 -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 v5 9/24] landlock: Add AUDIT_LANDLOCK_ACCESS and log ptrace  denials

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

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

--
paul-moore.com

Powered by blists - more mailing lists