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

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

On Nov 22, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.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@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-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 ?

--
paul-moore.com