TODO: Update sample with audit's creation time and now-removed parent
hierarchy

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_LOGLESS flag).  These records are identified by
AUDIT_LANDLOCK_DOM_INFO.

Log domain deletion with the AUDIT_LANDLOCK_DOM_DROP record type when
a domain was previously logged.  This makes it possible for user space
to free potential resources when a domain ID will never show again.

The logged domain properties include:
- the domain ID
- creation time
- its creator's PID
- its creator's UID
- its creator's executable path (exe)
- its creator's command line (comm)

This require each domain to save some task properties at creation time:
time, PID, credentials, exe path, and comm.

It should be noted that we cannot use audit event's serial numbers
because there may not be any related event.  However, it is still useful
to use the same potential timestamp instead of a close one.  What really
matter is how old the related Landlock domain is, not the uniquiness of
the creation time.  If audit is not enabled, Landlock creates its own
timestamp.  This timestamp will be exposed to user space with a future
unprivileged interface.

Audit event sample for a first denial:

  type=LL_DENY msg=audit(1732186800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
  type=LL_DOM_INFO msg=audit(1732186800.349:44): domain=195ba459b creation=1732186800.345 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 logged domains deletion:

  type=LL_DOM_DROP msg=audit(1732186800.393:45): domain=195ba459b

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-11-mic@digikod.net
---

Questions:
- Should we also log the creator's loginuid?
- Should we also log the creator's sessionid?

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   |  2 +
 security/landlock/audit.c    | 74 ++++++++++++++++++++++++++++++++++++
 security/landlock/audit.h    |  7 ++++
 security/landlock/domain.c   | 41 ++++++++++++++++++++
 security/landlock/domain.h   | 43 +++++++++++++++++++++
 security/landlock/ruleset.c  |  7 ++++
 security/landlock/ruleset.h  |  1 +
 security/landlock/syscalls.c |  1 +
 8 files changed, 176 insertions(+)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 60c909c396c0..a72f7b3403be 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -147,6 +147,8 @@
 #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 */
+#define AUDIT_LANDLOCK_DOM_INFO	1424	/* Landlock domain properties */
+#define AUDIT_LANDLOCK_DOM_DROP	1425	/* Landlock domain release */
 
 #define AUDIT_FIRST_KERN_ANOM_MSG   1700
 #define AUDIT_LAST_KERN_ANOM_MSG    1799
diff --git a/security/landlock/audit.c b/security/landlock/audit.c
index eab6b3a8a231..2d0a96797dd4 100644
--- a/security/landlock/audit.c
+++ b/security/landlock/audit.c
@@ -8,8 +8,11 @@
 #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"
 #include "domain.h"
 #include "ruleset.h"
 
@@ -30,6 +33,41 @@ 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_DOM_INFO);
+	if (!ab)
+		return;
+
+	WARN_ON_ONCE(node->id == 0);
+	audit_log_format(ab, "domain=%llx creation=%llu.%03lu pid=%d uid=%u",
+			 node->id,
+			 /* See audit_log_start() */
+			 (unsigned long long)node->creation.tv_sec,
+			 node->creation.tv_nsec / 1000000, pid_nr(node->pid),
+			 from_kuid(&init_user_ns, node->cred->uid));
+	audit_log_d_path(ab, " exe=", &node->exe);
+	audit_log_format(ab, " comm=");
+	audit_log_untrustedstring(ab, node->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)
 {
@@ -116,6 +154,42 @@ void landlock_log_denial(const struct landlock_ruleset *const domain,
 	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
+ *
+ * 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 (!audit_enabled)
+		return;
+
+	/* 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_DOM_DROP);
+	if (!ab)
+		return;
+
+	audit_log_format(ab, "domain=%llx", domain->hierarchy->id);
+	audit_log_end(ab);
 }
 
 #ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
diff --git a/security/landlock/audit.h b/security/landlock/audit.h
index f33095afcd2f..7a1b1652f21b 100644
--- a/security/landlock/audit.h
+++ b/security/landlock/audit.h
@@ -36,11 +36,18 @@ struct landlock_request {
 
 #ifdef CONFIG_AUDIT
 
+void landlock_log_drop_domain(const struct landlock_ruleset *const domain);
+
 void landlock_log_denial(const struct landlock_ruleset *const domain,
 			 const struct landlock_request *const request);
 
 #else /* CONFIG_AUDIT */
 
+static inline void
+landlock_log_drop_domain(const struct landlock_ruleset *const domain)
+{
+}
+
 static inline void
 landlock_log_denial(const struct landlock_ruleset *const domain,
 		    const struct landlock_request *const request)
diff --git a/security/landlock/domain.c b/security/landlock/domain.c
index 965e4dc21975..69b15c059583 100644
--- a/security/landlock/domain.c
+++ b/security/landlock/domain.c
@@ -5,17 +5,58 @@
  * Copyright © 2024 Microsoft Corporation
  */
 
+#include <linux/audit.h>
+#include <linux/cred.h>
+#include <linux/file.h>
+#include <linux/mm.h>
+#include <linux/path.h>
+#include <linux/pid.h>
+#include <linux/sched.h>
+#include <linux/timekeeping.h>
+
 #include "domain.h"
 #include "id.h"
 
+static struct path get_current_exe(void)
+{
+	struct path exe_path = {};
+	struct file *exe_file;
+	struct mm_struct *mm = current->mm;
+
+	if (!mm)
+		return exe_path;
+
+	exe_file = get_mm_exe_file(mm);
+	if (!exe_file)
+		return exe_path;
+
+	exe_path = exe_file->f_path;
+	path_get(&exe_path);
+	fput(exe_file);
+	return exe_path;
+}
+
 /**
  * landlock_init_current_hierarchy - Partially initialize landlock_hierarchy
  *
  * @hierarchy: The hierarchy to initialize.
  *
+ * The current task is referenced as the domain creator.  The subjective
+ * credentials must not be in an overridden state.
+ *
  * @hierarchy->parent and @hierarchy->usage should already be set.
  */
 void landlock_init_current_hierarchy(struct landlock_hierarchy *const hierarchy)
 {
+	hierarchy->creation = audit_get_ctime(audit_context());
+	if (!hierarchy->creation.tv_sec)
+		ktime_get_coarse_real_ts64(&hierarchy->creation);
+
+	hierarchy->log_status = LANDLOCK_LOG_PENDING;
 	hierarchy->id = landlock_get_id(1);
+	WARN_ON_ONCE(current_cred() != current_real_cred());
+	hierarchy->cred = get_current_cred();
+	hierarchy->pid = get_pid(task_pid(current));
+	hierarchy->exe = get_current_exe();
+	get_task_comm(hierarchy->comm, current);
 }
diff --git a/security/landlock/domain.h b/security/landlock/domain.h
index f82d831ca0a7..4a2864a66047 100644
--- a/security/landlock/domain.h
+++ b/security/landlock/domain.h
@@ -10,8 +10,18 @@
 #ifndef _SECURITY_LANDLOCK_DOMAIN_H
 #define _SECURITY_LANDLOCK_DOMAIN_H
 
+#include <linux/cred.h>
 #include <linux/mm.h>
+#include <linux/path.h>
+#include <linux/pid.h>
 #include <linux/refcount.h>
+#include <linux/sched.h>
+#include <linux/time64.h>
+
+enum landlock_log_status {
+	LANDLOCK_LOG_PENDING = 0,
+	LANDLOCK_LOG_RECORDED,
+};
 
 /**
  * struct landlock_hierarchy - Node in a domain hierarchy
@@ -29,10 +39,37 @@ struct landlock_hierarchy {
 	refcount_t usage;
 
 #ifdef CONFIG_AUDIT
+	/**
+	 * @log_status: Whether this domain should be logged or not.  Because
+	 * concurrent log entries may be created at the same time, it is still
+	 * possible to have several domain records of the same domain.
+	 */
+	enum landlock_log_status log_status;
 	/**
 	 * @id: Landlock domain ID, sets once at domain creation time.
 	 */
 	u64 id;
+	/**
+	 * @creation: Time of the domain creation (i.e. syscall entry as used
+	 * in audit context).
+	 */
+	struct timespec64 creation;
+	/**
+	 * @cred: Credential of the domain's creator.
+	 */
+	const struct cred *cred;
+	/**
+	 * @pid: Creator's PID.
+	 */
+	struct pid *pid;
+	/**
+	 * @exe: Creator's exe path.
+	 */
+	struct path exe;
+	/**
+	 * @comm: Command line of the domain's creator at creation time.
+	 */
+	char comm[TASK_COMM_LEN];
 #endif /* CONFIG_AUDIT */
 };
 
@@ -48,6 +85,12 @@ static inline void landlock_put_hierarchy(struct landlock_hierarchy *hierarchy)
 	while (hierarchy && refcount_dec_and_test(&hierarchy->usage)) {
 		const struct landlock_hierarchy *const freeme = hierarchy;
 
+#ifdef CONFIG_AUDIT
+		put_cred(hierarchy->cred);
+		put_pid(hierarchy->pid);
+		path_put(&hierarchy->exe);
+#endif /* CONFIG_AUDIT */
+
 		hierarchy = hierarchy->parent;
 		kfree(freeme);
 	}
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 7a88fd9b2473..4619c7f63d05 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -508,6 +508,9 @@ static void free_ruleset_work(struct work_struct *const work)
 void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset)
 {
 	if (ruleset && refcount_dec_and_test(&ruleset->usage)) {
+		/* Logs with the current context. */
+		landlock_log_drop_domain(ruleset);
+
 		INIT_WORK(&ruleset->work_free, free_ruleset_work);
 		schedule_work(&ruleset->work_free);
 	}
@@ -519,6 +522,10 @@ void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset)
  * @parent: Parent domain.
  * @ruleset: New ruleset to be merged.
  *
