Hi,
This patch series adds audit support to Landlock.
Logging denied requests is useful for different use cases:
- sysadmins: to look for users' issues,
- security experts: to detect attack attempts,
- power users: to understand denials,
- developers: to ease sandboxing support and get feedback from users.
Because of its unprivileged nature, Landlock can compose standalone
security policies (i.e. domains). To make logs useful, they need to
contain the most relevant Landlock domain that denied an action, and the
reason of such denial. This translates to the latest nested domain and
the related blockers: missing access rights or other kind of
restrictions.
# Changes from previous version
This fourth patch series mainly adds a new AUDIT_EXE_LANDLOCK_DENY rule
type to filter Landlock denials according to the executable that loaded
the policy responsible for this restriction. New tests are added on top
of that.
Domain's metadata are now stored in a dedicated struct landlock_details
that contains the resolved exe's path, because we cannot keep a
reference to the related struct path. This fixes umount of the
mount point containing a binary that restricted itself (if the domain is
still alive). Add a dedicated test to check this issue.
Formatting of blockers are slightly improved.
Audit timestamps are no longer exported but dedicated Landlock
timestamps are use instead for domain creation.
The new landlock_restrict_self()'s flag is renamed to
LANDLOCK_RESTRICT_SELF_QUIET.
# Design
Log records are created for any denied actions caused by a Landlock
policy, which means that a well-sandboxed applications should not log
anything except for unattended access requests that might be the result
of attacks or bugs.
However, sandbox tools creating restricted environments could lead to
abundant log entries because the sandboxed processes may not be aware of
the related restrictions. To avoid log spam, the
landlock_restrict_self(2) syscall gets a new
LANDLOCK_RESTRICT_SELF_QUIET flag to not log denials related to this
specific domain. Except for well-understood exceptions, this flag
should not be set. Indeed, applications sandboxing themselves should
only try to bypass their own sandbox if they are compromised, which
should ring a bell thanks to log events.
When an action is denied, the related Landlock domain ID is specified.
If this domain was not previously described in a log record, one is
created. This record contains the domain ID, its creation time, and
informations about the process that enforced the restriction (at the
time of the call to landlock_restrict_self): PID, UID, executable path,
and name (comm).
This new approach also brings building blocks for an upcoming
unprivileged introspection interface. The unique Landlock IDs will be
useful to tie audit log entries to running processes, and to get
properties of the related Landlock domains. This will replace the
previously logged ruleset properties.
# Samples
Here are two examples of log events (see serial numbers):
$ LL_FS_RO=/ LL_FS_RW=/ LL_SCOPED=s LL_FORCE_LOG=1 ./sandboxer kill 1
type=LANDLOCK_DENY msg=audit(1729738800.268:30): domain=1a6fdc66f blockers=scope.signal opid=1 ocomm="systemd"
type=LANDLOCK_DOM_INFO msg=audit(1729738800.268:30): domain=1a6fdc66f creation=1729738800.264 pid=286 uid=0 exe="/root/sandboxer" comm="sandboxer"UID="root"
type=SYSCALL msg=audit(1729738800.268:30): arch=c000003e syscall=62 success=no exit=-1 [..] ppid=272 pid=286 auid=0 uid=0 gid=0 [...] comm="kill" [...]
type=PROCTITLE msg=audit(1729738800.268:30): proctitle=6B696C6C0031
type=LANDLOCK_DOM_DROP msg=audit(1729738800.324:31): domain=1a6fdc66f denials=1
$ LL_FS_RO=/ LL_FS_RW=/tmp LL_FORCE_LOG=1 ./sandboxer sh -c "echo > /etc/passwd"
type=LANDLOCK_DENY msg=audit(1729738800.221:33): domain=1a6fdc679 blockers=fs.write_file path="/dev/tty" dev="devtmpfs" ino=9
type=LANDLOCK_DOM_INFO msg=audit(1729738800.221:33): domain=1a6fdc679 creation=1729738800.217 pid=289 uid=0 exe="/root/sandboxer" comm="sandboxer"UID="root"
type=SYSCALL msg=audit(1729738800.221:33): arch=c000003e syscall=257 success=no exit=-13 [...] ppid=272 pid=289 auid=0 uid=0 gid=0 [...] comm="sh" [...]
type=PROCTITLE msg=audit(1729738800.221:33): proctitle=7368002D63006563686F203E202F6574632F706173737764
type=LANDLOCK_DENY msg=audit(1729738800.221:34): domain=1a6fdc679 blockers=fs.write_file path="/etc/passwd" dev="vda2" ino=143821
type=SYSCALL msg=audit(1729738800.221:34): arch=c000003e syscall=257 success=no exit=-13 [...] ppid=272 pid=289 auid=0 uid=0 gid=0 [...] comm="sh" [...]
type=PROCTITLE msg=audit(1729738800.221:34): proctitle=7368002D63006563686F203E202F6574632F706173737764
type=LANDLOCK_DOM_DROP msg=audit(1729738800.261:35): domain=1a6fdc679 denials=2
# Future changes
I'll add more tests to check each kind of denied access.
We might want to add new audit rule types to filter according to other
domain properties (e.g. UID, AUID, session ID), but
AUDIT_EXE_LANDLOCK_DENY should be enough to mute buggy programs before
fixing them.
# Previous versions
v3: https://lore.kernel.org/r/20241122143353.59367-1-mic@digikod.net
v2: https://lore.kernel.org/r/20241022161009.982584-1-mic@digikod.net
v1: https://lore.kernel.org/r/20230921061641.273654-1-mic@digikod.net
Regards,
Mickaël Salaün (30):
lsm: Only build lsm_audit.c if CONFIG_SECURITY and CONFIG_AUDIT are
set
lsm: Add audit_log_lsm_data() helper
landlock: Factor out check_access_path()
landlock: Add unique ID generator
landlock: Move access types
landlock: Simplify initially denied access rights
landlock: Move domain hierarchy management and export helpers
landlock: Add AUDIT_LANDLOCK_DENY and log ptrace denials
landlock: Add AUDIT_LANDLOCK_DOM_{INFO,DROP} and log domain properties
landlock: Log mount-related denials
landlock: Align partial refer access checks with final ones
selftests/landlock: Add test to check partial access in a mount tree
landlock: Optimize file path walks and prepare for audit support
landlock: Log file-related denials
landlock: Log truncate and IOCTL denials
landlock: Log TCP bind and connect denials
landlock: Log scoped denials
landlock: Control log events with LANDLOCK_RESTRICT_SELF_QUIET
samples/landlock: Do not log denials from the sandboxer by default
selftests/landlock: Fix error message
selftests/landlock: Add wrappers.h
selftests/landlock: Add layout1.umount_sandboxer tests
selftests/landlock: Extend tests for landlock_restrict_self()'s flags
selftests/landlock: Add tests for audit and
LANDLOCK_RESTRICT_SELF_QUIET
selftests/landlock: Add audit tests for ptrace
landlock: Export and rename landlock_get_inode_object()
fs: Add iput() cleanup helper
audit,landlock: Add AUDIT_EXE_LANDLOCK_DENY rule type
selftests/landlock: Test audit rule with AUDIT_EXE_LANDLOCK_DOM
selftests/landlock: Test compatibility with audit rule lists
Documentation/userspace-api/landlock.rst | 2 +-
MAINTAINERS | 1 +
include/linux/audit.h | 11 +
include/linux/fs.h | 6 +-
include/linux/landlock.h | 41 ++
include/linux/lsm_audit.h | 22 +
include/uapi/linux/audit.h | 6 +-
include/uapi/linux/landlock.h | 14 +
kernel/audit.c | 4 +-
kernel/audit.h | 5 +-
kernel/auditfilter.c | 30 +-
kernel/auditsc.c | 31 ++
samples/landlock/sandboxer.c | 35 +-
security/Kconfig | 5 +
security/Makefile | 2 +-
security/landlock/.kunitconfig | 2 +
security/landlock/Makefile | 15 +-
security/landlock/access.h | 100 ++++
security/landlock/audit.c | 510 ++++++++++++++++++
security/landlock/audit.h | 76 +++
security/landlock/domain.c | 339 ++++++++++++
security/landlock/domain.h | 145 +++++
security/landlock/fs.c | 305 ++++++++---
security/landlock/fs.h | 12 +
security/landlock/id.c | 249 +++++++++
security/landlock/id.h | 25 +
security/landlock/net.c | 51 +-
security/landlock/object.h | 4 +-
security/landlock/ruleset.c | 38 +-
security/landlock/ruleset.h | 95 ++--
security/landlock/setup.c | 2 +
security/landlock/syscalls.c | 28 +-
security/landlock/task.c | 152 +++++-
security/lsm_audit.c | 27 +-
tools/testing/kunit/configs/all_tests.config | 2 +
tools/testing/selftests/landlock/Makefile | 2 +-
tools/testing/selftests/landlock/audit.h | 371 +++++++++++++
tools/testing/selftests/landlock/audit_test.c | 389 +++++++++++++
tools/testing/selftests/landlock/base_test.c | 19 +-
tools/testing/selftests/landlock/common.h | 40 +-
tools/testing/selftests/landlock/config | 1 +
tools/testing/selftests/landlock/fs_test.c | 151 +++++-
.../testing/selftests/landlock/ptrace_test.c | 67 ++-
.../selftests/landlock/sandbox-and-launch.c | 82 +++
tools/testing/selftests/landlock/wait-pipe.c | 70 +++
tools/testing/selftests/landlock/wrappers.h | 47 ++
46 files changed, 3360 insertions(+), 271 deletions(-)
create mode 100644 include/linux/landlock.h
create mode 100644 security/landlock/access.h
create mode 100644 security/landlock/audit.c
create mode 100644 security/landlock/audit.h
create mode 100644 security/landlock/domain.c
create mode 100644 security/landlock/domain.h
create mode 100644 security/landlock/id.c
create mode 100644 security/landlock/id.h
create mode 100644 tools/testing/selftests/landlock/audit.h
create mode 100644 tools/testing/selftests/landlock/audit_test.c
create mode 100644 tools/testing/selftests/landlock/sandbox-and-launch.c
create mode 100644 tools/testing/selftests/landlock/wait-pipe.c
create mode 100644 tools/testing/selftests/landlock/wrappers.h
base-commit: 9d89551994a430b50c4fffcb1e617a057fa76e20
--
2.47.1
When CONFIG_AUDIT is set, its CONFIG_NET dependency is also set, and the
dev_get_by_index and init_net symbols (used by dump_common_audit_data)
are found by the linker. dump_common_audit_data() should then failed to
build when CONFIG_NET is not set. However, because the compiler is
smart, it knows that audit_log_start() always return NULL when
!CONFIG_AUDIT, and it doesn't build the body of common_lsm_audit(). As
a side effect, dump_common_audit_data() is not built and the linker
doesn't error out because of missing symbols.
Let's only build lsm_audit.o when CONFIG_SECURITY and CONFIG_AUDIT are
both set, which is checked with the new CONFIG_HAS_SECURITY_AUDIT.
ipv4_skb_to_auditdata() and ipv6_skb_to_auditdata() are only used by
Smack if CONFIG_AUDIT is set, so they don't need fake implementations.
Because common_lsm_audit() is used in multiple places without
CONFIG_AUDIT checks, add a fake implementation.
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: James Morris <jmorris@namei.org>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Serge E. Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-2-mic@digikod.net
---
Merged in the LSM's next tree. It will be part of Linux v6.13:
https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git/commit/?h=next&id=7ccbe076d987598b04b4b9c9b61f042291f9cc77
Changes since v2:
- Add CONFIG_HAS_SECURITY_AUDIT to fix the build with AUDIT &&
!SECURITY, reported by Guenter Roeck.
---
include/linux/lsm_audit.h | 14 ++++++++++++++
security/Kconfig | 5 +++++
security/Makefile | 2 +-
3 files changed, 20 insertions(+), 1 deletion(-)
Extract code from dump_common_audit_data() into the audit_log_lsm_data()
helper. This helps reuse common LSM audit data while not abusing
AUDIT_AVC records because of the common_lsm_audit() helper.
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: James Morris <jmorris@namei.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
Acked-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-3-mic@digikod.net
---
Changes since v3:
- Rebase on top of the v6.13's get_task_comm() fix.
- Add Acked-by Paul.
Changes since v1:
- Fix commit message (spotted by Paul).
- Constify dump_common_audit_data()'s and audit_log_lsm_data()'s "a"
argument.
- Fix build without CONFIG_NET: see previous patch.
---
include/linux/lsm_audit.h | 8 ++++++++
security/lsm_audit.c | 27 ++++++++++++++++++---------
2 files changed, 26 insertions(+), 9 deletions(-)
Landlock IDs can be generated to uniquely identify Landlock objects.
For now, only Landlock domains get an ID at creation time. These IDs
map to immutable domain hierarchies.
Landlock IDs have important properties:
- They are unique during the lifetime of the running system thanks to
the 64-bit values: at worse, 2^60 - 2*2^32 useful IDs.
- They are always greater than 2^32 and must then be stored in 64-bit
integer types.
- The initial ID (at boot time) is randomly picked between 2^32 and
2^33, which limits collisions in logs between different boots.
- IDs are sequential, which enables users to order them.
- IDs may not be consecutive but increase with a random 2^4 step, which
limits side channels.
Such IDs can be exposed to unprivileged processes, even if it is not the
case with this audit patch series. The domain IDs will be useful for
user space to identify sandboxes and get their properties.
These Landlock IDs are more robust that other absolute kernel IDs such
as pipe's inodes which rely on a shared global counter.
For checkpoint/restore features (i.e. CRIU), we could easily implement a
privileged interface (e.g. sysfs) to set the next ID counter.
IDR/IDA are not used because we only need a bijection from Landlock
objects to Landlock IDs, and we must not recycle IDs. This enables us
to identify all Landlock objects during the lifetime of the system (e.g.
in logs), but not to access an object from an ID nor know if an ID is
assigned. Using a counter is simpler, it scales (i.e. avoids growing
memory footprint), and it does not require locking. We'll use proper
file descriptors (with IDs used as inode numbers) to access Landlock
objects.
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/20250108154338.1129069-5-mic@digikod.net
---
Changes since v3:
- Rename landlock_get_id_range() helper to reflect the "range" of IDs.
- Add docstring for landlock_get_id_range().
Changes since v2:
- Extend commit message.
- Rename global_counter to next_id.
- Fix KUnit's test __init types, spotted by kernel test robot.
Changes since v1:
- New patch.
---
security/landlock/.kunitconfig | 2 +
security/landlock/Makefile | 2 +
security/landlock/id.c | 249 +++++++++++++++++++
security/landlock/id.h | 25 ++
security/landlock/setup.c | 2 +
tools/testing/kunit/configs/all_tests.config | 2 +
6 files changed, 282 insertions(+)
create mode 100644 security/landlock/id.c
create mode 100644 security/landlock/id.h
Move LANDLOCK_ACCESS_FS_INITIALLY_DENIED, access_mask_t, struct
access_mask, and struct access_masks_all to a dedicated access.h file.
Rename LANDLOCK_ACCESS_FS_INITIALLY_DENIED to
_LANDLOCK_ACCESS_FS_INITIALLY_DENIED to make it clear that it's not part
of UAPI. Add some newlines when appropriate.
This file will be extended with following commits, and it will help to
avoid dependency loops.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-6-mic@digikod.net
---
Changes since v2:
- Rebased on the (now merged) masks improvement patches.
- Move ACCESS_FS_OPTIONAL to a following patch introducing deny_masks_t,
spotted by Francis Laniel.
- Move and rename LANDLOCK_ACCESS_FS_INITIALLY_DENIED to
_LANDLOCK_ACCESS_FS_INITIALLY_DENIED.
Changes since v1:
- New patch
---
security/landlock/access.h | 62 +++++++++++++++++++++++++++++++++++++
security/landlock/fs.c | 3 +-
security/landlock/fs.h | 1 +
security/landlock/ruleset.c | 1 +
security/landlock/ruleset.h | 47 ++--------------------------
5 files changed, 68 insertions(+), 46 deletions(-)
create mode 100644 security/landlock/access.h
@@ -9,58 +9,15 @@#ifndef _SECURITY_LANDLOCK_RULESET_H#define _SECURITY_LANDLOCK_RULESET_H-#include<linux/bitops.h>-#include<linux/build_bug.h>-#include<linux/kernel.h>#include<linux/mutex.h>#include<linux/rbtree.h>#include<linux/refcount.h>#include<linux/workqueue.h>-#include<uapi/linux/landlock.h>+#include"access.h"#include"limits.h"#include"object.h"-/*-*Allaccessrightsthataredeniedbydefaultwhethertheyarehandledornot-*byaruleset/layer.ThismustbeORedwithallruleset->access_masks[]-*entrieswhenweneedtogettheabsolutehandledaccessmasks.-*/-/* clang-format off */-#define LANDLOCK_ACCESS_FS_INITIALLY_DENIED ( \-LANDLOCK_ACCESS_FS_REFER)-/* clang-format on */--typedefu16access_mask_t;-/* Makes sure all filesystem access rights can be stored. */-static_assert(BITS_PER_TYPE(access_mask_t)>=LANDLOCK_NUM_ACCESS_FS);-/* Makes sure all network access rights can be stored. */-static_assert(BITS_PER_TYPE(access_mask_t)>=LANDLOCK_NUM_ACCESS_NET);-/* Makes sure all scoped rights can be stored. */-static_assert(BITS_PER_TYPE(access_mask_t)>=LANDLOCK_NUM_SCOPE);-/* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */-static_assert(sizeof(unsignedlong)>=sizeof(access_mask_t));--/* Ruleset access masks. */-structaccess_masks{-access_mask_tfs:LANDLOCK_NUM_ACCESS_FS;-access_mask_tnet:LANDLOCK_NUM_ACCESS_NET;-access_mask_tscope:LANDLOCK_NUM_SCOPE;-};--unionaccess_masks_all{-structaccess_masksmasks;-u32all;-};--/* Makes sure all fields are covered. */-static_assert(sizeof(typeof_member(unionaccess_masks_all,masks))==-sizeof(typeof_member(unionaccess_masks_all,all)));--typedefu16layer_mask_t;-/* Makes sure all layers can be checked. */-static_assert(BITS_PER_TYPE(layer_mask_t)>=LANDLOCK_MAX_NUM_LAYERS);-/***structlandlock_layer-Accessrightsforagivenlayer*/
@@ -366,7 +323,7 @@ landlock_get_fs_access_mask(const struct landlock_ruleset *const ruleset,{/* Handles all initially denied by default access rights. */returnruleset->access_masks[layer_level].fs|-LANDLOCK_ACCESS_FS_INITIALLY_DENIED;+_LANDLOCK_ACCESS_FS_INITIALLY_DENIED;}staticinlineaccess_mask_t
Upgrade domain's handled access masks when creating a domain from a
ruleset, instead of converting them at runtime. This is more consistent
and helps with audit support.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-7-mic@digikod.net
---
Changes since v2:
- New patch.
---
security/landlock/access.h | 17 ++++++++++++++++-
security/landlock/fs.c | 10 +---------
security/landlock/ruleset.c | 3 ++-
3 files changed, 19 insertions(+), 11 deletions(-)
@@ -20,7 +20,8 @@/**Allaccessrightsthataredeniedbydefaultwhethertheyarehandledornot*byaruleset/layer.ThismustbeORedwithallruleset->access_masks[]-*entrieswhenweneedtogettheabsolutehandledaccessmasks.+*entrieswhenweneedtogettheabsolutehandledaccessmasks,see+*landlock_upgrade_handled_access_masks().*//* clang-format off */#define _LANDLOCK_ACCESS_FS_INITIALLY_DENIED ( \
@@ -59,4 +60,18 @@ typedef u16 layer_mask_t;/* Makes sure all layers can be checked. */static_assert(BITS_PER_TYPE(layer_mask_t)>=LANDLOCK_MAX_NUM_LAYERS);+/* Upgrades with all initially denied by default access rights. */+staticinlinestructaccess_masks+landlock_upgrade_handled_access_masks(structaccess_masksaccess_masks)+{+/*+*Allaccessrightsthataredeniedbydefaultwhethertheyare+*explicitlyhandledornot.+*/+if(access_masks.fs)+access_masks.fs|=_LANDLOCK_ACCESS_FS_INITIALLY_DENIED;++returnaccess_masks;+}+#endif /* _SECURITY_LANDLOCK_ACCESS_H */
Add audit support for sb_mount, move_mount, sb_umount, sb_remount, and
sb_pivot_root hooks.
The new related blocker is "fs.change_layout".
Add and use a new landlock_match_layer_level() helper.
Audit event sample:
type=LANDLOCK_DENY msg=audit(1729738800.349:44): domain=195ba459b blockers=fs.change_layout name="/" dev="tmpfs" ino=1
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-11-mic@digikod.net
---
Changes since v3:
- Cosmetic change to the "fs.change_layout" name.
Changes since v2:
- Log the domain that denied the action because not all layers block FS
layout changes.
- Fix landlock_match_layer_level().
Changes since v1:
- Rebased on the TCP patch series.
- Don't log missing permissions, only domain layer, and then remove the
permission word (suggested by Günther)
---
security/landlock/audit.c | 3 ++
security/landlock/audit.h | 1 +
security/landlock/fs.c | 64 ++++++++++++++++++++++++++++++++++---
security/landlock/ruleset.h | 31 ++++++++++++++++++
4 files changed, 94 insertions(+), 5 deletions(-)
Create a new domain.h file containing the struct landlock_hierarchy
definition and helpers. This type will grow with audit support. This
also prepares for a new domain type.
Export landlock_get_hierarchy() and landlock_put_hierarchy() that will
be used by audit in a following commit.
Clean up Makefile entries.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-8-mic@digikod.net
---
Changes since v3:
- Export landlock_get_hierarchy() and landlock_put_hierarchy().
- Clean up Makefile entries.
Changes since v1:
- New patch.
---
MAINTAINERS | 1 +
include/linux/landlock.h | 31 +++++++++++++++++++++++++++++++
security/landlock/Makefile | 11 +++++++++--
security/landlock/domain.c | 29 +++++++++++++++++++++++++++++
security/landlock/domain.h | 31 +++++++++++++++++++++++++++++++
security/landlock/ruleset.c | 22 ++++------------------
security/landlock/ruleset.h | 17 +----------------
security/landlock/task.c | 1 +
8 files changed, 107 insertions(+), 36 deletions(-)
create mode 100644 include/linux/landlock.h
create mode 100644 security/landlock/domain.c
create mode 100644 security/landlock/domain.h
Add a new AUDIT_LANDLOCK_DENY record type dedicated to any Landlock
denials. This encodes access verdict into the type to be able to filter
such denied requests with audit rules. Moreover, it would not make
sense for Landlock to log allowed requests (by default).
AUDIT_LANDLOCK_DENY indicates that something unexpected happened. A
following commit will allow to filter such denials according to the task
that created the related security policy.
The AUDIT_LANDLOCK_DENY 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_DENY 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
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.
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/20250108154338.1129069-9-mic@digikod.net
---
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 | 4 +-
security/landlock/audit.c | 137 ++++++++++++++++++++++++++++++++++++
security/landlock/audit.h | 52 ++++++++++++++
security/landlock/domain.c | 18 +++++
security/landlock/domain.h | 22 ++++++
security/landlock/ruleset.c | 6 ++
security/landlock/task.c | 93 ++++++++++++++++++------
8 files changed, 310 insertions(+), 25 deletions(-)
create mode 100644 security/landlock/audit.c
create mode 100644 security/landlock/audit.h
@@ -504,6 +505,7 @@ static void free_ruleset_work(struct work_struct *const work)free_ruleset(ruleset);}+/* Only called by hook_cred_free(). */voidlandlock_put_ruleset_deferred(structlandlock_ruleset*construleset){if(ruleset&&refcount_dec_and_test(&ruleset->usage)){
@@ -38,41 +40,29 @@ static bool domain_scope_le(const struct landlock_ruleset *const parent,{conststructlandlock_hierarchy*walker;+/* Quick return for non-landlocked tasks. */if(!parent)returntrue;+if(!child)returnfalse;+for(walker=child->hierarchy;walker;walker=walker->parent){if(walker==parent->hierarchy)/* @parent is in the scoped hierarchy of @child. */returntrue;}+/* There is no relationship between @parent and @child. */returnfalse;}-staticbooltask_is_scoped(conststructtask_struct*constparent,-conststructtask_struct*constchild)-{-boolis_scoped;-conststructlandlock_ruleset*dom_parent,*dom_child;--rcu_read_lock();-dom_parent=landlock_get_task_domain(parent);-dom_child=landlock_get_task_domain(child);-is_scoped=domain_scope_le(dom_parent,dom_child);-rcu_read_unlock();-returnis_scoped;-}--staticinttask_ptrace(conststructtask_struct*constparent,-conststructtask_struct*constchild)+staticintdomain_ptrace(conststructlandlock_ruleset*constparent,+conststructlandlock_ruleset*constchild){-/* Quick return for non-landlocked tasks. */-if(!landlocked(parent))-return0;-if(task_is_scoped(parent,child))+if(domain_scope_le(parent,child))return0;+return-EPERM;}
@@ -92,7 +82,36 @@ static int task_ptrace(const struct task_struct *const parent,staticinthook_ptrace_access_check(structtask_struct*constchild,constunsignedintmode){-returntask_ptrace(current,child);+conststructlandlock_ruleset*parent_dom,*child_dom;+structlandlock_requestrequest={+.type=LANDLOCK_REQUEST_PTRACE,+.audit={+.type=LSM_AUDIT_DATA_TASK,+.u.tsk=child,+},+};+interr;++/* Quick return for non-landlocked tasks. */+parent_dom=landlock_get_current_domain();+if(!parent_dom)+return0;++rcu_read_lock();+child_dom=landlock_get_task_domain(child);+err=domain_ptrace(parent_dom,child_dom);+rcu_read_unlock();++/*+*Fortheptrace_access_checkcase,welogthecurrent/parentdomain+*andthechildtask.+*/+if(err&&!(mode&PTRACE_MODE_NOAUDIT)){+request.layer_plus_one=parent_dom->num_layers;+landlock_log_denial(parent_dom,&request);+}++returnerr;}/**
@@ -109,7 +128,35 @@ static int hook_ptrace_access_check(struct task_struct *const child,*/staticinthook_ptrace_traceme(structtask_struct*constparent){-returntask_ptrace(parent,current);+conststructlandlock_ruleset*parent_dom,*child_dom;+structlandlock_requestrequest={+.type=LANDLOCK_REQUEST_PTRACE,+.audit={+.type=LSM_AUDIT_DATA_TASK,+.u.tsk=parent,+},+};+interr;++child_dom=landlock_get_current_domain();+rcu_read_lock();+parent_dom=landlock_get_task_domain(parent);+err=domain_ptrace(parent_dom,child_dom);++/*+*Fortheptrace_tracemecase,welogthedomainwhichisthecauseof+*thedenial,whichmeanstheparentdomaininsteadofthecurrent+*domain.Thismaylookweirdbecausetheptrace_tracemeactionisa+*requesttobetraced,butthesemanticisconsistentwith+*hook_ptrace_access_check().+*/+if(err){+request.layer_plus_one=parent_dom->num_layers;+landlock_log_denial(parent_dom,&request);+}++rcu_read_unlock();+returnerr;}/**
@@ -128,7 +175,7 @@ static bool domain_is_scoped(const struct landlock_ruleset *const client,access_mask_tscope){intclient_layer,server_layer;-structlandlock_hierarchy*client_walker,*server_walker;+conststructlandlock_hierarchy*client_walker,*server_walker;/* Quick return if client has no domain */if(WARN_ON_ONCE(!client))
Always synchronize access_masked_parent* with access_request_parent*
according to allowed_parent*. This is required for audit support to be
able to get back to the reason of denial.
In a rename/link action, instead of always checking a rule two times for
the same parent directory of the source and the destination files, only
check it when an action on a child was not already allowed. This also
enables us to keep consistent allowed_parent* status, which is required
to get back to the reason of denial.
For internal mount points, only upgrade allowed_parent* to true but do
not wrongfully set both of them to false otherwise. This is also
required to get back to the reason of denial.
This does not impact the current behavior but slightly optimize code and
prepare for audit support that needs to know the exact reason why an
access was denied.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-14-mic@digikod.net
---
Changes since v2:
- New patch.
---
security/landlock/fs.c | 44 ++++++++++++++++++++++++++----------------
1 file changed, 27 insertions(+), 17 deletions(-)
@@ -854,15 +854,6 @@ static bool is_access_to_paths_allowed(child1_is_directory,layer_masks_parent2,layer_masks_child2,child2_is_directory))){-allowed_parent1=scope_to_request(-access_request_parent1,layer_masks_parent1);-allowed_parent2=scope_to_request(-access_request_parent2,layer_masks_parent2);--/* Stops when all accesses are granted. */-if(allowed_parent1&&allowed_parent2)-break;-/**Now,downgradestheremainingchecksfromdomain*handledaccessestorequestedaccesses.
@@ -870,15 +861,32 @@ static bool is_access_to_paths_allowed(is_dom_check=false;access_masked_parent1=access_request_parent1;access_masked_parent2=access_request_parent2;++allowed_parent1=+allowed_parent1||+scope_to_request(access_masked_parent1,+layer_masks_parent1);+allowed_parent2=+allowed_parent2||+scope_to_request(access_masked_parent2,+layer_masks_parent2);++/* Stops when all accesses are granted. */+if(allowed_parent1&&allowed_parent2)+break;}rule=find_rule(domain,walker_path.dentry);-allowed_parent1=landlock_unmask_layers(-rule,access_masked_parent1,layer_masks_parent1,-ARRAY_SIZE(*layer_masks_parent1));-allowed_parent2=landlock_unmask_layers(-rule,access_masked_parent2,layer_masks_parent2,-ARRAY_SIZE(*layer_masks_parent2));+allowed_parent1=allowed_parent1||+landlock_unmask_layers(+rule,access_masked_parent1,+layer_masks_parent1,+ARRAY_SIZE(*layer_masks_parent1));+allowed_parent2=allowed_parent2||+landlock_unmask_layers(+rule,access_masked_parent2,+layer_masks_parent2,+ARRAY_SIZE(*layer_masks_parent2));/* Stops when a rule from each layer grants access. */if(allowed_parent1&&allowed_parent2)
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_DOM_INFO type.
The AUDIT_LANDLOCK_DOM_INFO message contains:
- the "domain" ID which is described,
- the "creation" time of this domain,
- 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").
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_DOM_INFO record follows the first AUDIT_LANDLOCK_DENY
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_DENY msg=audit(1732186800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
type=LANDLOCK_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 a following denial:
type=LANDLOCK_DENY 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 new AUDIT_LANDLOCK_DOM_DROP record type
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 AUDIT_LANDLOCK_DOM_DROP message contains:
- the "domain" ID which is being freed,
- the number of "denials" accounted to this domain, which is at least 1.
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_DOM_DROP msg=audit(1732186800.393:46): domain=195ba459b 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/20250108154338.1129069-10-mic@digikod.net
---
Questions about AUDIT_LANDLOCK_DOM_INFO messages (keeping in mind that
each logged metadata may need to be stored for the lifetime of each
domain):
- Should we also log the initially restricted task's loginuid?
- Should we also log the initially restricted task's sessionid?
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 | 2 +
security/landlock/audit.c | 88 ++++++++++++++++++++++++++++-
security/landlock/audit.h | 7 +++
security/landlock/domain.c | 109 ++++++++++++++++++++++++++++++++++++
security/landlock/domain.h | 63 +++++++++++++++++++++
security/landlock/ruleset.c | 6 ++
6 files changed, 274 insertions(+), 1 deletion(-)
@@ -111,11 +159,49 @@ void landlock_log_denial(const struct landlock_ruleset *const domain,if(!ab)return;-youngest_denied=get_hierarchy(domain,request->layer_plus_one-1);audit_log_format(ab,"domain=%llx blockers=",youngest_denied->id);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-Createanauditrecordwhenadomainisdeleted+*+*@domain:Thedomainbeingdeleted.+*+*Onlydomainswhichpreviouslyappearedintheauditlogsareloggedagain.+*Thisisusefultoknowwhenadomainwillnevershowagainintheauditlog.+*+*Thisrecordisnotdirectlytiedtoasyscallentry.+*+*Calledbythecred_free()hook,inanuninterruptiblecontext.+*/+voidlandlock_log_drop_domain(conststructlandlock_ruleset*constdomain)+{+structaudit_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 denials=%llu",domain->hierarchy->id,+atomic64_read(&domain->hierarchy->num_denials));+audit_log_end(ab);}#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
@@ -31,16 +44,112 @@ void landlock_put_hierarchy(struct landlock_hierarchy *hierarchy)#ifdef CONFIG_AUDIT+/**+*get_current_exe-Getthecurrent'sexecutablepath,ifany+*+*@path_str:Returnedpointertoapathstringwithalifetimetiedtothe+*returnedbuffer,ifany.+*@path_size:Returnedsizeofthe@pathstring(includingthetrailingnull+*character),ifany.+*+*Returns:Apointertoanallocatedbufferwhere@pathpointto,%NULLif+*thereisnoexecutablepath,oranerrorotherwise.+*/+staticconstvoid*get_current_exe(constchar**path_str,size_t*path_size)+{+structmm_struct*mm=current->mm;+structfile*file__free(fput)=NULL;+char*buffer__free(kfree)=NULL;+constchar*path;+size_tsize;++/* Adds 11 extra characters for the potential " (deleted)" suffix. */+constsize_tbuffer_size=PATH_MAX+11;++if(!mm)+returnNULL;++file=get_mm_exe_file(mm);+if(!file)+returnNULL;++buffer=kmalloc(buffer_size,GFP_KERNEL);+if(!buffer)+returnERR_PTR(-ENOMEM);++path=d_path(&file->f_path,buffer,buffer_size);+if(WARN_ON_ONCE(IS_ERR(path)))+/* Should never happen according to buffer_size. */+returnERR_CAST(path);++size=buffer+buffer_size-path;+if(WARN_ON_ONCE(size<=0))+returnERR_PTR(-ENAMETOOLONG);++*path_size=size;+*path_str=path;+returnno_free_ptr(buffer);+}++/*+*Returns:Anewlyallocatedobjectdescribingadomain,oranerror+*otherwise.+*/+staticstructlandlock_details*get_current_details(void)+{+/* Cf. audit_log_d_path_exe() */+staticconstcharnull_path[]="(null)";+constchar*path_str=null_path;+size_tpath_size=sizeof(null_path);+structlandlock_details*details;+constvoid*buffer__free(kfree)=NULL;++buffer=get_current_exe(&path_str,&path_size);+if(IS_ERR(buffer))+returnERR_CAST(buffer);++/*+*Createthenewdetailsaccordingtothepath'slength.Donot+*allocatewithGFP_KERNEL_ACCOUNTbecauseitisindependentfromthe+*caller.+*/+details=+kzalloc(struct_size(details,exe_path,path_size),GFP_KERNEL);+if(!details)+returnERR_PTR(-ENOMEM);++memcpy(details->exe_path,path_str,path_size);+ktime_get_coarse_real_ts64(&details->creation);++WARN_ON_ONCE(current_cred()!=current_real_cred());+details->cred=get_current_cred();+details->pid=get_pid(task_pid(current));+get_task_comm(details->comm,current);+returndetails;+}+/***landlock_init_current_hierarchy-Partiallyinitializelandlock_hierarchy**@hierarchy:Thehierarchytoinitialize.*+*Thecurrenttaskisreferencedasthedomainrestrictor.Thesubjective+*credentialsmustnotbeinanoverriddenstate.+**@hierarchy->parentand@hierarchy->usageshouldalreadybeset.*/intlandlock_init_current_hierarchy(structlandlock_hierarchy*consthierarchy){+structlandlock_details*details;++details=get_current_details();+if(IS_ERR(details))+returnPTR_ERR(details);++hierarchy->details=details;hierarchy->id=landlock_get_id_range(1);+hierarchy->log_status=LANDLOCK_LOG_PENDING;+atomic64_set(&hierarchy->num_denials,0);return0;}
Add audit support for path_mkdir, path_mknod, path_symlink, path_unlink,
path_rmdir, path_truncate, path_link, path_rename, and file_open hooks.
The dedicated blockers are:
- fs.execute
- fs.write_file
- fs.read_file
- fs.read_dir
- fs.remove_dir
- fs.remove_file
- fs.make_char
- fs.make_dir
- fs.make_reg
- fs.make_sock
- fs.make_fifo
- fs.make_block
- fs.make_sym
- fs.refer
- fs.truncate
- fs.ioctl_dev
Audit event sample for a denied link action:
type=LANDLOCK_DENY msg=audit(1729738800.349:44): domain=195ba459b blockers=fs.refer path="/usr/bin" dev="vda2" ino=351
type=LANDLOCK_DENY msg=audit(1729738800.349:44): domain=195ba459b blockers=fs.make_reg,fs.refer path="/usr/local" dev="vda2" ino=365
We could pack blocker names (e.g. "fs:make_reg,refer") but that would
increase complexity for the kernel and log parsers. Moreover, this
could not handle blockers of different classes (e.g. fs and net). Make
it simple and flexible instead.
Add KUnit tests to check the identification from a layer_mask_t array of
the first layer level denying such request.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-15-mic@digikod.net
---
Changes since v3:
- Rename blockers from fs_* to fs.*
- Extend commit message.
Changes since v2:
- Replace integer with bool in log_blockers().
- Always initialize youngest_layer, spotted by Francis Laniel.
- Fix incorrect log reason by using access_masked_parent1 instead of
access_request_parent1 (thanks to the previous fix patches).
- Clean up formatting.
Changes since v1:
- Move audit code to the ptrace patch.
- Revamp logging and support the path_link and path_rename hooks.
- Add KUnit tests.
---
security/landlock/audit.c | 177 ++++++++++++++++++++++++++++++++++++--
security/landlock/audit.h | 9 ++
security/landlock/fs.c | 65 +++++++++++---
3 files changed, 234 insertions(+), 17 deletions(-)
@@ -145,7 +291,25 @@ void landlock_log_denial(const struct landlock_ruleset *const domain,if(!is_valid_request(request))return;-youngest_denied=get_hierarchy(domain,request->layer_plus_one-1);+missing=request->access;+if(missing){+size_tyoungest_layer;++/* Gets the nearest domain that denies the request. */+if(request->layer_masks){+youngest_layer=get_denied_layer(+domain,&missing,request->layer_masks,+request->layer_masks_size);+}else{+/* This will change with the next commit. */+WARN_ON_ONCE(1);+youngest_layer=domain->num_layers;+}+youngest_denied=get_hierarchy(domain,youngest_layer);+}else{+youngest_denied=+get_hierarchy(domain,request->layer_plus_one-1);+}/**Consistentlykeepstrackofthenumberofdeniedaccessrequests
@@ -1108,6 +1133,7 @@ static int current_check_refer_path(struct dentry *const old_dentry,structdentry*old_parent;layer_mask_tlayer_masks_parent1[LANDLOCK_NUM_ACCESS_FS]={},layer_masks_parent2[LANDLOCK_NUM_ACCESS_FS]={};+structlandlock_requestrequest1={},request2={};if(!dom)return0;
@@ -1139,10 +1165,13 @@ static int current_check_refer_path(struct dentry *const old_dentry,access_request_parent1=landlock_init_layer_masks(dom,access_request_parent1|access_request_parent2,&layer_masks_parent1,LANDLOCK_KEY_INODE);-if(is_access_to_paths_allowed(-dom,new_dir,access_request_parent1,-&layer_masks_parent1,NULL,0,NULL,NULL))+if(is_access_to_paths_allowed(dom,new_dir,+access_request_parent1,+&layer_masks_parent1,&request1,+NULL,0,NULL,NULL,NULL))return0;++landlock_log_denial(dom,&request1);return-EACCES;}
@@ -1177,12 +1206,22 @@ static int current_check_refer_path(struct dentry *const old_dentry,*parentaccessrights.Thiswillbeusefultocomparewiththe*destinationparentaccessrights.*/-if(is_access_to_paths_allowed(-dom,&mnt_dir,access_request_parent1,&layer_masks_parent1,-old_dentry,access_request_parent2,&layer_masks_parent2,-exchange?new_dentry:NULL))+if(is_access_to_paths_allowed(dom,&mnt_dir,access_request_parent1,+&layer_masks_parent1,&request1,+old_dentry,access_request_parent2,+&layer_masks_parent2,&request2,+exchange?new_dentry:NULL))return0;+if(request1.access){+request1.audit.u.path.dentry=old_parent;+landlock_log_denial(dom,&request1);+}+if(request2.access){+request2.audit.u.path.dentry=new_dir->dentry;+landlock_log_denial(dom,&request2);+}+/**ThisprioritizesEACCESoverEXDEVforallactions,including*renameswithRENAME_EXCHANGE.
@@ -1562,6 +1601,7 @@ static int hook_file_open(struct file *const file)conststructlandlock_ruleset*constdom=landlock_get_applicable_domain(landlock_cred(file->f_cred)->domain,any_fs);+structlandlock_requestrequest={};if(!dom)return0;
@@ -1587,7 +1627,7 @@ static int hook_file_open(struct file *const file)dom,&file->f_path,landlock_init_layer_masks(dom,full_access_request,&layer_masks,LANDLOCK_KEY_INODE),-&layer_masks,NULL,0,NULL,NULL)){+&layer_masks,&request,NULL,0,NULL,NULL,NULL)){allowed_access=full_access_request;}else{unsignedlongaccess_bit;
@@ -1617,6 +1657,9 @@ static int hook_file_open(struct file *const file)if((open_access_request&allowed_access)==open_access_request)return0;+/* Sets access to reflect the actual request. */+request.access=open_access_request;+landlock_log_denial(dom,&request);return-EACCES;}
Fix a logical issue that could have been visible if the source or the
destination of a rename/link action was allowed for either the source or
the destination but not both. However, this logical bug is unreachable
because either:
- the rename/link action is allowed by the access rights tied to the
same mount point (without relying on access rights in a parent mount
point) and the access request is allowed (i.e. allow_parent1 and
allow_parent2 are true in current_check_refer_path),
- or a common rule in a parent mount point updates the access check for
the source and the destination (cf. is_access_to_paths_allowed).
See the following layout1.refer_part_mount_tree_is_allowed test that
work with and without this fix.
This fix does not impact current code but it is required for the audit
support.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-12-mic@digikod.net
---
Changes since v2:
- New patch.
---
security/landlock/fs.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
Add audit support to the file_truncate and file_ioctl hooks.
Add a deny_masks_t type and related helpers to store the domain's layer
level per optional access rights (i.e. LANDLOCK_ACCESS_FS_TRUNCATE and
LANDLOCK_ACCESS_FS_IOCTL_DEV) when opening a file, which cannot be
inferred later. In practice, the landlock_file_security blob size is
unchanged because this new one-byte deny_masks field follows the
existing two-bytes allowed_access field.
Implementing deny_masks_t with a bitfield instead of a struct enables a
generic implementation to store and extract layer levels.
Add KUnit tests to check the identification of a layer level from a
deny_masks_t, and the computation of a deny_masks_t from an access right
with its layer level or a layer_mask_t array.
Audit event sample:
type=LANDLOCK_DENY msg=audit(1729738800.349:44): domain=195ba459b blockers=fs.ioctl_dev path="/dev/tty" dev="devtmpfs" ino=9
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-16-mic@digikod.net
---
Changes since v3:
- Rename get_layer_from_deny_masks().
Changes since v2:
- Fix !CONFIG_AUDIT build warning.
- Rename ACCESS_FS_OPTIONAL to _LANDLOCK_ACCESS_FS_OPTIONAL.
---
security/landlock/access.h | 23 +++++++
security/landlock/audit.c | 102 ++++++++++++++++++++++++++--
security/landlock/audit.h | 4 ++
security/landlock/domain.c | 133 +++++++++++++++++++++++++++++++++++++
security/landlock/domain.h | 8 +++
security/landlock/fs.c | 51 ++++++++++++++
security/landlock/fs.h | 9 +++
7 files changed, 325 insertions(+), 5 deletions(-)
@@ -28,6 +28,12 @@LANDLOCK_ACCESS_FS_REFER)/* clang-format on */+/* clang-format off */+#define _LANDLOCK_ACCESS_FS_OPTIONAL ( \+LANDLOCK_ACCESS_FS_TRUNCATE|\+LANDLOCK_ACCESS_FS_IOCTL_DEV)+/* clang-format on */+typedefu16access_mask_t;/* Makes sure all filesystem access rights can be stored. */
@@ -60,6 +66,23 @@ typedef u16 layer_mask_t;/* Makes sure all layers can be checked. */static_assert(BITS_PER_TYPE(layer_mask_t)>=LANDLOCK_MAX_NUM_LAYERS);+/*+*Tracksdomainsresponsibleofadeniedaccess.Thisisrequiredtoavoid+*storingineachobjectthefulllayer_masks[]requiredbyupdate_request().+*/+typedefu8deny_masks_t;++/*+*Makessurealloptionalaccessrightscanbetiedtoalayerindex(cf.+*get_deny_mask).+*/+static_assert(BITS_PER_TYPE(deny_masks_t)>=+(HWEIGHT(LANDLOCK_MAX_NUM_LAYERS-1)*+HWEIGHT(_LANDLOCK_ACCESS_FS_OPTIONAL)));++/* LANDLOCK_MAX_NUM_LAYERS must be a power of two (cf. deny_masks_t assert). */+static_assert(HWEIGHT(LANDLOCK_MAX_NUM_LAYERS)==1);+/* Upgrades with all initially denied by default access rights. */staticinlinestructaccess_maskslandlock_upgrade_handled_access_masks(structaccess_masksaccess_masks)
@@ -253,22 +255,111 @@ static void test_get_denied_layer(struct kunit *const test)#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */+staticsize_t+get_layer_from_deny_masks(access_mask_t*constaccess_request,+constaccess_mask_tall_existing_optional_access,+constdeny_masks_tdeny_masks)+{+constunsignedlongaccess_opt=all_existing_optional_access;+constunsignedlongaccess_req=*access_request;+access_mask_tmissing=0;+size_tyoungest_layer=0;+size_taccess_index=0;+unsignedlongaccess_bit;++/* This will require change with new object types. */+WARN_ON_ONCE(access_opt!=_LANDLOCK_ACCESS_FS_OPTIONAL);++for_each_set_bit(access_bit,&access_opt,+BITS_PER_TYPE(access_mask_t)){+if(access_req&BIT(access_bit)){+constsize_tlayer=+(deny_masks>>(access_index*4))&+(LANDLOCK_MAX_NUM_LAYERS-1);++if(layer>youngest_layer){+youngest_layer=layer;+missing=BIT(access_bit);+}elseif(layer==youngest_layer){+missing|=BIT(access_bit);+}+}+access_index++;+}++*access_request=missing;+returnyoungest_layer;+}++#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST++staticvoidtest_get_layer_from_deny_masks(structkunit*consttest)+{+deny_masks_tdeny_mask;+access_mask_taccess;++/* truncate:0 ioctl_dev:2 */+deny_mask=0x20;++access=LANDLOCK_ACCESS_FS_TRUNCATE;+KUNIT_EXPECT_EQ(test,0,+get_layer_from_deny_masks(&access,+_LANDLOCK_ACCESS_FS_OPTIONAL,+deny_mask));+KUNIT_EXPECT_EQ(test,access,LANDLOCK_ACCESS_FS_TRUNCATE);++access=LANDLOCK_ACCESS_FS_TRUNCATE|LANDLOCK_ACCESS_FS_IOCTL_DEV;+KUNIT_EXPECT_EQ(test,2,+get_layer_from_deny_masks(&access,+_LANDLOCK_ACCESS_FS_OPTIONAL,+deny_mask));+KUNIT_EXPECT_EQ(test,access,LANDLOCK_ACCESS_FS_IOCTL_DEV);++/* truncate:15 ioctl_dev:15 */+deny_mask=0xff;++access=LANDLOCK_ACCESS_FS_TRUNCATE;+KUNIT_EXPECT_EQ(test,15,+get_layer_from_deny_masks(&access,+_LANDLOCK_ACCESS_FS_OPTIONAL,+deny_mask));+KUNIT_EXPECT_EQ(test,access,LANDLOCK_ACCESS_FS_TRUNCATE);++access=LANDLOCK_ACCESS_FS_TRUNCATE|LANDLOCK_ACCESS_FS_IOCTL_DEV;+KUNIT_EXPECT_EQ(test,15,+get_layer_from_deny_masks(&access,+_LANDLOCK_ACCESS_FS_OPTIONAL,+deny_mask));+KUNIT_EXPECT_EQ(test,access,+LANDLOCK_ACCESS_FS_TRUNCATE|+LANDLOCK_ACCESS_FS_IOCTL_DEV);+}++#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */+staticboolis_valid_request(conststructlandlock_request*constrequest){if(WARN_ON_ONCE(!(!!request->layer_plus_one^!!request->access)))returnfalse;if(request->access){-if(WARN_ON_ONCE(!request->layer_masks))+if(WARN_ON_ONCE(!(!!request->layer_masks^+!!request->all_existing_optional_access)))returnfalse;}else{-if(WARN_ON_ONCE(request->layer_masks))+if(WARN_ON_ONCE(request->layer_masks||+request->all_existing_optional_access))returnfalse;}if(WARN_ON_ONCE(!!request->layer_masks^!!request->layer_masks_size))returnfalse;+if(request->deny_masks){+if(WARN_ON_ONCE(!request->all_existing_optional_access))+returnfalse;+}+returntrue;}
@@ -301,9 +392,9 @@ void landlock_log_denial(const struct landlock_ruleset *const domain,domain,&missing,request->layer_masks,request->layer_masks_size);}else{-/* This will change with the next commit. */-WARN_ON_ONCE(1);-youngest_layer=domain->num_layers;+youngest_layer=get_layer_from_deny_masks(+&missing,request->all_existing_optional_access,+request->deny_masks);}youngest_denied=get_hierarchy(domain,youngest_layer);}else{
@@ -377,6 +468,7 @@ static struct kunit_case test_cases[] = {/* clang-format off */KUNIT_CASE(test_get_hierarchy),KUNIT_CASE(test_get_denied_layer),+KUNIT_CASE(test_get_layer_from_deny_masks),{}/* clang-format on */};
@@ -153,4 +158,132 @@ int landlock_init_current_hierarchy(struct landlock_hierarchy *const hierarchy)return0;}+staticdeny_masks_t+get_layer_deny_mask(constaccess_mask_tall_existing_optional_access,+constunsignedlongaccess_bit,constsize_tlayer)+{+unsignedlongaccess_weight;++/* This may require change with new object types. */+WARN_ON_ONCE(all_existing_optional_access!=+_LANDLOCK_ACCESS_FS_OPTIONAL);++if(WARN_ON_ONCE(layer>=LANDLOCK_MAX_NUM_LAYERS))+return0;++access_weight=hweight_long(all_existing_optional_access&+GENMASK(access_bit,0));+if(WARN_ON_ONCE(access_weight<1))+return0;++returnlayer+<<((access_weight-1)*HWEIGHT(LANDLOCK_MAX_NUM_LAYERS-1));+}++#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST++staticvoidtest_get_layer_deny_mask(structkunit*consttest)+{+constunsignedlongtruncate=BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE);+constunsignedlongioctl_dev=BIT_INDEX(LANDLOCK_ACCESS_FS_IOCTL_DEV);++KUNIT_EXPECT_EQ(test,0,+get_layer_deny_mask(_LANDLOCK_ACCESS_FS_OPTIONAL,+truncate,0));+KUNIT_EXPECT_EQ(test,0x3,+get_layer_deny_mask(_LANDLOCK_ACCESS_FS_OPTIONAL,+truncate,3));++KUNIT_EXPECT_EQ(test,0,+get_layer_deny_mask(_LANDLOCK_ACCESS_FS_OPTIONAL,+ioctl_dev,0));+KUNIT_EXPECT_EQ(test,0xf0,+get_layer_deny_mask(_LANDLOCK_ACCESS_FS_OPTIONAL,+ioctl_dev,15));+}++#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */++deny_masks_t+landlock_get_deny_masks(constaccess_mask_tall_existing_optional_access,+constaccess_mask_toptional_access,+constlayer_mask_t(*constlayer_masks)[],+constsize_tlayer_masks_size)+{+constunsignedlongaccess_opt=optional_access;+unsignedlongaccess_bit;+deny_masks_tdeny_masks=0;++/* This may require change with new object types. */+WARN_ON_ONCE(access_opt!=+(optional_access&all_existing_optional_access));++if(WARN_ON_ONCE(!layer_masks))+return0;++if(WARN_ON_ONCE(!access_opt))+return0;++for_each_set_bit(access_bit,&access_opt,layer_masks_size){+constlayer_mask_tmask=(*layer_masks)[access_bit];++if(!mask)+continue;++/* __fls(1) == 0 */+deny_masks|=get_layer_deny_mask(all_existing_optional_access,+access_bit,__fls(mask));+}+returndeny_masks;+}++#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST++staticvoidtest_landlock_get_deny_masks(structkunit*consttest)+{+constlayer_mask_tlayers1[BITS_PER_TYPE(access_mask_t)]={+[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)]=BIT_ULL(0)|+BIT_ULL(9),+[BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE)]=BIT_ULL(1),+[BIT_INDEX(LANDLOCK_ACCESS_FS_IOCTL_DEV)]=BIT_ULL(2)|+BIT_ULL(0),+};++KUNIT_EXPECT_EQ(test,0x1,+landlock_get_deny_masks(_LANDLOCK_ACCESS_FS_OPTIONAL,+LANDLOCK_ACCESS_FS_TRUNCATE,+&layers1,ARRAY_SIZE(layers1)));+KUNIT_EXPECT_EQ(test,0x20,+landlock_get_deny_masks(_LANDLOCK_ACCESS_FS_OPTIONAL,+LANDLOCK_ACCESS_FS_IOCTL_DEV,+&layers1,ARRAY_SIZE(layers1)));+KUNIT_EXPECT_EQ(+test,0x21,+landlock_get_deny_masks(_LANDLOCK_ACCESS_FS_OPTIONAL,+LANDLOCK_ACCESS_FS_TRUNCATE|+LANDLOCK_ACCESS_FS_IOCTL_DEV,+&layers1,ARRAY_SIZE(layers1)));+}++#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */++#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST++staticstructkunit_casetest_cases[]={+/* clang-format off */+KUNIT_CASE(test_get_layer_deny_mask),+KUNIT_CASE(test_landlock_get_deny_masks),+{}+/* clang-format on */+};++staticstructkunit_suitetest_suite={+.name="landlock_domain",+.test_cases=test_cases,+};++kunit_test_suite(test_suite);++#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */+#endif /* CONFIG_AUDIT */
Add layout1.refer_part_mount_tree_is_allowed to test the masked logical
issue regarding collect_domain_accesses() calls followed by the
is_access_to_paths_allowed() check in current_check_refer_path(). See
previous commit.
This test should work without the previous fix as well, but it enables
us to make sure future changes will not have impact regarding this
behavior.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-13-mic@digikod.net
---
Changes since v2:
- New patch.
---
tools/testing/selftests/landlock/fs_test.c | 54 ++++++++++++++++++++--
1 file changed, 50 insertions(+), 4 deletions(-)
@@ -263,13 +263,27 @@ static int hook_unix_stream_connect(struct sock *const sock,conststructlandlock_ruleset*constdom=landlock_get_applicable_domain(landlock_get_current_domain(),unix_scope);+structlsm_network_auditaudit_net={+.sk=other,+};+structlandlock_requestrequest={+.type=LANDLOCK_REQUEST_SCOPE_ABSTRACT_UNIX_SOCKET,+.audit={+.type=LSM_AUDIT_DATA_NET,+.u.net=&audit_net,+},+};/* Quick return for non-landlocked tasks. */if(!dom)return0;-if(is_abstract_socket(other)&&sock_is_scoped(other,dom))+if(is_abstract_socket(other)&&sock_is_scoped(other,dom)){+request.layer_plus_one=+landlock_match_layer_level(dom,unix_scope)+1;+landlock_log_denial(dom,&request);return-EPERM;+}return0;}
@@ -280,6 +294,16 @@ static int hook_unix_may_send(struct socket *const sock,conststructlandlock_ruleset*constdom=landlock_get_applicable_domain(landlock_get_current_domain(),unix_scope);+structlsm_network_auditaudit_net={+.sk=other->sk,+};+structlandlock_requestrequest={+.type=LANDLOCK_REQUEST_SCOPE_ABSTRACT_UNIX_SOCKET,+.audit={+.type=LSM_AUDIT_DATA_NET,+.u.net=&audit_net,+},+};if(!dom)return0;
@@ -291,8 +315,12 @@ static int hook_unix_may_send(struct socket *const sock,if(unix_peer(sock->sk)==other->sk)return0;-if(is_abstract_socket(other->sk)&&sock_is_scoped(other->sk,dom))+if(is_abstract_socket(other->sk)&&sock_is_scoped(other->sk,dom)){+request.layer_plus_one=+landlock_match_layer_level(dom,unix_scope)+1;+landlock_log_denial(dom,&request);return-EPERM;+}return0;}
@@ -307,6 +335,13 @@ static int hook_task_kill(struct task_struct *const p,{boolis_scoped;conststructlandlock_ruleset*dom;+structlandlock_requestrequest={+.type=LANDLOCK_REQUEST_SCOPE_SIGNAL,+.audit={+.type=LSM_AUDIT_DATA_TASK,+.u.tsk=p,+},+};if(cred){/* Dealing with USB IO. */
@@ -324,8 +359,12 @@ static int hook_task_kill(struct task_struct *const p,is_scoped=domain_is_scoped(dom,landlock_get_task_domain(p),LANDLOCK_SCOPE_SIGNAL);rcu_read_unlock();-if(is_scoped)+if(is_scoped){+request.layer_plus_one=+landlock_match_layer_level(dom,signal_scope)+1;+landlock_log_denial(dom,&request);return-EPERM;+}return0;}
@@ -334,6 +373,13 @@ static int hook_file_send_sigiotask(struct task_struct *tsk,structfown_struct*fown,intsignum){conststructlandlock_ruleset*dom;+structlandlock_requestrequest={+.type=LANDLOCK_REQUEST_SCOPE_SIGNAL,+.audit={+.type=LSM_AUDIT_DATA_TASK,+.u.tsk=tsk,+},+};boolis_scoped=false;/* Lock already held by send_sigio() and send_sigurg(). */
@@ -349,8 +395,12 @@ static int hook_file_send_sigiotask(struct task_struct *tsk,is_scoped=domain_is_scoped(dom,landlock_get_task_domain(tsk),LANDLOCK_SCOPE_SIGNAL);rcu_read_unlock();-if(is_scoped)+if(is_scoped){+request.layer_plus_one=+landlock_match_layer_level(dom,signal_scope)+1;+landlock_log_denial(dom,&request);return-EPERM;+}return0;}
Most of the time we want to log denied access because they should not
happen and such information helps diagnose issues. However, when
sandboxing processes that we know will try to access denied resources
(e.g. unknown, bogus, or malicious binary), we might want to not log
related access requests that might fill up logs.
To disable any log for a specific Landlock domain, add a
LANDLOCK_RESTRICT_SELF_QUIET optional flag to the
landlock_restrict_self() system call.
Because this flag is set for a specific Landlock domain, it makes it
possible to selectively mask some access requests that would be logged
by a parent domain, which might be handy for unprivileged processes to
limit logs. However, system administrators should still use the audit
filtering mechanism.
There is intentionally no audit nor sysctl configuration to re-enable
these quiet domains. This is delegated to the user space program.
Increment the Landlock ABI version to reflect this interface change.
Cc: Günther Noack <gnoack@google.com>
Cc: Paul Moore <paul@paul-moore.com>
Closes: https://github.com/landlock-lsm/linux/issues/3
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-19-mic@digikod.net
---
Changes since v3:
- Rename LANDLOCK_RESTRICT_SELF_LOGLESS to LANDLOCK_RESTRICT_SELF_QUIET.
"quiet" is already used by kernel's cmdline to disable most log
messages, so this name makes sense for Landlock.
- Improve the LANDLOCK_ABI_VERSION comment.
Changes since v2:
- Update ABI version test.
---
Documentation/userspace-api/landlock.rst | 2 +-
include/uapi/linux/landlock.h | 14 ++++++++++
security/landlock/audit.c | 3 +++
security/landlock/domain.h | 1 +
security/landlock/syscalls.c | 28 ++++++++++++++++----
tools/testing/selftests/landlock/base_test.c | 2 +-
6 files changed, 43 insertions(+), 7 deletions(-)
@@ -8,7 +8,7 @@ Landlock: unprivileged access control =====================================:Author: Mickaël Salaün-:Date: October 2024+:Date: January 2025 The goal of Landlock is to enable restriction of ambient rights (e.g. global filesystem or network access) for a set of processes. Because Landlock
@@ -472,9 +481,12 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,!ns_capable_noaudit(current_user_ns(),CAP_SYS_ADMIN))return-EPERM;-/* No flag for now. */-if(flags)-return-EINVAL;+if(flags){+if(flags==LANDLOCK_RESTRICT_SELF_QUIET)+is_quiet=true;+else+return-EINVAL;+}/* Gets and checks the ruleset. */ruleset=get_ruleset_from_fd(ruleset_fd,FMODE_CAN_READ);
@@ -499,6 +511,12 @@ SYSCALL_DEFINE2(landlock_restrict_self, const int, ruleset_fd, const __u32,gotoout_put_creds;}+if(is_quiet){+#ifdef CONFIG_AUDIT+new_dom->hierarchy->log_status=LANDLOCK_LOG_DISABLED;+#endif /* CONFIG_AUDIT */+}+/* Replaces the old (prepared) domain. */landlock_put_ruleset(new_llcred->domain);new_llcred->domain=new_dom;
The global variable errno may not be set in test_execute(). Do not use
it in related error message.
Cc: Günther Noack <gnoack@google.com>
Fixes: e1199815b47b ("selftests/landlock: Add user space tests")
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-21-mic@digikod.net
---
Changes since v3:
- New patch.
---
tools/testing/selftests/landlock/fs_test.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -30,34 +28,6 @@/* TEST_F_FORK() should not be used for new tests. */#define TEST_F_FORK(fixture_name, test_name) TEST_F(fixture_name, test_name)-#ifndef landlock_create_ruleset-staticinlineint-landlock_create_ruleset(conststructlandlock_ruleset_attr*constattr,-constsize_tsize,const__u32flags)-{-returnsyscall(__NR_landlock_create_ruleset,attr,size,flags);-}-#endif--#ifndef landlock_add_rule-staticinlineintlandlock_add_rule(constintruleset_fd,-constenumlandlock_rule_typerule_type,-constvoid*construle_attr,-const__u32flags)-{-returnsyscall(__NR_landlock_add_rule,ruleset_fd,rule_type,rule_attr,-flags);-}-#endif--#ifndef landlock_restrict_self-staticinlineintlandlock_restrict_self(constintruleset_fd,-const__u32flags)-{-returnsyscall(__NR_landlock_restrict_self,ruleset_fd,flags);-}-#endif-staticvoid_init_caps(struct__test_metadata*const_metadata,booldrop_all){cap_tcap_p;
Add the restrict_self_flags test suite to check that
LANDLOCK_RESTRICT_SELF_QUIET is valid but not the next bit. Some checks
are similar to restrict_self_checks_ordering's ones.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-24-mic@digikod.net
---
Changes since v3:
- Use a last_flag variable.
Changes since v2:
- New patch.
---
tools/testing/selftests/landlock/base_test.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
Add audit_test.c to check with and without LANDLOCK_RESTRICT_SELF_QUIET
against the three Landlock audit record types: AUDIT_LANDLOCK_DENY,
AUDIT_LANDLOCK_DOM_INFO, and AUDIT_LANDLOCK_DOM_DROP.
Tests are run with audit filters to ensure the audit records come from
the running tests. Moreover, because there can only be one audit
process, tests would failed if run in parallel. Because of audit
limitations, tests can only be run in the initial namespace.
The audit test helpers were inspired by libaudit and
tools/testing/selftests/net/netfilter/audit_logread.c
Cc: Günther Noack <gnoack@google.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Phil Sutter <phil@nwl.cc>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-25-mic@digikod.net
---
Changes since v3:
- Improve audit_request() to check Netlink errors and handle multiple
replies.
- Make audit_filter_exe() more generic to handle several audit rule
lists.
- Merge audit_init_state() into audit_init() and create
audit_init_with_exe_filter() to handle AUDIT_EXE_LANDLOCK_DENY with an
arbitrary path.
- Add matches_log_dom_info().
Changes since v2:
- New patch.
---
tools/testing/selftests/landlock/audit.h | 370 ++++++++++++++++++
tools/testing/selftests/landlock/audit_test.c | 158 ++++++++
tools/testing/selftests/landlock/common.h | 2 +
tools/testing/selftests/landlock/config | 1 +
4 files changed, 531 insertions(+)
create mode 100644 tools/testing/selftests/landlock/audit.h
create mode 100644 tools/testing/selftests/landlock/audit_test.c
@@ -37,6 +38,7 @@ static void _init_caps(struct __test_metadata *const _metadata, bool drop_all)/* Only these three capabilities are useful for the tests. */constcap_value_tcaps[]={/* clang-format off */+CAP_AUDIT_CONTROL,CAP_DAC_OVERRIDE,CAP_MKNOD,CAP_NET_ADMIN,
Add tests for all ptrace actions. This improves all the ptrace tests by
making sure that the restrictions comes from Landlock, and with the
expected objects. These extended tests are like enhanced errno checks
that make sure Landlock enforcement is consistent.
Test coverage for security/landlock is 93.4% of 1619 lines according to
gcc/gcov-14.
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/20250108154338.1129069-26-mic@digikod.net
---
Changes since v3:
- Update test coverage.
Changes since v2:
- New patch.
---
.../testing/selftests/landlock/ptrace_test.c | 67 +++++++++++++++++--
1 file changed, 63 insertions(+), 4 deletions(-)
@@ -17,6 +18,7 @@#include<sys/wait.h>#include<unistd.h>+#include"audit.h"#include"common.h"/* Copied from security/yama/yama_lsm.c */
@@ -85,9 +87,27 @@ static int get_yama_ptrace_scope(void)returnret;}-/* clang-format off */-FIXTURE(hierarchy){};-/* clang-format on */+staticintmatches_log_ptrace(struct__test_metadata*const_metadata,+intaudit_fd,constpid_topid)+{+staticconstcharlog_template[]=REGEX_LANDLOCK_PREFIX+" blockers=ptrace opid=%d ocomm=\"ptrace_test\"$";+charlog_match[sizeof(log_template)+10];+intlog_match_len;++log_match_len=+snprintf(log_match,sizeof(log_match),log_template,opid);+if(log_match_len>sizeof(log_match))+return-E2BIG;++returnaudit_match_record(audit_fd,AUDIT_LANDLOCK_DENY,log_match);+}++FIXTURE(hierarchy)+{+structaudit_filteraudit_filter;+intaudit_fd;+};FIXTURE_VARIANT(hierarchy){
@@ -245,10 +265,16 @@ FIXTURE_VARIANT_ADD(hierarchy, deny_with_forked_domain) {FIXTURE_SETUP(hierarchy){+disable_caps(_metadata);+set_cap(_metadata,CAP_AUDIT_CONTROL);+self->audit_fd=audit_init_with_exe_filter(&self->audit_filter);+EXPECT_LE(0,self->audit_fd);+clear_cap(_metadata,CAP_AUDIT_CONTROL);}-FIXTURE_TEARDOWN(hierarchy)+FIXTURE_TEARDOWN_PARENT(hierarchy){+EXPECT_EQ(0,audit_cleanup(-1,NULL));}/* Test PTRACE_TRACEME and PTRACE_ATTACH for parent and child. */
@@ -336,17 +363,29 @@ TEST_F(hierarchy, trace)err_proc_read=test_ptrace_read(parent);if(can_read_parent){EXPECT_EQ(0,err_proc_read);+EXPECT_EQ(-EAGAIN,+matches_log_ptrace(_metadata,self->audit_fd,+parent));}else{EXPECT_EQ(EACCES,err_proc_read);+EXPECT_EQ(0,+matches_log_ptrace(_metadata,self->audit_fd,+parent));}/* Tests PTRACE_ATTACH on the parent. */ret=ptrace(PTRACE_ATTACH,parent,NULL,0);if(can_trace_parent){EXPECT_EQ(0,ret);+EXPECT_EQ(-EAGAIN,+matches_log_ptrace(_metadata,self->audit_fd,+parent));}else{EXPECT_EQ(-1,ret);EXPECT_EQ(EPERM,errno);+EXPECT_EQ(can_read_parent?-EAGAIN:0,+matches_log_ptrace(_metadata,self->audit_fd,+parent));}if(ret==0){ASSERT_EQ(parent,waitpid(parent,&status,0));
@@ -358,9 +397,16 @@ TEST_F(hierarchy, trace)ret=ptrace(PTRACE_TRACEME);if(can_trace_child){EXPECT_EQ(0,ret);+EXPECT_EQ(-EAGAIN,+matches_log_ptrace(_metadata,self->audit_fd,+parent));}else{EXPECT_EQ(-1,ret);EXPECT_EQ(EPERM,errno);+/* We should indeed see the parent process. */+EXPECT_EQ(can_read_child?-EAGAIN:0,+matches_log_ptrace(_metadata,self->audit_fd,+parent));}/*
@@ -408,17 +454,25 @@ TEST_F(hierarchy, trace)err_proc_read=test_ptrace_read(child);if(can_read_child){EXPECT_EQ(0,err_proc_read);+EXPECT_EQ(-EAGAIN,+matches_log_ptrace(_metadata,self->audit_fd,child));}else{EXPECT_EQ(EACCES,err_proc_read);+EXPECT_EQ(0,+matches_log_ptrace(_metadata,self->audit_fd,child));}/* Tests PTRACE_ATTACH on the child. */ret=ptrace(PTRACE_ATTACH,child,NULL,0);if(can_trace_child){EXPECT_EQ(0,ret);+EXPECT_EQ(-EAGAIN,+matches_log_ptrace(_metadata,self->audit_fd,child));}else{EXPECT_EQ(-1,ret);EXPECT_EQ(EPERM,errno);+EXPECT_EQ(can_read_child?-EAGAIN:0,+matches_log_ptrace(_metadata,self->audit_fd,child));}if(ret==0){
@@ -434,6 +488,11 @@ TEST_F(hierarchy, trace)if(WIFSIGNALED(status)||!WIFEXITED(status)||WEXITSTATUS(status)!=EXIT_SUCCESS)_metadata->exit_code=KSFT_FAIL;++/* Makes sure there is no superfluous logged records. */+audit_count_records(self->audit_fd,&records);+EXPECT_EQ(0,records.deny);+EXPECT_EQ(0,records.info);}TEST_HARNESS_MAIN
This will be used by security/landlock/audit.c in a following commit.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-27-mic@digikod.net
---
Changes since v3:
- New patch.
---
security/landlock/fs.c | 22 ++++++++++++----------
security/landlock/fs.h | 2 ++
2 files changed, 14 insertions(+), 10 deletions(-)
Check that a domain is not tied to the executable file that created it.
For instance, that could happen if a Landlock domain took a reference to
a struct path.
Move global path names to common.h and replace copy_binary() with a more
generic copy_file() helper.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-23-mic@digikod.net
---
Changes since v3:
- New patch to check issue from v2.
---
tools/testing/selftests/landlock/Makefile | 2 +-
tools/testing/selftests/landlock/common.h | 3 +
tools/testing/selftests/landlock/fs_test.c | 94 +++++++++++++++++--
.../selftests/landlock/sandbox-and-launch.c | 82 ++++++++++++++++
tools/testing/selftests/landlock/wait-pipe.c | 42 +++++++++
5 files changed, 213 insertions(+), 10 deletions(-)
create mode 100644 tools/testing/selftests/landlock/sandbox-and-launch.c
create mode 100644 tools/testing/selftests/landlock/wait-pipe.c
@@ -28,6 +28,9 @@/* TEST_F_FORK() should not be used for new tests. */#define TEST_F_FORK(fixture_name, test_name) TEST_F(fixture_name, test_name)+staticconstcharbin_sandbox_and_launch[]="./sandbox-and-launch";+staticconstcharbin_wait_pipe[]="./wait-pipe";+staticvoid_init_caps(struct__test_metadata*const_metadata,booldrop_all){cap_tcap_p;
@@ -1976,11 +1976,10 @@ static void copy_binary(struct __test_metadata *const _metadata,{TH_LOG("Failed to open \"%s\": %s",dst_path,strerror(errno));}-src_fd=open(BINARY_PATH,O_RDONLY|O_CLOEXEC);+src_fd=open(src_path,O_RDONLY|O_CLOEXEC);ASSERT_LE(0,src_fd){-TH_LOG("Failed to open \""BINARY_PATH"\": %s",-strerror(errno));+TH_LOG("Failed to open \"%s\": %s",src_path,strerror(errno));}ASSERT_EQ(0,fstat(src_fd,&statbuf));ASSERT_EQ(statbuf.st_size,
@@ -2048,6 +2047,83 @@ TEST_F_FORK(layout1, execute)test_execute(_metadata,0,file1_s1d3);}+TEST_F_FORK(layout1,umount_sandboxer)+{+intpipe_child[2],pipe_parent[2];+charbuf_parent;+pid_tchild;+intstatus;++copy_file(_metadata,bin_sandbox_and_launch,file1_s3d3);+ASSERT_EQ(0,pipe2(pipe_child,0));+ASSERT_EQ(0,pipe2(pipe_parent,0));++child=fork();+ASSERT_LE(0,child);+if(child==0){+charpipe_child_str[12],pipe_parent_str[12];+char*constargv[]={(char*)file1_s3d3,+(char*)bin_wait_pipe,pipe_child_str,+pipe_parent_str,NULL};++/* Passes the pipe FDs to the executed binary and its child. */+EXPECT_EQ(0,close(pipe_child[0]));+EXPECT_EQ(0,close(pipe_parent[1]));+snprintf(pipe_child_str,sizeof(pipe_child_str),"%d",+pipe_child[1]);+snprintf(pipe_parent_str,sizeof(pipe_parent_str),"%d",+pipe_parent[0]);++/*+*Weneedbin_sandbox_and_launch(copiedinsidethemountas+*file1_s3d3)toexecutebin_wait_pipe(outsidethemount)to+*makesurethemountpointwillnotbeEBUSYbecauseof+*file1_s3d3beinginuse.Thisavoidsapotentialrace+*conditionbetweenthefollowingread()andumount()calls.+*/+ASSERT_EQ(0,execve(argv[0],argv,NULL))+{+TH_LOG("Failed to execute \"%s\": %s",argv[0],+strerror(errno));+};+_exit(1);+return;+}++EXPECT_EQ(0,close(pipe_child[1]));+EXPECT_EQ(0,close(pipe_parent[0]));++/* Waits for the child to sandbox itself. */+EXPECT_EQ(1,read(pipe_child[0],&buf_parent,1));++/* Tests that the sandboxer is tied to its mount point. */+set_cap(_metadata,CAP_SYS_ADMIN);+EXPECT_EQ(-1,umount(dir_s3d2));+EXPECT_EQ(EBUSY,errno);+clear_cap(_metadata,CAP_SYS_ADMIN);++/* Signals the child to launch a grandchild. */+EXPECT_EQ(1,write(pipe_parent[1],".",1));++/* Waits for the grandchild. */+EXPECT_EQ(1,read(pipe_child[0],&buf_parent,1));++/* Tests that the domain's sandboxer is not tied to its mount point. */+set_cap(_metadata,CAP_SYS_ADMIN);+EXPECT_EQ(0,umount(dir_s3d2))+{+TH_LOG("Failed to umount \"%s\": %s",dir_s3d2,+strerror(errno));+};+clear_cap(_metadata,CAP_SYS_ADMIN);++/* Signals the grandchild to terminate. */+EXPECT_EQ(1,write(pipe_parent[1],".",1));+ASSERT_EQ(child,waitpid(child,&status,0));+ASSERT_EQ(1,WIFEXITED(status));+ASSERT_EQ(0,WEXITSTATUS(status));+}+TEST_F_FORK(layout1,link){conststructrulelayer1[]={
Add a simple scope-based helper to put an inode reference, similar to
the fput() helper.
This is used in a following commit.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-28-mic@digikod.net
---
Changes since v3:
- New patch.
---
include/linux/fs.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Landlock manages a set of standalone security policies, which can be
loaded by any process. Because a sandbox policy may contain errors and
can lead to log spam, we need a way to exclude some of them. It is
simple and it makes sense to identify Landlock domains (i.e. security
policies) per binary path that loaded such policy.
Add a new AUDIT_EXE_LANDLOCK_DENY rule type to enables system
administrator to filter logs according to the origin or the security
policy responsible for a denial.
AUDIT_EXE identifies a property of the task calling the kernel, whereas
AUDIT_EXE_LANDLOCK_DENY identifies a property of a task that restricted
the task calling the kernel. AUDIT_EXE_LANDLOCK_DENY leverages most of
AUDIT_EXE's code to track files and compare them.
AUDIT_EXE_LANDLOCK_DENY is only handled by these audit rule lists:
- AUDIT_FILTER_EXCLUDE
- AUDIT_FILTER_EXIT
- AUDIT_FILTER_URING_EXIT
Add a new audit_set_landlock_hierarchy() helper to enrich the audit
context with the Landlock domain's creator which is the origin of the
current denial (if any).
Pass the current audit context to audit_filter() to be able to filter
according to the Landlock domain creator that denied the current action.
Add a new landlock_read_domain_exe() helper for audit to compare a
Landlock domain creator's inode and device numbers with a rule.
If scripts are not directly executed but passed to an interpreter, like
with AUDIT_EXE and /proc/self/exe, only this interpreter's path will
show in the logs. Scripts enforcing a security policy should then be
directly executed to differentiate between different scripts.
It does not make sense to add dedicated LSM hooks because it would not
make sense to treat all current and future LSM policies the same, and
there is currently only Landlock that handles different standalone and
unprivileged security policies. Indeed, AUDIT_EXE_LANDLOCK_DENY has a
clear semantic: it identifies the source of a Landlock denial.
In the future, we might want to extend this filtering capability with
other properties of tasks that restrict themselves with Landlock (e.g.
UID, loginuid, sessionid). This could be useful on systems where users
can bring their own executable code (which can already spam logs). For
now, AUDIT_EXE_LANDLOCK_DENY is enough to exclude buggy sandboxed
applications that may spam logs.
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/20250108154338.1129069-29-mic@digikod.net
---
We could have something like this to filter Landlock domains:
-a never,exclude -F exe_landlock_deny=/usr/bin/buggy-sandboxed-app
Changes since v3:
- New patch.
---
include/linux/audit.h | 11 ++++++++
include/linux/landlock.h | 10 +++++++
include/uapi/linux/audit.h | 1 +
kernel/audit.c | 4 +--
kernel/audit.h | 5 +++-
kernel/auditfilter.c | 30 ++++++++++++++++++++-
kernel/auditsc.c | 31 ++++++++++++++++++++++
security/landlock/audit.c | 4 +++
security/landlock/domain.c | 54 ++++++++++++++++++++++++++++++++++++--
security/landlock/domain.h | 20 ++++++++++++++
security/landlock/fs.c | 4 +--
security/landlock/object.h | 4 ++-
12 files changed, 169 insertions(+), 9 deletions(-)
@@ -305,6 +306,7 @@ extern void audit_seccomp(unsigned long syscall, long signr, int code);externvoidaudit_seccomp_actions_logged(constchar*names,constchar*old_names,intres);externvoid__audit_ptrace(structtask_struct*t);+externvoid__audit_set_landlock_hierarchy(structlandlock_hierarchy*hierarchy);staticinlinevoidaudit_set_context(structtask_struct*task,structaudit_context*ctx){
@@ -407,6 +414,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)caseAUDIT_FILETYPE:caseAUDIT_FIELD_COMPARE:caseAUDIT_EXE:+caseAUDIT_EXE_LANDLOCK_DENY:/* only equal and not equal valid ops */if(f->op!=Audit_not_equal&&f->op!=Audit_equal)return-EINVAL;
@@ -749,6 +759,7 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)return1;break;caseAUDIT_EXE:+caseAUDIT_EXE_LANDLOCK_DENY:/* both paths exist based on above type compare */if(strcmp(audit_mark_path(a->exe),audit_mark_path(b->exe)))
@@ -1328,7 +1340,8 @@ int audit_compare_dname_path(const struct qstr *dname, const char *path, int parreturnstrncmp(p,dname->name,dlen);}-intaudit_filter(intmsgtype,unsignedintlisttype)+intaudit_filter(intmsgtype,unsignedintlisttype,+conststructaudit_context*ctx){structaudit_entry*e;intret=1;/* Audit by default */
@@ -1381,6 +1394,21 @@ int audit_filter(int msgtype, unsigned int listtype)if(f->op==Audit_not_equal)result=!result;break;+caseAUDIT_EXE_LANDLOCK_DENY:+if(ctx&&ctx->landlock_hierarchy){+ino_tino=0;+dev_tdev=0;++result=+landlock_read_domain_exe(+ctx->landlock_hierarchy,+&ino,&dev)&&+audit_mark_compare(e->rule.exe,+ino,dev);+if(f->op==Audit_not_equal)+result=!result;+}+break;default:gotounlock_and_return;}
Add audit_rule.exe_landlock_domain tests to filter Landlock denials
according to the binary that created the sandbox.
The wait-pipe.c test program is updated to sandbox itself and send a
(denied) signal to its parent.
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/20250108154338.1129069-30-mic@digikod.net
---
Changes since v3:
- New patch.
---
tools/testing/selftests/landlock/audit.h | 1 +
tools/testing/selftests/landlock/audit_test.c | 153 ++++++++++++++++++
tools/testing/selftests/landlock/wait-pipe.c | 30 +++-
3 files changed, 183 insertions(+), 1 deletion(-)
@@ -140,6 +140,7 @@ static int audit_filter_exe(const int audit_fd,switch(filter->record_type){caseAUDIT_EXE:+caseAUDIT_EXE_LANDLOCK_DENY:break;default:return-EINVAL;
@@ -155,4 +173,139 @@ TEST_F(audit, fs_deny)_metadata->exit_code=KSFT_FAIL;}+FIXTURE(audit_rule)+{+structaudit_filteraudit_filter_main,audit_filter_test;+intaudit_fd;+};++FIXTURE_VARIANT(audit_rule)+{+constboolwith_exe_landlock_deny_child;+};++/* clang-format off */+FIXTURE_VARIANT_ADD(audit_rule,exe_landlock_deny_child){+/* clang-format on */+.with_exe_landlock_deny_child=true,+};++/* clang-format off */+FIXTURE_VARIANT_ADD(audit_rule,exe_landlock_deny_parent){+/* clang-format on */+.with_exe_landlock_deny_child=false,+};++FIXTURE_SETUP(audit_rule)+{+constchar*path=NULL;++disable_caps(_metadata);+set_cap(_metadata,CAP_AUDIT_CONTROL);++if(variant->with_exe_landlock_deny_child)+/* Filter on the sandboxer instead of the current exe. */+path=bin_wait_pipe;++self->audit_fd=audit_init();+EXPECT_LE(0,self->audit_fd)+{+constchar*error_msg;++/* kill "$(auditctl -s | sed -ne 's/^pid \([0-9]\+\)$/\1/p')" */+if(self->audit_fd==-EEXIST)+error_msg="socket already in use (e.g. auditd)";+else+error_msg=strerror(-self->audit_fd);+TH_LOG("Failed to initialize audit: %s",error_msg);+}++/* Applies main filter for the test task. */+EXPECT_EQ(0,audit_init_filter_exe(AUDIT_EXE,&self->audit_filter_main,+bin_wait_pipe));+EXPECT_EQ(0,audit_filter_exe(self->audit_fd,&self->audit_filter_main,+AUDIT_ADD_RULE,AUDIT_FILTER_EXCLUDE));++/* Applies test filter for the test task or the current task. */+EXPECT_EQ(0,audit_init_filter_exe(AUDIT_EXE_LANDLOCK_DENY,+&self->audit_filter_test,path));+EXPECT_EQ(0,audit_filter_exe(self->audit_fd,&self->audit_filter_test,+AUDIT_ADD_RULE,AUDIT_FILTER_EXCLUDE));++clear_cap(_metadata,CAP_AUDIT_CONTROL);+}++FIXTURE_TEARDOWN(audit_rule)+{+set_cap(_metadata,CAP_AUDIT_CONTROL);+EXPECT_EQ(0,audit_filter_exe(self->audit_fd,&self->audit_filter_main,+AUDIT_DEL_RULE,AUDIT_FILTER_EXCLUDE));+EXPECT_EQ(0,audit_filter_exe(self->audit_fd,&self->audit_filter_test,+AUDIT_DEL_RULE,AUDIT_FILTER_EXCLUDE));+clear_cap(_metadata,CAP_AUDIT_CONTROL);+EXPECT_EQ(0,close(self->audit_fd));+}++TEST_F(audit_rule,exe_landlock_deny)+{+structaudit_recordsrecords;+intpipe_child[2],pipe_parent[2];+charbuf_parent;+pid_tchild;+intstatus;++ASSERT_EQ(0,pipe2(pipe_child,0));+ASSERT_EQ(0,pipe2(pipe_parent,0));++child=fork();+ASSERT_LE(0,child);+if(child==0){+charpipe_child_str[12],pipe_parent_str[12];+char*constargv[]={(char*)bin_wait_pipe,pipe_child_str,+pipe_parent_str,NULL};++/* Passes the pipe FDs to the executed binary. */+EXPECT_EQ(0,close(pipe_child[0]));+EXPECT_EQ(0,close(pipe_parent[1]));+snprintf(pipe_child_str,sizeof(pipe_child_str),"%d",+pipe_child[1]);+snprintf(pipe_parent_str,sizeof(pipe_parent_str),"%d",+pipe_parent[0]);++ASSERT_EQ(0,execve(argv[0],argv,NULL))+{+TH_LOG("Failed to execute \"%s\": %s",argv[0],+strerror(errno));+};+_exit(1);+return;+}++EXPECT_EQ(0,close(pipe_child[1]));+EXPECT_EQ(0,close(pipe_parent[0]));++/* Waits for the child. */+EXPECT_EQ(1,read(pipe_child[0],&buf_parent,1));++/* Tests that there was no denial until now. */+audit_count_records(self->audit_fd,&records);+EXPECT_EQ(0,records.deny);++/* Signals the child to terminate. */+EXPECT_EQ(1,write(pipe_parent[1],".",1));++/* Tests that the audit record only matches the child. */+if(variant->with_exe_landlock_deny_child){+EXPECT_EQ(0,matches_log_signal(_metadata,self->audit_fd,+getpid()));+}else{+audit_count_records(self->audit_fd,&records);+EXPECT_EQ(0,records.deny);+}++ASSERT_EQ(child,waitpid(child,&status,0));+ASSERT_EQ(1,WIFEXITED(status));+ASSERT_EQ(0,WEXITSTATUS(status));+}+TEST_HARNESS_MAIN
@@ -26,6 +37,20 @@ int main(int argc, char *argv[])pipe_child=atoi(argv[1]);pipe_parent=atoi(argv[2]);+ruleset_fd=+landlock_create_ruleset(&ruleset_attr,sizeof(ruleset_attr),0);+if(ruleset_fd<0){+perror("Failed to create a ruleset");+return1;+}++prctl(PR_SET_NO_NEW_PRIVS,1,0,0,0);+if(landlock_restrict_self(ruleset_fd,0)){+perror("Failed to restrict self");+return1;+}+close(ruleset_fd);+/* Signals that we are waiting. */if(write(pipe_child,".",1)!=1){perror("Failed to write to first argument");
@@ -38,5 +63,8 @@ int main(int argc, char *argv[])return1;}+/* Tries to send a signal. */+kill(getppid(),0);+return0;}
Add compatibility.lists tests to make sure AUDIT_EXE_LANDLOCK_DENY is
only allowed for AUDIT_FILTER_EXCLUDE, AUDIT_FILTER_EXIT, and
AUDIT_FILTER_URING_EXIT.
Test coverage for security/landlock is 93.5% of 1635 lines according to
gcc/gcov-14.
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/20250108154338.1129069-31-mic@digikod.net
---
Changes since v3:
- New patch.
---
tools/testing/selftests/landlock/audit_test.c | 78 +++++++++++++++++++
1 file changed, 78 insertions(+)
@@ -308,4 +308,82 @@ TEST_F(audit_rule, exe_landlock_deny)ASSERT_EQ(0,WEXITSTATUS(status));}+FIXTURE(compatibility)+{+structaudit_filterfilter_self;+intaudit_fd;+};++FIXTURE_SETUP(compatibility)+{+disable_caps(_metadata);+set_cap(_metadata,CAP_AUDIT_CONTROL);+self->audit_fd=audit_init_with_exe_filter(&self->filter_self);+EXPECT_LE(0,self->audit_fd)+{+constchar*error_msg;++/* kill "$(auditctl -s | sed -ne 's/^pid \([0-9]\+\)$/\1/p')" */+if(self->audit_fd==-EEXIST)+error_msg="socket already in use (e.g. auditd)";+else+error_msg=strerror(-self->audit_fd);+TH_LOG("Failed to initialize audit: %s",error_msg);+}+clear_cap(_metadata,CAP_AUDIT_CONTROL);+}++FIXTURE_TEARDOWN(compatibility)+{+set_cap(_metadata,CAP_AUDIT_CONTROL);+EXPECT_EQ(0,audit_cleanup(self->audit_fd,&self->filter_self));+clear_cap(_metadata,CAP_AUDIT_CONTROL);+}++TEST_F(compatibility,lists)+{+structaudit_filterfilter_test;+size_tnum_ok=0;+__u32list;++EXPECT_EQ(0,audit_init_filter_exe(AUDIT_EXE_LANDLOCK_DENY,+&filter_test,NULL));+set_cap(_metadata,CAP_AUDIT_CONTROL);++for(list=0;list<AUDIT_NR_FILTERS;list++){+interr;++switch(list){+caseAUDIT_FILTER_EXIT:+caseAUDIT_FILTER_EXCLUDE:+caseAUDIT_FILTER_URING_EXIT:+num_ok++;+err=0;+break;+default:+err=-EINVAL;+break;+}++/*+*TestingAUDIT_FILTER_ENTRYprints"auditfilter:+*AUDIT_FILTER_ENTRYisdeprecated" in kernel logs.+*/+EXPECT_EQ(err,audit_filter_exe(self->audit_fd,&filter_test,+AUDIT_ADD_RULE,list))+{+TH_LOG("Unexpected result for list %u",list);+}+EXPECT_EQ(err,audit_filter_exe(self->audit_fd,&filter_test,+AUDIT_DEL_RULE,list))+{+TH_LOG("Unexpected result for list %u",list);+}+}++/* Makes sure the three accepted lists are checked. */+EXPECT_EQ(3,num_ok);+clear_cap(_metadata,CAP_AUDIT_CONTROL);+}+TEST_HARNESS_MAIN
Do not pollute audit logs because of unknown sandboxed programs.
Indeed, the sandboxer's security policy might not be fitted to the set
of sandboxed processes that could be spawned (e.g. from a shell).
The LANDLOCK_RESTRICT_SELF_QUIET flag should be used for all similar
sandboxer tools by default. Only natively-sandboxed programs should not
use this flag.
For test purpose, parse the LL_FORCE_LOG environment variable to still
log denials.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-20-mic@digikod.net
---
Changes since v3:
- Extend error message, suggested by Francis Laniel.
Changes since v2:
- New patch.
---
samples/landlock/sandboxer.c | 35 ++++++++++++++++++++++++++++++++---
1 file changed, 32 insertions(+), 3 deletions(-)
@@ -315,6 +316,9 @@ static const char help[] =" - \"a\" to restrict opening abstract unix sockets\n"" - \"s\" to restrict sending signals\n""\n"+"A sandboxer should not log denied access requests to avoid spamming logs, "+"but to test audit we can set "ENV_FORCE_LOG_NAME"=1\n"+"\n""Example:\n"ENV_FS_RO_NAME"=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "ENV_FS_RW_NAME"=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
@@ -333,7 +337,7 @@ int main(const int argc, char *const argv[], char *const *const envp)constchar*cmd_path;char*const*cmd_argv;intruleset_fd,abi;-char*env_port_name;+char*env_port_name,*env_force_log;__u64access_fs_ro=ACCESS_FS_ROUGHLY_READ,access_fs_rw=ACCESS_FS_ROUGHLY_READ|ACCESS_FS_ROUGHLY_WRITE;
@@ -344,6 +348,8 @@ int main(const int argc, char *const argv[], char *const *const envp).scoped=LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET|LANDLOCK_SCOPE_SIGNAL,};+/* Do not pollute audit logs because of unknown sandboxed programs. */+intrestrict_flags=LANDLOCK_RESTRICT_SELF_QUIET;if(argc<2){fprintf(stderr,help,argv[0]);
@@ -415,6 +421,12 @@ int main(const int argc, char *const argv[], char *const *const envp)/* Removes LANDLOCK_SCOPE_* for ABI < 6 */ruleset_attr.scoped&=~(LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET|LANDLOCK_SCOPE_SIGNAL);+__attribute__((fallthrough));+case6:+/* Removes LANDLOCK_RESTRICT_SELF_QUIET for ABI < 7 */+restrict_flags&=~LANDLOCK_RESTRICT_SELF_QUIET;++/* Must be printed for any ABI < LANDLOCK_ABI_LAST. */fprintf(stderr,"Hint: You should update the running kernel ""to leverage Landlock features "
@@ -449,6 +461,23 @@ int main(const int argc, char *const argv[], char *const *const envp)if(check_ruleset_scope(ENV_SCOPED_NAME,&ruleset_attr))return1;+/* Enables optional logs. */+env_force_log=getenv(ENV_FORCE_LOG_NAME);+if(env_force_log){+if(strcmp(env_force_log,"1")!=0){+fprintf(stderr,"Unknown value for "ENV_FORCE_LOG_NAME+" (only \"1\" is handled)\n");+return1;+}+if(!(restrict_flags&LANDLOCK_RESTRICT_SELF_QUIET)){+fprintf(stderr,+"Audit logs not supported by current kernel\n");+return1;+}+restrict_flags&=~LANDLOCK_RESTRICT_SELF_QUIET;+unsetenv(ENV_FORCE_LOG_NAME);+}+ruleset_fd=landlock_create_ruleset(&ruleset_attr,sizeof(ruleset_attr),0);if(ruleset_fd<0){
@@ -476,7 +505,7 @@ int main(const int argc, char *const argv[], char *const *const envp)perror("Failed to restrict privileges");gotoerr_close_ruleset;}-if(landlock_restrict_self(ruleset_fd,0)){+if(landlock_restrict_self(ruleset_fd,restrict_flags)){perror("Failed to enforce ruleset");gotoerr_close_ruleset;}
Merge check_access_path() into current_check_access_path() and make
hook_path_mknod() use it.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-4-mic@digikod.net
---
Changes since v1:
- Rebased on the TCP patch series.
- Remove inlining removal which was merged.
---
security/landlock/fs.c | 32 +++++++++++---------------------
1 file changed, 11 insertions(+), 21 deletions(-)
On Wed, Jan 08, 2025 at 04:43:13PM +0100, Mickaël Salaün wrote:
Move LANDLOCK_ACCESS_FS_INITIALLY_DENIED, access_mask_t, struct
access_mask, and struct access_masks_all to a dedicated access.h file.
Rename LANDLOCK_ACCESS_FS_INITIALLY_DENIED to
_LANDLOCK_ACCESS_FS_INITIALLY_DENIED to make it clear that it's not part
of UAPI. Add some newlines when appropriate.
This file will be extended with following commits, and it will help to
avoid dependency loops.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-6-mic@digikod.net
Pushed in my next tree to simplify next patch series.
quoted hunk
---
Changes since v2:
- Rebased on the (now merged) masks improvement patches.
- Move ACCESS_FS_OPTIONAL to a following patch introducing deny_masks_t,
spotted by Francis Laniel.
- Move and rename LANDLOCK_ACCESS_FS_INITIALLY_DENIED to
_LANDLOCK_ACCESS_FS_INITIALLY_DENIED.
Changes since v1:
- New patch
---
security/landlock/access.h | 62 +++++++++++++++++++++++++++++++++++++
security/landlock/fs.c | 3 +-
security/landlock/fs.h | 1 +
security/landlock/ruleset.c | 1 +
security/landlock/ruleset.h | 47 ++--------------------------
5 files changed, 68 insertions(+), 46 deletions(-)
create mode 100644 security/landlock/access.h
@@ -9,58 +9,15 @@#ifndef _SECURITY_LANDLOCK_RULESET_H#define _SECURITY_LANDLOCK_RULESET_H-#include<linux/bitops.h>-#include<linux/build_bug.h>-#include<linux/kernel.h>#include<linux/mutex.h>#include<linux/rbtree.h>#include<linux/refcount.h>#include<linux/workqueue.h>-#include<uapi/linux/landlock.h>+#include"access.h"#include"limits.h"#include"object.h"-/*-*Allaccessrightsthataredeniedbydefaultwhethertheyarehandledornot-*byaruleset/layer.ThismustbeORedwithallruleset->access_masks[]-*entrieswhenweneedtogettheabsolutehandledaccessmasks.-*/-/* clang-format off */-#define LANDLOCK_ACCESS_FS_INITIALLY_DENIED ( \-LANDLOCK_ACCESS_FS_REFER)-/* clang-format on */--typedefu16access_mask_t;-/* Makes sure all filesystem access rights can be stored. */-static_assert(BITS_PER_TYPE(access_mask_t)>=LANDLOCK_NUM_ACCESS_FS);-/* Makes sure all network access rights can be stored. */-static_assert(BITS_PER_TYPE(access_mask_t)>=LANDLOCK_NUM_ACCESS_NET);-/* Makes sure all scoped rights can be stored. */-static_assert(BITS_PER_TYPE(access_mask_t)>=LANDLOCK_NUM_SCOPE);-/* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */-static_assert(sizeof(unsignedlong)>=sizeof(access_mask_t));--/* Ruleset access masks. */-structaccess_masks{-access_mask_tfs:LANDLOCK_NUM_ACCESS_FS;-access_mask_tnet:LANDLOCK_NUM_ACCESS_NET;-access_mask_tscope:LANDLOCK_NUM_SCOPE;-};--unionaccess_masks_all{-structaccess_masksmasks;-u32all;-};--/* Makes sure all fields are covered. */-static_assert(sizeof(typeof_member(unionaccess_masks_all,masks))==-sizeof(typeof_member(unionaccess_masks_all,all)));--typedefu16layer_mask_t;-/* Makes sure all layers can be checked. */-static_assert(BITS_PER_TYPE(layer_mask_t)>=LANDLOCK_MAX_NUM_LAYERS);-/***structlandlock_layer-Accessrightsforagivenlayer*/
@@ -366,7 +323,7 @@ landlock_get_fs_access_mask(const struct landlock_ruleset *const ruleset,{/* Handles all initially denied by default access rights. */returnruleset->access_masks[layer_level].fs|-LANDLOCK_ACCESS_FS_INITIALLY_DENIED;+_LANDLOCK_ACCESS_FS_INITIALLY_DENIED;}staticinlineaccess_mask_t
On Wed, Jan 08, 2025 at 04:43:14PM +0100, Mickaël Salaün wrote:
Upgrade domain's handled access masks when creating a domain from a
ruleset, instead of converting them at runtime. This is more consistent
and helps with audit support.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-7-mic@digikod.net
Pushed in my next tree to simplify next patch series.
@@ -20,7 +20,8 @@/**Allaccessrightsthataredeniedbydefaultwhethertheyarehandledornot*byaruleset/layer.ThismustbeORedwithallruleset->access_masks[]-*entrieswhenweneedtogettheabsolutehandledaccessmasks.+*entrieswhenweneedtogettheabsolutehandledaccessmasks,see+*landlock_upgrade_handled_access_masks().*//* clang-format off */#define _LANDLOCK_ACCESS_FS_INITIALLY_DENIED ( \
@@ -59,4 +60,18 @@ typedef u16 layer_mask_t;/* Makes sure all layers can be checked. */static_assert(BITS_PER_TYPE(layer_mask_t)>=LANDLOCK_MAX_NUM_LAYERS);+/* Upgrades with all initially denied by default access rights. */+staticinlinestructaccess_masks+landlock_upgrade_handled_access_masks(structaccess_masksaccess_masks)+{+/*+*Allaccessrightsthataredeniedbydefaultwhethertheyare+*explicitlyhandledornot.+*/+if(access_masks.fs)+access_masks.fs|=_LANDLOCK_ACCESS_FS_INITIALLY_DENIED;++returnaccess_masks;+}+#endif /* _SECURITY_LANDLOCK_ACCESS_H */
On Wed, Jan 08, 2025 at 04:43:19PM +0100, Mickaël Salaün wrote:
Fix a logical issue that could have been visible if the source or the
destination of a rename/link action was allowed for either the source or
the destination but not both. However, this logical bug is unreachable
because either:
- the rename/link action is allowed by the access rights tied to the
same mount point (without relying on access rights in a parent mount
point) and the access request is allowed (i.e. allow_parent1 and
allow_parent2 are true in current_check_refer_path),
- or a common rule in a parent mount point updates the access check for
the source and the destination (cf. is_access_to_paths_allowed).
See the following layout1.refer_part_mount_tree_is_allowed test that
work with and without this fix.
This fix does not impact current code but it is required for the audit
support.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-12-mic@digikod.net
Pushed in my next tree to simplify next patch series.
quoted hunk
---
Changes since v2:
- New patch.
---
security/landlock/fs.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
On Wed, Jan 08, 2025 at 04:43:20PM +0100, Mickaël Salaün wrote:
Add layout1.refer_part_mount_tree_is_allowed to test the masked logical
issue regarding collect_domain_accesses() calls followed by the
is_access_to_paths_allowed() check in current_check_refer_path(). See
previous commit.
This test should work without the previous fix as well, but it enables
us to make sure future changes will not have impact regarding this
behavior.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-13-mic@digikod.net
Pushed in my next tree to simplify next patch series.
quoted hunk
---
Changes since v2:
- New patch.
---
tools/testing/selftests/landlock/fs_test.c | 54 ++++++++++++++++++++--
1 file changed, 50 insertions(+), 4 deletions(-)
On Wed, Jan 08, 2025 at 04:43:21PM +0100, Mickaël Salaün wrote:
Always synchronize access_masked_parent* with access_request_parent*
according to allowed_parent*. This is required for audit support to be
able to get back to the reason of denial.
In a rename/link action, instead of always checking a rule two times for
the same parent directory of the source and the destination files, only
check it when an action on a child was not already allowed. This also
enables us to keep consistent allowed_parent* status, which is required
to get back to the reason of denial.
For internal mount points, only upgrade allowed_parent* to true but do
not wrongfully set both of them to false otherwise. This is also
required to get back to the reason of denial.
This does not impact the current behavior but slightly optimize code and
prepare for audit support that needs to know the exact reason why an
access was denied.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-14-mic@digikod.net
Pushed in my next tree to simplify next patch series.
quoted hunk
---
Changes since v2:
- New patch.
---
security/landlock/fs.c | 44 ++++++++++++++++++++++++++----------------
1 file changed, 27 insertions(+), 17 deletions(-)
@@ -854,15 +854,6 @@ static bool is_access_to_paths_allowed(child1_is_directory,layer_masks_parent2,layer_masks_child2,child2_is_directory))){-allowed_parent1=scope_to_request(-access_request_parent1,layer_masks_parent1);-allowed_parent2=scope_to_request(-access_request_parent2,layer_masks_parent2);--/* Stops when all accesses are granted. */-if(allowed_parent1&&allowed_parent2)-break;-/**Now,downgradestheremainingchecksfromdomain*handledaccessestorequestedaccesses.
@@ -870,15 +861,32 @@ static bool is_access_to_paths_allowed(is_dom_check=false;access_masked_parent1=access_request_parent1;access_masked_parent2=access_request_parent2;++allowed_parent1=+allowed_parent1||+scope_to_request(access_masked_parent1,+layer_masks_parent1);+allowed_parent2=+allowed_parent2||+scope_to_request(access_masked_parent2,+layer_masks_parent2);++/* Stops when all accesses are granted. */+if(allowed_parent1&&allowed_parent2)+break;}rule=find_rule(domain,walker_path.dentry);-allowed_parent1=landlock_unmask_layers(-rule,access_masked_parent1,layer_masks_parent1,-ARRAY_SIZE(*layer_masks_parent1));-allowed_parent2=landlock_unmask_layers(-rule,access_masked_parent2,layer_masks_parent2,-ARRAY_SIZE(*layer_masks_parent2));+allowed_parent1=allowed_parent1||+landlock_unmask_layers(+rule,access_masked_parent1,+layer_masks_parent1,+ARRAY_SIZE(*layer_masks_parent1));+allowed_parent2=allowed_parent2||+landlock_unmask_layers(+rule,access_masked_parent2,+layer_masks_parent2,+ARRAY_SIZE(*layer_masks_parent2));/* Stops when a rule from each layer grants access. */if(allowed_parent1&&allowed_parent2)
On Wed, Jan 08, 2025 at 04:43:28PM +0100, Mickaël Salaün wrote:
The global variable errno may not be set in test_execute(). Do not use
it in related error message.
Cc: Günther Noack <gnoack@google.com>
Fixes: e1199815b47b ("selftests/landlock: Add user space tests")
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-21-mic@digikod.net
Pushed in my next tree to simplify next patch series.
quoted hunk
---
Changes since v3:
- New patch.
---
tools/testing/selftests/landlock/fs_test.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
@@ -30,34 +28,6 @@/* TEST_F_FORK() should not be used for new tests. */#define TEST_F_FORK(fixture_name, test_name) TEST_F(fixture_name, test_name)-#ifndef landlock_create_ruleset-staticinlineint-landlock_create_ruleset(conststructlandlock_ruleset_attr*constattr,-constsize_tsize,const__u32flags)-{-returnsyscall(__NR_landlock_create_ruleset,attr,size,flags);-}-#endif--#ifndef landlock_add_rule-staticinlineintlandlock_add_rule(constintruleset_fd,-constenumlandlock_rule_typerule_type,-constvoid*construle_attr,-const__u32flags)-{-returnsyscall(__NR_landlock_add_rule,ruleset_fd,rule_type,rule_attr,-flags);-}-#endif--#ifndef landlock_restrict_self-staticinlineintlandlock_restrict_self(constintruleset_fd,-const__u32flags)-{-returnsyscall(__NR_landlock_restrict_self,ruleset_fd,flags);-}-#endif-staticvoid_init_caps(struct__test_metadata*const_metadata,booldrop_all){cap_tcap_p;
On Wed, Jan 08, 2025 at 04:43:30PM +0100, Mickaël Salaün wrote:
Check that a domain is not tied to the executable file that created it.
For instance, that could happen if a Landlock domain took a reference to
a struct path.
Move global path names to common.h and replace copy_binary() with a more
generic copy_file() helper.
Cc: Günther Noack <gnoack@google.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-23-mic@digikod.net
Pushed in my next tree to simplify next patch series.
@@ -28,6 +28,9 @@/* TEST_F_FORK() should not be used for new tests. */#define TEST_F_FORK(fixture_name, test_name) TEST_F(fixture_name, test_name)+staticconstcharbin_sandbox_and_launch[]="./sandbox-and-launch";+staticconstcharbin_wait_pipe[]="./wait-pipe";+staticvoid_init_caps(struct__test_metadata*const_metadata,booldrop_all){cap_tcap_p;
@@ -1976,11 +1976,10 @@ static void copy_binary(struct __test_metadata *const _metadata,{TH_LOG("Failed to open \"%s\": %s",dst_path,strerror(errno));}-src_fd=open(BINARY_PATH,O_RDONLY|O_CLOEXEC);+src_fd=open(src_path,O_RDONLY|O_CLOEXEC);ASSERT_LE(0,src_fd){-TH_LOG("Failed to open \""BINARY_PATH"\": %s",-strerror(errno));+TH_LOG("Failed to open \"%s\": %s",src_path,strerror(errno));}ASSERT_EQ(0,fstat(src_fd,&statbuf));ASSERT_EQ(statbuf.st_size,
@@ -2048,6 +2047,83 @@ TEST_F_FORK(layout1, execute)test_execute(_metadata,0,file1_s1d3);}+TEST_F_FORK(layout1,umount_sandboxer)+{+intpipe_child[2],pipe_parent[2];+charbuf_parent;+pid_tchild;+intstatus;++copy_file(_metadata,bin_sandbox_and_launch,file1_s3d3);+ASSERT_EQ(0,pipe2(pipe_child,0));+ASSERT_EQ(0,pipe2(pipe_parent,0));++child=fork();+ASSERT_LE(0,child);+if(child==0){+charpipe_child_str[12],pipe_parent_str[12];+char*constargv[]={(char*)file1_s3d3,+(char*)bin_wait_pipe,pipe_child_str,+pipe_parent_str,NULL};++/* Passes the pipe FDs to the executed binary and its child. */+EXPECT_EQ(0,close(pipe_child[0]));+EXPECT_EQ(0,close(pipe_parent[1]));+snprintf(pipe_child_str,sizeof(pipe_child_str),"%d",+pipe_child[1]);+snprintf(pipe_parent_str,sizeof(pipe_parent_str),"%d",+pipe_parent[0]);++/*+*Weneedbin_sandbox_and_launch(copiedinsidethemountas+*file1_s3d3)toexecutebin_wait_pipe(outsidethemount)to+*makesurethemountpointwillnotbeEBUSYbecauseof+*file1_s3d3beinginuse.Thisavoidsapotentialrace+*conditionbetweenthefollowingread()andumount()calls.+*/+ASSERT_EQ(0,execve(argv[0],argv,NULL))+{+TH_LOG("Failed to execute \"%s\": %s",argv[0],+strerror(errno));+};+_exit(1);+return;+}++EXPECT_EQ(0,close(pipe_child[1]));+EXPECT_EQ(0,close(pipe_parent[0]));++/* Waits for the child to sandbox itself. */+EXPECT_EQ(1,read(pipe_child[0],&buf_parent,1));++/* Tests that the sandboxer is tied to its mount point. */+set_cap(_metadata,CAP_SYS_ADMIN);+EXPECT_EQ(-1,umount(dir_s3d2));+EXPECT_EQ(EBUSY,errno);+clear_cap(_metadata,CAP_SYS_ADMIN);++/* Signals the child to launch a grandchild. */+EXPECT_EQ(1,write(pipe_parent[1],".",1));++/* Waits for the grandchild. */+EXPECT_EQ(1,read(pipe_child[0],&buf_parent,1));++/* Tests that the domain's sandboxer is not tied to its mount point. */+set_cap(_metadata,CAP_SYS_ADMIN);+EXPECT_EQ(0,umount(dir_s3d2))+{+TH_LOG("Failed to umount \"%s\": %s",dir_s3d2,+strerror(errno));+};+clear_cap(_metadata,CAP_SYS_ADMIN);++/* Signals the grandchild to terminate. */+EXPECT_EQ(1,write(pipe_parent[1],".",1));+ASSERT_EQ(child,waitpid(child,&status,0));+ASSERT_EQ(1,WIFEXITED(status));+ASSERT_EQ(0,WEXITSTATUS(status));+}+TEST_F_FORK(layout1,link){conststructrulelayer1[]={
Al, Christian, this standalone patch could be useful to others. Feel
free to pick it in your tree.
On Wed, Jan 08, 2025 at 04:43:35PM +0100, Mickaël Salaün wrote:
quoted hunk
Add a simple scope-based helper to put an inode reference, similar to
the fput() helper.
This is used in a following commit.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20250108154338.1129069-28-mic@digikod.net
---
Changes since v3:
- New patch.
---
include/linux/fs.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
On Wed, Jan 8, 2025 at 4:44 PM Mickaël Salaün [off-list ref] wrote:
Add a simple scope-based helper to put an inode reference, similar to
the fput() helper.
Cleaning up inode references with scope-based cleanup seems dangerous
to me because, unlike most resources, holding a reference to an inode
beyond the lifetime of the associated superblock can actually cause
memory corruption; and scope-based cleanup is designed based on the
idea that the order and precise location of dropping a reference don't
matter so much.
So I would prefer to either not do this, or at least have some big
warning comment about usage requirements of scope-based inode
references.
From: Christian Brauner <brauner@kernel.org> Date: 2025-01-13 14:37:00
On Wed, 08 Jan 2025 16:43:35 +0100, Mickaël Salaün wrote:
Add a simple scope-based helper to put an inode reference, similar to
the fput() helper.
This is used in a following commit.
Applied to the vfs-6.14.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.14.misc branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.14.misc
[27/30] fs: Add iput() cleanup helper
https://git.kernel.org/vfs/vfs/c/38b1ff0bcff1
This looks unsafe: Once the reference to the file has been dropped
(which happens implicitly on return from get_current_exe()), nothing
holds a reference on the mount point or superblock anymore (the file
was previously holding a reference to the mount point through
->f_path.mnt), and so the superblock can be torn down and freed. But
the reference to the inode lives longer and is only cleaned up on
return from the caller get_current_details().
So I think this code can hit the error check for "Busy inodes after
unmount" in generic_shutdown_super(), which indicates that in theory,
use-after-free can occur.
For context, here are two older kernel security issues that also
involved superblock UAF due to assuming that it's possible to just
hold refcounted references to inodes:
https://project-zero.issues.chromium.org/42451116https://project-zero.issues.chromium.org/379667898
For fixing this, one option would be to copy the entire "struct path"
(which holds references on both the mount point and the inode) instead
of just copying the inode pointer.
From: Christian Brauner <brauner@kernel.org> Date: 2025-01-13 15:00:39
On Mon, Jan 13, 2025 at 03:00:20PM +0100, Jann Horn wrote:
On Wed, Jan 8, 2025 at 4:44 PM Mickaël Salaün [off-list ref] wrote:
quoted
Add a simple scope-based helper to put an inode reference, similar to
the fput() helper.
Cleaning up inode references with scope-based cleanup seems dangerous
to me because, unlike most resources, holding a reference to an inode
beyond the lifetime of the associated superblock can actually cause
memory corruption; and scope-based cleanup is designed based on the
idea that the order and precise location of dropping a reference don't
matter so much.
That's in general a good point and I know there's been opposition to
this in the past when we discussed this. So fine by me.
This looks unsafe: Once the reference to the file has been dropped
s/looks/is/g
(which happens implicitly on return from get_current_exe()), nothing
holds a reference on the mount point or superblock anymore (the file
was previously holding a reference to the mount point through
->f_path.mnt), and so the superblock can be torn down and freed. But
the reference to the inode lives longer and is only cleaned up on
return from the caller get_current_details().
So I think this code can hit the error check for "Busy inodes after
unmount" in generic_shutdown_super(), which indicates that in theory,
use-after-free can occur.
Yep, it sure would.
For context, here are two older kernel security issues that also
involved superblock UAF due to assuming that it's possible to just
hold refcounted references to inodes:
https://project-zero.issues.chromium.org/42451116https://project-zero.issues.chromium.org/379667898
For fixing this, one option would be to copy the entire "struct path"
(which holds references on both the mount point and the inode) instead
of just copying the inode pointer.
This looks unsafe: Once the reference to the file has been dropped
(which happens implicitly on return from get_current_exe()), nothing
holds a reference on the mount point or superblock anymore (the file
was previously holding a reference to the mount point through
->f_path.mnt), and so the superblock can be torn down and freed. But
the reference to the inode lives longer and is only cleaned up on
return from the caller get_current_details().
So I think this code can hit the error check for "Busy inodes after
unmount" in generic_shutdown_super(), which indicates that in theory,
use-after-free can occur.
For context, here are two older kernel security issues that also
involved superblock UAF due to assuming that it's possible to just
hold refcounted references to inodes:
https://project-zero.issues.chromium.org/42451116https://project-zero.issues.chromium.org/379667898
Thanks for the detailed explanation!
For fixing this, one option would be to copy the entire "struct path"
(which holds references on both the mount point and the inode) instead
of just copying the inode pointer.
On Mon, Jan 13, 2025 at 04:00:31PM +0100, Christian Brauner wrote:
On Mon, Jan 13, 2025 at 03:00:20PM +0100, Jann Horn wrote:
quoted
On Wed, Jan 8, 2025 at 4:44 PM Mickaël Salaün [off-list ref] wrote:
quoted
Add a simple scope-based helper to put an inode reference, similar to
the fput() helper.
Cleaning up inode references with scope-based cleanup seems dangerous
to me because, unlike most resources, holding a reference to an inode
beyond the lifetime of the associated superblock can actually cause
memory corruption; and scope-based cleanup is designed based on the
idea that the order and precise location of dropping a reference don't
matter so much.
That's in general a good point and I know there's been opposition to
this in the past when we discussed this. So fine by me.
I didn't have an opportunity to respond to your reply to my v3 comments
before you posted v4, but I see you've decided to stick with _DENY as
opposed to _ACCESS (or something similar). Let me copy your reply
below so I can respond appropriately ...
A stronger type with the "denied" semantic makes more sense to me,
especially for Landlock which is unprivileged, and it makes it clear
that it should only impact performance and log size (i.e. audit log
creation) for denied actions.
This is not consistent with how audit is typically used. Please
convert to AUDIT_LANDLOCK_ACCESS, or something similar.
The next patch
series will also contain a new kind of audit rule to specifically
identify the origin of the policy that created this denied event, which
should make more sense.
Generally speaking audit only wants to support a small number of message
types dedicated to a specific LSM. If you're aware of additional message
types that you plan to propose in a future patchset, it's probably a
time to discuss those now.
Because of its unprivileged nature, Landlock will never log granted
accesses by default. In the future, we might want a permissive-like
mode for Landlock, but this will be optional, and I would also strongly
prefer to add new audit record types for new semantics.
Once again, this isn't consistent with how audit is typically used and
I'm not seeing a compelling reason to rework how things are done. Please
stick with encoding the success/failure, accept/reject, etc. states in
audit record fields, not the message types themselves.
--
paul-moore.com
From: Paul Moore <paul@paul-moore.com> Date: 2025-01-15 23:53:09
On Jan 8, 2025 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= [off-list ref] wrote:
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_DOM_INFO type.
The AUDIT_LANDLOCK_DOM_INFO message contains:
- the "domain" ID which is described,
- the "creation" time of this domain,
- 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").
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_DOM_INFO record follows the first AUDIT_LANDLOCK_DENY
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_DENY msg=audit(1732186800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
type=LANDLOCK_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 a following denial:
type=LANDLOCK_DENY 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 new AUDIT_LANDLOCK_DOM_DROP record type
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 AUDIT_LANDLOCK_DOM_DROP message contains:
- the "domain" ID which is being freed,
- the number of "denials" accounted to this domain, which is at least 1.
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.
Wait, what rate limiting? Landlock shouldn't be adding any audit event
rate limiting beyond the queue management knobs built into the audit
subsystem. If you are comfortable rate limiting the logging of an event
it is a good sign that it probably shouldn't be an audit event.
The audit subsystem is for security releveant events, not diagnostic,
debugging, or other "nice to know" messages.
Audit event sample for a deletion of a domain that denied something:
type=LANDLOCK_DOM_DROP msg=audit(1732186800.393:46): domain=195ba459b denials=2
As mentioned earlier, I don't like the number of different Landlock
specific audit record types that are being created. I'm going to
suggest combining the LANDLOCK_DOM_INFO and LANDLOCK_DOM_DROP
records into one (LANDLOCK_DOM?) and using an "op=" field to indicate
creation/registration or destruction/unregistration of the domain ID.
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/20250108154338.1129069-10-mic@digikod.net
---
Questions about AUDIT_LANDLOCK_DOM_INFO messages (keeping in mind that
each logged metadata may need to be stored for the lifetime of each
domain):
- Should we also log the initially restricted task's loginuid?
- Should we also log the initially restricted task's sessionid?
I'm still struggling to understand why you need to log the domain's
creation time if you are connecting various Landlock audit events for a
single domain by the domain ID. To be clear, I'm not opposed if you
want to include it, it just seems like there is a disconnect between
how audit is typically used and what you are proposing.
quoted hunk
+ /*+ * 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);+}+
From: Paul Moore <paul@paul-moore.com> Date: 2025-01-15 23:53:10
On Jan 8, 2025 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= [off-list ref] wrote:
Landlock manages a set of standalone security policies, which can be
loaded by any process. Because a sandbox policy may contain errors and
can lead to log spam, we need a way to exclude some of them. It is
simple and it makes sense to identify Landlock domains (i.e. security
policies) per binary path that loaded such policy.
Add a new AUDIT_EXE_LANDLOCK_DENY rule type to enables system
administrator to filter logs according to the origin or the security
policy responsible for a denial.
For reasons similar to why I didn't want to expose the audit timestamp
to users outside of audit, I'm not very enthusiastic about expanding
the audit filtering code at this point in time.
I'm not saying "no" exactly, just "not right now".
--
paul-moore.com
I didn't have an opportunity to respond to your reply to my v3 comments
before you posted v4, but I see you've decided to stick with _DENY as
opposed to _ACCESS (or something similar). Let me copy your reply
below so I can respond appropriately ...
quoted
A stronger type with the "denied" semantic makes more sense to me,
especially for Landlock which is unprivileged, and it makes it clear
that it should only impact performance and log size (i.e. audit log
creation) for denied actions.
This is not consistent with how audit is typically used. Please
convert to AUDIT_LANDLOCK_ACCESS, or something similar.
OK
quoted
The next patch
series will also contain a new kind of audit rule to specifically
identify the origin of the policy that created this denied event, which
should make more sense.
Generally speaking audit only wants to support a small number of message
types dedicated to a specific LSM. If you're aware of additional message
types that you plan to propose in a future patchset, it's probably a
time to discuss those now.
The only other audit record type I'm thinking about would be one
dedicated to "potentially denied access", something similar to SELinux's
permissive mode.
quoted
Because of its unprivileged nature, Landlock will never log granted
accesses by default. In the future, we might want a permissive-like
mode for Landlock, but this will be optional, and I would also strongly
prefer to add new audit record types for new semantics.
Once again, this isn't consistent with how audit is typically used and
I'm not seeing a compelling reason to rework how things are done. Please
stick with encoding the success/failure, accept/reject, etc. states in
audit record fields, not the message types themselves.
On Wed, Jan 15, 2025 at 06:53:07PM -0500, Paul Moore wrote:
On Jan 8, 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_DOM_INFO type.
The AUDIT_LANDLOCK_DOM_INFO message contains:
- the "domain" ID which is described,
- the "creation" time of this domain,
- 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").
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_DOM_INFO record follows the first AUDIT_LANDLOCK_DENY
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_DENY msg=audit(1732186800.349:44): domain=195ba459b blockers=ptrace opid=1 ocomm="systemd"
type=LANDLOCK_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 a following denial:
type=LANDLOCK_DENY 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 new AUDIT_LANDLOCK_DOM_DROP record type
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 AUDIT_LANDLOCK_DOM_DROP message contains:
- the "domain" ID which is being freed,
- the number of "denials" accounted to this domain, which is at least 1.
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.
Wait, what rate limiting? Landlock shouldn't be adding any audit event
rate limiting beyond the queue management knobs built into the audit
subsystem. If you are comfortable rate limiting the logging of an event
it is a good sign that it probably shouldn't be an audit event.
I was talking about current audit's rate limiting.
The audit subsystem is for security releveant events, not diagnostic,
debugging, or other "nice to know" messages.
I agree, my goal was to only log denials with the minimal required
information to make sense of it. "minimal" may be subjective though :)
quoted
Audit event sample for a deletion of a domain that denied something:
type=LANDLOCK_DOM_DROP msg=audit(1732186800.393:46): domain=195ba459b denials=2
As mentioned earlier, I don't like the number of different Landlock
specific audit record types that are being created. I'm going to
suggest combining the LANDLOCK_DOM_INFO and LANDLOCK_DOM_DROP
records into one (LANDLOCK_DOM?) and using an "op=" field to indicate
creation/registration or destruction/unregistration of the domain ID.
I can squash them but they're just not about the same semantic at all.
One is an asynchronous event that describe a domain, and the other is a
synchronous event that informs about the end of a domain. I think it
would be a missed opportunity to have cleaner messages and to simplify
log parsers. Out of curiosity, are there existing audit record types
that mix semantic like this?
I wanted to give users the ability to filter each kind of record. We'll
not be able to differentiate each kind of record with audit rules if we
use the same record type. I don't understand why adding more audit
record types is an issue.
If we use an "op" field to differentiate these two types of information,
it would probably be "op=information" instead of "op=creation" because
the audit's timestamp will not identify the creation time of this
domain.
quoted
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/20250108154338.1129069-10-mic@digikod.net
---
Questions about AUDIT_LANDLOCK_DOM_INFO messages (keeping in mind that
each logged metadata may need to be stored for the lifetime of each
domain):
- Should we also log the initially restricted task's loginuid?
- Should we also log the initially restricted task's sessionid?
I'm still struggling to understand why you need to log the domain's
creation time if you are connecting various Landlock audit events for a
single domain by the domain ID. To be clear, I'm not opposed if you
want to include it, it just seems like there is a disconnect between
how audit is typically used and what you are proposing.
For the reasons explained in this commit message, domain's creation cannot
be logged synchronously as other audit events. However, timestamps are
useful to place them in the logs and order them according to other log
messages (i.e. to enrich log with more metadata). Without this domain's
creation timestamp, we cannot know when it was created. This
information is not strictly required but I think it can help get back to
the creation/creator of a domain. I'll remove it if you think it
doesn't make sense to have such information with audit or if it falls
into the "nice to know" category.
quoted
+ /*+ * 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);+}+
On Wed, Jan 15, 2025 at 06:53:08PM -0500, Paul Moore wrote:
On Jan 8, 2025 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= [off-list ref] wrote:
quoted
Landlock manages a set of standalone security policies, which can be
loaded by any process. Because a sandbox policy may contain errors and
can lead to log spam, we need a way to exclude some of them. It is
simple and it makes sense to identify Landlock domains (i.e. security
policies) per binary path that loaded such policy.
Add a new AUDIT_EXE_LANDLOCK_DENY rule type to enables system
administrator to filter logs according to the origin or the security
policy responsible for a denial.
For reasons similar to why I didn't want to expose the audit timestamp
to users outside of audit, I'm not very enthusiastic about expanding
the audit filtering code at this point in time.
I'm not saying "no" exactly, just "not right now".
Contrary to MAC systems which are configured and tuned by one entity
(e.g. the sysadmin), Landlock security policies can be configured by a
lot of different entities (e.g. sysadmin, app developers, users). One
noisy policy (e.g. a buggy sandboxed tar called in a loop by maintenance
scripts) could easily flood the audit logs without the ability for
sysadmins to filter such policy. They will only be able to filter all
users that *may* trigger such log (by executing the buggy sandboxed
app), which would mean almost all users, which would mask all other
legitimate Landlock events, then nullifying the entire audit support for
Landlock.
My plan was to extend these new kind of filter types to PID, UID, GID,
and LOGINUID (a subset of the audit filter exclude list) to give the
necessary flexibility to filter policy creators.
We really need a way to filter policy creators, and that needs to be
part of the (initial) Landlock audit support for it to be viable.
What do you propose?
From: Paul Moore <paul@paul-moore.com> Date: 2025-01-16 20:00:58
On Thu, Jan 16, 2025 at 5:49 AM Mickaël Salaün [off-list ref] wrote:
On Wed, Jan 15, 2025 at 06:53:06PM -0500, Paul Moore wrote:
quoted
On Jan 8, 2025 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= [off-list ref] wrote:
...
quoted
quoted
The next patch
series will also contain a new kind of audit rule to specifically
identify the origin of the policy that created this denied event, which
should make more sense.
Generally speaking audit only wants to support a small number of message
types dedicated to a specific LSM. If you're aware of additional message
types that you plan to propose in a future patchset, it's probably a
time to discuss those now.
The only other audit record type I'm thinking about would be one
dedicated to "potentially denied access", something similar to SELinux's
permissive mode.
In this case the "audit way" to handle this would be to add a
"permissive=[0|1]" field, or similar, to the AUDIT_LANDLOCK_ACCESS
message. If this is something you are definitely going to add to
Landlock, I might suggest adding the "permissive=" field now so it is
present from the start.
--
paul-moore.com
From: Paul Moore <paul@paul-moore.com> Date: 2025-01-16 20:20:04
On Thu, Jan 16, 2025 at 5:51 AM Mickaël Salaün [off-list ref] wrote:
On Wed, Jan 15, 2025 at 06:53:07PM -0500, Paul Moore wrote:
quoted
On Jan 8, 2025 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= [off-list ref] wrote:
...
quoted
The audit subsystem is for security releveant events, not diagnostic,
debugging, or other "nice to know" messages.
I agree, my goal was to only log denials with the minimal required
information to make sense of it. "minimal" may be subjective though :)
As a data point, it may be worth mentioning that it is not uncommon
for heavy audit users to generate more than 1TB of audit logs in a
day. So yes, the scale of audit can vary quite a bit.
quoted
quoted
Audit event sample for a deletion of a domain that denied something:
type=LANDLOCK_DOM_DROP msg=audit(1732186800.393:46): domain=195ba459b denials=2
As mentioned earlier, I don't like the number of different Landlock
specific audit record types that are being created. I'm going to
suggest combining the LANDLOCK_DOM_INFO and LANDLOCK_DOM_DROP
records into one (LANDLOCK_DOM?) and using an "op=" field to indicate
creation/registration or destruction/unregistration of the domain ID.
I can squash them but they're just not about the same semantic at all.
One is an asynchronous event that describe a domain, and the other is a
synchronous event that informs about the end of a domain.
From my perspective, both messages report the status/lifecycle of a
Landlock domain ID and are thus well suited for a single message type.
Personally I question the value of this information, it seems overly
verbose when the access control decisions are the important things,
but you seem to believe this information is important so fine, we'll
add a message type for the domain information, but you only get one.
If we use an "op" field to differentiate these two types of information,
it would probably be "op=information" instead of "op=creation" because
the audit's timestamp will not identify the creation time of this
domain.
Call it whatever you like, I personally don't care so long as you
don't reuse a single "op=" value for multiple "operations".
I'm still struggling to understand why you need to log the domain's
creation time if you are connecting various Landlock audit events for a
single domain by the domain ID. To be clear, I'm not opposed if you
want to include it, it just seems like there is a disconnect between
how audit is typically used and what you are proposing.
For the reasons explained in this commit message, domain's creation cannot
be logged synchronously as other audit events.
Yeah, I've read that, and I disagree. You *could* log a task's domain
creation synchronously as creation of a Landlock sandbox is a
process/syscall triggered event, you're *choosing* not to do so until
a denial occurs. That's okay, but I'm tired of hearing that used as
an excuse to do other silly things.
However, timestamps are
useful to place them in the logs and order them according to other log
messages (i.e. to enrich log with more metadata). Without this domain's
creation timestamp, we cannot know when it was created.
From my perspective this is because you are choosing not to record a
Landlock domain creation event. I still have not read any reason why
this is not possible, only a design decision which is now causing you
to do some other unusual things from an audit perspective.
This
information is not strictly required but I think it can help get back to
the creation/creator of a domain. I'll remove it if you think it
doesn't make sense to have such information with audit or if it falls
into the "nice to know" category.
Ignoring my comments above for a moment, it does really seem to fall
into the "nice to know" category to me, but if you feel strongly about
it, it's okay to leave in ... it just isn't really consistent with how
things are generally audited.
--
paul-moore.com
From: Paul Moore <paul@paul-moore.com> Date: 2025-01-16 20:24:17
On Thu, Jan 16, 2025 at 5:57 AM Mickaël Salaün [off-list ref] wrote:
On Wed, Jan 15, 2025 at 06:53:08PM -0500, Paul Moore wrote:
quoted
On Jan 8, 2025 =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= [off-list ref] wrote:
quoted
Landlock manages a set of standalone security policies, which can be
loaded by any process. Because a sandbox policy may contain errors and
can lead to log spam, we need a way to exclude some of them. It is
simple and it makes sense to identify Landlock domains (i.e. security
policies) per binary path that loaded such policy.
Add a new AUDIT_EXE_LANDLOCK_DENY rule type to enables system
administrator to filter logs according to the origin or the security
policy responsible for a denial.
For reasons similar to why I didn't want to expose the audit timestamp
to users outside of audit, I'm not very enthusiastic about expanding
the audit filtering code at this point in time.
I'm not saying "no" exactly, just "not right now".
Contrary to MAC systems which are configured and tuned by one entity
(e.g. the sysadmin), Landlock security policies can be configured by a
lot of different entities (e.g. sysadmin, app developers, users). One
noisy policy (e.g. a buggy sandboxed tar called in a loop by maintenance
scripts) could easily flood the audit logs without the ability for
sysadmins to filter such policy. They will only be able to filter all
users that *may* trigger such log (by executing the buggy sandboxed
app), which would mean almost all users, which would mask all other
legitimate Landlock events, then nullifying the entire audit support for
Landlock.
My plan was to extend these new kind of filter types to PID, UID, GID,
and LOGINUID (a subset of the audit filter exclude list) to give the
necessary flexibility to filter policy creators.
We really need a way to filter policy creators, and that needs to be
part of the (initial) Landlock audit support for it to be viable.
What do you propose?
If I recall correctly, there are other patches in the patchset which
provide some filtering of audit Landlock messages using mechanisms
specific to Landlock, that may be an option here. If not, perhaps the
audit log is not the best way to log Landlock events?
--
paul-moore.com