git: c9756953bded - main - inpcb: Further restrict binding to a port owned by a different UID
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Mon, 23 Dec 2024 15:41:13 UTC
The branch main has been updated by markj:
URL: https://cgit.FreeBSD.org/src/commit/?id=c9756953bded0d8428027fa3e812c9bdac069252
commit c9756953bded0d8428027fa3e812c9bdac069252
Author: Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2024-12-23 15:31:11 +0000
Commit: Mark Johnston <markj@FreeBSD.org>
CommitDate: 2024-12-23 15:41:06 +0000
inpcb: Further restrict binding to a port owned by a different UID
See commit 4f02a7d739b3 for more background.
I cannot see a good reason to continue ignoring mismatching UIDs when
binding to INADDR_ANY. Looking at the sdr.V2.4a7n sources (mentioned in
bugzilla PR 7713), there is a CANT_MCAST_BIND hack wherein the
application binds to INADDR_ANY instead of a multicast address, but
CANT_MCAST_BIND isn't defined for FreeBSD builds.
It seems unlikely that we still have a use-case for allowing sockets
from different UIDs to bind to the same port when binding to the
unspecified address. And, as noted in D47832, applications like sdr
would have been broken by the inverted SO_REUSEPORT check removed in
that revision, apparently without any bug reports. Let's break
compatibility and simply disallow this case outright.
Also, add some comments, remove a hack in a regression test which tests
this funtionality, and add a new regression test to exercise the
remaining checks that were added in commit 4658dc8325e03.
MFC after: 1 month
Sponsored by: Klara, Inc.
Sponsored by: Stormshield
Differential Revision: https://reviews.freebsd.org/D47870
---
sys/netinet/in_pcb.c | 11 ++++-
sys/netinet6/in6_pcb.c | 11 ++++-
tests/sys/netinet/socket_afinet.c | 86 ++++++++++++++++++++++++++++++++++-----
3 files changed, 94 insertions(+), 14 deletions(-)
diff --git a/sys/netinet/in_pcb.c b/sys/netinet/in_pcb.c
index 432cae68062a..11bc68a3915a 100644
--- a/sys/netinet/in_pcb.c
+++ b/sys/netinet/in_pcb.c
@@ -919,13 +919,20 @@ in_pcbbind_avail(struct inpcb *inp, const struct in_addr laddr,
if (!IN_MULTICAST(ntohl(laddr.s_addr)) &&
priv_check_cred(inp->inp_cred, PRIV_NETINET_REUSEPORT) != 0) {
+ /*
+ * If a socket owned by a different user is already
+ * bound to this port, fail. In particular, SO_REUSE*
+ * can only be used to share a port among sockets owned
+ * by the same user.
+ *
+ * However, we can share a port with a connected socket
+ * which has a unique 4-tuple.
+ */
t = in_pcblookup_local(inp->inp_pcbinfo, laddr, lport,
INPLOOKUP_WILDCARD, cred);
if (t != NULL &&
(inp->inp_socket->so_type != SOCK_STREAM ||
in_nullhost(t->inp_faddr)) &&
- (!in_nullhost(laddr) ||
- !in_nullhost(t->inp_laddr)) &&
(inp->inp_cred->cr_uid != t->inp_cred->cr_uid))
return (EADDRINUSE);
}
diff --git a/sys/netinet6/in6_pcb.c b/sys/netinet6/in6_pcb.c
index 51f2a29918fe..b44fe5ed3553 100644
--- a/sys/netinet6/in6_pcb.c
+++ b/sys/netinet6/in6_pcb.c
@@ -239,13 +239,20 @@ in6_pcbbind_avail(struct inpcb *inp, const struct sockaddr_in6 *sin6,
if (!IN6_IS_ADDR_MULTICAST(laddr) &&
priv_check_cred(inp->inp_cred, PRIV_NETINET_REUSEPORT) !=
0) {
+ /*
+ * If a socket owned by a different user is already
+ * bound to this port, fail. In particular, SO_REUSE*
+ * can only be used to share a port among sockets owned
+ * by the same user.
+ *
+ * However, we can share a port with a connected socket
+ * which has a unique 4-tuple.
+ */
t = in6_pcblookup_local(inp->inp_pcbinfo, laddr, lport,
INPLOOKUP_WILDCARD, cred);
if (t != NULL &&
(inp->inp_socket->so_type != SOCK_STREAM ||
IN6_IS_ADDR_UNSPECIFIED(&t->in6p_faddr)) &&
- (!IN6_IS_ADDR_UNSPECIFIED(laddr) ||
- !IN6_IS_ADDR_UNSPECIFIED(&t->in6p_laddr)) &&
(inp->inp_cred->cr_uid != t->inp_cred->cr_uid))
return (EADDRINUSE);
diff --git a/tests/sys/netinet/socket_afinet.c b/tests/sys/netinet/socket_afinet.c
index ba8c03af46a6..6fc98d982602 100644
--- a/tests/sys/netinet/socket_afinet.c
+++ b/tests/sys/netinet/socket_afinet.c
@@ -337,7 +337,8 @@ ATF_TC_BODY(socket_afinet_bindany, tc)
* Returns true if the bind succeeded, and false if it failed with EADDRINUSE.
*/
static bool
-child_bind(const atf_tc_t *tc, int type, struct sockaddr *sa, int opt, bool unpriv)
+child_bind(const atf_tc_t *tc, int type, struct sockaddr *sa, int opt,
+ bool unpriv)
{
const char *user;
pid_t child;
@@ -483,16 +484,8 @@ multibind_test(const atf_tc_t *tc, int domain, int type)
/*
* Multi-binding is only allowed when both
* sockets have the same owner.
- *
- * XXX-MJ we for some reason permit this when
- * binding to the unspecified address, but I
- * don't think that's right
*/
- if (flags[flagi] && opts[opti] != 0 &&
- opts[opti] != SO_REUSEADDR && opti == optj)
- ATF_REQUIRE(res);
- else
- ATF_REQUIRE(!res);
+ ATF_REQUIRE(!res);
}
ATF_REQUIRE(close(s) == 0);
}
@@ -517,6 +510,78 @@ ATF_TC_BODY(socket_afinet_multibind, tc)
multibind_test(tc, AF_INET6, SOCK_DGRAM);
}
+static void
+bind_connected_port_test(const atf_tc_t *tc, int domain)
+{
+ struct sockaddr_in sin;
+ struct sockaddr_in6 sin6;
+ struct sockaddr *sinp;
+ int error, sd[3], tmp;
+ bool res;
+
+ /*
+ * Create a connected socket pair.
+ */
+ sd[0] = socket(domain, SOCK_STREAM, 0);
+ ATF_REQUIRE_MSG(sd[0] >= 0, "socket failed: %s", strerror(errno));
+ sd[1] = socket(domain, SOCK_STREAM, 0);
+ ATF_REQUIRE_MSG(sd[1] >= 0, "socket failed: %s", strerror(errno));
+ if (domain == PF_INET) {
+ memset(&sin, 0, sizeof(sin));
+ sin.sin_family = AF_INET;
+ sin.sin_len = sizeof(sin);
+ sin.sin_addr.s_addr = htonl(INADDR_ANY);
+ sin.sin_port = htons(0);
+ sinp = (struct sockaddr *)&sin;
+ } else {
+ ATF_REQUIRE(domain == PF_INET6);
+ memset(&sin6, 0, sizeof(sin6));
+ sin6.sin6_family = AF_INET6;
+ sin6.sin6_len = sizeof(sin6);
+ sin6.sin6_addr = in6addr_any;
+ sin6.sin6_port = htons(0);
+ sinp = (struct sockaddr *)&sin6;
+ }
+
+ error = bind(sd[0], sinp, sinp->sa_len);
+ ATF_REQUIRE_MSG(error == 0, "bind failed: %s", strerror(errno));
+ error = listen(sd[0], 1);
+ ATF_REQUIRE_MSG(error == 0, "listen failed: %s", strerror(errno));
+
+ error = getsockname(sd[0], sinp, &(socklen_t){ sinp->sa_len });
+ ATF_REQUIRE_MSG(error == 0, "getsockname failed: %s", strerror(errno));
+
+ error = connect(sd[1], sinp, sinp->sa_len);
+ ATF_REQUIRE_MSG(error == 0, "connect failed: %s", strerror(errno));
+ tmp = accept(sd[0], NULL, NULL);
+ ATF_REQUIRE_MSG(tmp >= 0, "accept failed: %s", strerror(errno));
+ ATF_REQUIRE(close(sd[0]) == 0);
+ sd[0] = tmp;
+
+ /* bind() should succeed even from an unprivileged user. */
+ res = child_bind(tc, SOCK_STREAM, sinp, 0, false);
+ ATF_REQUIRE(!res);
+ res = child_bind(tc, SOCK_STREAM, sinp, 0, true);
+ ATF_REQUIRE(!res);
+}
+
+/*
+ * Normally bind() prevents port stealing by a different user, even when
+ * SO_REUSE* are specified. However, if the port is bound by a connected
+ * socket, then it's fair game.
+ */
+ATF_TC(socket_afinet_bind_connected_port);
+ATF_TC_HEAD(socket_afinet_bind_connected_port, tc)
+{
+ atf_tc_set_md_var(tc, "require.user", "root");
+ atf_tc_set_md_var(tc, "require.config", "unprivileged_user");
+}
+ATF_TC_BODY(socket_afinet_bind_connected_port, tc)
+{
+ bind_connected_port_test(tc, AF_INET);
+ bind_connected_port_test(tc, AF_INET6);
+}
+
ATF_TP_ADD_TCS(tp)
{
ATF_TP_ADD_TC(tp, socket_afinet);
@@ -527,6 +592,7 @@ ATF_TP_ADD_TCS(tp)
ATF_TP_ADD_TC(tp, socket_afinet_stream_reconnect);
ATF_TP_ADD_TC(tp, socket_afinet_bindany);
ATF_TP_ADD_TC(tp, socket_afinet_multibind);
+ ATF_TP_ADD_TC(tp, socket_afinet_bind_connected_port);
return atf_no_error();
}