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: <96ba0f20-682a-be03-e6dd-d6f42f080493@huawei.com> Date: Thu, 6 Apr 2023 13:30:32 +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:28 PM, Mickaël Salaün пишет: > > 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, > }; > Thanks. Already took a look. > > >>> >>>> 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