| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f11b7754-b879-20be-0b22-94d94a68de71@digikod.net>
Date: Mon, 29 Aug 2022 15:10:12 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>
Cc: willemdebruijn.kernel@...il.com,
linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
netfilter-devel@...r.kernel.org, anton.sirazetdinov@...wei.com
Subject: Re: [PATCH v6 00/17] Network support for Landlock
On 27/08/2022 15:30, Konstantin Meskhidze (A) wrote:
>
>
> 7/28/2022 4:17 PM, Mickaël Salaün пишет:
[...]
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index 59229be378d6..669de66094ed 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -19,8 +19,8 @@
>> #include "limits.h"
>> #include "object.h"
>>
>> -// TODO: get back to u16 thanks to ruleset->net_access_mask
>> -typedef u32 access_mask_t;
>> +/* Rule access mask. */
>> +typedef u16 access_mask_t;
>> /* Makes sure all filesystem access rights can be stored. */
>> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
>> /* Makes sure all network access rights can be stored. */
>> @@ -28,6 +28,12 @@ static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
>> /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
>> static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>>
>> +/* Ruleset access masks. */
>> +typedef u16 access_masks_t;
>> +/* Makes sure all ruleset access rights can be stored. */
>> +static_assert(BITS_PER_TYPE(access_masks_t) >=
>> + LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET);
>> +
>> typedef u16 layer_mask_t;
>> /* Makes sure all layers can be checked. */
>> static_assert(BITS_PER_TYPE(layer_mask_t) >= LANDLOCK_MAX_NUM_LAYERS);
>> @@ -47,16 +53,33 @@ struct landlock_layer {
>> access_mask_t access;
>> };
>>
>> +/**
>> + * union landlock_key - Key of a ruleset's red-black tree
>> + */
>> union landlock_key {
>> struct landlock_object *object;
>> uintptr_t data;
>> };
>>
>> +/**
>> + * enum landlock_key_type - Type of &union landlock_key
>> + */
>> enum landlock_key_type {
>> + /**
>> + * @LANDLOCK_KEY_INODE: Type of &landlock_ruleset.root_inode's node
>> + * keys.
>> + */
>> LANDLOCK_KEY_INODE = 1,
>> + /**
>> + * @LANDLOCK_KEY_NET_PORT: Type of &landlock_ruleset.root_net_port's
>> + * node keys.
>> + */
>> LANDLOCK_KEY_NET_PORT = 2,
>> };
>>
>> +/**
>> + * struct landlock_id - Unique rule identifier for a ruleset
>> + */
>> struct landlock_id {
>> union landlock_key key;
>> const enum landlock_key_type type;
>> @@ -113,15 +136,17 @@ struct landlock_hierarchy {
>> */
>> struct landlock_ruleset {
>> /**
>> - * @root: Root of a red-black tree containing &struct landlock_rule
>> - * nodes. Once a ruleset is tied to a process (i.e. as a domain), this
>> - * tree is immutable until @usage reaches zero.
>> + * @root_inode: Root of a red-black tree containing &struct
>> + * landlock_rule nodes with inode object. Once a ruleset is tied to a
>> + * process (i.e. as a domain), this tree is immutable until @usage
>> + * reaches zero.
>> */
>> struct rb_root root_inode;
>> /**
>> - * @root_net_port: Root of a red-black tree containing object nodes
>> - * for network port. Once a ruleset is tied to a process (i.e. as a domain),
>> - * this tree is immutable until @usage reaches zero.
>> + * @root_net_port: Root of a red-black tree containing &struct
>> + * landlock_rule nodes with network port. Once a ruleset is tied to a
>> + * process (i.e. as a domain), this tree is immutable until @usage
>> + * reaches zero.
>> */
>> struct rb_root root_net_port;
>> /**
>> @@ -162,32 +187,25 @@ struct landlock_ruleset {
>> */
>> u32 num_layers;
>> /**
>> - * TODO: net_access_mask: Contains the subset of network
>> - * actions that are restricted by a ruleset.
>> - */
>> - access_mask_t net_access_mask;
>> - /**
>> - * @access_masks: Contains the subset of filesystem
>> - * actions that are restricted by a ruleset. A domain
>> - * saves all layers of merged rulesets in a stack
>> - * (FAM), starting from the first layer to the last
>> - * one. These layers are used when merging rulesets,
>> - * for user space backward compatibility (i.e.
>> - * future-proof), and to properly handle merged
>> + * @access_masks: Contains the subset of filesystem and
>> + * network actions that are restricted by a ruleset.
>> + * A domain saves all layers of merged rulesets in a
>> + * stack (FAM), starting from the first layer to the
>> + * last one. These layers are used when merging
>> + * rulesets, for user space backward compatibility
>> + * (i.e. future-proof), and to properly handle merged
>> * rulesets without overlapping access rights. These
>> * layers are set once and never changed for the
>> * lifetime of the ruleset.
>> */
>> - // TODO: rename (back) to fs_access_mask because layers
>> - // are only useful for file hierarchies.
>> - access_mask_t access_masks[];
>> + access_masks_t access_masks[];
>> };
>> };
>> };
>>
>> struct landlock_ruleset *
>> -landlock_create_ruleset(const access_mask_t access_mask_fs,
>> - const access_mask_t access_mask_net);
>> +landlock_create_ruleset(const access_mask_t fs_access_mask,
>> + const access_mask_t net_access_mask);
>>
>> void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
>> void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
>> @@ -210,41 +228,7 @@ static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
>> refcount_inc(&ruleset->usage);
>> }
>>
>> -// TODO: These helpers should not be required thanks to the new ruleset->net_access_mask.
>> -/* A helper function to set a filesystem mask. */
>> -static inline void
>> -landlock_set_fs_access_mask(struct landlock_ruleset *ruleset,
>> - const access_mask_t access_mask_fs, u16 mask_level)
>> -{
>> - ruleset->access_masks[mask_level] = access_mask_fs;
>> -}
>> -
>> -/* A helper function to get a filesystem mask. */
>> -static inline u32
>> -landlock_get_fs_access_mask(const struct landlock_ruleset *ruleset,
>> - u16 mask_level)
>> -{
>> - return (ruleset->access_masks[mask_level] & LANDLOCK_MASK_ACCESS_FS);
>> -}
>> -
>> -/* A helper function to set a network mask. */
>> -static inline void
>> -landlock_set_net_access_mask(struct landlock_ruleset *ruleset,
>> - const access_mask_t access_mask_net,
>> - u16 mask_level)
>> -{
>> - ruleset->access_masks[mask_level] |=
>> - (access_mask_net << LANDLOCK_MASK_SHIFT_NET);
>> -}
>> -
>> -/* A helper function to get a network mask. */
>> -static inline u32
>> -landlock_get_net_access_mask(const struct landlock_ruleset *ruleset,
>> - u16 mask_level)
>> -{
>> - return (ruleset->access_masks[mask_level] >> LANDLOCK_MASK_SHIFT_NET);
>> -}
>> -
>> +// TODO: Remove if only relevant for fs.c
>> access_mask_t get_handled_accesses(const struct landlock_ruleset *const domain,
>> const u16 rule_type, const u16 num_access);
>>
>> @@ -258,4 +242,50 @@ access_mask_t init_layer_masks(const struct landlock_ruleset *const domain,
>> layer_mask_t (*const layer_masks)[],
>> const enum landlock_key_type key_type);
>>
>> +static inline void
>> +landlock_add_fs_access_mask(struct landlock_ruleset *const ruleset,
>> + const access_mask_t fs_access_mask,
>> + const u16 layer_level)
>> +{
>> + access_mask_t fs_mask = fs_access_mask & LANDLOCK_MASK_ACCESS_FS;
>> +
>> + /* Should already be checked in sys_landlock_create_ruleset(). */
>> + WARN_ON_ONCE(fs_access_mask != fs_mask);
>> + // TODO: Add tests to check "|=" and not "=" > Is it kunit test? If so, do you want to add this kind of tests in future
> landlock versions?
In this sixth patch series, landlock_set_fs_access_mask() was replacing
the content of access_masks[] whereas landlock_set_net_access_mask() was
ORing it. It didn't lead to a bug because landlock_set_fs_access_mask()
was called before landlock_set_net_access_mask(), but it was brittle.
Anyway, it was a good reminder to add a test to check that filesystem
and network restrictions work well together. This can be added as a
basic filesystem test using a ruleset handling network restrictions but
no network rule (in fs_test.c), and as a basic network test using a
ruleset handling filesystem restrictions but no filestem rule (in
net_test.c).
This could also be part of a kunit test in the future.
>> + ruleset->access_masks[layer_level] |=
>> + (fs_mask << LANDLOCK_SHIFT_ACCESS_FS);
>> +}
>> +
>> +static inline void
>> +landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
>> + const access_mask_t net_access_mask,
>> + const u16 layer_level)
>> +{
>> + access_mask_t net_mask = net_access_mask & LANDLOCK_MASK_ACCESS_NET;
>> +
>> + /* Should already be checked in sys_landlock_create_ruleset(). */
>> + WARN_ON_ONCE(net_access_mask != net_mask);
>> + // TODO: Add tests to check "|=" and not "="
> The same above.
> I'm going add invalid network attribute checking into TEST_F(socket,
> inval) test in coming patch.
Good
Powered by blists - more mailing lists