On Fri, Feb 14, 2025 at 05:52:49PM -0500, Paul Moore wrote:

On Jan 31, 2025 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= [off-list ref] wrote:

quoted

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_QUIET flag).  These records are identified with
the new AUDIT_LANDLOCK_DOMAIN type.

The AUDIT_LANDLOCK_DOMAIN message contains:
- the "domain" ID which is described;
- the "status" which can either be "allocated" or "deallocated";
- the "mode" which is for now only "enforcing";
- for the "allocated" status, a minimal set of properties to easily
  identify the task that loaded the domain's policy with
  landlock_restrict_self(2): "pid", "uid", executable path ("exe"), and
  command line ("comm");
- for the "deallocated" state, the number of "denials" accounted to this
  domain, which is at least 1.

This requires each domain to save these task properties at creation
time in the new struct landlock_details.  A reference to the PID is kept
for the lifetime of the domain to avoid race conditions when
investigating the related task.  The executable path is resolved and
stored to not keep a reference to the filesystem and block related
actions.  All these metadata are stored for the lifetime of the related
domain and should then be minimal.  The required memory is not accounted
to the task calling landlock_restrict_self(2) contrary to most other
Landlock allocations (see related comment).

The AUDIT_LANDLOCK_DOMAIN record follows the first AUDIT_LANDLOCK_ACCESS
record for the same domain, which is always followed by AUDIT_SYSCALL
and AUDIT_PROCTITLE.  This is in line with the audit logic to first
record the cause of an event, and then add context with other types of
record.

Audit event sample for a first denial:

  type=LANDLOCK_ACCESS msg=audit(1732186800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
  type=LANDLOCK_DOMAIN msg=audit(1732186800.349:44): domain=195ba459b status=allocated mode=enforcing 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

Audit event sample for a following denial:

  type=LANDLOCK_ACCESS msg=audit(1732186800.372:45): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
  type=SYSCALL msg=audit(1732186800.372:45): arch=c000003e syscall=101 success=no [...] pid=300 auid=0

Log domain deletion with the "deallocated" state when a domain was
previously logged.  This makes it possible for log parsers to free
potential resources when a domain ID will never show again.

The number of denied access requests is useful to easily check how many
access requests a domain blocked and potentially if some of them are
missing in logs because of audit rate limiting or audit rules.  Rate
limiting could also drop this record though.

Audit event sample for a deletion of a domain that denied something:

  type=LANDLOCK_DOMAIN msg=audit(1732186800.393:46): domain=195ba459b status=deallocated denials=2

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/20250131163059.1139617-11-mic@digikod.net (local)
---
Changes since v4:
- Rename AUDIT_LANDLOCK_DOM_{INFO,DROP} to AUDIT_LANDLOCK_DOMAIN and add
  a "status" field, as requested by Paul.
- Add a harcoded "mode=enforcing" to leave room for a potential future
  permissive mode, as suggested by Paul.
- Remove the "creation" timestamp, as suggested by Paul.
- Move LANDLOCK_PATH_MAX_SIZE to domain.h, check the size of the
  greatest landlock_details at build time, and improve comments.
- Improve audit check in landlock_log_drop_domain().
- Add missing headers.
- Fix typo in comment.
- Rebase on top of the landlock_log_denial() and subject type changes.

Changes since v3:
- Log number of denied access requests with AUDIT_LANDLOCK_DOM_DROP
  records, suggested by Tyler.
- Do not store a struct path pointer but the resolved string instead.
  This enables us to not block unmount of the initially restricted task
  executable's mount point.  See the new get_current_info() and
  get_current_exe().  A following patch add tests for this case.
- Create and allocate a new struct landlock_details for initially
  restricted task's information.
- Remove audit_get_ctime() call, as requested by Paul.  We now always
  have a standalone timestamp per Landlock domain creations.
- Fix docstring.

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  |   1 +
 security/landlock/audit.c   |  90 ++++++++++++++++++++++++++++++--
 security/landlock/audit.h   |   7 +++
 security/landlock/domain.c  | 101 ++++++++++++++++++++++++++++++++++++
 security/landlock/domain.h  |  68 ++++++++++++++++++++++++
 security/landlock/ruleset.c |   6 +++
 6 files changed, 270 insertions(+), 3 deletions(-)

Some minor questions below, but from an audit perspective this is okay.

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

quoted

diff --git a/security/landlock/audit.c b/security/landlock/audit.c
index b0dde6bcfb76..a5b055306757 100644
--- a/security/landlock/audit.c
+++ b/security/landlock/audit.c

@@ -8,6 +8,8 @@
 #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"

@@ -31,6 +33,40 @@ 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_DOMAIN);

