Implement LANDLOCK_ADD_RULE_QUIET
Subsystems (3)
v2 cover Implement LANDLOCK_ADD_RULE_QUIET
Hi,
This is the v2 of the "quiet flag" series, implementing the feature as proposed in [1].
v1: https://lore.kernel.org/all/[email protected]/
The quiet flag allows a sandboxer to suppress audit logs for uninteresting denials. The flag can be set on objects and inherits downward in the filesystem hierarchy. On a denial, the youngest denying layer's quiet flag setting decides whether to audit. The motivation for this feature is to reduce audit noise, and also prepare for a future supervisor feature which will use this bit to suppress supervisor notifications.
In this version, the most significant change is that we now have a quiet access mask in the ruleset_attr, which gets eventually stored in the hierarchy. This allows the user to specify which access should be affected by quiet bits. One can then, for example, make it such that read accesses to certain files are not audited (but still denied), but all writes are still audited, regardless of location.
This version also implements quiet support for optional accesses (truncate and ioctl), scope denials (signal, abstract unix socket), addresses suggestions from v1 review, and further enhances sandboxer to allow full customization of which access to quiet. Network and scope access quieting are now also supported by the sandboxer via additional environment variables.
I still haven't added any selftests yet but did some testing with sandboxer. I would like this to be reviewed as it stands, before finishing up the tests which I will hopefully add in v3.
Patches removed since v1:
- landlock/access: Improve explanation on the deny_masks_t
Demo:
/# LL_FS_RO=/usr LL_FS_RW= LL_FORCE_LOG=1 LL_FS_QUIET=/dev:/tmp:/etc LL_FS_QUIET_ACCESS=r ./sandboxer bash
...
audit: type=1423 audit(1759680175.562:195): domain=15bb25f6b blockers=fs.write_file,fs.read_file path="/dev/tty" dev="devtmpfs" ino=11
^^^^^^^^
# note: because write is not quieted, we see the above line. blockers
# contains read as well since that's the originally requested access.
audit: type=1424 audit(1759680175.562:195): domain=15bb25f6b status=allocated mode=enforcing pid=616 uid=0 exe="/sandboxer" comm="sandboxer"
audit: type=1300 audit(1759680175.562:195): arch=c000003e syscall=257 success=no exit=-13 a0=ffffffffffffff9c a1=5565c86113d1 a2=802 a3=0 items=0 ppid=605 pid=616 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="bash" exe="/usr/bin/bash" key=(null)
audit: type=1327 audit(1759680175.562:195): proctitle="bash"
bash: cannot set terminal process group (605): Inappropriate ioctl for device
bash: no job control in this shell
bash: /etc/bash.bashrc: Permission denied
audit: type=1423 audit(1759680175.570:196): domain=15bb25f6b blockers=fs.read_file path="/.bash_history" dev="virtiofs" ino=36963
^^^^^^^^
# read outside /dev:/tmp:/etc - not quieted
audit: type=1300 audit(1759680175.570:196): arch=c000003e syscall=257 success=no exit=-13 a0=ffffffffffffff9c a1=5565c868e400 a2=0 a3=0 items=0 ppid=605 pid=616 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="bash" exe="/usr/bin/bash" key=(null)
audit: type=1327 audit(1759680175.570:196): proctitle="bash"
audit: type=1423 audit(1759680175.570:197): domain=15bb25f6b blockers=fs.read_file path="/.bash_history" dev="virtiofs" ino=36963
audit: type=1300 audit(1759680175.570:197): arch=c000003e syscall=257 success=no exit=-13 a0=ffffffffffffff9c a1=5565c868e400 a2=0 a3=0 items=0 ppid=605 pid=616 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="bash" exe="/usr/bin/bash" key=(null)
audit: type=1327 audit(1759680175.570:197): proctitle="bash"
bash-5.2# head /etc/passwd
head: cannot open '/etc/passwd' for reading: Permission denied
^^^^^^^^
# reads to /etc are quieted
bash-5.2# echo evil >> /etc/passwd
bash: /etc/passwd: Permission denied
audit: type=1423 audit(1759680227.030:198): domain=15bb25f6b blockers=fs.write_file path="/etc/passwd" dev="virtiofs" ino=790
^^^^^^^^
# writes are not quieted
audit: type=1300 audit(1759680227.030:198): arch=c000003e syscall=257 success=no exit=-13 a0=ffffffffffffff9c a1=5565c86ab030 a2=441 a3=1b6 items=0 ppid=605 pid=616 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="bash" exe="/usr/bin/bash" key=(null)
audit: type=1327 audit(1759680227.030:198): proctitle="bash"Design:
- The user can set the quiet flag for a layer on any part of the fs hierarchy (whether it allows any access on it or not), and the flag inherits down (no support for "cancelling" the inheritance of the flag in specific subdirectories).
- The youngest layer that denies a request gets to decide whether the denial is audited or not. This means that a compromised binary, for example, cannot "turn off" Landlock auditing when it tries to access files, unless it denies access to the files itself. There is some debate to be had on whether, if a parent layer sets the quiet flag, but the request is denied by a deeper layer, whether Landlock should still audit anyway (since the rule author of the child layer likely did not expect the denial, so it would be good diagnostic). The current approach is to ignore the quiet on the parent layer and audit anyway.
All existing kselftests pass.
[1]: https://github.com/landlock-lsm/linux/issues/44#issuecomment-2876500918
Kind regards,
Tingmao
Tingmao Wang (6):
landlock: Add a place for flags to layer rules
landlock: Add API support and docs for the quiet flags landlock/audit: Check for quiet flag in landlock_log_denial landlock/audit: Fix wrong type usage samples/landlock: Add quiet flag support to sandboxer Implement quiet for optional accesses
include/uapi/linux/landlock.h | 64 +++++++++ samples/landlock/sandboxer.c | 133 +++++++++++++++++-- security/landlock/audit.c | 113 +++++++++++++--- security/landlock/audit.h | 4 +- security/landlock/domain.c | 23 ++++ security/landlock/domain.h | 10 ++ security/landlock/fs.c | 103 ++++++++------ security/landlock/fs.h | 36 +++-- security/landlock/net.c | 11 +- security/landlock/net.h | 3 +- security/landlock/ruleset.c | 19 ++- security/landlock/ruleset.h | 39 +++++- security/landlock/syscalls.c | 72 +++++++--- security/landlock/task.c | 12 +- tools/testing/selftests/landlock/base_test.c | 4 +- 15 files changed, 538 insertions(+), 108 deletions(-)
base-commit: e5f0a698b34ed76002dc5cff3804a61c80233a7a
Signature · 1 line
2.51.0
PATCH v2 1/6 landlock: Add a place for flags to layer rules
To avoid unnecessarily increasing the size of struct landlock_layer, we make the layer level a u8 and use the space to store the flags struct.
---
Note: conflict with 9a868cdbe6 ("landlock: Fix handling of disconnected directories") on mic/next but is not hard to fix - need to backup the collected rule flags too.
Changes since v1:
- Comment changes
security/landlock/fs.c | 75 ++++++++++++++++++++++++------------- security/landlock/net.c | 3 +- security/landlock/ruleset.c | 9 ++++- security/landlock/ruleset.h | 26 ++++++++++++- 4 files changed, 82 insertions(+), 31 deletions(-)
security/landlock/fs.c +48 -27
@@ -756,10 +756,12 @@ static bool is_access_to_paths_allowed(const struct path *const path, const access_mask_t access_request_parent1, layer_mask_t (*const layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS], + struct collected_rule_flags *const rule_flags_parent1, struct landlock_request *const log_request_parent1, struct dentry *const dentry_child1, const access_mask_t access_request_parent2, layer_mask_t (*const layer_masks_parent2)[LANDLOCK_NUM_ACCESS_FS], + struct collected_rule_flags *const rule_flags_parent2, struct landlock_request *const log_request_parent2, struct dentry *const dentry_child2) {
@@ -810,22 +812,30 @@ static bool is_access_to_paths_allowed( }if (unlikely (dentry_child1)) { +/* +* The rule_flags for child1 should have been included in +* rule_flags_masks_parent1 already. We do not bother about it +* for domain check. +*/ landlock_unmask_layers (find_rule (domain, dentry_child1),landlock_init_layer_masks ( domain,LANDLOCK_MASK_ACCESS_FS , &_layer_masks_child1,LANDLOCK_KEY_INODE ), - &_layer_masks_child1,ARRAY_SIZE (_layer_masks_child1)); + &_layer_masks_child1,ARRAY_SIZE (_layer_masks_child1), +NULL ); layer_masks_child1 = &_layer_masks_child1; child1_is_directory =d_is_dir (dentry_child1); }if (unlikely (dentry_child2)) { +/* See above comment for why NULL is passed as rule_flags_masks */ landlock_unmask_layers (find_rule (domain, dentry_child2),landlock_init_layer_masks ( domain,LANDLOCK_MASK_ACCESS_FS , &_layer_masks_child2,LANDLOCK_KEY_INODE ), - &_layer_masks_child2,ARRAY_SIZE (_layer_masks_child2)); + &_layer_masks_child2,ARRAY_SIZE (_layer_masks_child2), +NULL ); layer_masks_child2 = &_layer_masks_child2; child2_is_directory =d_is_dir (dentry_child2); }
@@ -881,16 +891,18 @@ static bool is_access_to_paths_allowed( } rule =find_rule (domain, walker_path.dentry); - 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)); + allowed_parent1 = + allowed_parent1 || +landlock_unmask_layers (rule, access_masked_parent1, + layer_masks_parent1, +ARRAY_SIZE (*layer_masks_parent1), + rule_flags_parent1); + allowed_parent2 = + allowed_parent2 || +landlock_unmask_layers (rule, access_masked_parent2, + layer_masks_parent2, +ARRAY_SIZE (*layer_masks_parent2), + rule_flags_parent2);/* Stops when a rule from each layer grants access. */ if (allowed_parent1 && allowed_parent2)
@@ -958,6 +970,7 @@ static int current_check_access_path(const struct path *const path,const struct landlock_cred_security *const subject =landlock_get_applicable_subject (current_cred (), masks,NULL );layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct collected_rule_flags rule_flags = {};struct landlock_request request = {};if (!subject)
@@ -967,8 +980,8 @@ static int current_check_access_path(const struct path *const path, access_request, &layer_masks, LANDLOCK_KEY_INODE);if (is_access_to_paths_allowed (subject->domain, path, access_request, - &layer_masks, &request,NULL ,0 ,NULL , -NULL ,NULL )) + &layer_masks, &rule_flags, &request, +NULL ,0 ,NULL ,NULL ,NULL ,NULL ))return 0 ;landlock_log_denial (subject, &request);
@@ -1032,7 +1045,8 @@ static access_mask_t maybe_remove(const struct dentry *const dentry)static bool collect_domain_accesses (const struct landlock_ruleset *const domain,const struct dentry *const mnt_root,struct dentry *dir, -layer_mask_t (*const layer_masks_dom)[LANDLOCK_NUM_ACCESS_FS ]) +layer_mask_t (*const layer_masks_dom)[LANDLOCK_NUM_ACCESS_FS ], +struct collected_rule_flags *const rule_flags) {unsigned long access_dom;bool ret = false;
@@ -1051,9 +1065,9 @@ static bool collect_domain_accesses(struct dentry *parent_dentry;/* Gets all layers allowing all domain accesses. */ -if (landlock_unmask_layers (find_rule (domain, dir), access_dom, - layer_masks_dom, -ARRAY_SIZE (*layer_masks_dom))) { +if (landlock_unmask_layers ( +find_rule (domain, dir), access_dom, layer_masks_dom, +ARRAY_SIZE (*layer_masks_dom), rule_flags)) {/* * Stops when all handled accesses are allowed by at * least one rule in each layer.
@@ -1140,6 +1154,8 @@ static int current_check_refer_path(struct dentry *const old_dentry,struct dentry *old_parent;layer_mask_t layer_masks_parent1[LANDLOCK_NUM_ACCESS_FS ] = {}, layer_masks_parent2[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct collected_rule_flags rule_flags_parent1 = {}, + rule_flags_parent2 = {};struct landlock_request request1 = {}, request2 = {};if (!subject)
@@ -1172,10 +1188,10 @@ static int current_check_refer_path(struct dentry *const old_dentry, subject->domain, access_request_parent1 | access_request_parent2, &layer_masks_parent1, LANDLOCK_KEY_INODE); -if (is_access_to_paths_allowed (subject->domain, new_dir, - access_request_parent1, - &layer_masks_parent1, &request1, -NULL ,0 ,NULL ,NULL ,NULL )) +if (is_access_to_paths_allowed ( + subject->domain, new_dir, access_request_parent1, + &layer_masks_parent1, &rule_flags_parent1, + &request1,NULL ,0 ,NULL ,NULL ,NULL ,NULL ))return 0 ;landlock_log_denial (subject, &request1);
@@ -1201,10 +1217,12 @@ static int current_check_refer_path(struct dentry *const old_dentry,/* new_dir->dentry is equal to new_dentry->d_parent */ allow_parent1 =collect_domain_accesses (subject->domain, mnt_dir.dentry, old_parent, - &layer_masks_parent1); + &layer_masks_parent1, + &rule_flags_parent1); allow_parent2 =collect_domain_accesses (subject->domain, mnt_dir.dentry, new_dir->dentry, - &layer_masks_parent2); + &layer_masks_parent2, + &rule_flags_parent2);if (allow_parent1 && allow_parent2)return 0 ;
@@ -1217,8 +1235,9 @@ static int current_check_refer_path(struct dentry *const old_dentry,*/ if (is_access_to_paths_allowed ( subject->domain, &mnt_dir, access_request_parent1, - &layer_masks_parent1, &request1, old_dentry, - access_request_parent2, &layer_masks_parent2, &request2, + &layer_masks_parent1, &rule_flags_parent1, &request1, + old_dentry, access_request_parent2, &layer_masks_parent2, + &rule_flags_parent2, &request2, exchange ? new_dentry :NULL ))return 0 ;
@@ -1616,6 +1635,7 @@ static bool is_device(const struct file *const file)static int hook_file_open (struct file *const file) {layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct collected_rule_flags rule_flags = {};access_mask_t open_access_request, full_access_request, allowed_access, optional_access; const struct landlock_cred_security *const subject =
@@ -1647,7 +1667,8 @@ static int hook_file_open(struct file *const file) landlock_init_layer_masks(subject->domain, full_access_request, &layer_masks, LANDLOCK_KEY_INODE), - &layer_masks, &request, NULL, 0, NULL, NULL, NULL)) { + &layer_masks, &rule_flags, &request, NULL, 0, NULL, NULL, + NULL, NULL)) { allowed_access = full_access_request; } else {unsigned long access_bit;
security/landlock/net.c +2 -1
@@ -48,6 +48,7 @@ static int current_check_access_socket(struct socket *const sock, {__be16 port;layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET ] = {}; +struct collected_rule_flags rule_flags = {};const struct landlock_rule *rule;struct landlock_id id = { .type =LANDLOCK_KEY_NET_PORT ,
@@ -179,7 +180,7 @@ static int current_check_access_socket(struct socket *const sock, access_request, &layer_masks, LANDLOCK_KEY_NET_PORT);if (landlock_unmask_layers (rule, access_request, &layer_masks, -ARRAY_SIZE (layer_masks))) +ARRAY_SIZE (layer_masks), &rule_flags))return 0 ; audit_net.family = address->sa_family;
security/landlock/ruleset.c +8 -1
@@ -616,7 +616,8 @@ landlock_find_rule(const struct landlock_ruleset *const ruleset,bool landlock_unmask_layers (const struct landlock_rule *const rule,const access_mask_t access_request,layer_mask_t (*const layer_masks)[], -const size_t masks_array_size) +const size_t masks_array_size, +struct collected_rule_flags *const rule_flags) {size_t layer_level;
@@ -643,6 +644,12 @@ bool landlock_unmask_layers(const struct landlock_rule *const rule,unsigned long access_bit;bool is_empty; +if (rule_flags) { +/* Collect rule flags for each layer */ +if (layer->flags.quiet) + rule_flags->quiet_masks |= layer_bit; + } +/* * Records in @layer_masks which layer grants access to each * requested access.
security/landlock/ruleset.h +24 -2
@@ -29,7 +29,18 @@ struct landlock_layer {/** * @level: Position of this layer in the layer stack. */ -u16 level; +u8 level; +/** +* @flags: Bitfield for special flags attached to this rule. +*/ +struct { +/** +* @quiet: Suppresses denial audit logs for the object covered by +* this rule in this domain. For filesystem rules, this inherits +* down the file hierarchy. +*/ +bool quiet:1 ; + } flags;/** * @access: Bitfield of allowed actions on the kernel object. They are * relative to the object type (e.g. %LANDLOCK_ACTION_FS_READ).
@@ -37,6 +48,16 @@ struct landlock_layer {access_mask_t access; }; +/** +* struct collected_rule_flags - Hold accumulated flags for each layer +*/ +struct collected_rule_flags { +/** +* @quiet_masks: Layers for which the quiet flag is effective. +*/ +layer_mask_t quiet_masks; +}; +/** * union landlock_key - Key of a ruleset's red-black tree */
@@ -304,7 +325,8 @@ landlock_get_scope_mask(const struct landlock_ruleset *const ruleset,bool landlock_unmask_layers (const struct landlock_rule *const rule,const access_mask_t access_request,layer_mask_t (*const layer_masks)[], -const size_t masks_array_size); +const size_t masks_array_size, +struct collected_rule_flags *const rule_flags); access_mask_t landlock_init_layer_masks(const struct landlock_ruleset *const domain,
Signature · 1 line
2.51.0
Mickaël Salaün
maintainerRe: [PATCH v2 1/6] landlock: Add a place for flags to layer rules
Quoted 37 lines · Tingmao Wang
To avoid unnecessarily increasing the size of struct
landlock_layer, we make the layer level a u8 and use the space to store the flags struct.Signed-off-by Tingmao Wang <[email protected]>---
Note: conflict with
9a868cdbe6("landlock: Fix handling of disconnected directories") on mic/next but is not hard to fix - need to backup the collected rule flags too.Changes since v1:
- Comment changessecurity/landlock/fs.c | 75 ++++++++++++++++++++++++------------- security/landlock/net.c | 3 +- security/landlock/ruleset.c | 9 ++++- security/landlock/ruleset.h | 26 ++++++++++++- 4 files changed, 82 insertions(+), 31 deletions(-)security/landlock/fs.c +2
-0@@ -756,10 +756,12 @@ static bool is_access_to_paths_allowed(const struct path *const path, const access_mask_t access_request_parent1, layer_mask_t (*const layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS], + struct collected_rule_flags *const rule_flags_parent1, struct landlock_request *const log_request_parent1, struct dentry *const dentry_child1, const access_mask_t access_request_parent2, layer_mask_t (*const layer_masks_parent2)[LANDLOCK_NUM_ACCESS_FS], + struct collected_rule_flags *const rule_flags_parent2, struct landlock_request *const log_request_parent2, struct dentry *const dentry_child2) {
security/landlock/fs.c static bool is_access_to_paths_allowed( · line 817
Quoted 6 lines
security/landlock/fs.c +3
-0@@ -810,22 +812,30 @@ static bool is_access_to_paths_allowed( }if (unlikely (dentry_child1)) { +/* +* The rule_flags for child1 should have been included in +* rule_flags_masks_parent1 already. We do not bother about it
rule_flags_parent1
security/landlock/fs.c static bool is_access_to_paths_allowed( · line 818
Quoted 1 line
security/landlock/fs.c +1
-0+* for domain check.
The main point about not setting rule_flags_parent* here and below is because the purpose of this few lines is to get the layer masks for the potential child dentries and compare them. Please improve the comment with this explanation too.
Quoted 55 lines
security/landlock/fs.c +18
-12+*/ landlock_unmask_layers (find_rule (domain, dentry_child1),landlock_init_layer_masks ( domain,LANDLOCK_MASK_ACCESS_FS , &_layer_masks_child1,LANDLOCK_KEY_INODE ), - &_layer_masks_child1,ARRAY_SIZE (_layer_masks_child1)); + &_layer_masks_child1,ARRAY_SIZE (_layer_masks_child1), +NULL ); layer_masks_child1 = &_layer_masks_child1; child1_is_directory =d_is_dir (dentry_child1); }if (unlikely (dentry_child2)) { +/* See above comment for why NULL is passed as rule_flags_masks */ landlock_unmask_layers (find_rule (domain, dentry_child2),landlock_init_layer_masks ( domain,LANDLOCK_MASK_ACCESS_FS , &_layer_masks_child2,LANDLOCK_KEY_INODE ), - &_layer_masks_child2,ARRAY_SIZE (_layer_masks_child2)); + &_layer_masks_child2,ARRAY_SIZE (_layer_masks_child2), +NULL ); layer_masks_child2 = &_layer_masks_child2; child2_is_directory =d_is_dir (dentry_child2); }@@ -881,16 +891,18 @@ static bool is_access_to_paths_allowed( } rule =find_rule (domain, walker_path.dentry); - 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)); + allowed_parent1 = + allowed_parent1 || +landlock_unmask_layers (rule, access_masked_parent1, + layer_masks_parent1, +ARRAY_SIZE (*layer_masks_parent1), + rule_flags_parent1); + allowed_parent2 = + allowed_parent2 || +landlock_unmask_layers (rule, access_masked_parent2, + layer_masks_parent2, +ARRAY_SIZE (*layer_masks_parent2), + rule_flags_parent2);/* Stops when a rule from each layer grants access. */ if (allowed_parent1 && allowed_parent2)
security/landlock/fs.c static int current_check_access_path(const struct path *const path, · line 973
Quoted 4 lines
security/landlock/fs.c +1
-0@@ -958,6 +970,7 @@ static int current_check_access_path(const struct path *const path,const struct landlock_cred_security *const subject =landlock_get_applicable_subject (current_cred (), masks,NULL );layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct collected_rule_flags rule_flags = {};
Why add it after the following variable?
Quoted 89 lines
security/landlock/fs.c +20
-14struct landlock_request request = {};if (!subject)@@ -967,8 +980,8 @@ static int current_check_access_path(const struct path *const path, access_request, &layer_masks, LANDLOCK_KEY_INODE);if (is_access_to_paths_allowed (subject->domain, path, access_request, - &layer_masks, &request,NULL ,0 ,NULL , -NULL ,NULL )) + &layer_masks, &rule_flags, &request, +NULL ,0 ,NULL ,NULL ,NULL ,NULL ))return 0 ;landlock_log_denial (subject, &request);@@ -1032,7 +1045,8 @@ static access_mask_t maybe_remove(const struct dentry *const dentry)static bool collect_domain_accesses (const struct landlock_ruleset *const domain,const struct dentry *const mnt_root,struct dentry *dir, -layer_mask_t (*const layer_masks_dom)[LANDLOCK_NUM_ACCESS_FS ]) +layer_mask_t (*const layer_masks_dom)[LANDLOCK_NUM_ACCESS_FS ], +struct collected_rule_flags *const rule_flags) {unsigned long access_dom;bool ret = false;@@ -1051,9 +1065,9 @@ static bool collect_domain_accesses(struct dentry *parent_dentry;/* Gets all layers allowing all domain accesses. */ -if (landlock_unmask_layers (find_rule (domain, dir), access_dom, - layer_masks_dom, -ARRAY_SIZE (*layer_masks_dom))) { +if (landlock_unmask_layers ( +find_rule (domain, dir), access_dom, layer_masks_dom, +ARRAY_SIZE (*layer_masks_dom), rule_flags)) {/* * Stops when all handled accesses are allowed by at * least one rule in each layer. @@ -1140,6 +1154,8 @@ static int current_check_refer_path(struct dentry *const old_dentry,struct dentry *old_parent;layer_mask_t layer_masks_parent1[LANDLOCK_NUM_ACCESS_FS ] = {}, layer_masks_parent2[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct collected_rule_flags rule_flags_parent1 = {}, + rule_flags_parent2 = {};struct landlock_request request1 = {}, request2 = {};if (!subject)@@ -1172,10 +1188,10 @@ static int current_check_refer_path(struct dentry *const old_dentry, subject->domain, access_request_parent1 | access_request_parent2, &layer_masks_parent1, LANDLOCK_KEY_INODE); -if (is_access_to_paths_allowed (subject->domain, new_dir, - access_request_parent1, - &layer_masks_parent1, &request1, -NULL ,0 ,NULL ,NULL ,NULL )) +if (is_access_to_paths_allowed ( + subject->domain, new_dir, access_request_parent1, + &layer_masks_parent1, &rule_flags_parent1, + &request1,NULL ,0 ,NULL ,NULL ,NULL ,NULL ))return 0 ;landlock_log_denial (subject, &request1);@@ -1201,10 +1217,12 @@ static int current_check_refer_path(struct dentry *const old_dentry,/* new_dir->dentry is equal to new_dentry->d_parent */ allow_parent1 =collect_domain_accesses (subject->domain, mnt_dir.dentry, old_parent, - &layer_masks_parent1); + &layer_masks_parent1, + &rule_flags_parent1); allow_parent2 =collect_domain_accesses (subject->domain, mnt_dir.dentry, new_dir->dentry, - &layer_masks_parent2); + &layer_masks_parent2, + &rule_flags_parent2);if (allow_parent1 && allow_parent2)return 0 ;@@ -1217,8 +1235,9 @@ static int current_check_refer_path(struct dentry *const old_dentry,*/ if (is_access_to_paths_allowed ( subject->domain, &mnt_dir, access_request_parent1, - &layer_masks_parent1, &request1, old_dentry, - access_request_parent2, &layer_masks_parent2, &request2, + &layer_masks_parent1, &rule_flags_parent1, &request1, + old_dentry, access_request_parent2, &layer_masks_parent2, + &rule_flags_parent2, &request2, exchange ? new_dentry :NULL ))return 0 ;
security/landlock/fs.c static bool is_device(const struct file *const file) · line 1638
Quoted 4 lines
security/landlock/fs.c +1
-0@@ -1616,6 +1635,7 @@ static bool is_device(const struct file *const file)static int hook_file_open (struct file *const file) {layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct collected_rule_flags rule_flags = {};
Ditto, please move after the other local variables.
Quoted 115 lines
security/landlock/fs.c +2
-1access_mask_t open_access_request, full_access_request, allowed_access, optional_access; const struct landlock_cred_security *const subject =@@ -1647,7 +1667,8 @@ static int hook_file_open(struct file *const file) landlock_init_layer_masks(subject->domain, full_access_request, &layer_masks, LANDLOCK_KEY_INODE), - &layer_masks, &request, NULL, 0, NULL, NULL, NULL)) { + &layer_masks, &rule_flags, &request, NULL, 0, NULL, NULL, + NULL, NULL)) { allowed_access = full_access_request; } else {unsigned long access_bit;security/landlock/net.c +2
-1@@ -48,6 +48,7 @@ static int current_check_access_socket(struct socket *const sock, {__be16 port;layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET ] = {}; +struct collected_rule_flags rule_flags = {};const struct landlock_rule *rule;struct landlock_id id = { .type =LANDLOCK_KEY_NET_PORT ,@@ -179,7 +180,7 @@ static int current_check_access_socket(struct socket *const sock, access_request, &layer_masks, LANDLOCK_KEY_NET_PORT);if (landlock_unmask_layers (rule, access_request, &layer_masks, -ARRAY_SIZE (layer_masks))) +ARRAY_SIZE (layer_masks), &rule_flags))return 0 ; audit_net.family = address->sa_family;security/landlock/ruleset.c +8
-1@@ -616,7 +616,8 @@ landlock_find_rule(const struct landlock_ruleset *const ruleset,bool landlock_unmask_layers (const struct landlock_rule *const rule,const access_mask_t access_request,layer_mask_t (*const layer_masks)[], -const size_t masks_array_size) +const size_t masks_array_size, +struct collected_rule_flags *const rule_flags) {size_t layer_level;@@ -643,6 +644,12 @@ bool landlock_unmask_layers(const struct landlock_rule *const rule,unsigned long access_bit;bool is_empty; +if (rule_flags) { +/* Collect rule flags for each layer */ +if (layer->flags.quiet) + rule_flags->quiet_masks |= layer_bit; + } +/* * Records in @layer_masks which layer grants access to each * requested access. security/landlock/ruleset.h +24
-2@@ -29,7 +29,18 @@ struct landlock_layer {/** * @level: Position of this layer in the layer stack. */ -u16 level; +u8 level; +/** +* @flags: Bitfield for special flags attached to this rule. +*/ +struct { +/** +* @quiet: Suppresses denial audit logs for the object covered by +* this rule in this domain. For filesystem rules, this inherits +* down the file hierarchy. +*/ +bool quiet:1 ; + } flags;/** * @access: Bitfield of allowed actions on the kernel object. They are * relative to the object type (e.g. %LANDLOCK_ACTION_FS_READ). @@ -37,6 +48,16 @@ struct landlock_layer {access_mask_t access; }; +/** +* struct collected_rule_flags - Hold accumulated flags for each layer +*/ +struct collected_rule_flags { +/** +* @quiet_masks: Layers for which the quiet flag is effective. +*/ +layer_mask_t quiet_masks; +}; +/** * union landlock_key - Key of a ruleset's red-black tree */ @@ -304,7 +325,8 @@ landlock_get_scope_mask(const struct landlock_ruleset *const ruleset,bool landlock_unmask_layers (const struct landlock_rule *const rule,const access_mask_t access_request,layer_mask_t (*const layer_masks)[], -const size_t masks_array_size); +const size_t masks_array_size, +struct collected_rule_flags *const rule_flags); access_mask_t landlock_init_layer_masks(const struct landlock_ruleset *const domain,Signature · 1 line
2.51.0
PATCH v2 2/6 landlock: Add API support and docs for the quiet flags
In reply to Tingmao WangAs for kselftests, for now we just change add_rule_checks_ordering to use a different invalid flag. I will add tests for the quiet flag in a later version.
According to pahole, even after adding the struct access_masks quiet_masks in struct landlock_hierarchy, the u32 log_* bitfield still only has a size of 2 bytes, so there's minimal wasted space.
---
Changes since v1:
- Per suggestion, added support for quieting only certain access bits, controlled by extra quiet_access_* fields in the ruleset_attr.
- Added docs for the extra fields and made updates to doc changes in v1. In particular, call out that the effect of LANDLOCK_ADD_RULE_QUIET is independent from the access bits passed in rule_attr
- landlock_add_rule will return -EINVAL when LANDLOCK_ADD_RULE_QUIET is used but the ruleset does not have any quiet access bits set for the given rule type.
- ABI version bump to v8
- Syntactic and comment changes per suggestion.
include/uapi/linux/landlock.h | 64 +++++++++++++++++ security/landlock/domain.h | 5 ++ security/landlock/fs.c | 4 +- security/landlock/fs.h | 2 +- security/landlock/net.c | 5 +- security/landlock/net.h | 3 +- security/landlock/ruleset.c | 10 ++- security/landlock/ruleset.h | 8 ++- security/landlock/syscalls.c | 72 +++++++++++++++----- tools/testing/selftests/landlock/base_test.c | 4 +- 10 files changed, 150 insertions(+), 27 deletions(-)
include/uapi/linux/landlock.h +64 -0
@@ -32,6 +32,19 @@* *handle* a wide range or all access rights that they know about at build time * (and that they have tested with a kernel that supported them all). * +* quiet_access_fs and quiet_access_net are bitmasks of actions for which +* a denial by this layer will not trigger an audit log if the +* corresponding object (or its children, for filesystem rules) is marked +* with the "quiet" bit via LANDLOCK_ADD_RULE_QUIET, even if audit logging +* would normally take place per landlock_restrict_self() flags. +* quiet_scoped is similar, except that it does not require marking any +* objects as quiet - if the ruleset is created with any bits set in +* quiet_scoped, then denial of such scoped resources will not trigger any +* audit log. These 3 fields are available since Landlock ABI version 8. +* +* quiet_access_fs, quiet_access_net and quiet_scoped must be a subset of +* handled_access_fs, handled_access_net and scoped respectively. +* * This structure can grow in future Landlock versions. */ struct landlock_ruleset_attr {
@@ -51,6 +64,24 @@ struct landlock_ruleset_attr {* resources (e.g. IPCs). */ __u64 scoped; + +/* Since ABI 8: */ + +/** +* @quiet_access_fs: Bitmask of filesystem actions which should not be +* audit logged if per-object quiet flag is set. +*/ +__u64 quiet_access_fs; +/** +* @quiet_access_net: Bitmask of network actions which should not be +* audit logged if per-object quiet flag is set. +*/ +__u64 quiet_access_net; +/** +* @quiet_scoped: Bitmask of scoped actions which should not be audit +* logged. +*/ +__u64 quiet_scoped; };/**
@@ -69,6 +100,39 @@ struct landlock_ruleset_attr {#define LANDLOCK_CREATE_RULESET_ERRATA (1U << 1)/* clang-format on */ +/** +* DOC: landlock_add_rule_flags +* +* **Flags** +* +* %LANDLOCK_ADD_RULE_QUIET +* Together with the quiet_* fields in struct landlock_ruleset_attr, +* this flag controls whether Landlock will log audit messages when +* access to the objects covered by this rule is denied by this layer. +* +* If audit logging is enabled, when Landlock denies an access, it will +* suppress the audit log if all of the following are true: +* +* - This layer is the innermost layer that denied the access; +* - The access is part of the quiet_* fields in the initial +* landlock_ruleset_attr; +* - The object (or one of its parents, for filesystem rules) is +* marked as "quiet" via LANDLOCK_ADD_RULE_QUIET. +* +* Because logging is only suppressed by a layer if the layer denies +* access, a sandboxed program cannot use this flag to "hide" access +* denials, without denying itself the access in the first place. +* +* The effect of this flag does not depend on the value of +* allowed_access in the passed in rule_attr. When this flag is +* present, the caller is also allowed to pass in an empty +* allowed_access. +*/ + +/* clang-format off */ +#define LANDLOCK_ADD_RULE_QUIET (1U << 0) +/* clang-format on */ +/** * DOC: landlock_restrict_self_flags *
security/landlock/domain.h +5 -0
@@ -114,6 +114,11 @@ struct landlock_hierarchy {* %LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON. Set to false by default. */ log_new_exec :1 ; +/** +* @quiet_masks: Bitmasks of access that should be quieted (i.e. not +* logged) if the related object is marked as quiet. +*/ +struct access_masks quiet_masks;#endif /* CONFIG_AUDIT */ };
security/landlock/fs.c +2 -2
@@ -322,7 +322,7 @@ static struct landlock_object *get_inode_object(struct inode *const inode)*/ int landlock_append_fs_rule (struct landlock_ruleset *const ruleset,const struct path *const path, -access_mask_t access_rights) +access_mask_t access_rights,const int flags) {int err;struct landlock_id id = {
@@ -343,7 +343,7 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,if (IS_ERR (id.key.object))return PTR_ERR (id.key.object);mutex_lock (&ruleset->lock); - err =landlock_insert_rule (ruleset, id, access_rights); + err =landlock_insert_rule (ruleset, id, access_rights, flags);mutex_unlock (&ruleset->lock);/* * No need to check for an error because landlock_insert_rule()
security/landlock/fs.h +1 -1
@@ -126,6 +126,6 @@ __init void landlock_add_fs_hooks(void);int landlock_append_fs_rule (struct landlock_ruleset *const ruleset,const struct path *const path, -access_mask_t access_hierarchy); +access_mask_t access_hierarchy,const int flags);#endif /* _SECURITY_LANDLOCK_FS_H */
security/landlock/net.c +3 -2
@@ -20,7 +20,8 @@#include "ruleset.h" int landlock_append_net_rule (struct landlock_ruleset *const ruleset, -const u16 port,access_mask_t access_rights) +const u16 port,access_mask_t access_rights, +const int flags) {int err;const struct landlock_id id = {
@@ -35,7 +36,7 @@ int landlock_append_net_rule(struct landlock_ruleset *const ruleset, ~landlock_get_net_access_mask (ruleset,0 );mutex_lock (&ruleset->lock); - err =landlock_insert_rule (ruleset, id, access_rights); + err =landlock_insert_rule (ruleset, id, access_rights, flags);mutex_unlock (&ruleset->lock);return err;
security/landlock/net.h +2 -1
@@ -16,7 +16,8 @@__init void landlock_add_net_hooks (void );int landlock_append_net_rule (struct landlock_ruleset *const ruleset, -const u16 port,access_mask_t access_rights); +const u16 port,access_mask_t access_rights, +const int flags);#else /* IS_ENABLED(CONFIG_INET) */ static inline void landlock_add_net_hooks (void ) {
security/landlock/ruleset.c +9 -1
@@ -21,6 +21,7 @@#include <linux/slab.h> #include <linux/spinlock.h> #include <linux/workqueue.h> +#include <uapi/linux/landlock.h> #include "access.h" #include "audit.h"
@@ -251,6 +252,7 @@ static int insert_rule(struct landlock_ruleset *const ruleset,if (WARN_ON_ONCE (this->layers[0 ].level !=0 ))return -EINVAL ; this->layers[0 ].access |= (*layers)[0 ].access; + this->layers[0 ].flags.quiet |= (*layers)[0 ].flags.quiet;return 0 ; }
@@ -297,12 +299,15 @@ static void build_check_layer(void)/* @ruleset must be locked by the caller. */ int landlock_insert_rule (struct landlock_ruleset *const ruleset,const struct landlock_id id, -const access_mask_t access) +const access_mask_t access,const int flags) {struct landlock_layer layers[] = { { .access = access,/* When @level is zero, insert_rule() extends @ruleset. */ .level =0 , + .flags = { + .quiet = !!(flags &LANDLOCK_ADD_RULE_QUIET ), + } } };build_check_layer ();
@@ -343,6 +348,7 @@ static int merge_tree(struct landlock_ruleset *const dst,return -EINVAL ; layers[0 ].access = walker_rule->layers[0 ].access; + layers[0 ].flags = walker_rule->layers[0 ].flags; err =insert_rule (dst, id, &layers,ARRAY_SIZE (layers));if (err)
@@ -573,6 +579,8 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,if (err)return ERR_PTR (err); + new_dom->hierarchy->quiet_masks = ruleset->quiet_masks; +return no_free_ptr (new_dom); }
security/landlock/ruleset.h +7 -1
@@ -193,6 +193,12 @@ struct landlock_ruleset {* non-merged ruleset (i.e. not a domain). */ u32 num_layers; +/** +* @quiet_masks: Stores the quiet flags for an unmerged +* ruleset. For a merged domain, this is stored in each +* layer's struct landlock_hierarchy instead. +*/ +struct access_masks quiet_masks;/** * @access_masks: Contains the subset of filesystem and * network actions that are restricted by a ruleset.
@@ -223,7 +229,7 @@ DEFINE_FREE(landlock_put_ruleset, struct landlock_ruleset *,int landlock_insert_rule (struct landlock_ruleset *const ruleset,const struct landlock_id id, -const access_mask_t access); +const access_mask_t access,const int flags);struct landlock_ruleset * landlock_merge_ruleset(struct landlock_ruleset *const parent,
security/landlock/syscalls.c +55 -17
189 lines not on this page · raw message
tools/testing/selftests/landlock/base_test.c +2 -2
@@ -76,7 +76,7 @@ TEST(abi_version)const struct landlock_ruleset_attr ruleset_attr = { .handled_access_fs =LANDLOCK_ACCESS_FS_READ_FILE , }; -ASSERT_EQ (7 ,landlock_create_ruleset (NULL ,0 , +ASSERT_EQ (8 ,landlock_create_ruleset (NULL ,0 ,LANDLOCK_CREATE_RULESET_VERSION )); ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
@@ -201,7 +201,7 @@ TEST(add_rule_checks_ordering)ASSERT_LE (0 , ruleset_fd);/* Checks invalid flags. */ -ASSERT_EQ (-1 ,landlock_add_rule (-1 ,0 ,NULL ,1 )); +ASSERT_EQ (-1 ,landlock_add_rule (-1 ,0 ,NULL ,100 ));ASSERT_EQ (EINVAL , errno);/* Checks invalid ruleset FD. */
Signature · 1 line
2.51.0
Mickaël Salaün
maintainerRe: [PATCH v2 2/6] landlock: Add API support and docs for the quiet flags
Quoted 40 lines · Tingmao Wang
As for kselftests, for now we just change
add_rule_checks_orderingto use a different invalid flag. I will add tests for the quiet flag in a later version.According to pahole, even after adding the struct
access_masksquiet_masksin structlandlock_hierarchy, the u32 log_* bitfield still only has a size of 2 bytes, so there's minimal wasted space.Signed-off-by Tingmao Wang <[email protected]>---
Changes since v1:
- Per suggestion, added support for quieting only certain access bits, controlled by extra quiet_access_* fields in theruleset_attr.
- Added docs for the extra fields and made updates to doc changes in v1. In particular, call out that the effect ofLANDLOCK_ADD_RULE_QUIETis independent from the access bits passed inrule_attr
-landlock_add_rulewill return-EINVALwhenLANDLOCK_ADD_RULE_QUIETis used but the ruleset does not have any quiet access bits set for the given rule type.
- ABI version bump to v8
- Syntactic and comment changes per suggestion.include/uapi/linux/landlock.h | 64 +++++++++++++++++ security/landlock/domain.h | 5 ++ security/landlock/fs.c | 4 +- security/landlock/fs.h | 2 +- security/landlock/net.c | 5 +- security/landlock/net.h | 3 +- security/landlock/ruleset.c | 10 ++- security/landlock/ruleset.h | 8 ++- security/landlock/syscalls.c | 72 +++++++++++++++----- tools/testing/selftests/landlock/base_test.c | 4 +- 10 files changed, 150 insertions(+), 27 deletions(-)include/uapi/linux/landlock.h +0
-0
include/uapi/linux/landlock.h line 35
Quoted 4 lines
include/uapi/linux/landlock.h +1
-0@@ -32,6 +32,19 @@* *handle* a wide range or all access rights that they know about at build time * (and that they have tested with a kernel that supported them all). * +* quiet_access_fs and quiet_access_net are bitmasks of actions for which
@quiet_access_fs and @quiet_access_fs
include/uapi/linux/landlock.h line 38
Quoted 3 lines
include/uapi/linux/landlock.h +3
-0+* a denial by this layer will not trigger an audit log if the +* corresponding object (or its children, for filesystem rules) is marked +* with the "quiet" bit via LANDLOCK_ADD_RULE_QUIET, even if audit logging
Why do you explicitly use "audit" before log? Wouldn't log be enough? The first "audit log" is fine but we can then just use "log".
include/uapi/linux/landlock.h line 45
Quoted 7 lines
include/uapi/linux/landlock.h +7
-0+* would normally take place per landlock_restrict_self() flags. +* quiet_scoped is similar, except that it does not require marking any +* objects as quiet - if the ruleset is created with any bits set in +* quiet_scoped, then denial of such scoped resources will not trigger any +* audit log. These 3 fields are available since Landlock ABI version 8. +* +* quiet_access_fs, quiet_access_net and quiet_scoped must be a subset of
ditto
include/uapi/linux/landlock.h line 46
Quoted 1 line
include/uapi/linux/landlock.h +1
-0+* handled_access_fs, handled_access_net and scoped respectively.
@handled_access_fs...
Quoted 16 lines
include/uapi/linux/landlock.h +9
-0+* * This structure can grow in future Landlock versions. */ struct landlock_ruleset_attr {@@ -51,6 +64,24 @@ struct landlock_ruleset_attr {* resources (e.g. IPCs). */ __u64 scoped; + +/* Since ABI 8: */ + +/** +* @quiet_access_fs: Bitmask of filesystem actions which should not be +* audit logged if per-object quiet flag is set. +*/ +__u64 quiet_access_fs;
include/uapi/linux/landlock.h struct landlock_ruleset_attr { · line 84
Quoted 10 lines
include/uapi/linux/landlock.h +10
-0+/** +* @quiet_access_net: Bitmask of network actions which should not be +* audit logged if per-object quiet flag is set. +*/ +__u64 quiet_access_net; +/** +* @quiet_scoped: Bitmask of scoped actions which should not be audit +* logged. +*/ +__u64 quiet_scoped;
For consistency, it would also make sense to be able to quiet implicit
restrictions: ptrace and FS topology changes. I'm not sure this is worth it for now wrt the existing landlock_restrict_self()'s log flags though.
We could add a ptrace scoped flag and a fs_change_topology "denied" flag (see https://lore.kernel.org/all/[email protected]/)
that would be set by default. Then a new quiet_denied field could also be added to landlock_ruleset_attr, and we'll get a consistent way to quiet any kind of audit log. But for now, let's stick with the current patch series.
Quoted 13 lines
include/uapi/linux/landlock.h +6
-0};/** @@ -69,6 +100,39 @@ struct landlock_ruleset_attr {#define LANDLOCK_CREATE_RULESET_ERRATA (1U << 1)/* clang-format on */ +/** +* DOC: landlock_add_rule_flags +* +* **Flags** +* +* %LANDLOCK_ADD_RULE_QUIET
include/uapi/linux/landlock.h struct landlock_ruleset_attr { · line 118
Quoted 10 lines
include/uapi/linux/landlock.h +10
-0+* Together with the quiet_* fields in struct landlock_ruleset_attr, +* this flag controls whether Landlock will log audit messages when +* access to the objects covered by this rule is denied by this layer. +* +* If audit logging is enabled, when Landlock denies an access, it will +* suppress the audit log if all of the following are true: +* +* - This layer is the innermost layer that denied the access; +* - The access is part of the quiet_* fields in the initial +* landlock_ruleset_attr;
I would say that *all* requested accesses are part of the quiet_* fields in the related struct landlock_ruleset_attr.
include/uapi/linux/landlock.h struct landlock_ruleset_attr { · line 120
Quoted 2 lines
include/uapi/linux/landlock.h +2
-0+* - The object (or one of its parents, for filesystem rules) is +* marked as "quiet" via LANDLOCK_ADD_RULE_QUIET.
Please consistently use %LANDLOCK_ADD_RULE_QUIET everywhere in the doc.
include/uapi/linux/landlock.h struct landlock_ruleset_attr { · line 130
Quoted 10 lines
include/uapi/linux/landlock.h +10
-0+* +* Because logging is only suppressed by a layer if the layer denies +* access, a sandboxed program cannot use this flag to "hide" access +* denials, without denying itself the access in the first place. +* +* The effect of this flag does not depend on the value of +* allowed_access in the passed in rule_attr. When this flag is +* present, the caller is also allowed to pass in an empty +* allowed_access. +*/
Excellent!
Quoted 408 lines
include/uapi/linux/landlock.h +5
-0+ +/* clang-format off */ +#define LANDLOCK_ADD_RULE_QUIET (1U << 0) +/* clang-format on */ +/** * DOC: landlock_restrict_self_flags * security/landlock/domain.h +5
-0@@ -114,6 +114,11 @@ struct landlock_hierarchy {* %LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON. Set to false by default. */ log_new_exec :1 ; +/** +* @quiet_masks: Bitmasks of access that should be quieted (i.e. not +* logged) if the related object is marked as quiet. +*/ +struct access_masks quiet_masks;#endif /* CONFIG_AUDIT */ };security/landlock/fs.c +2
-2@@ -322,7 +322,7 @@ static struct landlock_object *get_inode_object(struct inode *const inode)*/ int landlock_append_fs_rule (struct landlock_ruleset *const ruleset,const struct path *const path, -access_mask_t access_rights) +access_mask_t access_rights,const int flags) {int err;struct landlock_id id = {@@ -343,7 +343,7 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,if (IS_ERR (id.key.object))return PTR_ERR (id.key.object);mutex_lock (&ruleset->lock); - err =landlock_insert_rule (ruleset, id, access_rights); + err =landlock_insert_rule (ruleset, id, access_rights, flags);mutex_unlock (&ruleset->lock);/* * No need to check for an error because landlock_insert_rule() security/landlock/fs.h +1
-1@@ -126,6 +126,6 @@ __init void landlock_add_fs_hooks(void);int landlock_append_fs_rule (struct landlock_ruleset *const ruleset,const struct path *const path, -access_mask_t access_hierarchy); +access_mask_t access_hierarchy,const int flags);#endif /* _SECURITY_LANDLOCK_FS_H */ security/landlock/net.c +3
-2@@ -20,7 +20,8 @@#include "ruleset.h" int landlock_append_net_rule (struct landlock_ruleset *const ruleset, -const u16 port,access_mask_t access_rights) +const u16 port,access_mask_t access_rights, +const int flags) {int err;const struct landlock_id id = {@@ -35,7 +36,7 @@ int landlock_append_net_rule(struct landlock_ruleset *const ruleset, ~landlock_get_net_access_mask (ruleset,0 );mutex_lock (&ruleset->lock); - err =landlock_insert_rule (ruleset, id, access_rights); + err =landlock_insert_rule (ruleset, id, access_rights, flags);mutex_unlock (&ruleset->lock);return err;security/landlock/net.h +2
-1@@ -16,7 +16,8 @@__init void landlock_add_net_hooks (void );int landlock_append_net_rule (struct landlock_ruleset *const ruleset, -const u16 port,access_mask_t access_rights); +const u16 port,access_mask_t access_rights, +const int flags);#else /* IS_ENABLED(CONFIG_INET) */ static inline void landlock_add_net_hooks (void ) {security/landlock/ruleset.c +9
-1@@ -21,6 +21,7 @@#include <linux/slab.h> #include <linux/spinlock.h> #include <linux/workqueue.h> +#include <uapi/linux/landlock.h> #include "access.h" #include "audit.h" @@ -251,6 +252,7 @@ static int insert_rule(struct landlock_ruleset *const ruleset,if (WARN_ON_ONCE (this->layers[0 ].level !=0 ))return -EINVAL ; this->layers[0 ].access |= (*layers)[0 ].access; + this->layers[0 ].flags.quiet |= (*layers)[0 ].flags.quiet;return 0 ; }@@ -297,12 +299,15 @@ static void build_check_layer(void)/* @ruleset must be locked by the caller. */ int landlock_insert_rule (struct landlock_ruleset *const ruleset,const struct landlock_id id, -const access_mask_t access) +const access_mask_t access,const int flags) {struct landlock_layer layers[] = { { .access = access,/* When @level is zero, insert_rule() extends @ruleset. */ .level =0 , + .flags = { + .quiet = !!(flags &LANDLOCK_ADD_RULE_QUIET ), + } } };build_check_layer ();@@ -343,6 +348,7 @@ static int merge_tree(struct landlock_ruleset *const dst,return -EINVAL ; layers[0 ].access = walker_rule->layers[0 ].access; + layers[0 ].flags = walker_rule->layers[0 ].flags; err =insert_rule (dst, id, &layers,ARRAY_SIZE (layers));if (err)@@ -573,6 +579,8 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,if (err)return ERR_PTR (err); + new_dom->hierarchy->quiet_masks = ruleset->quiet_masks; +return no_free_ptr (new_dom); }security/landlock/ruleset.h +7
-1@@ -193,6 +193,12 @@ struct landlock_ruleset {* non-merged ruleset (i.e. not a domain). */ u32 num_layers; +/** +* @quiet_masks: Stores the quiet flags for an unmerged +* ruleset. For a merged domain, this is stored in each +* layer's struct landlock_hierarchy instead. +*/ +struct access_masks quiet_masks;/** * @access_masks: Contains the subset of filesystem and * network actions that are restricted by a ruleset. @@ -223,7 +229,7 @@ DEFINE_FREE(landlock_put_ruleset, struct landlock_ruleset *,int landlock_insert_rule (struct landlock_ruleset *const ruleset,const struct landlock_id id, -const access_mask_t access); +const access_mask_t access,const int flags);struct landlock_ruleset * landlock_merge_ruleset(struct landlock_ruleset *const parent,security/landlock/syscalls.c +55
-17@@ -102,8 +102,11 @@ static void build_check_abi(void) ruleset_size =sizeof (ruleset_attr.handled_access_fs); ruleset_size +=sizeof (ruleset_attr.handled_access_net); ruleset_size +=sizeof (ruleset_attr.scoped); + ruleset_size +=sizeof (ruleset_attr.quiet_access_fs); + ruleset_size +=sizeof (ruleset_attr.quiet_access_net); + ruleset_size +=sizeof (ruleset_attr.quiet_scoped);BUILD_BUG_ON (sizeof (ruleset_attr) != ruleset_size); -BUILD_BUG_ON (sizeof (ruleset_attr) !=24 ); +BUILD_BUG_ON (sizeof (ruleset_attr) !=48 ); path_beneath_size =sizeof (path_beneath_attr.allowed_access); path_beneath_size +=sizeof (path_beneath_attr.parent_fd);@@ -161,7 +164,7 @@ static const struct file_operations ruleset_fops = {* Documentation/userspace-api/landlock.rst should be updated to reflect the * UAPI change. */ -const int landlock_abi_version =7 ; +const int landlock_abi_version =8 ;/** * sys_landlock_create_ruleset - Create a new ruleset @@ -185,6 +188,8 @@ const int landlock_abi_version = 7;* * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time; * - %EINVAL: unknown @flags, or unknown access, or unknown scope, or too small @size; +* - %EINVAL: quiet_access_fs or quiet_fs_net is not a subset of the +* corresponding handled_access_fs or handled_access_net; * - %E2BIG: @attr or @size inconsistencies; * - %EFAULT: @attr or @size inconsistencies; * - %ENOMSG: empty &landlock_ruleset_attr.handled_access_fs. @@ -241,6 +246,21 @@ SYSCALL_DEFINE3(landlock_create_ruleset,if ((ruleset_attr.scoped |LANDLOCK_MASK_SCOPE ) !=LANDLOCK_MASK_SCOPE )return -EINVAL ; +/* +* Check that quiet masks are subsets of the respective handled masks. +* Because of the checks above this is sufficient to also ensure that +* the quiet masks are valid access masks. +*/ +if ((ruleset_attr.quiet_access_fs | ruleset_attr.handled_access_fs) != + ruleset_attr.handled_access_fs) +return -EINVAL ; +if ((ruleset_attr.quiet_access_net | ruleset_attr.handled_access_net) != + ruleset_attr.handled_access_net) +return -EINVAL ; +if ((ruleset_attr.quiet_scoped | ruleset_attr.scoped) != + ruleset_attr.scoped) +return -EINVAL ; +/* Checks arguments and transforms to kernel struct. */ ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs, ruleset_attr.handled_access_net,@@ -248,6 +268,10 @@ SYSCALL_DEFINE3(landlock_create_ruleset,if (IS_ERR (ruleset))return PTR_ERR (ruleset); + ruleset->quiet_masks.fs = ruleset_attr.quiet_access_fs; + ruleset->quiet_masks.net = ruleset_attr.quiet_access_net; + ruleset->quiet_masks.scope = ruleset_attr.quiet_scoped; +/* Creates anonymous FD referring to the ruleset. */ ruleset_fd =anon_inode_getfd ("[landlock-ruleset]" , &ruleset_fops, ruleset,O_RDWR |O_CLOEXEC );@@ -312,7 +336,7 @@ static int get_path_from_fd(const s32 fd, struct path *const path) }static int add_rule_path_beneath (struct landlock_ruleset *const ruleset, -const void __user *const rule_attr) +const void __user *const rule_attr,int flags) {struct landlock_path_beneath_attr path_beneath_attr;struct path path;@@ -327,9 +351,10 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,/* * Informs about useless rule: empty allowed_access (i.e. deny rules) -* are ignored in path walks. +* are ignored in path walks. However, the rule is not useless if it +* is there to hold a quiet flag */ -if (!path_beneath_attr.allowed_access) +if (!flags && !path_beneath_attr.allowed_access)return -ENOMSG ;/* Checks that allowed_access matches the @ruleset constraints. */ @@ -337,6 +362,10 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,if ((path_beneath_attr.allowed_access | mask) != mask)return -EINVAL ; +/* Check for useless quiet flag */ +if (flags &LANDLOCK_ADD_RULE_QUIET && !ruleset->quiet_masks.fs) +return -EINVAL ; +/* Gets and checks the new rule. */ err =get_path_from_fd (path_beneath_attr.parent_fd, &path);if (err)@@ -344,13 +373,13 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,/* Imports the new rule. */ err =landlock_append_fs_rule (ruleset, &path, - path_beneath_attr.allowed_access); + path_beneath_attr.allowed_access, flags);path_put (&path);return err; }static int add_rule_net_port (struct landlock_ruleset *ruleset, -const void __user *const rule_attr) +const void __user *const rule_attr,int flags) {struct landlock_net_port_attr net_port_attr;int res;@@ -363,9 +392,10 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,/* * Informs about useless rule: empty allowed_access (i.e. deny rules) -* are ignored by network actions. +* are ignored by network actions. However, the rule is not useless +* if it is there to hold a quiet flag */ -if (!net_port_attr.allowed_access) +if (!flags && !net_port_attr.allowed_access)return -ENOMSG ;/* Checks that allowed_access matches the @ruleset constraints. */ @@ -373,13 +403,17 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,if ((net_port_attr.allowed_access | mask) != mask)return -EINVAL ; +/* Check for useless quiet flag */ +if (flags &LANDLOCK_ADD_RULE_QUIET && !ruleset->quiet_masks.net) +return -EINVAL ; +/* Denies inserting a rule with port greater than 65535. */ if (net_port_attr.port >U16_MAX )return -EINVAL ;/* Imports the new rule. */ return landlock_append_net_rule (ruleset, net_port_attr.port, - net_port_attr.allowed_access); + net_port_attr.allowed_access, flags); }/** @@ -390,7 +424,7 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,* @rule_type: Identify the structure type pointed to by @rule_attr: * %LANDLOCK_RULE_PATH_BENEATH or %LANDLOCK_RULE_NET_PORT. * @rule_attr: Pointer to a rule (matching the @rule_type). -* @flags: Must be 0. +* @flags: Must be 0 or %LANDLOCK_ADD_RULE_QUIET. * * This system call enables to define a new rule and add it to an existing * ruleset. @@ -400,20 +434,25 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,* - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time; * - %EAFNOSUPPORT: @rule_type is %LANDLOCK_RULE_NET_PORT but TCP/IP is not * supported by the running kernel; -* - %EINVAL: @flags is not 0; +* - %EINVAL: @flags is not valid; * - %EINVAL: The rule accesses are inconsistent (i.e. * &landlock_path_beneath_attr.allowed_access or * &landlock_net_port_attr.allowed_access is not a subset of the ruleset * handled accesses) * - %EINVAL: &landlock_net_port_attr.port is greater than 65535; +* - %EINVAL: LANDLOCK_ADD_RULE_QUIET is passed but the ruleset has no +* quiet access bits set for the corresponding rule type. * - %ENOMSG: Empty accesses (e.g. &landlock_path_beneath_attr.allowed_access is -* 0); +* 0) and no flags; * - %EBADF: @ruleset_fd is not a file descriptor for the current thread, or a * member of @rule_attr is not a file descriptor as expected; * - %EBADFD: @ruleset_fd is not a ruleset file descriptor, or a member of * @rule_attr is not the expected file descriptor type; * - %EPERM: @ruleset_fd has no write access to the underlying ruleset; * - %EFAULT: @rule_attr was not a valid address. +* +* .. kernel-doc:: include/uapi/linux/landlock.h +* :identifiers: landlock_add_rule_flags */ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd, const enum landlock_rule_type, rule_type,@@ -424,8 +463,7 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,if (!is_initialized ())return -EOPNOTSUPP ; -/* No flag for now. */ -if (flags) +if (flags && flags !=LANDLOCK_ADD_RULE_QUIET )return -EINVAL ;/* Gets and checks the ruleset. */ @@ -435,9 +473,9 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,switch (rule_type) {case LANDLOCK_RULE_PATH_BENEATH : -return add_rule_path_beneath (ruleset, rule_attr); +return add_rule_path_beneath (ruleset, rule_attr, flags);case LANDLOCK_RULE_NET_PORT : -return add_rule_net_port (ruleset, rule_attr); +return add_rule_net_port (ruleset, rule_attr, flags);default :return -EINVAL ; }tools/testing/selftests/landlock/base_test.c +2
-2@@ -76,7 +76,7 @@ TEST(abi_version)const struct landlock_ruleset_attr ruleset_attr = { .handled_access_fs =LANDLOCK_ACCESS_FS_READ_FILE , }; -ASSERT_EQ (7 ,landlock_create_ruleset (NULL ,0 , +ASSERT_EQ (8 ,landlock_create_ruleset (NULL ,0 ,LANDLOCK_CREATE_RULESET_VERSION )); ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,@@ -201,7 +201,7 @@ TEST(add_rule_checks_ordering)ASSERT_LE (0 , ruleset_fd);/* Checks invalid flags. */ -ASSERT_EQ (-1 ,landlock_add_rule (-1 ,0 ,NULL ,1 )); +ASSERT_EQ (-1 ,landlock_add_rule (-1 ,0 ,NULL ,100 ));ASSERT_EQ (EINVAL , errno);/* Checks invalid ruleset FD. */ Signature · 1 line
2.51.0
PATCH v2 3/6 landlock/audit: Check for quiet flag in landlock_log_denial
In reply to Tingmao WangSuppresses logging if the flag is effective on the youngest layer.
This does not handle optional access logging yet - to do that correctly we will need to expand deny_masks to support representing "don't log anything" in a later commit.
---
Changes since v1:
- Supports the new quiet access masks.
- Support quieting scope requests (but not ptrace and attempted mounting for now)
security/landlock/audit.c | 70 +++++++++++++++++++++++++++++++++++-- security/landlock/audit.h | 3 +- security/landlock/fs.c | 18 +++++----- security/landlock/net.c | 3 +- security/landlock/ruleset.h | 5 +++ security/landlock/task.c | 12 +++---- 6 files changed, 92 insertions(+), 19 deletions(-)
security/landlock/audit.c +68 -2
@@ -381,19 +381,39 @@ static bool is_valid_request(const struct landlock_request *const request)return true; } +static access_mask_t +pick_access_mask_for_req_type (const enum landlock_request_type type, +const struct access_masks access_masks) +{ +switch (type) { +case LANDLOCK_REQUEST_FS_ACCESS : +return access_masks.fs; +case LANDLOCK_REQUEST_NET_ACCESS : +return access_masks.net; +default : +WARN_ONCE (1 ,"Invalid request type %d passed to %s" , type, + __func__); +return 0 ; + } +} +/** * landlock_log_denial - Create audit records related to a denial * * @subject: The Landlock subject's credential denying an action. * @request: Detail of the user space request. +* @rule_flags: The flags for the matched rule, or no_rule_flags (zero) if +* this is a scope request (no particular object involved). */ void landlock_log_denial (const struct landlock_cred_security *const subject, -const struct landlock_request *const request) +const struct landlock_request *const request, +const struct collected_rule_flags rule_flags) {struct audit_buffer *ab;struct landlock_hierarchy *youngest_denied;size_t youngest_layer; -access_mask_t missing; +access_mask_t missing, quiet_mask; +bool quiet_flag_on_rule = false, quiet_applicable_to_access = false;if (WARN_ON_ONCE (!subject || !subject->domain || !subject->domain->hierarchy || !request))
@@ -436,6 +456,52 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,if (!audit_enabled)return ; +/* +* Checks if the object is marked quiet by the layer that denied the +* request. If it's a different layer that marked it as quiet, but +* that layer is not the one that denied the request, we should still +* audit log the denial. +*/ + quiet_flag_on_rule = !!(rule_flags.quiet_masks &BIT (youngest_layer)); + +if (quiet_flag_on_rule) { +/* +* This is not a scope request, since rule_flags is not zero. We +* now check if the denied requests are all covered by the layer's +* quiet access bits. +*/ + quiet_mask =pick_access_mask_for_req_type ( + request->type, youngest_denied->quiet_masks); + quiet_applicable_to_access = (quiet_mask & missing) == missing; + +if (quiet_applicable_to_access) +return ; + }else { + quiet_mask = youngest_denied->quiet_masks.scope; +switch (request->type) { +case LANDLOCK_REQUEST_SCOPE_SIGNAL : + quiet_applicable_to_access = + !!(quiet_mask &LANDLOCK_SCOPE_SIGNAL ); +break ; +case LANDLOCK_REQUEST_SCOPE_ABSTRACT_UNIX_SOCKET : + quiet_applicable_to_access = + !!(quiet_mask & +LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET ); +break ; +/* +* Leave LANDLOCK_REQUEST_PTRACE and +* LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY unhandled for now - they are +* never quiet +*/ +default : +break ; + } + +if (quiet_applicable_to_access) { +return ; + } + } +/* Checks if the current exec was restricting itself. */ if (subject->domain_exec &BIT (youngest_layer)) {/* Ignores denials for the same execution. */
security/landlock/audit.h +2 -1
@@ -56,7 +56,8 @@ struct landlock_request {void landlock_log_drop_domain (const struct landlock_hierarchy *const hierarchy);void landlock_log_denial (const struct landlock_cred_security *const subject, -const struct landlock_request *const request); +const struct landlock_request *const request, +const struct collected_rule_flags rule_flags);#else /* CONFIG_AUDIT */
security/landlock/fs.c +9 -9
@@ -984,7 +984,7 @@ static int current_check_access_path(const struct path *const path, NULL, 0, NULL, NULL, NULL, NULL))return 0 ; -landlock_log_denial (subject, &request); +landlock_log_denial (subject, &request, rule_flags);return -EACCES ; }
@@ -1194,7 +1194,7 @@ static int current_check_refer_path(struct dentry *const old_dentry, &request1, NULL, 0, NULL, NULL, NULL, NULL))return 0 ; -landlock_log_denial (subject, &request1); +landlock_log_denial (subject, &request1, rule_flags_parent1);return -EACCES ; }
@@ -1243,11 +1243,11 @@ static int current_check_refer_path(struct dentry *const old_dentry,if (request1.access) { request1.audit.u.path.dentry = old_parent; -landlock_log_denial (subject, &request1); +landlock_log_denial (subject, &request1, rule_flags_parent1); }if (request2.access) { request2.audit.u.path.dentry = new_dir->dentry; -landlock_log_denial (subject, &request2); +landlock_log_denial (subject, &request2, rule_flags_parent2); }/*
@@ -1403,7 +1403,7 @@ log_fs_change_topology_path(const struct landlock_cred_security *const subject, .u.path = *path, }, .layer_plus_one = handle_layer + 1, - }); + }, no_rule_flags); } static void log_fs_change_topology_dentry(
@@ -1417,7 +1417,7 @@ static void log_fs_change_topology_dentry( .u.dentry = dentry, }, .layer_plus_one = handle_layer + 1, - }); + }, no_rule_flags); }/*
@@ -1705,7 +1705,7 @@ static int hook_file_open(struct file *const file)/* Sets access to reflect the actual request. */ request.access = open_access_request; -landlock_log_denial (subject, &request); +landlock_log_denial (subject, &request, rule_flags);return -EACCES ; }
@@ -1735,7 +1735,7 @@ static int hook_file_truncate(struct file *const file)#ifdef CONFIG_AUDIT .deny_masks = landlock_file(file)->deny_masks,#endif /* CONFIG_AUDIT */ - }); + }, no_rule_flags);return -EACCES ; }
@@ -1774,7 +1774,7 @@ static int hook_file_ioctl_common(const struct file *const file,#ifdef CONFIG_AUDIT .deny_masks = landlock_file(file)->deny_masks,#endif /* CONFIG_AUDIT */ - }); + }, no_rule_flags);return -EACCES ; }
security/landlock/net.c +2 -1
@@ -193,7 +193,8 @@ static int current_check_access_socket(struct socket *const sock, .access = access_request, .layer_masks = &layer_masks, .layer_masks_size =ARRAY_SIZE (layer_masks), - }); + }, + rule_flags);return -EACCES ; }
security/landlock/ruleset.h +5 -0
@@ -58,6 +58,11 @@ struct collected_rule_flags {layer_mask_t quiet_masks; }; +/** +* no_rule_flags - Convenience constant for an empty collected_rule_flags +*/ +static const struct collected_rule_flags no_rule_flags = {0 }; +/** * union landlock_key - Key of a ruleset's red-black tree */
security/landlock/task.c +6 -6
@@ -115,7 +115,7 @@ static int hook_ptrace_access_check(struct task_struct *const child, .u.tsk = child, }, .layer_plus_one = parent_subject->domain->num_layers, - }); + }, no_rule_flags);return err; }
@@ -161,7 +161,7 @@ static int hook_ptrace_traceme(struct task_struct *const parent) .u.tsk = current, }, .layer_plus_one = parent_subject->domain->num_layers, - }); + }, no_rule_flags);return err; }
@@ -290,7 +290,7 @@ static int hook_unix_stream_connect(struct sock *const sock, }, }, .layer_plus_one = handle_layer + 1, - }); + }, no_rule_flags);return -EPERM ; }
@@ -327,7 +327,7 @@ static int hook_unix_may_send(struct socket *const sock, }, }, .layer_plus_one = handle_layer + 1, - }); + }, no_rule_flags);return -EPERM ; }
@@ -383,7 +383,7 @@ static int hook_task_kill(struct task_struct *const p, .u.tsk = p, }, .layer_plus_one = handle_layer + 1, - }); + }, no_rule_flags);return -EPERM ; }
@@ -426,7 +426,7 @@ static int hook_file_send_sigiotask(struct task_struct *tsk,#ifdef CONFIG_AUDIT .layer_plus_one = landlock_file(fown->file)->fown_layer + 1,#endif /* CONFIG_AUDIT */ - }); + }, no_rule_flags);return -EPERM ; }
Signature · 1 line
2.51.0
Mickaël Salaün
maintainerRe: [PATCH v2 3/6] landlock/audit: Check for quiet flag in landlock_log_denial
Just use "landlock: " as subject prefix.
Quoted 27 lines · Tingmao Wang
Suppresses logging if the flag is effective on the youngest layer.
This does not handle optional access logging yet - to do that correctly we will need to expand
deny_masksto support representing "don't log anything" in a later commit.Signed-off-by Tingmao Wang <[email protected]>---
Changes since v1:
- Supports the new quiet access masks.
- Support quieting scope requests (but not ptrace and attempted mounting for now)security/landlock/audit.c | 70 +++++++++++++++++++++++++++++++++++-- security/landlock/audit.h | 3 +- security/landlock/fs.c | 18 +++++----- security/landlock/net.c | 3 +- security/landlock/ruleset.h | 5 +++ security/landlock/task.c | 12 +++---- 6 files changed, 92 insertions(+), 19 deletions(-)security/landlock/audit.c +0
-0
security/landlock/audit.c static bool is_valid_request(const struct landlock_request *const request) · line 385
Quoted 5 lines
security/landlock/audit.c +2
-0@@ -381,19 +381,39 @@ static bool is_valid_request(const struct landlock_request *const request)return true; } +static access_mask_t +pick_access_mask_for_req_type(const enum landlock_request_type type,
pick_access_mask_for_request_type
Quoted 67 lines
security/landlock/audit.c +47
-2+ const struct access_masks access_masks) +{ +switch (type) { +case LANDLOCK_REQUEST_FS_ACCESS : +return access_masks.fs; +case LANDLOCK_REQUEST_NET_ACCESS : +return access_masks.net; +default : +WARN_ONCE (1 ,"Invalid request type %d passed to %s" , type, + __func__); +return 0 ; + } +} +/** * landlock_log_denial - Create audit records related to a denial * * @subject: The Landlock subject's credential denying an action. * @request: Detail of the user space request. +* @rule_flags: The flags for the matched rule, or no_rule_flags (zero) if +* this is a scope request (no particular object involved). */ void landlock_log_denial (const struct landlock_cred_security *const subject, -const struct landlock_request *const request) +const struct landlock_request *const request, +const struct collected_rule_flags rule_flags) {struct audit_buffer *ab;struct landlock_hierarchy *youngest_denied;size_t youngest_layer; -access_mask_t missing; +access_mask_t missing, quiet_mask; +bool quiet_flag_on_rule = false, quiet_applicable_to_access = false;if (WARN_ON_ONCE (!subject || !subject->domain || !subject->domain->hierarchy || !request))@@ -436,6 +456,52 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,if (!audit_enabled)return ; +/* +* Checks if the object is marked quiet by the layer that denied the +* request. If it's a different layer that marked it as quiet, but +* that layer is not the one that denied the request, we should still +* audit log the denial. +*/ + quiet_flag_on_rule = !!(rule_flags.quiet_masks &BIT (youngest_layer)); + +if (quiet_flag_on_rule) { +/* +* This is not a scope request, since rule_flags is not zero. We +* now check if the denied requests are all covered by the layer's +* quiet access bits. +*/ + quiet_mask =pick_access_mask_for_req_type ( + request->type, youngest_denied->quiet_masks); + quiet_applicable_to_access = (quiet_mask & missing) == missing; + +if (quiet_applicable_to_access) +return ; + }else { + quiet_mask = youngest_denied->quiet_masks.scope; +switch (request->type) { +case LANDLOCK_REQUEST_SCOPE_SIGNAL : + quiet_applicable_to_access = + !!(quiet_mask &LANDLOCK_SCOPE_SIGNAL ); +break ;
security/landlock/audit.c void landlock_log_denial(const struct landlock_cred_security *const subject, · line 495
Quoted 10 lines
security/landlock/audit.c +10
-0+case LANDLOCK_REQUEST_SCOPE_ABSTRACT_UNIX_SOCKET : + quiet_applicable_to_access = + !!(quiet_mask & +LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET ); +break ; +/* +* Leave LANDLOCK_REQUEST_PTRACE and +* LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY unhandled for now - they are +* never quiet +*/
This also covers the case where the object is not quiet.
security/landlock/audit.c void landlock_log_denial(const struct landlock_cred_security *const subject, · line 498
Quoted 3 lines
security/landlock/audit.c +3
-0+default : +break ; + }
I find this if/else block a bit verbose but I didn't find a better way...
security/landlock/audit.c void landlock_log_denial(const struct landlock_cred_security *const subject, · line 502
Quoted 4 lines
security/landlock/audit.c +4
-0+ +if (quiet_applicable_to_access) { +return ; + }
We can still move this quiet_applicable_to_access check after the block (and without the curly braces).
security/landlock/audit.c void landlock_log_denial(const struct landlock_cred_security *const subject, · line 507
Quoted 5 lines
security/landlock/audit.c +2
-0+ } +/* Checks if the current exec was restricting itself. */ if (subject->domain_exec &BIT (youngest_layer)) {/* Ignores denials for the same execution. */
This domain_exec block would be better before the quiet_flag_on_rule use.
Quoted 114 lines
security/landlock/audit.h +2
-1@@ -56,7 +56,8 @@ struct landlock_request {void landlock_log_drop_domain (const struct landlock_hierarchy *const hierarchy);void landlock_log_denial (const struct landlock_cred_security *const subject, -const struct landlock_request *const request); +const struct landlock_request *const request, +const struct collected_rule_flags rule_flags);#else /* CONFIG_AUDIT */ security/landlock/fs.c +9
-9@@ -984,7 +984,7 @@ static int current_check_access_path(const struct path *const path, NULL, 0, NULL, NULL, NULL, NULL))return 0 ; -landlock_log_denial (subject, &request); +landlock_log_denial (subject, &request, rule_flags);return -EACCES ; }@@ -1194,7 +1194,7 @@ static int current_check_refer_path(struct dentry *const old_dentry, &request1, NULL, 0, NULL, NULL, NULL, NULL))return 0 ; -landlock_log_denial (subject, &request1); +landlock_log_denial (subject, &request1, rule_flags_parent1);return -EACCES ; }@@ -1243,11 +1243,11 @@ static int current_check_refer_path(struct dentry *const old_dentry,if (request1.access) { request1.audit.u.path.dentry = old_parent; -landlock_log_denial (subject, &request1); +landlock_log_denial (subject, &request1, rule_flags_parent1); }if (request2.access) { request2.audit.u.path.dentry = new_dir->dentry; -landlock_log_denial (subject, &request2); +landlock_log_denial (subject, &request2, rule_flags_parent2); }/* @@ -1403,7 +1403,7 @@ log_fs_change_topology_path(const struct landlock_cred_security *const subject, .u.path = *path, }, .layer_plus_one = handle_layer + 1, - }); + }, no_rule_flags); } static void log_fs_change_topology_dentry(@@ -1417,7 +1417,7 @@ static void log_fs_change_topology_dentry( .u.dentry = dentry, }, .layer_plus_one = handle_layer + 1, - }); + }, no_rule_flags); }/* @@ -1705,7 +1705,7 @@ static int hook_file_open(struct file *const file)/* Sets access to reflect the actual request. */ request.access = open_access_request; -landlock_log_denial (subject, &request); +landlock_log_denial (subject, &request, rule_flags);return -EACCES ; }@@ -1735,7 +1735,7 @@ static int hook_file_truncate(struct file *const file)#ifdef CONFIG_AUDIT .deny_masks = landlock_file(file)->deny_masks,#endif /* CONFIG_AUDIT */ - }); + }, no_rule_flags);return -EACCES ; }@@ -1774,7 +1774,7 @@ static int hook_file_ioctl_common(const struct file *const file,#ifdef CONFIG_AUDIT .deny_masks = landlock_file(file)->deny_masks,#endif /* CONFIG_AUDIT */ - }); + }, no_rule_flags);return -EACCES ; }security/landlock/net.c +2
-1@@ -193,7 +193,8 @@ static int current_check_access_socket(struct socket *const sock, .access = access_request, .layer_masks = &layer_masks, .layer_masks_size =ARRAY_SIZE (layer_masks), - }); + }, + rule_flags);return -EACCES ; }security/landlock/ruleset.h +0
-0
security/landlock/ruleset.h struct collected_rule_flags { · line 64
Quoted 7 lines
security/landlock/ruleset.h +4
-0@@ -58,6 +58,11 @@ struct collected_rule_flags {layer_mask_t quiet_masks; }; +/** +* no_rule_flags - Convenience constant for an empty collected_rule_flags +*/ +static const struct collected_rule_flags no_rule_flags = {0 };
You can remove the "0" for consistency.
Quoted 65 lines
security/landlock/ruleset.h +1
-0+/** * union landlock_key - Key of a ruleset's red-black tree */ security/landlock/task.c +6
-6@@ -115,7 +115,7 @@ static int hook_ptrace_access_check(struct task_struct *const child, .u.tsk = child, }, .layer_plus_one = parent_subject->domain->num_layers, - }); + }, no_rule_flags);return err; }@@ -161,7 +161,7 @@ static int hook_ptrace_traceme(struct task_struct *const parent) .u.tsk = current, }, .layer_plus_one = parent_subject->domain->num_layers, - }); + }, no_rule_flags);return err; }@@ -290,7 +290,7 @@ static int hook_unix_stream_connect(struct sock *const sock, }, }, .layer_plus_one = handle_layer + 1, - }); + }, no_rule_flags);return -EPERM ; }@@ -327,7 +327,7 @@ static int hook_unix_may_send(struct socket *const sock, }, }, .layer_plus_one = handle_layer + 1, - }); + }, no_rule_flags);return -EPERM ; }@@ -383,7 +383,7 @@ static int hook_task_kill(struct task_struct *const p, .u.tsk = p, }, .layer_plus_one = handle_layer + 1, - }); + }, no_rule_flags);return -EPERM ; }@@ -426,7 +426,7 @@ static int hook_file_send_sigiotask(struct task_struct *tsk,#ifdef CONFIG_AUDIT .layer_plus_one = landlock_file(fown->file)->fown_layer + 1,#endif /* CONFIG_AUDIT */ - }); + }, no_rule_flags);return -EPERM ; }Signature · 1 line
2.51.0
Re: [PATCH v2 3/6] landlock/audit: Check for quiet flag in landlock_log_denial
Quoted 1 line · Mickaël Salaün
Just use "landlock: " as subject prefix.
What about samples/landlock or selftests/landlock (in the next version)? Should they also just be landlock: ?
Mickaël Salaün
maintainerRe: [PATCH v2 3/6] landlock/audit: Check for quiet flag in landlock_log_denial
Quoted 5 lines · Tingmao Wang
Quoted 1 line · Mickaël Salaün
Just use "landlock: " as subject prefix.
What about
samples/landlockor selftests/landlock (in the next version)? Should they also just be landlock: ?
No, those are good. The idea is that these three prefixes identify Landlock-related code (and potential different/complementary maintainers) in different places. It's not useful to identify part of Landlock inside one file hierarchy. In doubt, just follow the same pattern as in previous commits:
- samples/landlock: Files in samples/landlock/
- selftests/landlock: Files in tools/testing/selftests/landlock/
- landlock: Files in security/landlock/, Documentation/*/landlock.rst, and Landlock catchall.
Re: [PATCH v2 3/6] landlock/audit: Check for quiet flag in landlock_log_denial
In reply to Mickaël SalaünQuoted 48 lines · Mickaël Salaün
Just use "landlock: " as subject prefix.
Quoted 32 lines · Tingmao Wang
[...]
security/landlock/audit.c +27
-0@@ -436,6 +456,52 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,if (!audit_enabled)return ; +/* +* Checks if the object is marked quiet by the layer that denied the +* request. If it's a different layer that marked it as quiet, but +* that layer is not the one that denied the request, we should still +* audit log the denial. +*/ + quiet_flag_on_rule = !!(rule_flags.quiet_masks &BIT (youngest_layer)); + +if (quiet_flag_on_rule) { +/* +* This is not a scope request, since rule_flags is not zero. We +* now check if the denied requests are all covered by the layer's +* quiet access bits. +*/ + quiet_mask =pick_access_mask_for_req_type ( + request->type, youngest_denied->quiet_masks); + quiet_applicable_to_access = (quiet_mask & missing) == missing; + +if (quiet_applicable_to_access) +return ; + }else { + quiet_mask = youngest_denied->quiet_masks.scope; +switch (request->type) { +case LANDLOCK_REQUEST_SCOPE_SIGNAL : + quiet_applicable_to_access = + !!(quiet_mask &LANDLOCK_SCOPE_SIGNAL ); +break ;security/landlock/audit.c void landlock_log_denial(const struct landlock_cred_security *const subject, · line 495
Quoted 10 lines
security/landlock/audit.c +10
-0+case LANDLOCK_REQUEST_SCOPE_ABSTRACT_UNIX_SOCKET : + quiet_applicable_to_access = + !!(quiet_mask & +LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET ); +break ; +/* +* Leave LANDLOCK_REQUEST_PTRACE and +* LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY unhandled for now - they are +* never quiet +*/ This also covers the case where the object is not quiet.
Quoted 3 lines
security/landlock/audit.c void landlock_log_denial(const struct landlock_cred_security *const subject, · line 498
Quoted 3 lines
security/landlock/audit.c +3
-0+default : +break ; + }
I went with adding a comment above the line
quiet_mask = youngest_denied->quiet_masks.scope;
PATCH v2 4/6 landlock/audit: Fix wrong type usage
In reply to Tingmao WangI think, based on my best understanding, that this type is likely a typo (even though in the end both are u16)
---
Changes since v1:
- Added Fixes tag
security/landlock/audit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
security/landlock/audit.c +1 -1
@@ -191,7 +191,7 @@ static size_t get_denied_layer(const struct landlock_ruleset *const domain,long youngest_layer =-1 ;for_each_set_bit (access_bit, &access_req, layer_masks_size) { -const access_mask_t mask = (*layer_masks)[access_bit]; +const layer_mask_t mask = (*layer_masks)[access_bit];long layer;if (!mask)
Signature · 1 line
2.51.0
PATCH v2 5/6 samples/landlock: Add quiet flag support to sandboxer
In reply to Tingmao WangAdds ability to set which access bits to quiet via LL_*_QUIET_ACCESS, and attach quiet flags to individual objects via LL_*_QUIET for fs and net.
---
Changes since v1:
- Added new environment variables to control which quiet access bits to set on the rule, and populate quiet_access_* from it.
- Added support for quieting net rules and scoped access. Renamed patch title.
- Increment ABI version
samples/landlock/sandboxer.c | 133 ++++++++++++++++++++++++++++++++--- 1 file changed, 124 insertions(+), 9 deletions(-)
samples/landlock/sandboxer.c +124 -9
@@ -58,9 +58,14 @@ static inline int landlock_restrict_self(const int ruleset_fd,#define ENV_FS_RO_NAME "LL_FS_RO"#define ENV_FS_RW_NAME "LL_FS_RW" +#define ENV_FS_QUIET_NAME "LL_FS_QUIET" +#define ENV_FS_QUIET_ACCESS_NAME "LL_FS_QUIET_ACCESS"#define ENV_TCP_BIND_NAME "LL_TCP_BIND"#define ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT" +#define ENV_NET_QUIET_NAME "LL_NET_QUIET" +#define ENV_NET_QUIET_ACCESS_NAME "LL_NET_QUIET_ACCESS"#define ENV_SCOPED_NAME "LL_SCOPED" +#define ENV_SCOPED_QUIET_ACCESS_NAME "LL_SCOPED_QUIET_ACCESS"#define ENV_FORCE_LOG_NAME "LL_FORCE_LOG"#define ENV_DELIMITER ":"
@@ -116,7 +121,7 @@ static int parse_path(char *env_path, const char ***const path_list)/* clang-format on */ static int populate_ruleset_fs (const char *const env_var,const int ruleset_fd, -const __u64 allowed_access) +const __u64 allowed_access,bool quiet) {int num_paths, i, ret =1 ;char *env_path_name;
@@ -166,7 +171,8 @@ static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,if (!S_ISDIR (statbuf.st_mode)) path_beneath.allowed_access &=ACCESS_FILE ;if (landlock_add_rule (ruleset_fd,LANDLOCK_RULE_PATH_BENEATH , - &path_beneath,0 )) { + &path_beneath, + quiet ?LANDLOCK_ADD_RULE_QUIET :0 )) {fprintf (stderr,"Failed to update the ruleset with \"%s\": %s\n" , path_list[i],strerror (errno));
@@ -184,7 +190,7 @@ static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd, }static int populate_ruleset_net (const char *const env_var,const int ruleset_fd, -const __u64 allowed_access) +const __u64 allowed_access,bool quiet) {int ret =1 ;char *env_port_name, *env_port_name_next, *strport;
@@ -212,7 +218,8 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd, } net_port.port = port;if (landlock_add_rule (ruleset_fd,LANDLOCK_RULE_NET_PORT , - &net_port,0 )) { + &net_port, + quiet ?LANDLOCK_ADD_RULE_QUIET :0 )) {fprintf (stderr,"Failed to update the ruleset with port \"%llu\": %s\n" , net_port.port,strerror (errno));
@@ -299,7 +306,55 @@ static bool check_ruleset_scope(const char *const env_var,/* clang-format on */ -#define LANDLOCK_ABI_LAST 7 +static int add_quiet_access (__u64 *const quiet_access, +const __u64 handled_access, +const char *const env_var,const bool default_all) +{ +char *env_quiet_access, *env_quiet_access_next, *str_access; + +if (default_all) + *quiet_access = handled_access; +else + *quiet_access =0 ; + + env_quiet_access =getenv (env_var); +if (!env_quiet_access) +return 0 ; + + env_quiet_access =strdup (env_quiet_access); + env_quiet_access_next = env_quiet_access; +unsetenv (env_var); + *quiet_access =0 ; + +while ((str_access =strsep (&env_quiet_access_next,ENV_DELIMITER ))) { +if (strcmp (str_access,"" ) ==0 ) +continue ; +else if (strcmp (str_access,"r" ) ==0 ) + *quiet_access |=ACCESS_FS_ROUGHLY_READ ; +else if (strcmp (str_access,"w" ) ==0 ) + *quiet_access |=ACCESS_FS_ROUGHLY_WRITE ; +else if (strcmp (str_access,"b" ) ==0 ) + *quiet_access |=LANDLOCK_ACCESS_NET_BIND_TCP ; +else if (strcmp (str_access,"c" ) ==0 ) + *quiet_access |=LANDLOCK_ACCESS_NET_CONNECT_TCP ; +else if (strcmp (str_access,"a" ) ==0 ) + *quiet_access |=LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET ; +else if (strcmp (str_access,"s" ) ==0 ) + *quiet_access |=LANDLOCK_SCOPE_SIGNAL ; +else { +fprintf (stderr,"Unknown quiet access \"%s\"\n" , + str_access); +free (env_quiet_access); +return -1 ; + } + } + +free (env_quiet_access); + *quiet_access &= handled_access; +return 0 ; +} + +#define LANDLOCK_ABI_LAST 8#define XSTR (s) #s#define STR (s) XSTR(s)
@@ -328,6 +383,20 @@ static const char help[] ="\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" +ENV_FS_QUIET_NAME " and " ENV_NET_QUIET_NAME ", both optional, can then be used " +"to make access to some denied paths or network ports not trigger audit logging.\n" +ENV_FS_QUIET_ACCESS_NAME " and " ENV_NET_QUIET_ACCESS_NAME " can be used to specify " +"which accesses should be quieted (defaults to all):\n" +"* " ENV_FS_QUIET_ACCESS_NAME ": file system accesses to quiet\n" +" - \"r\" to quiet all file/dir read accesses\n" +" - \"w\" to quiet all file/dir write accesses\n" +"* " ENV_NET_QUIET_ACCESS_NAME ": network accesses to quiet\n" +" - \"b\" to quiet bind denials\n" +" - \"c\" to quiet connect denials\n" +"In addition, " ENV_SCOPED_QUIET_ACCESS_NAME " can be set to quiet all denials for " +"scoped actions (defaults to none).\n" +" - \"a\" to quiet abstract unix socket denials\n" +" - \"s\" to quiet signal denials\n" "\n" "Example:\n" ENV_FS_RO_NAME "=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
@@ -357,7 +426,12 @@ int main(const int argc, char *const argv[], char *const *const envp) LANDLOCK_ACCESS_NET_CONNECT_TCP, .scoped =LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET | LANDLOCK_SCOPE_SIGNAL, + .quiet_access_fs = 0, + .quiet_access_net = 0, + .quiet_scoped = 0, }; + +bool quiet_supported = true;int supported_restrict_flags =LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON ;int set_restrict_flags =0 ;
@@ -444,6 +518,11 @@ int main(const int argc, char *const argv[], char *const *const envp)"provided by ABI version %d (instead of %d).\n" , LANDLOCK_ABI_LAST, abi); __attribute__((fallthrough)); +case 7 : +/* Don't add quiet flags for ABI < 8 later on */ + quiet_supported = false; + + __attribute__((fallthrough));case LANDLOCK_ABI_LAST :break ;default :
@@ -490,6 +569,25 @@ int main(const int argc, char *const argv[], char *const *const envp)unsetenv (ENV_FORCE_LOG_NAME ); } +/* +* Add quiet for fs/net handled access bits. Doing this alone has no +* effect unless we later add quiet rules per FS_QUIET/NET_QUIET. +*/ +if (quiet_supported) { +if (add_quiet_access (&ruleset_attr.quiet_access_fs, + ruleset_attr.handled_access_fs, +ENV_FS_QUIET_ACCESS_NAME , true)) +return 1 ; +if (add_quiet_access (&ruleset_attr.quiet_access_net, + ruleset_attr.handled_access_net, +ENV_NET_QUIET_ACCESS_NAME , true)) +return 1 ; +if (add_quiet_access (&ruleset_attr.quiet_scoped, + ruleset_attr.scoped, +ENV_SCOPED_QUIET_ACCESS_NAME , false)) +return 1 ; + } + ruleset_fd =landlock_create_ruleset (&ruleset_attr,sizeof (ruleset_attr),0 );if (ruleset_fd <0 ) {
@@ -497,22 +595,39 @@ int main(const int argc, char *const argv[], char *const *const envp)return 1 ; } -if (populate_ruleset_fs (ENV_FS_RO_NAME , ruleset_fd, access_fs_ro)) { +if (populate_ruleset_fs (ENV_FS_RO_NAME , ruleset_fd, access_fs_ro, + false)) { goto err_close_ruleset; } -if (populate_ruleset_fs (ENV_FS_RW_NAME , ruleset_fd, access_fs_rw)) { +if (populate_ruleset_fs (ENV_FS_RW_NAME , ruleset_fd, access_fs_rw, + false)) { goto err_close_ruleset; } +/* Don't require this env to be present */ +if (quiet_supported &&getenv (ENV_FS_QUIET_NAME )) { +if (populate_ruleset_fs (ENV_FS_QUIET_NAME , ruleset_fd,0 , + true)) { + goto err_close_ruleset; + } + }if (populate_ruleset_net (ENV_TCP_BIND_NAME , ruleset_fd, -LANDLOCK_ACCESS_NET_BIND_TCP )) { +LANDLOCK_ACCESS_NET_BIND_TCP , false)) { goto err_close_ruleset; }if (populate_ruleset_net (ENV_TCP_CONNECT_NAME , ruleset_fd, -LANDLOCK_ACCESS_NET_CONNECT_TCP )) { +LANDLOCK_ACCESS_NET_CONNECT_TCP , false)) { goto err_close_ruleset; } +/* Don't require this env to be present */ +if (quiet_supported &&getenv (ENV_NET_QUIET_NAME )) { +if (populate_ruleset_net (ENV_NET_QUIET_NAME , ruleset_fd,0 , + true)) { + goto err_close_ruleset; + } + } +if (prctl (PR_SET_NO_NEW_PRIVS ,1 ,0 ,0 ,0 )) {perror ("Failed to restrict privileges" ); goto err_close_ruleset;
Signature · 1 line
2.51.0
PATCH v2 6/6 Implement quiet for optional accesses
In reply to Tingmao WangSince the existing deny_masks can only store 2x4bit of layer index, with no way to represent "no layer", we need to either expand it or have another field. This commit uses the latter approach - we add another field to store which optional access (of the 2) are covered by quiet rules in their respective layers as stored in deny_masks.
We can avoid making struct landlock_file_security larger by converting the existing fown_layer to a 4bit field. This commit does that, and adds test to ensure that it is large enough for LANDLOCK_MAX_NUM_LAYERS-1.
---
Changes since v1:
- New patch
security/landlock/audit.c | 55 +++++++++++++++++++++++--------------- security/landlock/audit.h | 1 + security/landlock/domain.c | 23 ++++++++++++++++ security/landlock/domain.h | 5 ++++ security/landlock/fs.c | 6 +++++ security/landlock/fs.h | 34 ++++++++++++++++++----- 6 files changed, 96 insertions(+), 28 deletions(-)
security/landlock/audit.c +34 -21
@@ -273,7 +273,7 @@ static void test_get_denied_layer(struct kunit *const test)static size_t get_layer_from_deny_masks (access_mask_t *const access_request,const access_mask_t all_existing_optional_access, -const deny_masks_t deny_masks) +const deny_masks_t deny_masks,u8 quiet_optional_accesses,bool *quiet) {const unsigned long access_opt = all_existing_optional_access;const unsigned long access_req = *access_request;
@@ -285,6 +285,7 @@ get_layer_from_deny_masks(access_mask_t *const access_request,/* This will require change with new object types. */ WARN_ON_ONCE (access_opt != _LANDLOCK_ACCESS_FS_OPTIONAL); + *quiet = false;for_each_set_bit (access_bit, &access_opt,BITS_PER_TYPE (access_mask_t)) {if (access_req &BIT (access_bit)) {
@@ -298,6 +299,11 @@ get_layer_from_deny_masks(access_mask_t *const access_request, }else if (layer == youngest_layer) { missing |=BIT (access_bit); } + +/* Make sure we set *quiet even if this is the first layer */ +if (layer >= youngest_layer) + *quiet = !!(quiet_optional_accesses & +BIT (access_index)); } access_index++; }
@@ -312,42 +318,49 @@ static void test_get_layer_from_deny_masks(struct kunit *const test) {deny_masks_t deny_mask;access_mask_t access; +u8 quiet_optional_accesses =0 ; +bool quiet; +bool expected_quiet = false;/* 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)); +get_layer_from_deny_masks ( + &access, _LANDLOCK_ACCESS_FS_OPTIONAL, + deny_mask, quiet_optional_accesses, &quiet));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_TRUNCATE ); +KUNIT_EXPECT_EQ (test, quiet, expected_quiet); 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)); +get_layer_from_deny_masks ( + &access, _LANDLOCK_ACCESS_FS_OPTIONAL, + deny_mask, quiet_optional_accesses, &quiet));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_IOCTL_DEV ); +KUNIT_EXPECT_EQ (test, quiet, expected_quiet);/* 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)); +get_layer_from_deny_masks ( + &access, _LANDLOCK_ACCESS_FS_OPTIONAL, + deny_mask, quiet_optional_accesses, &quiet));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_TRUNCATE ); +KUNIT_EXPECT_EQ (test, quiet, expected_quiet); 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)); +get_layer_from_deny_masks ( + &access, _LANDLOCK_ACCESS_FS_OPTIONAL, + deny_mask, quiet_optional_accesses, &quiet));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_TRUNCATE |LANDLOCK_ACCESS_FS_IOCTL_DEV ); +KUNIT_EXPECT_EQ (test, quiet, expected_quiet); }#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
@@ -413,7 +426,7 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,struct landlock_hierarchy *youngest_denied;size_t youngest_layer;access_mask_t missing, quiet_mask; -bool quiet_flag_on_rule = false, quiet_applicable_to_access = false; +bool object_quiet_flag = false, quiet_applicable_to_access = false;if (WARN_ON_ONCE (!subject || !subject->domain || !subject->domain->hierarchy || !request))
@@ -429,10 +442,13 @@ void landlock_log_denial(const struct landlock_cred_security *const subject, youngest_layer =get_denied_layer ( subject->domain, &missing, request->layer_masks, request->layer_masks_size); + object_quiet_flag = !!(rule_flags.quiet_masks &BIT (youngest_layer)); }else { youngest_layer =get_layer_from_deny_masks ( &missing, request->all_existing_optional_access, - request->deny_masks); + request->deny_masks, + request->quiet_optional_accesses, + &object_quiet_flag); } youngest_denied =get_hierarchy (subject->domain, youngest_layer);
@@ -462,13 +478,10 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,* that layer is not the one that denied the request, we should still * audit log the denial. */ - quiet_flag_on_rule = !!(rule_flags.quiet_masks &BIT (youngest_layer)); - -if (quiet_flag_on_rule) { +if (object_quiet_flag) {/* -* This is not a scope request, since rule_flags is not zero. We -* now check if the denied requests are all covered by the layer's -* quiet access bits. +* We now check if the denied requests are all covered by the +* layer's quiet access bits. */ quiet_mask =pick_access_mask_for_req_type ( request->type, youngest_denied->quiet_masks);
security/landlock/audit.h +1 -0
@@ -49,6 +49,7 @@ struct landlock_request {/* Required fields for requests with deny masks. */ const access_mask_t all_existing_optional_access;deny_masks_t deny_masks; +u8 quiet_optional_accesses; };#ifdef CONFIG_AUDIT
security/landlock/domain.c +23 -0
@@ -212,6 +212,29 @@ landlock_get_deny_masks(const access_mask_t all_existing_optional_access,return deny_masks; } +u8 landlock_get_quiet_optional_accesses ( +const access_mask_t all_existing_optional_access, +const deny_masks_t deny_masks, +const struct collected_rule_flags rule_flags) +{ +const unsigned long access_opt = all_existing_optional_access; +size_t access_index =0 ; +unsigned long access_bit; +u8 quiet_optional_accesses =0 ; + +for_each_set_bit (access_bit, &access_opt, +BITS_PER_TYPE (access_mask_t)) { +const u8 layer = (deny_masks >> (access_index *4 )) & + (LANDLOCK_MAX_NUM_LAYERS -1 ); +const bool is_quiet = !!(rule_flags.quiet_masks &BIT (layer)); + +if (is_quiet) + quiet_optional_accesses |=BIT (access_index); + access_index++; + } +return quiet_optional_accesses; +} +#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST static void test_landlock_get_deny_masks (struct kunit *const test)
security/landlock/domain.h +5 -0
@@ -130,6 +130,11 @@ landlock_get_deny_masks(const access_mask_t all_existing_optional_access,const layer_mask_t (*const layer_masks)[], size_t layer_masks_size); +u8 landlock_get_quiet_optional_accesses ( +const access_mask_t all_existing_optional_access, +const deny_masks_t deny_masks, +const struct collected_rule_flags rule_flags); +int landlock_init_hierarchy_log (struct landlock_hierarchy *const hierarchy); static inline void
security/landlock/fs.c +6 -0
@@ -1698,6 +1698,10 @@ static int hook_file_open(struct file *const file)landlock_file (file)->deny_masks =landlock_get_deny_masks ( _LANDLOCK_ACCESS_FS_OPTIONAL, optional_access, &layer_masks,ARRAY_SIZE (layer_masks)); +landlock_file (file)->quiet_optional_accesses = +landlock_get_quiet_optional_accesses ( + _LANDLOCK_ACCESS_FS_OPTIONAL, +landlock_file (file)->deny_masks, rule_flags);#endif /* CONFIG_AUDIT */ if ((open_access_request & allowed_access) == open_access_request)
@@ -1734,6 +1738,7 @@ static int hook_file_truncate(struct file *const file) .access = LANDLOCK_ACCESS_FS_TRUNCATE,#ifdef CONFIG_AUDIT .deny_masks = landlock_file(file)->deny_masks, + .quiet_optional_accesses = landlock_file(file)->quiet_optional_accesses,#endif /* CONFIG_AUDIT */ }, no_rule_flags);return -EACCES ;
@@ -1773,6 +1778,7 @@ static int hook_file_ioctl_common(const struct file *const file, .access = LANDLOCK_ACCESS_FS_IOCTL_DEV,#ifdef CONFIG_AUDIT .deny_masks = landlock_file(file)->deny_masks, + .quiet_optional_accesses = landlock_file(file)->quiet_optional_accesses,#endif /* CONFIG_AUDIT */ }, no_rule_flags);return -EACCES ;
security/landlock/fs.h +27 -7
@@ -63,11 +63,19 @@ struct landlock_file_security {* _LANDLOCK_ACCESS_FS_OPTIONAL). */ deny_masks_t deny_masks; +/** +* @quiet_optional_accesses: Stores which optional accesses are +* covered by quiet rules within the layer referred to in deny_masks, +* one access per bit. Does not take into account whether the quiet +* access bits are actually set in the layer's corresponding +* landlock_hierarchy. +*/ + u8 quiet_optional_accesses:2;/** * @fown_layer: Layer level of @fown_subject->domain with * LANDLOCK_SCOPE_SIGNAL. */ -u8 fown_layer; + u8 fown_layer:4;#endif /* CONFIG_AUDIT */ /**
@@ -80,13 +88,24 @@ struct landlock_file_security {struct landlock_cred_security fown_subject; }; -#ifdef CONFIG_AUDIT +static void build_check_file_security (void ) +{ +const struct landlock_file_security file_sec = { + .quiet_optional_accesses = ~0 , + .fown_layer = ~0 , + }; + +/* +* Make sure quiet_optional_accesses has enough bits to cover all +* optional accesses +*/ +BUILD_BUG_ON (__const_hweight8 (file_sec.quiet_optional_accesses) < +__const_hweight64 (_LANDLOCK_ACCESS_FS_OPTIONAL)); +/* Makes sure all layers can be identified. */ +BUILD_BUG_ON (file_sec.fown_layer <LANDLOCK_MAX_NUM_LAYERS -1 ); +} -/* Makes sure all layers can be identified. */ -/* clang-format off */ -static_assert ((typeof_member (struct landlock_file_security ,fown_layer ))~0 >= -LANDLOCK_MAX_NUM_LAYERS ); -/* clang-format off */ +#ifdef CONFIG_AUDIT #endif /* CONFIG_AUDIT */
@@ -107,6 +126,7 @@ struct landlock_superblock_security {static inline struct landlock_file_security *landlock_file (const struct file *const file) { +build_check_file_security ();return file->f_security + landlock_blob_sizes.lbs_file; }
Signature · 1 line
2.51.0
Mickaël Salaün
maintainerRe: [PATCH v2 6/6] Implement quiet for optional accesses
This extra patch makes the review easier, but it should be squashed into the others if possible.
Quoted 46 lines · Tingmao Wang
Since the existing
deny_maskscan only store 2x4bit of layer index, with no way to represent "no layer", we need to either expand it or have another field. This commit uses the latter approach - we add another field to store which optional access (of the 2) are covered by quiet rules in their respective layers as stored indeny_masks.We can avoid making struct
landlock_file_securitylarger by converting the existingfown_layerto a 4bit field. This commit does that, and adds test to ensure that it is large enough for LANDLOCK_MAX_NUM_LAYERS-1.Signed-off-by Tingmao Wang <[email protected]>---
Changes since v1:
- New patchsecurity/landlock/audit.c | 55 +++++++++++++++++++++++--------------- security/landlock/audit.h | 1 + security/landlock/domain.c | 23 ++++++++++++++++ security/landlock/domain.h | 5 ++++ security/landlock/fs.c | 6 +++++ security/landlock/fs.h | 34 ++++++++++++++++++----- 6 files changed, 96 insertions(+), 28 deletions(-)security/landlock/audit.c +2
-1@@ -273,7 +273,7 @@ static void test_get_denied_layer(struct kunit *const test)static size_t get_layer_from_deny_masks (access_mask_t *const access_request,const access_mask_t all_existing_optional_access, -const deny_masks_t deny_masks) +const deny_masks_t deny_masks,u8 quiet_optional_accesses,bool *quiet) {const unsigned long access_opt = all_existing_optional_access;const unsigned long access_req = *access_request;@@ -285,6 +285,7 @@ get_layer_from_deny_masks(access_mask_t *const access_request,/* This will require change with new object types. */ WARN_ON_ONCE (access_opt != _LANDLOCK_ACCESS_FS_OPTIONAL); + *quiet = false;for_each_set_bit (access_bit, &access_opt,BITS_PER_TYPE (access_mask_t)) {if (access_req &BIT (access_bit)) {
security/landlock/audit.c get_layer_from_deny_masks(access_mask_t *const access_request, · line 303
Quoted 5 lines
security/landlock/audit.c +2
-0@@ -298,6 +299,11 @@ get_layer_from_deny_masks(access_mask_t *const access_request, }else if (layer == youngest_layer) { missing |=BIT (access_bit); } + +/* Make sure we set *quiet even if this is the first layer */
Missing final dot.
security/landlock/audit.c get_layer_from_deny_masks(access_mask_t *const access_request, · line 306
Quoted 3 lines
security/landlock/audit.c +3
-0+if (layer >= youngest_layer) + *quiet = !!(quiet_optional_accesses & +BIT (access_index));
This code is good but a bit confusing at first, especially without more context than this patch provides, where we don't see the relation between layer and youngest_layer. Anyway, please extend the comment to say that quiet is always overridden for the youngest layer.
Quoted 123 lines
security/landlock/audit.c +27
-20} access_index++; }@@ -312,42 +318,49 @@ static void test_get_layer_from_deny_masks(struct kunit *const test) {deny_masks_t deny_mask;access_mask_t access; +u8 quiet_optional_accesses =0 ; +bool quiet; +bool expected_quiet = false;/* 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)); +get_layer_from_deny_masks ( + &access, _LANDLOCK_ACCESS_FS_OPTIONAL, + deny_mask, quiet_optional_accesses, &quiet));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_TRUNCATE ); +KUNIT_EXPECT_EQ (test, quiet, expected_quiet); 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)); +get_layer_from_deny_masks ( + &access, _LANDLOCK_ACCESS_FS_OPTIONAL, + deny_mask, quiet_optional_accesses, &quiet));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_IOCTL_DEV ); +KUNIT_EXPECT_EQ (test, quiet, expected_quiet);/* 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)); +get_layer_from_deny_masks ( + &access, _LANDLOCK_ACCESS_FS_OPTIONAL, + deny_mask, quiet_optional_accesses, &quiet));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_TRUNCATE ); +KUNIT_EXPECT_EQ (test, quiet, expected_quiet); 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)); +get_layer_from_deny_masks ( + &access, _LANDLOCK_ACCESS_FS_OPTIONAL, + deny_mask, quiet_optional_accesses, &quiet));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_TRUNCATE |LANDLOCK_ACCESS_FS_IOCTL_DEV ); +KUNIT_EXPECT_EQ (test, quiet, expected_quiet); }#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ @@ -413,7 +426,7 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,struct landlock_hierarchy *youngest_denied;size_t youngest_layer;access_mask_t missing, quiet_mask; -bool quiet_flag_on_rule = false, quiet_applicable_to_access = false; +bool object_quiet_flag = false, quiet_applicable_to_access = false;if (WARN_ON_ONCE (!subject || !subject->domain || !subject->domain->hierarchy || !request))@@ -429,10 +442,13 @@ void landlock_log_denial(const struct landlock_cred_security *const subject, youngest_layer =get_denied_layer ( subject->domain, &missing, request->layer_masks, request->layer_masks_size); + object_quiet_flag = !!(rule_flags.quiet_masks &BIT (youngest_layer)); }else { youngest_layer =get_layer_from_deny_masks ( &missing, request->all_existing_optional_access, - request->deny_masks); + request->deny_masks, + request->quiet_optional_accesses, + &object_quiet_flag); } youngest_denied =get_hierarchy (subject->domain, youngest_layer);@@ -462,13 +478,10 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,* that layer is not the one that denied the request, we should still * audit log the denial. */ - quiet_flag_on_rule = !!(rule_flags.quiet_masks &BIT (youngest_layer)); - -if (quiet_flag_on_rule) { +if (object_quiet_flag) {/* -* This is not a scope request, since rule_flags is not zero. We -* now check if the denied requests are all covered by the layer's -* quiet access bits. +* We now check if the denied requests are all covered by the +* layer's quiet access bits. */ quiet_mask =pick_access_mask_for_req_type ( request->type, youngest_denied->quiet_masks);security/landlock/audit.h +1
-0@@ -49,6 +49,7 @@ struct landlock_request {/* Required fields for requests with deny masks. */ const access_mask_t all_existing_optional_access;deny_masks_t deny_masks; +u8 quiet_optional_accesses; };#ifdef CONFIG_AUDIT security/landlock/domain.c +0
-0
security/landlock/domain.c landlock_get_deny_masks(const access_mask_t all_existing_optional_access, · line 214
Quoted 3 lines
security/landlock/domain.c +0
-0@@ -212,6 +212,29 @@ landlock_get_deny_masks(const access_mask_t all_existing_optional_access,return deny_masks; }
Just using u8 is confusing. Please document what is the "type" of the returned value, and use a dedicated typedef instead of u8 (see my other comment about static_assert). This typedef should probably be named optional_access_t and have a size less or equal to access_t's one.
security/landlock/domain.c landlock_get_deny_masks(const access_mask_t all_existing_optional_access, · line 223
Quoted 9 lines
security/landlock/domain.c +9
-0+u8 landlock_get_quiet_optional_accesses ( +const access_mask_t all_existing_optional_access, +const deny_masks_t deny_masks, +const struct collected_rule_flags rule_flags) +{ +const unsigned long access_opt = all_existing_optional_access; +size_t access_index =0 ; +unsigned long access_bit; +u8 quiet_optional_accesses =0 ;
As for deny_masks_t, we should define an "optional_access_t" type with appropriate safeguard to guarantee that it can always hold all optional access rights (see static_assert for deny_masks_t in access.h).
We should also copy the WARN_ON_ONCE() check fromget_layer_from_deny_masks().
Quoted 90 lines
security/landlock/domain.c +14
-0+ +for_each_set_bit (access_bit, &access_opt, +BITS_PER_TYPE (access_mask_t)) { +const u8 layer = (deny_masks >> (access_index *4 )) & + (LANDLOCK_MAX_NUM_LAYERS -1 ); +const bool is_quiet = !!(rule_flags.quiet_masks &BIT (layer)); + +if (is_quiet) + quiet_optional_accesses |=BIT (access_index); + access_index++; + } +return quiet_optional_accesses; +} +#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST static void test_landlock_get_deny_masks (struct kunit *const test)security/landlock/domain.h +5
-0@@ -130,6 +130,11 @@ landlock_get_deny_masks(const access_mask_t all_existing_optional_access,const layer_mask_t (*const layer_masks)[], size_t layer_masks_size); +u8 landlock_get_quiet_optional_accesses ( +const access_mask_t all_existing_optional_access, +const deny_masks_t deny_masks, +const struct collected_rule_flags rule_flags); +int landlock_init_hierarchy_log (struct landlock_hierarchy *const hierarchy); static inline voidsecurity/landlock/fs.c +6
-0@@ -1698,6 +1698,10 @@ static int hook_file_open(struct file *const file)landlock_file (file)->deny_masks =landlock_get_deny_masks ( _LANDLOCK_ACCESS_FS_OPTIONAL, optional_access, &layer_masks,ARRAY_SIZE (layer_masks)); +landlock_file (file)->quiet_optional_accesses = +landlock_get_quiet_optional_accesses ( + _LANDLOCK_ACCESS_FS_OPTIONAL, +landlock_file (file)->deny_masks, rule_flags);#endif /* CONFIG_AUDIT */ if ((open_access_request & allowed_access) == open_access_request)@@ -1734,6 +1738,7 @@ static int hook_file_truncate(struct file *const file) .access = LANDLOCK_ACCESS_FS_TRUNCATE,#ifdef CONFIG_AUDIT .deny_masks = landlock_file(file)->deny_masks, + .quiet_optional_accesses = landlock_file(file)->quiet_optional_accesses,#endif /* CONFIG_AUDIT */ }, no_rule_flags);return -EACCES ;@@ -1773,6 +1778,7 @@ static int hook_file_ioctl_common(const struct file *const file, .access = LANDLOCK_ACCESS_FS_IOCTL_DEV,#ifdef CONFIG_AUDIT .deny_masks = landlock_file(file)->deny_masks, + .quiet_optional_accesses = landlock_file(file)->quiet_optional_accesses,#endif /* CONFIG_AUDIT */ }, no_rule_flags);return -EACCES ;security/landlock/fs.h +9
-1@@ -63,11 +63,19 @@ struct landlock_file_security {* _LANDLOCK_ACCESS_FS_OPTIONAL). */ deny_masks_t deny_masks; +/** +* @quiet_optional_accesses: Stores which optional accesses are +* covered by quiet rules within the layer referred to in deny_masks, +* one access per bit. Does not take into account whether the quiet +* access bits are actually set in the layer's corresponding +* landlock_hierarchy. +*/ + u8 quiet_optional_accesses:2;/** * @fown_layer: Layer level of @fown_subject->domain with * LANDLOCK_SCOPE_SIGNAL. */ -u8 fown_layer; + u8 fown_layer:4;#endif /* CONFIG_AUDIT */ /**
security/landlock/fs.h struct landlock_file_security { · line 91
Quoted 5 lines
security/landlock/fs.h +1
-1@@ -80,13 +88,24 @@ struct landlock_file_security {struct landlock_cred_security fown_subject; }; -#ifdef CONFIG_AUDIT +static void build_check_file_security (void )
You can move this function to fs.c and call it inhook_file_alloc_security() instead.
security/landlock/fs.h struct landlock_file_security { · line 102
Quoted 11 lines
security/landlock/fs.h +11
-0+{ +const struct landlock_file_security file_sec = { + .quiet_optional_accesses = ~0 , + .fown_layer = ~0 , + }; + +/* +* Make sure quiet_optional_accesses has enough bits to cover all +* optional accesses +*/ + BUILD_BUG_ON(__const_hweight8(file_sec.quiet_optional_accesses) <
We should be able to use HWEIGHT() instead.
Quoted 24 lines
security/landlock/fs.h +6
-5+ __const_hweight64(_LANDLOCK_ACCESS_FS_OPTIONAL)); +/* Makes sure all layers can be identified. */ +BUILD_BUG_ON (file_sec.fown_layer <LANDLOCK_MAX_NUM_LAYERS -1 ); +} -/* Makes sure all layers can be identified. */ -/* clang-format off */ -static_assert ((typeof_member (struct landlock_file_security ,fown_layer ))~0 >= -LANDLOCK_MAX_NUM_LAYERS ); -/* clang-format off */ +#ifdef CONFIG_AUDIT #endif /* CONFIG_AUDIT */ @@ -107,6 +126,7 @@ struct landlock_superblock_security {static inline struct landlock_file_security *landlock_file (const struct file *const file) { +build_check_file_security ();return file->f_security + landlock_blob_sizes.lbs_file; }Signature · 1 line
2.51.0
Re: [PATCH v2 6/6] Implement quiet for optional accesses
Quoted 31 lines · Mickaël Salaün
[...]
security/landlock/domain.c landlock_get_deny_masks(const access_mask_t all_existing_optional_access, · line 214
Quoted 9 lines · Tingmao Wang
[..]
security/landlock/domain.c +0
-0@@ -212,6 +212,29 @@ landlock_get_deny_masks(const access_mask_t all_existing_optional_access,return deny_masks; }Just using u8 is confusing. Please document what is the "type" of the returned value, and use a dedicated typedef instead of u8 (see my other comment about
static_assert). This typedef should probably be namedoptional_access_tand have a size less or equal to access_t's one.security/landlock/domain.c landlock_get_deny_masks(const access_mask_t all_existing_optional_access, · line 223
Quoted 9 lines
security/landlock/domain.c +9
-0+u8 landlock_get_quiet_optional_accesses ( +const access_mask_t all_existing_optional_access, +const deny_masks_t deny_masks, +const struct collected_rule_flags rule_flags) +{ +const unsigned long access_opt = all_existing_optional_access; +size_t access_index =0 ; +unsigned long access_bit; +u8 quiet_optional_accesses =0 ;As for
deny_masks_t, we should define an "optional_access_t" type with appropriate safeguard to guarantee that it can always hold all optional access rights (seestatic_assertfordeny_masks_tinaccess.h).
Quoted 2 lines
We should also copy the
WARN_ON_ONCE()check fromget_layer_from_deny_masks().
I don't see how that WARN_ON_ONCE is applicable here since we're no longer dealing with the optional_access... Can you clarify?
Quoted 12 lines
security/landlock/domain.c struct landlock_file_security { · line 91
Quoted 8 lines
[...]
security/landlock/domain.c +1
-1@@ -80,13 +88,24 @@ struct landlock_file_security {struct landlock_cred_security fown_subject; }; -#ifdef CONFIG_AUDIT +static void build_check_file_security (void )You can move this function to
fs.cand call it inhook_file_alloc_security()instead.
Quoted 13 lines
security/landlock/domain.c struct landlock_file_security { · line 102
Quoted 11 lines
security/landlock/domain.c +11
-0+{ +const struct landlock_file_security file_sec = { + .quiet_optional_accesses = ~0 , + .fown_layer = ~0 , + }; + +/* +* Make sure quiet_optional_accesses has enough bits to cover all +* optional accesses +*/ + BUILD_BUG_ON(__const_hweight8(file_sec.quiet_optional_accesses) <We should be able to use
HWEIGHT()instead.
I tried it and unfortunately it doesn't seem to work :(
security/landlock/fs.c: In function ‘build_check_file_security’:
./include/linux/compiler.h:201:82: error: expression in static assertion is not constant
201 | #define __BUILD_BUG_ON_ZERO_MSG(e, msg, ...) ((int)sizeof(struct {_Static_assert(!(e), msg);}))
| ^~~~
././include/linux/compiler_types.h:577:23: note: in definition of macro ‘__compiletime_assert’
577 | if (!(condition)) \
| ^~~~~~~~~
././include/linux/compiler_types.h:597:9: note: in expansion of macro ‘_compiletime_assert’
597 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:50:9: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
50 | BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
| ^~~~~~~~~~~~~~~~
security/landlock/fs.c:1769:9: note: in expansion of macro ‘BUILD_BUG_ON’
1769 | BUILD_BUG_ON(HWEIGHT(file_sec.quiet_optional_accesses) <
| ^~~~~~~~~~~~
./include/linux/build_bug.h:17:9: note: in expansion of macro ‘__BUILD_BUG_ON_ZERO_MSG’
17 | __BUILD_BUG_ON_ZERO_MSG(e, ##__VA_ARGS__, #e " is true")
| ^~~~~~~~~~~~~~~~~~~~~~~
./include/asm-generic/bitops/const_hweight.h:37:23: note: in expansion of macro ‘BUILD_BUG_ON_ZERO’
37 | #define HWEIGHT64(w) (BUILD_BUG_ON_ZERO(!__builtin_constant_p(w)) + __const_hweight64(w))
| ^~~~~~~~~~~~~~~~~
./include/asm-generic/bitops/const_hweight.h:42:22: note: in expansion of macro ‘HWEIGHT64’
42 | #define HWEIGHT(w) HWEIGHT64((u64)w)
| ^~~~~~~~~
security/landlock/fs.c:1769:22: note: in expansion of macro ‘HWEIGHT’
1769 | BUILD_BUG_ON(HWEIGHT(file_sec.quiet_optional_accesses) <
| ^~~~~~~
Quoted 25 lines
Quoted 25 lines
security/landlock/domain.c +6
-5+ __const_hweight64(_LANDLOCK_ACCESS_FS_OPTIONAL)); +/* Makes sure all layers can be identified. */ +BUILD_BUG_ON (file_sec.fown_layer <LANDLOCK_MAX_NUM_LAYERS -1 ); +} -/* Makes sure all layers can be identified. */ -/* clang-format off */ -static_assert ((typeof_member (struct landlock_file_security ,fown_layer ))~0 >= -LANDLOCK_MAX_NUM_LAYERS ); -/* clang-format off */ +#ifdef CONFIG_AUDIT #endif /* CONFIG_AUDIT */ @@ -107,6 +126,7 @@ struct landlock_superblock_security {static inline struct landlock_file_security *landlock_file (const struct file *const file) { +build_check_file_security ();return file->f_security + landlock_blob_sizes.lbs_file; }Signature · 1 line
2.51.0
Mickaël Salaün
maintainerRe: [PATCH v2 6/6] Implement quiet for optional accesses
Quoted 32 lines · Tingmao Wang
Quoted 31 lines · Mickaël Salaün
[...]
security/landlock/domain.c landlock_get_deny_masks(const access_mask_t all_existing_optional_access, · line 214
Quoted 9 lines · Tingmao Wang
[..]
security/landlock/domain.c +0
-0@@ -212,6 +212,29 @@ landlock_get_deny_masks(const access_mask_t all_existing_optional_access,return deny_masks; }Just using u8 is confusing. Please document what is the "type" of the returned value, and use a dedicated typedef instead of u8 (see my other comment about
static_assert). This typedef should probably be namedoptional_access_tand have a size less or equal to access_t's one.security/landlock/domain.c landlock_get_deny_masks(const access_mask_t all_existing_optional_access, · line 223
Quoted 9 lines
security/landlock/domain.c +9
-0+u8 landlock_get_quiet_optional_accesses ( +const access_mask_t all_existing_optional_access, +const deny_masks_t deny_masks, +const struct collected_rule_flags rule_flags) +{ +const unsigned long access_opt = all_existing_optional_access; +size_t access_index =0 ; +unsigned long access_bit; +u8 quiet_optional_accesses =0 ;As for
deny_masks_t, we should define an "optional_access_t" type with appropriate safeguard to guarantee that it can always hold all optional access rights (seestatic_assertfordeny_masks_tinaccess.h).
Quoted 5 lines
Quoted 2 lines
We should also copy the
WARN_ON_ONCE()check fromget_layer_from_deny_masks().I don't see how that
WARN_ON_ONCEis applicable here since we're no longer dealing with theoptional_access... Can you clarify?
WARN_ON_ONCE(access_opt != _LANDLOCK_ACCESS_FS_OPTIONAL);
Quoted 52 lines
Quoted 13 lines
security/landlock/domain.c struct landlock_file_security { · line 91
Quoted 9 lines
[...]
security/landlock/domain.c +1
-1@@ -80,13 +88,24 @@ struct landlock_file_security {struct landlock_cred_security fown_subject; }; -#ifdef CONFIG_AUDIT +static void build_check_file_security (void )You can move this function to
fs.cand call it inhook_file_alloc_security()instead.Quoted 13 lines
security/landlock/domain.c struct landlock_file_security { · line 102
Quoted 11 lines
security/landlock/domain.c +11
-0+{ +const struct landlock_file_security file_sec = { + .quiet_optional_accesses = ~0 , + .fown_layer = ~0 , + }; + +/* +* Make sure quiet_optional_accesses has enough bits to cover all +* optional accesses +*/ + BUILD_BUG_ON(__const_hweight8(file_sec.quiet_optional_accesses) <We should be able to use
HWEIGHT()instead.I tried it and unfortunately it doesn't seem to work :(
security/landlock/fs.c: In function ‘build_check_file_security’:
./include/linux/compiler.h:201:82: error: expression in static assertion is not constant
201 | #define__BUILD_BUG_ON_ZERO_MSG(e, msg, ...)((int)sizeof(struct {_Static_assert(!(e), msg);}))
| ^~~~
././include/linux/compiler_types.h:577:23: note: in definition of macro ‘__compiletime_assert’
577 | if (!(condition)) \
| ^~~~~~~~~
././include/linux/compiler_types.h:597:9: note: in expansion of macro ‘_compiletime_assert’
597 |_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
39 | #defineBUILD_BUG_ON_MSG(cond, msg)compiletime_assert(!(cond), msg)
| ^~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:50:9: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
50 |BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
| ^~~~~~~~~~~~~~~~
security/landlock/fs.c:1769:9: note: in expansion of macro ‘BUILD_BUG_ON’
1769 | BUILD_BUG_ON(HWEIGHT(file_sec.quiet_optional_accesses) <
| ^~~~~~~~~~~~
./include/linux/build_bug.h:17:9: note: in expansion of macro ‘__BUILD_BUG_ON_ZERO_MSG’
17 |__BUILD_BUG_ON_ZERO_MSG(e, ##__VA_ARGS__, #e " is true")
| ^~~~~~~~~~~~~~~~~~~~~~~
./include/asm-generic/bitops/const_hweight.h:37:23: note: in expansion of macro ‘BUILD_BUG_ON_ZERO’
Quoted 9 lines
37 | #define HWEIGHT64(w) (BUILD_BUG_ON_ZERO(!__builtin_constant_p(w)) +
__const_hweight64(w))
| ^~~~~~~~~~~~~~~~~
./include/asm-generic/bitops/const_hweight.h:42:22: note: in expansion of macro ‘HWEIGHT64’
42 | #define HWEIGHT(w) HWEIGHT64((u64)w)
| ^~~~~~~~~
security/landlock/fs.c:1769:22: note: in expansion of macro ‘HWEIGHT’
1769 | BUILD_BUG_ON(HWEIGHT(file_sec.quiet_optional_accesses) <
| ^~~~~~~
Indeed, it works with Clang but not GCC.
Consistently always using __const_hweight64() might help avoid future issue if quiet_optional_accesses grows more than 8 bits though.
Quoted 26 lines
Quoted 25 lines
Quoted 25 lines
security/landlock/domain.c +6
-5+ __const_hweight64(_LANDLOCK_ACCESS_FS_OPTIONAL)); +/* Makes sure all layers can be identified. */ +BUILD_BUG_ON (file_sec.fown_layer <LANDLOCK_MAX_NUM_LAYERS -1 ); +} -/* Makes sure all layers can be identified. */ -/* clang-format off */ -static_assert ((typeof_member (struct landlock_file_security ,fown_layer ))~0 >= -LANDLOCK_MAX_NUM_LAYERS ); -/* clang-format off */ +#ifdef CONFIG_AUDIT #endif /* CONFIG_AUDIT */ @@ -107,6 +126,7 @@ struct landlock_superblock_security {static inline struct landlock_file_security *landlock_file (const struct file *const file) { +build_check_file_security ();return file->f_security + landlock_blob_sizes.lbs_file; }Signature · 1 line
2.51.0
Re: [PATCH v2 6/6] Implement quiet for optional accesses
In reply to Mickaël SalaünQuoted 2 lines · Mickaël Salaün
This extra patch makes the review easier, but it should be squashed into the others if possible.
Done in v3.
Quoted 33 lines · Tingmao Wang
Quoted 23 lines
[...]
security/landlock/audit.c +2
-1@@ -273,7 +273,7 @@ static void test_get_denied_layer(struct kunit *const test)static size_t get_layer_from_deny_masks (access_mask_t *const access_request,const access_mask_t all_existing_optional_access, -const deny_masks_t deny_masks) +const deny_masks_t deny_masks,u8 quiet_optional_accesses,bool *quiet) {const unsigned long access_opt = all_existing_optional_access;const unsigned long access_req = *access_request;@@ -285,6 +285,7 @@ get_layer_from_deny_masks(access_mask_t *const access_request,/* This will require change with new object types. */ WARN_ON_ONCE (access_opt != _LANDLOCK_ACCESS_FS_OPTIONAL); + *quiet = false;for_each_set_bit (access_bit, &access_opt,BITS_PER_TYPE (access_mask_t)) {if (access_req &BIT (access_bit)) {security/landlock/audit.c get_layer_from_deny_masks(access_mask_t *const access_request, · line 303
Quoted 5 lines
security/landlock/audit.c +2
-0@@ -298,6 +299,11 @@ get_layer_from_deny_masks(access_mask_t *const access_request, }else if (layer == youngest_layer) { missing |=BIT (access_bit); } + +/* Make sure we set *quiet even if this is the first layer */ Missing final dot.
Quoted 8 lines
security/landlock/audit.c get_layer_from_deny_masks(access_mask_t *const access_request, · line 306
Quoted 3 lines
security/landlock/audit.c +3
-0+if (layer >= youngest_layer) + *quiet = !!(quiet_optional_accesses & +BIT (access_index));This code is good but a bit confusing at first, especially without more context than this patch provides, where we don't see the relation between layer and
youngest_layer. Anyway, please extend the comment to say that quiet is always overridden for the youngest layer.
TBH I didn't really like it either. I've moved this around a bit in the latest version - can you check if that is clearer?
Mickaël Salaün
maintainerThis patch series is not simple but really good, thanks! I mostly have nicking comments. I'm looking forward the kselftests.
Quoted 126 lines · Tingmao Wang
Hi,
This is the v2 of the "quiet flag" series, implementing the feature as proposed in [1].
v1: https://lore.kernel.org/all/[email protected]/
The quiet flag allows a sandboxer to suppress audit logs for uninteresting denials. The flag can be set on objects and inherits downward in the filesystem hierarchy. On a denial, the youngest denying layer's quiet flag setting decides whether to audit. The motivation for this feature is to reduce audit noise, and also prepare for a future supervisor feature which will use this bit to suppress supervisor notifications.
In this version, the most significant change is that we now have a quiet access mask in the
ruleset_attr, which gets eventually stored in the hierarchy. This allows the user to specify which access should be affected by quiet bits. One can then, for example, make it such that read accesses to certain files are not audited (but still denied), but all writes are still audited, regardless of location.This version also implements quiet support for optional accesses (truncate and ioctl), scope denials (signal, abstract unix socket), addresses suggestions from v1 review, and further enhances sandboxer to allow full customization of which access to quiet. Network and scope access quieting are now also supported by the sandboxer via additional environment variables.
I still haven't added any selftests yet but did some testing with sandboxer. I would like this to be reviewed as it stands, before finishing up the tests which I will hopefully add in v3.
Patches removed since v1:
- landlock/access: Improve explanation on thedeny_masks_tDemo:
/# LL_FS_RO=/usr LL_FS_RW= LL_FORCE_LOG=1 LL_FS_QUIET=/dev:/tmp:/etc LL_FS_QUIET_ACCESS=r ./sandboxer bash ... audit: type=1423 audit(1759680175.562:195): domain=15bb25f6b blockers=fs.write_file,fs.read_file path="/dev/tty" dev="devtmpfs" ino=11 ^^^^^^^^ # note: because write is not quieted, we see the above line. blockers # contains read as well since that's the originally requested access. audit: type=1424 audit(1759680175.562:195): domain=15bb25f6b status=allocated mode=enforcing pid=616 uid=0 exe="/sandboxer" comm="sandboxer" audit: type=1300 audit(1759680175.562:195): arch=c000003e syscall=257 success=no exit=-13 a0=ffffffffffffff9c a1=5565c86113d1 a2=802 a3=0 items=0 ppid=605 pid=616 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="bash" exe="/usr/bin/bash" key=(null) audit: type=1327 audit(1759680175.562:195): proctitle="bash" bash: cannot set terminal process group (605): Inappropriate ioctl for device bash: no job control in this shell bash: /etc/bash.bashrc: Permission denied audit: type=1423 audit(1759680175.570:196): domain=15bb25f6b blockers=fs.read_file path="/.bash_history" dev="virtiofs" ino=36963 ^^^^^^^^ # read outside /dev:/tmp:/etc - not quieted audit: type=1300 audit(1759680175.570:196): arch=c000003e syscall=257 success=no exit=-13 a0=ffffffffffffff9c a1=5565c868e400 a2=0 a3=0 items=0 ppid=605 pid=616 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="bash" exe="/usr/bin/bash" key=(null) audit: type=1327 audit(1759680175.570:196): proctitle="bash" audit: type=1423 audit(1759680175.570:197): domain=15bb25f6b blockers=fs.read_file path="/.bash_history" dev="virtiofs" ino=36963 audit: type=1300 audit(1759680175.570:197): arch=c000003e syscall=257 success=no exit=-13 a0=ffffffffffffff9c a1=5565c868e400 a2=0 a3=0 items=0 ppid=605 pid=616 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="bash" exe="/usr/bin/bash" key=(null) audit: type=1327 audit(1759680175.570:197): proctitle="bash" bash-5.2# head /etc/passwd head: cannot open '/etc/passwd' for reading: Permission denied ^^^^^^^^ # reads to /etc are quieted bash-5.2# echo evil >> /etc/passwd bash: /etc/passwd: Permission denied audit: type=1423 audit(1759680227.030:198): domain=15bb25f6b blockers=fs.write_file path="/etc/passwd" dev="virtiofs" ino=790 ^^^^^^^^ # writes are not quieted audit: type=1300 audit(1759680227.030:198): arch=c000003e syscall=257 success=no exit=-13 a0=ffffffffffffff9c a1=5565c86ab030 a2=441 a3=1b6 items=0 ppid=605 pid=616 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="bash" exe="/usr/bin/bash" key=(null) audit: type=1327 audit(1759680227.030:198): proctitle="bash"Design:
- The user can set the quiet flag for a layer on any part of the fs hierarchy (whether it allows any access on it or not), and the flag inherits down (no support for "cancelling" the inheritance of the flag in specific subdirectories).
- The youngest layer that denies a request gets to decide whether the denial is audited or not. This means that a compromised binary, for example, cannot "turn off" Landlock auditing when it tries to access files, unless it denies access to the files itself. There is some debate to be had on whether, if a parent layer sets the quiet flag, but the request is denied by a deeper layer, whether Landlock should still audit anyway (since the rule author of the child layer likely did not expect the denial, so it would be good diagnostic). The current approach is to ignore the quiet on the parent layer and audit anyway.
All existing kselftests pass.
[1]: https://github.com/landlock-lsm/linux/issues/44#issuecomment-2876500918
Kind regards,
TingmaoTingmao Wang (6):
landlock: Add a place for flags to layer rules
landlock: Add API support and docs for the quiet flags landlock/audit: Check for quiet flag inlandlock_log_deniallandlock/audit: Fix wrong type usagesamples/landlock: Add quiet flag support to sandboxer Implement quiet for optional accessesinclude/uapi/linux/landlock.h | 64 +++++++++ samples/landlock/sandboxer.c | 133 +++++++++++++++++-- security/landlock/audit.c | 113 +++++++++++++--- security/landlock/audit.h | 4 +- security/landlock/domain.c | 23 ++++ security/landlock/domain.h | 10 ++ security/landlock/fs.c | 103 ++++++++------ security/landlock/fs.h | 36 +++-- security/landlock/net.c | 11 +- security/landlock/net.h | 3 +- security/landlock/ruleset.c | 19 ++- security/landlock/ruleset.h | 39 +++++- security/landlock/syscalls.c | 72 +++++++--- security/landlock/task.c | 12 +- tools/testing/selftests/landlock/base_test.c | 4 +- 15 files changed, 538 insertions(+), 108 deletions(-)base-commit:
e5f0a698b34ed76002dc5cff3804a61c80233a7aSignature · 1 line
2.51.0