landlock: Fix TCP handling of short AF_UNSPEC addresses

current_check_access_socket() treats AF_UNSPEC addresses as
AF_INET ones, and only later adds special case handling to
allow connect(AF_UNSPEC), and on IPv4 sockets
bind(AF_UNSPEC+INADDR_ANY).
This would be fine except AF_UNSPEC addresses can be as
short as a bare AF_UNSPEC sa_family_t field, and nothing
more. The AF_INET code path incorrectly enforces a length of
sizeof(struct sockaddr_in) instead.

Move AF_UNSPEC edge case handling up inside the switch-case,
before the address is (potentially incorrectly) treated as
AF_INET.

Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
Link: https://lore.kernel.org/r/20251027190726.626244-4-matthieu@buffet.re
Signed-off-by: Mickaël Salaün <mic@digikod.net>

diff --git a/security/landlock/net.c b/security/landlock/net.c
index 1f3915a90a808b44cb6beeb64e0d23ee5615c7ed..e6367e30e5b0ece2f9731a74fe252e49ecfcac2d 100644 (file)
--- a/security/landlock/net.c
+++ b/security/landlock/net.c
@@ -71,6 +71,61 @@ static int current_check_access_socket(struct socket *const sock,
 
        switch (address->sa_family) {
        case AF_UNSPEC:
+               if (access_request == LANDLOCK_ACCESS_NET_CONNECT_TCP) {
+                       /*
+                        * 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.
+                        *
+                        * For a TCP access control system, this request is
+                        * legitimate. Let the network stack handle potential
+                        * inconsistencies and return -EINVAL if needed.
+                        */
+                       return 0;
+               } else if (access_request == LANDLOCK_ACCESS_NET_BIND_TCP) {
+                       /*
+                        * Binding to an AF_UNSPEC address is treated
+                        * differently by IPv4 and IPv6 sockets. The socket's
+                        * family may change under our feet due to
+                        * setsockopt(IPV6_ADDRFORM), but that's ok: we either
+                        * reject entirely or require
+                        * %LANDLOCK_ACCESS_NET_BIND_TCP for the given port, so
+                        * it cannot be used to bypass the policy.
+                        *
+                        * IPv4 sockets map AF_UNSPEC to AF_INET for
+                        * retrocompatibility for bind accesses, only if the
+                        * address is INADDR_ANY (cf. __inet_bind). IPv6
+                        * sockets always reject it.
+                        *
+                        * Checking the address is required to not wrongfully
+                        * return -EACCES instead of -EAFNOSUPPORT or -EINVAL.
+                        * We could return 0 and let the network stack handle
+                        * these checks, but it is safer to return a proper
+                        * error and test consistency thanks to kselftest.
+                        */
+                       if (sock->sk->__sk_common.skc_family == AF_INET) {
+                               const struct sockaddr_in *const sockaddr =
+                                       (struct sockaddr_in *)address;
+
+                               if (addrlen < sizeof(struct sockaddr_in))
+                                       return -EINVAL;
+
+                               if (sockaddr->sin_addr.s_addr !=
+                                   htonl(INADDR_ANY))
+                                       return -EAFNOSUPPORT;
+                       } else {
+                               if (addrlen < SIN6_LEN_RFC2133)
+                                       return -EINVAL;
+                               else
+                                       return -EAFNOSUPPORT;
+                       }
+               } else {
+                       WARN_ON_ONCE(1);
+               }
+               /* Only for bind(AF_UNSPEC+INADDR_ANY) on IPv4 socket. */
+               fallthrough;
        case AF_INET: {
                const struct sockaddr_in *addr4;
 
@@ -119,57 +174,18 @@ static int current_check_access_socket(struct socket *const sock,
                return 0;
        }
 
-       /* Specific AF_UNSPEC handling. */
-       if (address->sa_family == 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.
-                *
-                * For a TCP access control system, this request is legitimate.
-                * Let the network stack handle potential inconsistencies and
-                * return -EINVAL if needed.
-                */
-               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.
-                *
-                * We could return 0 and let the network stack handle these
-                * checks, but it is safer to return a proper error and test
-                * consistency thanks to kselftest.
-                */
-               if (access_request == LANDLOCK_ACCESS_NET_BIND_TCP) {
-                       /* addrlen has already been checked for AF_UNSPEC. */
-                       const struct sockaddr_in *const sockaddr =
-                               (struct sockaddr_in *)address;
-
-                       if (sock->sk->__sk_common.skc_family != AF_INET)
-                               return -EINVAL;
-
-                       if (sockaddr->sin_addr.s_addr != htonl(INADDR_ANY))
-                               return -EAFNOSUPPORT;
-               }
-       } else {
-               /*
-                * Checks sa_family consistency to not wrongfully return
-                * -EACCES instead of -EINVAL.  Valid sa_family changes are
-                * only (from AF_INET or AF_INET6) to AF_UNSPEC.
-                *
-                * We could return 0 and let the network stack handle this
-                * check, but it is safer to return a proper error and test
-                * consistency thanks to kselftest.
-                */
-               if (address->sa_family != sock->sk->__sk_common.skc_family)
-                       return -EINVAL;
-       }
+       /*
+        * Checks sa_family consistency to not wrongfully return
+        * -EACCES instead of -EINVAL.  Valid sa_family changes are
+        * only (from AF_INET or AF_INET6) to AF_UNSPEC.
+        *
+        * We could return 0 and let the network stack handle this
+        * check, but it is safer to return a proper error and test
+        * consistency thanks to kselftest.
+        */
+       if (address->sa_family != sock->sk->__sk_common.skc_family &&
+           address->sa_family != AF_UNSPEC)
+               return -EINVAL;
 
        id.key.data = (__force uintptr_t)port;
        BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));