You use __GFP_NOWARN in the other calls to audit_log_start(), did you
mean to use it here as well?

Good catch, this is at least inconsistent.

My initial though was that we should use __GFP_NOWARN for access
requests that can occure frequently, and common_lsm_audit() already uses
this flag, and the fact that audit events might not be logged at all for
other reasons (e.g. burst of entries).  But __GFP_NOWARN is not use for
all audit_log_start() calls.

For domain allocations and deallocations, this should rarely happen
during the life of a process, and it looked reasonable to warn about an
issue at this point.

Are there guidance about __GFP_NOWARN for audit or other subsystems?

quoted

+	if (!ab)
+		return;
+
+	WARN_ON_ONCE(node->id == 0);
+	audit_log_format(
+		ab,
+		"domain=%llx status=allocated mode=enforcing pid=%d uid=%u exe=",
+		node->id, pid_nr(node->details->pid),
+		from_kuid(&init_user_ns, node->details->cred->uid));
+	audit_log_untrustedstring(ab, node->details->exe_path);
+	audit_log_format(ab, " comm=");
+	audit_log_untrustedstring(ab, node->details->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)
 {

quoted

@@ -106,16 +142,24 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,
 	if (!is_valid_request(request))
 		return;
 
-	if (!unlikely(audit_context() && audit_enabled))
-		return;
-
 	youngest_layer = request->layer_plus_one - 1;
 	youngest_denied = get_hierarchy(subject->domain, youngest_layer);
 
+	/*
+	 * Consistently keeps track of the number of denied access requests
+	 * even if audit is currently disabled, if audit rules currently
+	 * exclude this record type, or if landlock_restrict_self(2)'s flags
+	 * quiet logs.
+	 */
+	atomic64_inc(&youngest_denied->num_denials);
+
 	/* Ignores denials after an execution. */
 	if (!(subject->domain_exec & (1 << youngest_layer)))
 		return;
 
+	if (!unlikely(audit_context() && audit_enabled))
+		return;
+

Not a big deal either way, but it seems like the check above should
probably be in patch 09/24.

It is in 09/24 but it's moved in this patch because we need to count
denials consistently, see a few lines above.

quoted

 	ab = audit_log_start(audit_context(), GFP_ATOMIC | __GFP_NOWARN,
 			     AUDIT_LANDLOCK_ACCESS);
 	if (!ab)

@@ -125,6 +169,46 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,
 	log_blockers(ab, request->type);
 	audit_log_lsm_data(ab, &request->audit);
 	audit_log_end(ab);
+
+	/* Logs this domain if it is the first time. */
+	log_node(youngest_denied);
+}
+
+/**
+ * landlock_log_drop_domain - Create an audit record when a domain is deleted
+ *
+ * @domain: The domain being deleted.
+ *
+ * Only domains which previously appeared in the audit logs are logged again.
+ * This is useful to know when a domain will never show again in the audit log.
+ *
+ * This record is not directly tied to a syscall entry.
+ *
+ * Called by the cred_free() hook, in an uninterruptible context.
+ */
+void landlock_log_drop_domain(const struct landlock_ruleset *const domain)
+{
+	struct audit_buffer *ab;
+
+	if (WARN_ON_ONCE(!domain->hierarchy))
+		return;
+
+	if (!unlikely(audit_enabled))
+		return;

I'm guessing you probably also want to check the audit context given
that you are doing it elsewhere?

The context is NULL when this function is called, I guess because it is
not directly triggered by a syscall but deferred to the domain
deallocation.

quoted

+	/* Ignores domains that were not logged.  */
+	if (READ_ONCE(domain->hierarchy->log_status) != LANDLOCK_LOG_RECORDED)
+		return;
+
+	ab = audit_log_start(audit_context(), GFP_ATOMIC,
+			     AUDIT_LANDLOCK_DOMAIN);
+	if (!ab)
+		return;
+
+	audit_log_format(ab, "domain=%llx status=deallocated denials=%llu",
+			 domain->hierarchy->id,
+			 atomic64_read(&domain->hierarchy->num_denials));
+	audit_log_end(ab);
 }
 
 #ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST

--
paul-moore.com