On Fri, Feb 14, 2025 at 05:52:47PM -0500, Paul Moore wrote: > On Jan 31, 2025 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <[email protected]> > 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 <[email protected]> > > Cc: Paul Moore <[email protected]> > > Signed-off-by: Mickaël Salaün <[email protected]> > > Link: https://lore.kernel.org/r/[email protected] > > --- > > 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.]
The mode is indeed specified in the _DOMAIN record. I think the syscall's success field should be enough for users in most cases, no need to duplicate information. > > Acked-by: Paul Moore <[email protected]> (Audit) > > -- > paul-moore.com >