+ * The current task is referenced as the domain creator.  The subjective
+ * credentials must not be in an overridden state (see
+ * landlock_init_current_hierarchy).
+ *
  * Returns the intersection of @parent and @ruleset, or returns @parent if
  * @ruleset is empty, or returns a duplicate of @ruleset if @parent is empty.
  */
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index 39169b6860e3..73636f214aac 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -12,6 +12,7 @@
 #include <linux/mutex.h>
 #include <linux/rbtree.h>
 #include <linux/refcount.h>
+#include <linux/sched.h>
 #include <linux/workqueue.h>
 
 #include "access.h"
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index c097d356fa45..335067e36feb 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -26,6 +26,7 @@
 #include <linux/uaccess.h>
 #include <uapi/linux/landlock.h>
 
+#include "audit.h"
 #include "cred.h"
 #include "fs.h"
 #include "limits.h"
-- 
2.47.0

On Nov 22, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> 
> TODO: Update sample with audit's creation time and now-removed parent
> hierarchy
> 
> 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_LOGLESS flag).  These records are identified by
> AUDIT_LANDLOCK_DOM_INFO.
> 
> Log domain deletion with the AUDIT_LANDLOCK_DOM_DROP record type when
> a domain was previously logged.  This makes it possible for user space
> to free potential resources when a domain ID will never show again.
> 
> The logged domain properties include:
> - the domain ID
> - creation time
> - its creator's PID
> - its creator's UID
> - its creator's executable path (exe)
> - its creator's command line (comm)
> 
> This require each domain to save some task properties at creation time:
> time, PID, credentials, exe path, and comm.
> 
> It should be noted that we cannot use audit event's serial numbers
> because there may not be any related event.  However, it is still useful
> to use the same potential timestamp instead of a close one.  What really
> matter is how old the related Landlock domain is, not the uniquiness of
> the creation time.  If audit is not enabled, Landlock creates its own
> timestamp.  This timestamp will be exposed to user space with a future
> unprivileged interface.
> 
> Audit event sample for a first denial:
> 
>   type=LL_DENY msg=audit(1732186800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
>   type=LL_DOM_INFO msg=audit(1732186800.349:44): domain=195ba459b creation=1732186800.345 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

As mentioned in patch 9/23, I don't want subsystems external to audit
to access the audit timestamp information, so the "creation=" field
in the audit event would need to be removed.  Assuming that the timestamp
was used either to reference the original domain creation and/or simply
provide some additional information for analysis, all of that information
should already be in the audit log, assuming of course that you are
logging domain creation (which you should, at least as an option).

Also, is there a good reason why the LL_DOM_INFO information can't be
recorded in the LL_DENY (or LL_ACCESS) record?  I think that would be
preferable.

> Audit event sample for logged domains deletion:
> 
>   type=LL_DOM_DROP msg=audit(1732186800.393:45): domain=195ba459b
> 
> 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-11-mic@digikod.net
> ---
> Questions:
> - Should we also log the creator's loginuid?
> - Should we also log the creator's sessionid?

Creation of a Landlock domain can only happen through the Landlock
syscalls, yes?  If so, that information should already be logged in
the associated syscall record (see the "auid=" and "ses=" fields )and
we generally try to avoid duplicating information across records in
the same audit event.

> 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   |  2 +
>  security/landlock/audit.c    | 74 ++++++++++++++++++++++++++++++++++++
>  security/landlock/audit.h    |  7 ++++
>  security/landlock/domain.c   | 41 ++++++++++++++++++++
>  security/landlock/domain.h   | 43 +++++++++++++++++++++
>  security/landlock/ruleset.c  |  7 ++++
>  security/landlock/ruleset.h  |  1 +
>  security/landlock/syscalls.c |  1 +
>  8 files changed, 176 insertions(+)
> 
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 60c909c396c0..a72f7b3403be 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -147,6 +147,8 @@
>  #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 */
> +#define AUDIT_LANDLOCK_DOM_INFO	1424	/* Landlock domain properties */
> +#define AUDIT_LANDLOCK_DOM_DROP	1425	/* Landlock domain release */

See my comment about regarding AUDIT_LANDLOCK_DOM_INFO.

Similar to my previous comments regarding AUDIT_LANDLOCK_DENY, it might
be a good idea to change AUDIT_LANDLOCK_DOM_DROP to simply
AUDIT_LANDLOCK_DOM and use an "op=" field to indicate a drop, creation,
or other event.

> diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> index eab6b3a8a231..2d0a96797dd4 100644
> --- a/security/landlock/audit.c
> +++ b/security/landlock/audit.c
> @@ -8,8 +8,11 @@
>  #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"
>  #include "domain.h"
>  #include "ruleset.h"
>  
> @@ -30,6 +33,41 @@ 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_DOM_INFO);
> +	if (!ab)
> +		return;
> +
> +	WARN_ON_ONCE(node->id == 0);

