Unverified Commit e4d82cbc authored by Matthieu Buffet's avatar Matthieu Buffet Committed by Mickaël Salaün
Browse files

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: fff69fb0 ("landlock: Support network rules with TCP bind and connect")
Signed-off-by: default avatarMatthieu Buffet <matthieu@buffet.re>
Link: https://lore.kernel.org/r/20251027190726.626244-4-matthieu@buffet.re


Signed-off-by: default avatarMickaël Salaün <mic@digikod.net>
parent 552dbf47

security/landlock/net.c

+67 −51
Original line number Diff line number Diff line
@@ -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,45 +174,6 @@ 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
@@ -167,9 +183,9 @@ static int current_check_access_socket(struct socket *const sock, 
	 * 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)

	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));