From nobody Mon Dec 23 15:41:13 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4YH2LK2CJxz5Wqgd; Mon, 23 Dec 2024 15:41:13 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R10" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4YH2LK1dQDz4kvR; Mon, 23 Dec 2024 15:41:13 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1734968473; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=3t7AndXF+Ft5zrUn0w4KZU1YreElTciHWhThhJQffRo=; b=OD84sHKhQvVJE7RP5QUa9ZIt/DtE2FQ7pmgeiwDS39bym6nxgRSdhinUf7GDaLv7NOKM6Z sd2XyxRfWqMvNrroL/Wf8DRMT1ydoeCq3lhRQE1rD8yX+rGA0AHpI+3qu9X9oa6GhHyt5B TJMuWxiieZripGTfckc1Yfld7Uir8JpMNdgk4A6Z5vM3nDobubnk8ZyHh41YMH6kRofzW5 U9CMxm6BPWa8BTliYeE3ZrC0yrXWGLg07Y+q2GR1aUHNm2KnHy1XlQInEOzkfE2vm9cTs6 oc6+E7XnT4w8tUXhwQMPAZpRK60p4Lno26mbGDpTynaFG89aJ2xh0RgIsENpLw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1734968473; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=3t7AndXF+Ft5zrUn0w4KZU1YreElTciHWhThhJQffRo=; b=CsHBQpt6jnkmP1Zep0cZSU/3ByHZQwxyd4Psj5lO3FTz+5TZWlHg+Wah14SBQdytAiBu8L Y0yNtawOBVG4WEwtKm+ZrEBN7xpYt16YB/uioNELkxPhCgtXZhlWIAWfrjDCxir4wIeR0u 6P+h+6L/ndcSK+McOZcU0XdUHgZ3HH7fzwAKJT84MLoVeFggwBoaeebkDwbxrpeNR7hegq AhIdRNUC5En84zrIImlWXrr7GWAsjlP62LpOll8HWHvSaEJoOJmhamIYm+oHKSPmzJhcHH gfY15cZyLpkff7TDQSdRcsgEb+ZG0TRUyyvmG9sr+snLEJmzu+0kJfcwFgfbzA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1734968473; a=rsa-sha256; cv=none; b=oEQjN/ffo6Oa7Vypbo+lhmfJ306+A3CJKXhq+75cVl+PrtdC/mwl3xC4uGoIni66V+Is3z jvX/RdSEMentZjPH0yu1ybs5aRIHZLFqJdDSXGOlyTWtfpIVDqSJ2iLhaIXWVX04cXyODl lQbVRxUCRH/Bvw2JG+p58NptCnkMILjD4OnkK1MVFigayV85cyEBvTNTfSrpTO123p/ZAZ 4opGs9oAkYy0L0NhqMsp357K48oVy1J0ti/V0WXhA6QFwvppyuvPJ4JjWKVa2xUKWkVu2E D1yY5bPpi+iJ24BwqSA3f0lIuSQgdXb+WmQLqD77O/cCLWh2oc7482N9Heak4Q== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4YH2LK1DnfzyjS; Mon, 23 Dec 2024 15:41:13 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 4BNFfDt8003761; Mon, 23 Dec 2024 15:41:13 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 4BNFfDDC003758; Mon, 23 Dec 2024 15:41:13 GMT (envelope-from git) Date: Mon, 23 Dec 2024 15:41:13 GMT Message-Id: <202412231541.4BNFfDDC003758@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Mark Johnston Subject: git: c9756953bded - main - inpcb: Further restrict binding to a port owned by a different UID List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: markj X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: c9756953bded0d8428027fa3e812c9bdac069252 Auto-Submitted: auto-generated The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=c9756953bded0d8428027fa3e812c9bdac069252 commit c9756953bded0d8428027fa3e812c9bdac069252 Author: Mark Johnston AuthorDate: 2024-12-23 15:31:11 +0000 Commit: Mark Johnston 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(); }