It seems like you might want to move the WARN_ON assertion up with the
other WARN_ON check?

> +	audit_log_format(ab, "domain=%llx creation=%llu.%03lu pid=%d uid=%u",
> +			 node->id,
> +			 /* See audit_log_start() */
> +			 (unsigned long long)node->creation.tv_sec,
> +			 node->creation.tv_nsec / 1000000, pid_nr(node->pid),
> +			 from_kuid(&init_user_ns, node->cred->uid));
> +	audit_log_d_path(ab, " exe=", &node->exe);
> +	audit_log_format(ab, " comm=");
> +	audit_log_untrustedstring(ab, node->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)
>  {

--
paul-moore.com

On Sat, Jan 04, 2025 at 08:23:51PM -0500, Paul Moore wrote:
> On Nov 22, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> > 
> > TODO: Update sample with audit's creation time and now-removed parent
> > hierarchy
> > 
> > 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_LOGLESS flag).  These records are identified by
> > AUDIT_LANDLOCK_DOM_INFO.
> > 
> > Log domain deletion with the AUDIT_LANDLOCK_DOM_DROP record type when
> > a domain was previously logged.  This makes it possible for user space
> > to free potential resources when a domain ID will never show again.
> > 
> > The logged domain properties include:
> > - the domain ID
> > - creation time
> > - its creator's PID
> > - its creator's UID
> > - its creator's executable path (exe)
> > - its creator's command line (comm)
> > 
> > This require each domain to save some task properties at creation time:
> > time, PID, credentials, exe path, and comm.
> > 
> > It should be noted that we cannot use audit event's serial numbers
> > because there may not be any related event.  However, it is still useful
> > to use the same potential timestamp instead of a close one.  What really
> > matter is how old the related Landlock domain is, not the uniquiness of
> > the creation time.  If audit is not enabled, Landlock creates its own
> > timestamp.  This timestamp will be exposed to user space with a future
> > unprivileged interface.
> > 
> > Audit event sample for a first denial:
> > 
> >   type=LL_DENY msg=audit(1732186800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
> >   type=LL_DOM_INFO msg=audit(1732186800.349:44): domain=195ba459b creation=1732186800.345 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
> 
> As mentioned in patch 9/23, I don't want subsystems external to audit
> to access the audit timestamp information, so the "creation=" field
> in the audit event would need to be removed.  Assuming that the timestamp
> was used either to reference the original domain creation and/or simply
> provide some additional information for analysis, all of that information
> should already be in the audit log, assuming of course that you are
> logging domain creation (which you should, at least as an option).

As explained in this patch, we don't want to (and cannot realistically)
log domain creations.  That would make the audit support for Landlock
unusable.  Moreover, these information is useless and only add noise
unless there is a denial, hence this asynchronous approach.  However,
users may want to log some syscalls, including landlock_restrict_self(),
and it would make audit logs more consistent using the same timestamp as
the Landlock domain creation time.  I'm wondering why exposing this
timestamp to the kernel would be an issue whereas it is already exposed
to user space.

If you're really opposed to it I can create a new unrelated timestamp
specific to Landlock.

> 
> Also, is there a good reason why the LL_DOM_INFO information can't be
> recorded in the LL_DENY (or LL_ACCESS) record?  I think that would be
> preferable.

The goal of the standalone LL_DOM_INFO record type is to limit useless
log verbosity.  Including this information in LL_DENY would have two
downsides:
- it would increases the length of *all* LL_DENY messages
- it would make it more difficult to extend this new mixed messages with
  access-related informations (e.g. file property) and domain-related
  informations (and associate them with either the object or the
  domain).

I prefer to have clear semantic with distinct and dedicated audit record
types.  Relying on different record type also enable users to
efficiently filter them.

> 
> > Audit event sample for logged domains deletion:
> > 
> >   type=LL_DOM_DROP msg=audit(1732186800.393:45): domain=195ba459b
> > 
> > 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-11-mic@digikod.net
> > ---
> > Questions:
> > - Should we also log the creator's loginuid?
> > - Should we also log the creator's sessionid?
> 
> Creation of a Landlock domain can only happen through the Landlock
> syscalls, yes?  If so, that information should already be logged in
> the associated syscall record (see the "auid=" and "ses=" fields )and
> we generally try to avoid duplicating information across records in
> the same audit event.

The specificity of Landlock compared to existing supported systems is
that we cannot log domain creation for the reason I explain before.
We might extend the asynchronous LL_DOM_INFO message with such
information in the future though.

> 
> > 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   |  2 +
> >  security/landlock/audit.c    | 74 ++++++++++++++++++++++++++++++++++++
> >  security/landlock/audit.h    |  7 ++++
> >  security/landlock/domain.c   | 41 ++++++++++++++++++++
> >  security/landlock/domain.h   | 43 +++++++++++++++++++++
> >  security/landlock/ruleset.c  |  7 ++++
> >  security/landlock/ruleset.h  |  1 +
> >  security/landlock/syscalls.c |  1 +
> >  8 files changed, 176 insertions(+)
> > 
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 60c909c396c0..a72f7b3403be 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -147,6 +147,8 @@
> >  #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 */
> > +#define AUDIT_LANDLOCK_DOM_INFO	1424	/* Landlock domain properties */
> > +#define AUDIT_LANDLOCK_DOM_DROP	1425	/* Landlock domain release */
> 
> See my comment about regarding AUDIT_LANDLOCK_DOM_INFO.
> 
> Similar to my previous comments regarding AUDIT_LANDLOCK_DENY, it might
> be a good idea to change AUDIT_LANDLOCK_DOM_DROP to simply
> AUDIT_LANDLOCK_DOM and use an "op=" field to indicate a drop, creation,
> or other event.

Using a dedicated audit record type enables users to efficiently filter
according to their type, and (in this specific case, sightly) reduce log
size.

> 
> > diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> > index eab6b3a8a231..2d0a96797dd4 100644
> > --- a/security/landlock/audit.c
> > +++ b/security/landlock/audit.c
> > @@ -8,8 +8,11 @@
> >  #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"
> >  #include "domain.h"
> >  #include "ruleset.h"
> >  
> > @@ -30,6 +33,41 @@ 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_DOM_INFO);
> > +	if (!ab)
> > +		return;
> > +
> > +	WARN_ON_ONCE(node->id == 0);
> 
> It seems like you might want to move the WARN_ON assertion up with the
> other WARN_ON check?

