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: <b91cc429-2772-e96c-7fb1-53f4b8d79abc@digikod.net> Date: Thu, 6 Apr 2023 12:31:55 +0200 From: Mickaël Salaün <mic@...ikod.net> To: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com> Cc: willemdebruijn.kernel@...il.com, gnoack3000@...il.com, linux-security-module@...r.kernel.org, netdev@...r.kernel.org, netfilter-devel@...r.kernel.org, yusongping@...wei.com, artem.kuzin@...wei.com Subject: Re: [PATCH v10 09/13] landlock: Add network rules and TCP hooks support On 05/04/2023 21:19, Konstantin Meskhidze (A) wrote: > > > 4/4/2023 8:02 PM, Mickaël Salaün пишет: >> >> On 04/04/2023 18:42, Mickaël Salaün wrote: >>> >>> On 04/04/2023 11:31, Konstantin Meskhidze (A) wrote: >>>> >>>> >>>> 3/31/2023 8:24 PM, Mickaël Salaün пишет: >>>>> >>>>> On 23/03/2023 09:52, 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 enable to restrict TCP socket binding and connection >>>>>> to specific ports. >>>>>> >>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com> >>>>>> --- >>>>>> >>>>>> 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 | 49 +++++ >>>>>> security/landlock/Kconfig | 1 + >>>>>> security/landlock/Makefile | 2 + >>>>>> security/landlock/limits.h | 6 +- >>>>>> security/landlock/net.c | 198 +++++++++++++++++++ >>>>>> security/landlock/net.h | 26 +++ >>>>>> security/landlock/ruleset.c | 52 ++++- >>>>>> security/landlock/ruleset.h | 63 +++++- >>>>>> security/landlock/setup.c | 2 + >>>>>> security/landlock/syscalls.c | 72 ++++++- >>>>>> tools/testing/selftests/landlock/base_test.c | 2 +- >>>>>> 11 files changed, 450 insertions(+), 23 deletions(-) >>>>>> create mode 100644 security/landlock/net.c >>>>>> create mode 100644 security/landlock/net.h >>>>> >>>>> [...] >>>>> >>>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c >>>>> >>>>> [...] >> >> >>>>>> +static int check_socket_access(struct socket *sock, struct sockaddr *address, int addrlen, u16 port, >>>>>> + access_mask_t access_request) >>>>>> +{ >>>>>> + int ret; >>>>>> + bool allowed = false; >>>>>> + layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {}; >>>>>> + const struct landlock_rule *rule; >>>>>> + access_mask_t handled_access; >>>>>> + const struct landlock_id id = { >>>>>> + .key.data = port, >>>>>> + .type = LANDLOCK_KEY_NET_PORT, >>>>>> + }; >>>>>> + const struct landlock_ruleset *const domain = get_current_net_domain(); >>>>>> + >>>>>> + if (WARN_ON_ONCE(!domain)) >>>>>> + return 0; >>>>>> + if (WARN_ON_ONCE(domain->num_layers < 1)) >>>>>> + return -EACCES; >>>>>> + /* Check if it's a TCP socket. */ >>>>>> + if (sock->type != SOCK_STREAM) >>>>>> + return 0; >>>>>> + >>>>>> + ret = check_addrlen(address, addrlen); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>>> + switch (address->sa_family) { >>>>>> + case AF_UNSPEC: >>>>>> + /* >>>>>> + * Connecting to an address with AF_UNSPEC dissolves the TCP >>>>>> + * association, which have the same effect as closing the >>>>>> + * connection while retaining the socket object (i.e., the file >>>>>> + * descriptor). As for dropping privileges, closing >>>>>> + * connections is always allowed. >>>>>> + */ >>>>>> + if (access_request == LANDLOCK_ACCESS_NET_CONNECT_TCP) >>>>>> + return 0; >>>>>> + >>>>>> + /* >>>>>> + * For compatibility reason, accept AF_UNSPEC for bind >>>>>> + * accesses (mapped to AF_INET) only if the address is >>>>>> + * INADDR_ANY (cf. __inet_bind). Checking the address is >>>>>> + * required to not wrongfully return -EACCES instead of >>>>>> + * -EAFNOSUPPORT. >>>>>> + */ >>>>>> + if (access_request == LANDLOCK_ACCESS_NET_BIND_TCP) { >>>>>> + const struct sockaddr_in *const sockaddr = >>>>>> + (struct sockaddr_in *)address; >>>>>> + >>>>>> + if (sockaddr->sin_addr.s_addr != htonl(INADDR_ANY)) >>>>>> + return -EAFNOSUPPORT; >>>>>> + } >>>>>> + >>>>>> + fallthrough; >>>>>> + case AF_INET: >>>>>> +#if IS_ENABLED(CONFIG_IPV6) >>>>>> + case AF_INET6: >>>>>> +#endif >> >> Some more fixes: >> >> You can move the port/id.key.data block from my patch here, where it is >> actually used. >> > Ok. Thank you. I will apply it. >> >>>>>> + rule = landlock_find_rule(domain, id); >>>>>> + handled_access = landlock_init_layer_masks( >>>>>> + domain, access_request, &layer_masks, >>>>>> + LANDLOCK_KEY_NET_PORT); >>>>>> + allowed = landlock_unmask_layers(rule, handled_access, >>>>>> + &layer_masks, >>>>>> + ARRAY_SIZE(layer_masks)); >> >> The `return allowed ? 0 : -EACCES;` should be here. >> >>>>>> + } >>>>>> + return allowed ? 0 : -EACCES; >> >> We should have `return 0;` here. >> > Got it. Thanks > >> We need a test for an sa_family different than AF_UNSPEC, AF_INET, and >> AF_INET6 to make sure everything else is allowed (e.g. AF_UNIX with >> SOCK_STREAM and another test with SOCK_DGRAM). Please make sure this new >> test will not pass with SOCK_STREAM and the current patch series, but of >> course it should pass with the next one. > > Do you mean AF_UNIX with SOCK_STREAM will not be passed as well as > AF_UNIX with SOCK_DGRAM? AF_UNIX with SOCK_STREAM would be denied with this patch series, which is a bug. AF_UNIX with SOCK_DGRAM should always be allowed with this patch series, which is correct. AF_UNIX with SOCK_STREAM or SOCK_DGRAM should always be allowed, and the next patch series should come with a new test to check this two kind of sockets.
Powered by blists - more mailing lists