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: <b38d6e13-bf10-1cc7-edbb-9973d526d08f@huawei.com> Date: Thu, 6 Apr 2023 13:37:58 +0300 From: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com> To: Mickaël Salaün <mic@...ikod.net> 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 4/6/2023 1:31 PM, Mickaël Salaün пишет: > > 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. Got it. Thanks. > .
Powered by blists - more mailing lists