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: <0e08c5d4-f5fd-e025-3f14-8e2ada3b7302@digikod.net> Date: Thu, 6 Apr 2023 12:28:26 +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 19:42, Konstantin Meskhidze (A) wrote: > > > 4/4/2023 7:42 PM, Mickaël Salaün пишет: >> >> 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_addrlen(const struct sockaddr *const address, int addrlen) >>>> >>>> const int addrlen >>> >>> Got it. >>>> >>>>> +{ >>>>> + if (addrlen < offsetofend(struct sockaddr, sa_family)) >>>>> + return -EINVAL; >>>>> + switch (address->sa_family) { >>>>> + case AF_UNSPEC: >>>>> + case AF_INET: >>>>> + if (addrlen < sizeof(struct sockaddr_in)) >>>>> + return -EINVAL; >>>>> + return 0; >>>>> +#if IS_ENABLED(CONFIG_IPV6) >>>>> + case AF_INET6: >>>>> + if (addrlen < SIN6_LEN_RFC2133) >>>>> + return -EINVAL; >>>>> + return 0; >>>>> +#endif >>>>> + } >>>>> + WARN_ON_ONCE(1); >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static u16 get_port(const struct sockaddr *const address) >>>>> +{ >>>>> + /* Gets port value in host byte order. */ >>>>> + switch (address->sa_family) { >>>>> + case AF_UNSPEC: >>>>> + case AF_INET: { >>>>> + const struct sockaddr_in *const sockaddr = >>>>> + (struct sockaddr_in *)address; >>>>> + return ntohs(sockaddr->sin_port); >>>> >>>> Storing ports in big endian (in rulesets) would avoid converting them >>>> every time the kernel checks a socket port. The above comment should >>>> then be updated too. >>> >>> I thought we came to a conclusion to stick to host endianess and >>> let kernel do the checks under the hood: >>> https://lore.kernel.org/linux-security-module/278ab07f-7583-a4e0-3d37-1bacd091531d@digikod.net/ >>> >>> Did I misunderstand something? >> >> We indeed stick to the host endianess for the UAPI/syscalls, but >> internally the kernel has to do the conversion with as it is currently >> done by calling ntohs(). To avoid calling ntohs() every time get_port() >> is called, we can instead only call htons() when creating rules (i.e. >> one-time htons call instead of multiple ntohs calls). >> > Do you mean we need to covert port in landlock_append_net_rule(): > > ... > > int err; > const struct landlock_id id = { > .key.data = ntohs(port), > .type = LANDLOCK_KEY_NET_PORT, > }; > BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data)); > ... > ???? landlock_append_net_rule() takes a u16 (host endianess, which is the case with this patch series) and should store a big endian 16-bit integer. See my patch: const struct landlock_id id = { - .key.data = port, + .key.data = (__force uintptr_t)htons(port), .type = LANDLOCK_KEY_NET_PORT, }; >> >>> Do you mean we need to do port converting __be16 -> u16 in >>> check_socket_access()??? >> >> Removing the ntohs() call from get_port() enables to return __be16 >> instead of u16, and check_socket_access() will then need to use the same >> type. > > Ok. I got it. Thanks. >> >> >>>> >>>> >>>>> + } >>>>> +#if IS_ENABLED(CONFIG_IPV6) >>>>> + case AF_INET6: { >>>>> + const struct sockaddr_in6 *const sockaddr_ip6 = >>>>> + (struct sockaddr_in6 *)address; >>>>> + return ntohs(sockaddr_ip6->sin6_port); >>>>> + } >>>>> +#endif >>>>> + } >>>>> + WARN_ON_ONCE(1); >>>>> + return 0; >>>>> +} >>>>> + >>>>> +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 >>>>> + 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)); >>>>> + } >>>>> + return allowed ? 0 : -EACCES; >>>>> +} >>>>> + >>>>> +static int hook_socket_bind(struct socket *sock, struct sockaddr *address, >>>>> + int addrlen) >>>>> +{ >>> + return check_socket_access(sock, address, addrlen, get_port(address), >>>>> + LANDLOCK_ACCESS_NET_BIND_TCP); >> >> get_port() is called before check_addrlen(), which is an issue. >> >> You'll find attached a patch for these fixes, please squash it in this >> one for the next version. >> >> I'll send other reviews by the end of the week. >> >> >>>>> +} >>>>> + >>>>> +static int hook_socket_connect(struct socket *sock, struct sockaddr *address, >>>>> + int addrlen) >>>>> +{ >>>>> + return check_socket_access(sock, address, addrlen, get_port(address), >>>>> + LANDLOCK_ACCESS_NET_CONNECT_TCP); >>>>> +} >>>> >>>> [...] >>>> .
Powered by blists - more mailing lists