Message-ID: <4b24e1d8-8249-ef0e-5069-90fb7b315503@huawei-partners.com>
Date: Fri, 24 Jan 2025 12:59:03 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Matthieu Buffet <matthieu@...fet.re>, Mickael Salaun <mic@...ikod.net>
CC: Gunther Noack <gnoack@...gle.com>, <konstantin.meskhidze@...wei.com>, Paul
 Moore <paul@...l-moore.com>, <linux-security-module@...r.kernel.org>,
	<netdev@...r.kernel.org>, <netfilter-devel@...r.kernel.org>, Pablo Neira
 Ayuso <pablo@...filter.org>, Jozsef Kadlecsik <kadlec@...filter.org>
Subject: Re: [PATCH v2 3/6] landlock: Add UDP sendmsg access control

On 1/21/2025 1:30 AM, Matthieu Buffet wrote:
> Hi,
> 
> (for netfilter folks added a bit late: this should be self-contained but 
> original patch is here[1], it now raises a question about netfilter hook 
> execution context at the end of this email - you can just skip to it if 
> not interested in the LSM part)
> 
> On 12/14/2024 7:45 PM, Matthieu Buffet wrote:
>> Add support for a LANDLOCK_ACCESS_NET_SENDTO_UDP access right,
>> complementing the two previous LANDLOCK_ACCESS_NET_CONNECT_UDP and
>> LANDLOCK_ACCESS_NET_BIND_UDP.
>> It allows denying and delegating the right to sendto() datagrams with an
>> explicit destination address and port, without requiring to connect() the
>> socket first.
>> [...]
>> +static int hook_socket_sendmsg(struct socket *const sock,
>> +                   struct msghdr *const msg, const int size)
>> +{
>> +    const struct landlock_ruleset *const dom =
>> +        landlock_get_applicable_domain(landlock_get_current_domain(),
>> +                           any_net);
>> +    const struct sockaddr *address = (const struct sockaddr 
>> *)msg->msg_name;
>> +    const int addrlen = msg->msg_namelen;
>> +    __be16 port;
>> +     [...]
>> +    if (!sk_is_udp(sock->sk))
>> +        return 0;
>> +
>> +    /* Checks for minimal header length to safely read sa_family. */
>> +    if (addrlen < offsetofend(typeof(*address), sa_family))
>> +        return -EINVAL;
>> +
>> +    switch (address->sa_family) {
>> +    case AF_UNSPEC:
>> +        /*
>> +         * Parsed as "no address" in udpv6_sendmsg(), which means
>> +         * we fall back into the case checked earlier: policy was
>> +         * enforced at connect() time, nothing to enforce here.
>> +         */
>> +        if (sock->sk->sk_prot == &udpv6_prot)
>> +            return 0;
>> +        /* Parsed as "AF_INET" in udp_sendmsg() */
>> +        fallthrough;
>> +    case AF_INET:
>> +        if (addrlen < sizeof(struct sockaddr_in))
>> +            return -EINVAL;
>> +        port = ((struct sockaddr_in *)address)->sin_port;
>> +        break;
>> +
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +    case AF_INET6:
>> +        if (addrlen < SIN6_LEN_RFC2133)
>> +            return -EINVAL;
>> +        port = ((struct sockaddr_in6 *)address)->sin6_port;
>> +        break;
>> +#endif /* IS_ENABLED(CONFIG_IPV6) */
>> +
>> +    default:
>> +        return -EAFNOSUPPORT;
>> +    }
>> +
>> +    return check_access_port(dom, LANDLOCK_ACCESS_NET_SENDTO_UDP, port);
>> +}
>> +
>>   static struct security_hook_list landlock_hooks[] __ro_after_init = {
>>       LSM_HOOK_INIT(socket_bind, hook_socket_bind),
>>       LSM_HOOK_INIT(socket_connect, hook_socket_connect),
>> +    LSM_HOOK_INIT(socket_sendmsg, hook_socket_sendmsg),
>>   };
> 
> Looking back at this part of the patch to fix the stupid #ifdef, I 
> noticed sk->sk_prot can change under our feet, just like sk->sk_family 
> as highlighted by Mikhail in [2] due to setsockopt(IPV6_ADDRFORM).
> Replacing the check with READ_ONCE(sock->sk->sk_family) == AF_INET6 or 
> even taking the socket's lock would not change anything:
> setsockopt(IPV6_ADDRFORM) runs concurrently and locklessly.
> 
> So with this patch, any Landlock domain with rights to connect(port A) 
> and no port allowed to be set explicitly in sendto() could actually 
> sendto(arbitrary port B) :
> 1. create an IPv6 UDP socket
> 2. connect it to (any IPv4-mapped-IPv6 like ::ffff:127.0.0.1, port A)
> 3a. sendmsg(AF_UNSPEC + actual IPv4 target, port B)
> 3b. race setsockopt(IPV6_ADDRFORM) on another thread
> 4. retry from 1. until sendmsg() succeeds
> 
> I've put together a quick PoC, the race works. SELinux does not have 
> this problem because it uses a netfilter hook, later down the packet 
> path. I see three "fixes", I probably missed some others:
> 
> A: block IPV6_ADDRFORM support in a setsockopt() hook, if UDP_SENDMSG is 
> handled. AFAIU, not an option since this breaks a userland API
> 
> B: remove sendmsg(AF_UNSPEC) support on IPv6 sockets. Same problem as A
> 
> C: use a netfilter NF_INET_LOCAL_OUT hook like selinux_ip_output() 
> instead of an LSM hook

We can naively follow the semantics of this flag: "This access right is
checked [...] when the destination address passed is not NULL", and
check address even for IPV6+AF_UNSPEC. Calling sendto() on IPV6 socket
with specified AF_UNSPEC address does not look like common or useful
practice and can be restricted.

> 
> For C, problem is to get the sender process' credentials, and ideally to 
> avoid tagging sockets (what SELinux uses to fetch its security context, 
> also why it does not have this problem). Otherwise, we would add another 
> case of varying semantics (like rights to truncate/ioctl) to keep in 
> mind for Landlock users, this time with sockets kept after enforcing a 
> new ruleset, or passed to/from another domain - not a fan.
> 
> I don't know if it is safe to assume for UDP that NF_INET_LOCAL_OUT 
> executes in process context: [3] doesn't specify, and [4] mentions the 
> possibility to execute in interrupt context due to e.g. retransmits, but 
> that does not apply to UDP. Looking at the code, it looks like it has to 
> run in process context to be able to make the syscall return EPERM if 
> the verdict is NF_DROP, but I don't know if that's something that can be 
> relied upon to be always true, including in future revisions. Could use 
> some input from someone knowledgeable in netfilter.
> 
> What do you think?
> 
> [1] 
> https://lore.kernel.org/all/20241214184540.3835222-1-matthieu@buffet.re/
> [2] https://lore.kernel.org/netdev/20241212.zoh7Eezee9ka@digikod.net/T/
> [3] 
> https://www.netfilter.org/documentation/HOWTO/netfilter-hacking-HOWTO-4.html#ss4.6
> [4] 
> https://netfilter-devel.vger.kernel.narkive.com/yZHiFEVh/execution-context-in-netfilter-hooks#post5

Powered by blists - more mailing lists