On Tue, Oct 10, 2023 at 06:29:56AM +0300, Konstantin Meskhidze (A) wrote: > > > 10/2/2023 11:26 PM, Mickaël Salaün пишет: > > Thanks for this new version Konstantin. I pushed this series, with minor > > changes, to -next. So far, no warning. But it needs some changes, mostly > > kernel-only, but also one with the handling of port 0 with bind (see my > > review below). > > > > On Wed, Sep 20, 2023 at 05:26:36PM +0800, Konstantin Meskhidze wrote: > > > This commit adds network rules support in the ruleset management > > > helpers and the landlock_create_ruleset syscall. > > > Refactor user space API to support network actions. Add new network > > > access flags, network rule and network attributes. Increment Landlock > > > ABI version. Expand access_masks_t to u32 to be sure network access > > > rights can be stored. Implement socket_bind() and socket_connect() > > > LSM hooks, which enables to restrict TCP socket binding and connection > > > to specific ports. > > > The new landlock_net_port_attr structure has two fields. The allowed_access > > > field contains the LANDLOCK_ACCESS_NET_* rights. The port field contains > > > the port value according to the allowed protocol. This field can > > > take up to a 64-bit value [1] but the maximum value depends on the related > > > protocol (e.g. 16-bit for TCP). > > > > > > [1] > > > https://lore.kernel.org/r/278ab07f-7583-a4e0-3d37-1bacd091531d@xxxxxxxxxxx > > > > > > Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx> > > > Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@xxxxxxxxxx> > > > --- > > > > > > Changes since v11: > > > * Replace dates with "2022-2023" in net.c/h files headers. > > > * Removes WARN_ON_ONCE(!domain) in check_socket_access(). > > > * Using "typeof(*address)" instead of offsetofend(struct sockaddr, sa_family). > > > * Renames LANDLOCK_RULE_NET_SERVICE to LANDLOCK_RULE_NET_PORT. > > > * Renames landlock_net_service_attr to landlock_net_port_attr. > > > * Defines two add_rule_net_service() functions according to > > > IS_ENABLED(CONFIG_INET) instead of changing the body of the only > > > function. > > > * Adds af_family consistency check while handling AF_UNSPEC specifically. > > > * Adds bind_access_mask in add_rule_net_service() to deny all rules with bind > > > action on port zero. > > > * Minor fixes. > > > * Refactors commit message. > > > > > > Changes since v10: > > > * Removes "packed" attribute. > > > * Applies Mickaёl's patch with some refactoring. > > > * Deletes get_port() and check_addrlen() helpers. > > > * Refactors check_socket_access() by squashing get_port() and > > > check_addrlen() helpers into it. > > > * Fixes commit message. > > > > > > Changes since v9: > > > * Changes UAPI port field to __u64. > > > * Moves shared code into check_socket_access(). > > > * Adds get_raw_handled_net_accesses() and > > > get_current_net_domain() helpers. > > > * Minor fixes. > > > > > > Changes since v8: > > > * Squashes commits. > > > * Refactors commit message. > > > * Changes UAPI port field to __be16. > > > * Changes logic of bind/connect hooks with AF_UNSPEC families. > > > * Adds address length checking. > > > * Minor fixes. > > > > > > Changes since v7: > > > * Squashes commits. > > > * Increments ABI version to 4. > > > * Refactors commit message. > > > * Minor fixes. > > > > > > Changes since v6: > > > * Renames landlock_set_net_access_mask() to landlock_add_net_access_mask() > > > because it OR values. > > > * Makes landlock_add_net_access_mask() more resilient incorrect values. > > > * Refactors landlock_get_net_access_mask(). > > > * Renames LANDLOCK_MASK_SHIFT_NET to LANDLOCK_SHIFT_ACCESS_NET and use > > > LANDLOCK_NUM_ACCESS_FS as value. > > > * Updates access_masks_t to u32 to support network access actions. > > > * Refactors landlock internal functions to support network actions with > > > landlock_key/key_type/id types. > > > > > > Changes since v5: > > > * Gets rid of partial revert from landlock_add_rule > > > syscall. > > > * Formats code with clang-format-14. > > > > > > Changes since v4: > > > * Refactors landlock_create_ruleset() - splits ruleset and > > > masks checks. > > > * Refactors landlock_create_ruleset() and landlock mask > > > setters/getters to support two rule types. > > > * Refactors landlock_add_rule syscall add_rule_path_beneath > > > function by factoring out get_ruleset_from_fd() and > > > landlock_put_ruleset(). > > > > > > Changes since v3: > > > * Splits commit. > > > * Adds network rule support for internal landlock functions. > > > * Adds set_mask and get_mask for network. > > > * Adds rb_root root_net_port. > > > > > > --- > > > include/uapi/linux/landlock.h | 47 ++++ > > > security/landlock/Kconfig | 3 +- > > > security/landlock/Makefile | 2 + > > > security/landlock/limits.h | 5 + > > > security/landlock/net.c | 241 +++++++++++++++++++ > > > security/landlock/net.h | 35 +++ > > > security/landlock/ruleset.c | 62 ++++- > > > security/landlock/ruleset.h | 59 ++++- > > > security/landlock/setup.c | 2 + > > > security/landlock/syscalls.c | 33 ++- > > > tools/testing/selftests/landlock/base_test.c | 2 +- > > > 11 files changed, 467 insertions(+), 24 deletions(-) > > > create mode 100644 security/landlock/net.c > > > create mode 100644 security/landlock/net.h > > > > > > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h > > > index 81d09ef9aa50..3b8400e8a4d9 100644 > > > --- a/include/uapi/linux/landlock.h > > > +++ b/include/uapi/linux/landlock.h > > > @@ -31,6 +31,12 @@ struct landlock_ruleset_attr { > > > * this access right. > > > */ > > > __u64 handled_access_fs; > > > + /** > > > + * @handled_access_net: Bitmask of actions (cf. `Network flags`_) > > > + * that is handled by this ruleset and should then be forbidden if no > > > + * rule explicitly allow them. > > > + */ > > > + __u64 handled_access_net; > > > }; > > > > > > /* > > > @@ -54,6 +60,11 @@ enum landlock_rule_type { > > > * landlock_path_beneath_attr . > > > */ > > > LANDLOCK_RULE_PATH_BENEATH = 1, > > > + /** > > > + * @LANDLOCK_RULE_NET_PORT: Type of a &struct > > > + * landlock_net_port_attr . > > > + */ > > > + LANDLOCK_RULE_NET_PORT = 2, > > > }; > > > > > > /** > > > @@ -79,6 +90,23 @@ struct landlock_path_beneath_attr { > > > */ > > > } __attribute__((packed)); > > > > > > +/** > > > + * struct landlock_net_port_attr - Network service definition > > > > "Network port definition" > > Ok. Thanks. > > > > > > > diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig > > > index c1e862a38410..10c099097533 100644 > > > --- a/security/landlock/Kconfig > > > +++ b/security/landlock/Kconfig > > > @@ -2,7 +2,8 @@ > > > > > > config SECURITY_LANDLOCK > > > bool "Landlock support" > > > - depends on SECURITY > > > + depends on SECURITY && !ARCH_EPHEMERAL_INODES > > > > !ARCH_EPHEMERAL_INODES is definitely gone now. > > Ok. Got it. > > > > > + select SECURITY_NETWORK > > > select SECURITY_PATH > > > help > > > Landlock is a sandboxing mechanism that enables processes to restrict > > > diff --git a/security/landlock/Makefile b/security/landlock/Makefile > > > index 7bbd2f413b3e..53d3c92ae22e 100644 > > > --- a/security/landlock/Makefile > > > +++ b/security/landlock/Makefile > > > @@ -2,3 +2,5 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o > > > > > > landlock-y := setup.o syscalls.o object.o ruleset.o \ > > > cred.o ptrace.o fs.o > > > + > > > +landlock-$(CONFIG_INET) += net.o > > > \ No newline at end of file > > > diff --git a/security/landlock/limits.h b/security/landlock/limits.h > > > index bafb3b8dc677..93c9c6f91556 100644 > > > --- a/security/landlock/limits.h > > > +++ b/security/landlock/limits.h > > > @@ -23,6 +23,11 @@ > > > #define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS) > > > #define LANDLOCK_SHIFT_ACCESS_FS 0 > > > > > > +#define LANDLOCK_LAST_ACCESS_NET LANDLOCK_ACCESS_NET_CONNECT_TCP > > > +#define LANDLOCK_MASK_ACCESS_NET ((LANDLOCK_LAST_ACCESS_NET << 1) - 1) > > > +#define LANDLOCK_NUM_ACCESS_NET __const_hweight64(LANDLOCK_MASK_ACCESS_NET) > > > +#define LANDLOCK_SHIFT_ACCESS_NET LANDLOCK_NUM_ACCESS_FS > > > + > > > /* clang-format on */ > > > > > > #endif /* _SECURITY_LANDLOCK_LIMITS_H */ > > > diff --git a/security/landlock/net.c b/security/landlock/net.c > > > new file mode 100644 > > > index 000000000000..62b830653e25 > > > --- /dev/null > > > +++ b/security/landlock/net.c > > > @@ -0,0 +1,241 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/* > > > + * Landlock LSM - Network management and hooks > > > + * > > > + * Copyright © 2022-2023 Huawei Tech. Co., Ltd. > > > + * Copyright © 2022-2023 Microsoft Corporation > > > + */ > > > + > > > +#include <linux/in.h> > > > +#include <linux/net.h> > > > +#include <linux/socket.h> > > > +#include <net/ipv6.h> > > > + > > > +#include "common.h" > > > +#include "cred.h" > > > +#include "limits.h" > > > +#include "net.h" > > > +#include "ruleset.h" > > > + > > > +int landlock_append_net_rule(struct landlock_ruleset *const ruleset, > > > + const u16 port, access_mask_t access_rights) > > > > This function is only used in add_rule_net_service(), so it should not > > be exported, and we can merge it (into landlock_add_rule_net_port). > > > Do I have to ignore it according your next mail thread: > https://lore.kernel.org/netdev/20231009.meet7uTaeghu@xxxxxxxxxxx/ > ???? Yes please ignore this part, the latest mail prevails. This should be a static function. > > > +{ > > > + int err; > > > + const struct landlock_id id = { > > > + .key.data = (__force uintptr_t)htons(port), > > > + .type = LANDLOCK_KEY_NET_PORT, > > > + }; > > > + > > > + BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data)); > > > + > > > + /* Transforms relative access rights to absolute ones. */ > > > + access_rights |= LANDLOCK_MASK_ACCESS_NET & > > > + ~landlock_get_net_access_mask(ruleset, 0); > > > + > > > + mutex_lock(&ruleset->lock); > > > + err = landlock_insert_rule(ruleset, id, access_rights); > > > + mutex_unlock(&ruleset->lock); > > > + > > > + return err; > > > +} > > > + > > > +int add_rule_net_service(struct landlock_ruleset *ruleset, > > > > We should only export functions with a "landlock_" prefix, and "service" > > is now replaced with "port", which gives landlock_add_rule_net_port(). > > > > For consistency, we should also rename add_rule_path_beneath() into > > landlock_add_rule_path_beneath(), move it into fs.c, and merge > > landlock_append_fs_rule() into it (being careful to not move the related > > code to ease review). This change should be part of the "landlock: > > Refactor landlock_add_rule() syscall" patch. Please be careful to keep > > the other changes happening in other patches. > > > Should I still update prefix according your suggestions (keep > add_rule_net_service() in syscalls.c) in the next mail thread or keep it as > it is: > https://lore.kernel.org/netdev/20231009.meet7uTaeghu@xxxxxxxxxxx/ > ???? No, the prefix notation is only useful for exported functions, just renamed it to add_rule_net_port() and make it static. > > > > > + const void __user *const rule_attr) > > > +{ > > > + struct landlock_net_port_attr net_port_attr; > > > + int res; > > > + access_mask_t mask, bind_access_mask; > > > + > > > + /* Copies raw user space buffer. */ > > > + res = copy_from_user(&net_port_attr, rule_attr, sizeof(net_port_attr)); > > > > We should include <linux/uaccess.h> because of copy_from_user(). > > > > Same for landlock_add_rule_path_beneath(). Same as above, this is not relevant anymore. > > > > > + if (res) > > > + return -EFAULT; > > > + > > > + /* > > > + * Informs about useless rule: empty allowed_access (i.e. deny rules) > > > + * are ignored by network actions. > > > + */ > > > + if (!net_port_attr.allowed_access) > > > + return -ENOMSG; > > > + > > > + /* > > > + * Checks that allowed_access matches the @ruleset constraints > > > + * (ruleset->access_masks[0] is automatically upgraded to 64-bits). > > > + */ > > > + mask = landlock_get_net_access_mask(ruleset, 0); > > > + if ((net_port_attr.allowed_access | mask) != mask) > > > + return -EINVAL; > > > + > > > + /* > > > + * Denies inserting a rule with port 0 (for bind action) or > > > + * higher than 65535. > > > + */ > > > + bind_access_mask = net_port_attr.allowed_access & > > > + LANDLOCK_ACCESS_NET_BIND_TCP; > > > + if (((net_port_attr.port == 0) && > > > + (bind_access_mask == LANDLOCK_ACCESS_NET_BIND_TCP)) || > > > > For context about "port 0 binding" see > > https://lore.kernel.org/all/7cb458f1-7aff-ccf3-abfd-b563bfc65b84@xxxxxxxxxx/ > > > > I previously said: > > > > > To say it another way, we should not allow to add a rule with port > > > > > 0 for > > > > > LANDLOCK_ACCESS_NET_BIND_TCP, but return -EINVAL in this case. This > > > > > limitation should be explained, documented and tested. > > > > Thinking more about this port 0 for bind (and after an interesting > > discussion with Eric), it would be a mistake to forbid a rule to bind on > > port 0 because this is very useful for some network services, and > > because it would not be reasonable to have an LSM hook to control such > > "random ports". Instead we should document what using this value means > > (i.e. pick a dynamic available port in a range defined by the sysadmin) > > and highlight the fact that it is controlled with the > > /proc/sys/net/ipv4/ip_local_port_range sysctl, which is also used by > > IPv6. > > > > We then need to test binding on port zero by getting the binded port > > (cf. getsockopt/getsockname) and checking that we can indeed connect to > > it. > > So like I understand refactoring will be like this: > 1. Allow bind for zero port. > 2. Update tests: > - add rule with port 0; > - bind "random" port; > - using getsockname to get binded port; > - connect to the socket using binded port; > > Correct?? Yes. You can also extend this test to try binding to a lower port (e.g. 1024, value of srv0.port), not part of ip_local_port_range, which should then be denied. > > > > > + (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); > > > +} > > > > > diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h > > > index 1ede2b9a79b7..9bd0483c64d4 100644 > > > --- a/security/landlock/ruleset.h > > > +++ b/security/landlock/ruleset.h > > > @@ -33,13 +33,16 @@ > > > 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. */ > > > +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; > > > +typedef u32 access_masks_t; > > > /* Makes sure all ruleset access rights can be stored. */ > > > -static_assert(BITS_PER_TYPE(access_masks_t) >= LANDLOCK_NUM_ACCESS_FS); > > > +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. */ > > > @@ -84,6 +87,11 @@ enum landlock_key_type { > > > * keys. > > > */ > > > LANDLOCK_KEY_INODE = 1, > > > + /** > > > + * @LANDLOCK_KEY_NET_PORT: Type of &landlock_ruleset.root_net_port's > > > + * node keys. > > > + */ > > > + LANDLOCK_KEY_NET_PORT = 2, > > > > You don't need to specify "2". > > Ok. thanks. > > > > > > > diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c > > > index 8a54e87dbb17..da6cbd0032ca 100644 > > > --- a/security/landlock/syscalls.c > > > +++ b/security/landlock/syscalls.c > > > @@ -29,6 +29,7 @@ > > > #include "cred.h" > > > #include "fs.h" > > > #include "limits.h" > > > +#include "net.h" > > > #include "ruleset.h" > > > #include "setup.h" > > > > > > @@ -74,7 +75,8 @@ static void build_check_abi(void) > > > { > > > struct landlock_ruleset_attr ruleset_attr; > > > struct landlock_path_beneath_attr path_beneath_attr; > > > - size_t ruleset_size, path_beneath_size; > > > + struct landlock_net_port_attr net_port_attr; > > > + size_t ruleset_size, path_beneath_size, net_service_size; > > > > net_port_size > > right. will be fixed. > > .