I wanted to limit this check but only do it before actually using this
ID.

> 
> > +	audit_log_format(ab, "domain=%llx creation=%llu.%03lu pid=%d uid=%u",
> > +			 node->id,
> > +			 /* See audit_log_start() */
> > +			 (unsigned long long)node->creation.tv_sec,
> > +			 node->creation.tv_nsec / 1000000, pid_nr(node->pid),
> > +			 from_kuid(&init_user_ns, node->cred->uid));
> > +	audit_log_d_path(ab, " exe=", &node->exe);
> > +	audit_log_format(ab, " comm=");
> > +	audit_log_untrustedstring(ab, node->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)
> >  {
> 
> --
> paul-moore.com
> 

On Mon, Jan 6, 2025 at 9:51 AM Mickaël Salaün <mic@digikod.net> wrote:
> On Sat, Jan 04, 2025 at 08:23:51PM -0500, Paul Moore wrote:
> > On Nov 22, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:

...

> > > Audit event sample for a first denial:
> > >
> > >   type=LL_DENY msg=audit(1732186800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
> > >   type=LL_DOM_INFO msg=audit(1732186800.349:44): domain=195ba459b creation=1732186800.345 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
> >
> > As mentioned in patch 9/23, I don't want subsystems external to audit
> > to access the audit timestamp information, so the "creation=" field
> > in the audit event would need to be removed.  Assuming that the timestamp
> > was used either to reference the original domain creation and/or simply
> > provide some additional information for analysis, all of that information
> > should already be in the audit log, assuming of course that you are
> > logging domain creation (which you should, at least as an option).
>
> As explained in this patch, we don't want to (and cannot realistically)
> log domain creations.  That would make the audit support for Landlock
> unusable.  Moreover, these information is useless and only add noise
> unless there is a denial, hence this asynchronous approach.

That's fine, just know that it doesn't change my thoughts on exposing
the audit timestamp.

> However,
> users may want to log some syscalls, including landlock_restrict_self(),
> and it would make audit logs more consistent using the same timestamp as
> the Landlock domain creation time.  I'm wondering why exposing this
> timestamp to the kernel would be an issue whereas it is already exposed
> to user space.

Currently there are no other users of the audit timestamp besides
audit.  Making the audit timestamp available to other subsystems makes
the timestamp less flexible over the long term as it would become, in
a way, part of the API that audit provides to other in-kernel users.

I still have hopes to rework a large chunk of the audit subsystem, and
keeping the interfaces between audit and the other in-kernel
subsystems makes that easier.

> If you're really opposed to it I can create a new unrelated timestamp
> specific to Landlock.

Yes, at this point in time I don't want to support exporting the audit
timestamp outside of audit.  My guess is that you probably want to use
some identifier, other than a timestamp, when trying to link Landlock
events (presumably the domain ID would do this?), but I don't pretend
to know the details of Landlock very well right now.

> > Also, is there a good reason why the LL_DOM_INFO information can't be
> > recorded in the LL_DENY (or LL_ACCESS) record?  I think that would be
> > preferable.
>
> The goal of the standalone LL_DOM_INFO record type is to limit useless
> log verbosity.  Including this information in LL_DENY would have two
> downsides:
> - it would increases the length of *all* LL_DENY messages

Are you ever going to emit a LL_ACCESS/LL_DENY record without a
LL_DOM_INFO record?

> - it would make it more difficult to extend this new mixed messages with
>   access-related informations (e.g. file property) and domain-related
>   informations (and associate them with either the object or the
>   domain).

How?  Please elaborate on this.

> > > Audit event sample for logged domains deletion:
> > >
> > >   type=LL_DOM_DROP msg=audit(1732186800.393:45): domain=195ba459b
> > >
> > > 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-11-mic@digikod.net
> > > ---
> > > Questions:
> > > - Should we also log the creator's loginuid?
> > > - Should we also log the creator's sessionid?
> >
> > Creation of a Landlock domain can only happen through the Landlock
> > syscalls, yes?  If so, that information should already be logged in
> > the associated syscall record (see the "auid=" and "ses=" fields )and
> > we generally try to avoid duplicating information across records in
> > the same audit event.
>
> The specificity of Landlock compared to existing supported systems is
> that we cannot log domain creation for the reason I explain before.

Can you provide a link to that explanation?  I'm sure you explained it
well, but I missed it when going over the patchset with a focus on
audit.

If the Landlock domain is created independent from any user/process
action, it likely doesn't make sense to log either the loginuid or
sessionid since the domain creation is happening independently from a
user session.

-- 
paul-moore.com

On Mon, Jan 06, 2025 at 04:56:50PM -0500, Paul Moore wrote:
> On Mon, Jan 6, 2025 at 9:51 AM Mickaël Salaün <mic@digikod.net> wrote:
> > On Sat, Jan 04, 2025 at 08:23:51PM -0500, Paul Moore wrote:
> > > On Nov 22, 2024 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <mic@digikod.net> wrote:
> 
> ...
> 
> > > > Audit event sample for a first denial:
> > > >
> > > >   type=LL_DENY msg=audit(1732186800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
> > > >   type=LL_DOM_INFO msg=audit(1732186800.349:44): domain=195ba459b creation=1732186800.345 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
> > >
> > > As mentioned in patch 9/23, I don't want subsystems external to audit
> > > to access the audit timestamp information, so the "creation=" field
> > > in the audit event would need to be removed.  Assuming that the timestamp
> > > was used either to reference the original domain creation and/or simply
> > > provide some additional information for analysis, all of that information
> > > should already be in the audit log, assuming of course that you are
> > > logging domain creation (which you should, at least as an option).
> >
> > As explained in this patch, we don't want to (and cannot realistically)
> > log domain creations.  That would make the audit support for Landlock
> > unusable.  Moreover, these information is useless and only add noise
> > unless there is a denial, hence this asynchronous approach.
> 
> That's fine, just know that it doesn't change my thoughts on exposing
> the audit timestamp.
> 
> > However,
> > users may want to log some syscalls, including landlock_restrict_self(),
> > and it would make audit logs more consistent using the same timestamp as
> > the Landlock domain creation time.  I'm wondering why exposing this
> > timestamp to the kernel would be an issue whereas it is already exposed
> > to user space.
> 
> Currently there are no other users of the audit timestamp besides
> audit.  Making the audit timestamp available to other subsystems makes
> the timestamp less flexible over the long term as it would become, in
> a way, part of the API that audit provides to other in-kernel users.
> 
> I still have hopes to rework a large chunk of the audit subsystem, and
> keeping the interfaces between audit and the other in-kernel
> subsystems makes that easier.

OK

> 
> > If you're really opposed to it I can create a new unrelated timestamp
> > specific to Landlock.
> 
> Yes, at this point in time I don't want to support exporting the audit
> timestamp outside of audit.  My guess is that you probably want to use
> some identifier, other than a timestamp, when trying to link Landlock
> events (presumably the domain ID would do this?), but I don't pretend
> to know the details of Landlock very well right now.

Correct, Landlock domain IDs are used to tie domain creation, denial,
and destruction/drop events (and their use will be extended to user
space in the future).

> 
> > > Also, is there a good reason why the LL_DOM_INFO information can't be
> > > recorded in the LL_DENY (or LL_ACCESS) record?  I think that would be
> > > preferable.
> >
> > The goal of the standalone LL_DOM_INFO record type is to limit useless
> > log verbosity.  Including this information in LL_DENY would have two
> > downsides:
> > - it would increases the length of *all* LL_DENY messages
> 
> Are you ever going to emit a LL_ACCESS/LL_DENY record without a
> LL_DOM_INFO record?

Yes, only the first LL_DENY (for a domain) emits an LL_DOM_INFO (for
this same domain), which is why this design is interesting: creation of
domains can happen at a high frequency (e.g. script executing a
sandboxed program in a loop, or just build a kernel with sandbox
compilers), and logging every domain creation would make 99% of these
events useless.  See log_status's LANDLOCK_LOG_RECORDED in log_node() in
this patch.

> 
> > - it would make it more difficult to extend this new mixed messages with
> >   access-related informations (e.g. file property) and domain-related
> >   informations (and associate them with either the object or the
> >   domain).
> 
> How?  Please elaborate on this.

I mean that appending intertwined (i.e. some might be related to define
a domain whereas others might be related to define an object) and
optional (e.g. a file object and a socket object are not defined the
same way) new fields to one message type makes the message less
predictable and more difficult to parse.

> 
> > > > Audit event sample for logged domains deletion:
> > > >
> > > >   type=LL_DOM_DROP msg=audit(1732186800.393:45): domain=195ba459b
> > > >
> > > > 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-11-mic@digikod.net
> > > > ---
> > > > Questions:
> > > > - Should we also log the creator's loginuid?
> > > > - Should we also log the creator's sessionid?
> > >
> > > Creation of a Landlock domain can only happen through the Landlock
> > > syscalls, yes?  If so, that information should already be logged in
> > > the associated syscall record (see the "auid=" and "ses=" fields )and
> > > we generally try to avoid duplicating information across records in
> > > the same audit event.
> >
> > The specificity of Landlock compared to existing supported systems is
> > that we cannot log domain creation for the reason I explain before.
> 
> Can you provide a link to that explanation?  I'm sure you explained it
> well, but I missed it when going over the patchset with a focus on
> audit.

That wasn't clear enough, I'll include the previous description in the
next series, but the basic design idea is defined in the cover letter:
https://lore.kernel.org/all/20241122143353.59367-1-mic@digikod.net/

> 
> If the Landlock domain is created independent from any user/process
> action, it likely doesn't make sense to log either the loginuid or
> sessionid since the domain creation is happening independently from a
> user session.

Landlock domain creations are a process action.  What we want to log are
the denials and a minimal context (e.g. which task created the related
domain).  I was wondering if we should (right now) include loginuid or
sessionid in this (asynchronous) context.