From nobody Tue Jan 14 14:43:31 2025 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 4YXX1c097Zz5kmDK; Tue, 14 Jan 2025 14:43:32 +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 "R11" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4YXX1b53GQz3gV1; Tue, 14 Jan 2025 14:43:31 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1736865811; 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=4t6Bxh4WKgJ2RdsUAvc7C5EJW26dW2HNKzMCh8/KHbI=; b=hOPk6IaAYwPVgkMCZ/L2oO3aG0rA4OmWRCAkjSMQ9xq/cXYK+O2RkTWOaD0tP6jGOs5/ie n9IuIBsLqSdCjpFbWEZ0vLgrBCWhDL7HGio90A/Ldxhza2+VB+Porj9jDyGRy0bG9991Sp j4AIBdH8/fDAZLHXcL+gnCt7gfmJCI4V+3XqSItGe/sbS/2eIrc6FKynKS6+93vnvuwWAF merrmIr+J6SKMaaN5hmTVg6Lb1VAb8L6tz1BKfQXk6dPmI//sXzUvxSzjauC6E1WSWRZiq SQwkMqjyjiSUEZVMxBO1F5ClKKFVf11tYgh/Mhzt82jC4XspOp6ZzT8POy/FaA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1736865811; 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=4t6Bxh4WKgJ2RdsUAvc7C5EJW26dW2HNKzMCh8/KHbI=; b=UrMNSOLxcgLlfStUUrUXGkAZixJn+curNaVhx4nxZQ/PzTM+dk6O60rh7IAmtafNooQI/4 fK4j0VWJ6fIiIItuaq1DHaDymUpszMAHI9fQ4OTKDEsAMKCAfpIteksrjGmfjReOkbcCt8 Xmr/+Ue8ZouKh7lld3MbJySnlYt76uqOW56i4kc5QQ4MjLPpQoP5bJUNWcaMNontA5hPAJ PnaeD7KHFH7i/n5KpLdqgE+B9PsR3/GYrjGpKdrLjOg25Yx3+wtt7CJWdPkhkQCsQExuRc sozhfOJiIsuxCIR2pO7GG/ku1Airbs5QInG9dB0lGnFNxxnhGhaYzTL4tAZQVA== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1736865811; a=rsa-sha256; cv=none; b=VcEOCWYWKteicAZscxGW4doPeiHRAOZn1Ek/bprObncOmHAw36ExzDnkXDA7UaXbcFwlmq vjS20t+o8w9UIp8M/QKX3URmzWDQX+4a3xYawRGny+m5sJBTYhliP0LG9/w24ysyNQUscO VA821sZb3h9uS2xKvZUeOXjTMjMI7BzM82bQ/Zg3ZxLYhpRN7T3joPVn4NHi1VbpcGFk5X hSulaLfgb7pypL8Geymih1KQswfGBGBPsUH7p92VZmk3z0/Ns1CmAKaNSZtNB8n/y0yAhZ ZEfNa/Orr+x4Ceh5Bl/D2EeQFqH1LS5TuNmZpzR/5PrVyWbzds3XmN+uYvDRXQ== ARC-Authentication-Results: i=1; mx1.freebsd.org; none 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 4YXX1b4cqDz1KQ1; Tue, 14 Jan 2025 14:43:31 +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 50EEhVRg014046; Tue, 14 Jan 2025 14:43:31 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 50EEhVVd014043; Tue, 14 Jan 2025 14:43:31 GMT (envelope-from git) Date: Tue, 14 Jan 2025 14:43:31 GMT Message-Id: <202501141443.50EEhVVd014043@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Mark Johnston Subject: git: cbf77cecec38 - stable/14 - inpcb: Remove bogus SO_REUSEPORT(_LB) checks in in_pcbbind() 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/stable/14 X-Git-Reftype: branch X-Git-Commit: cbf77cecec38dfcdd5fdc24fc1fba9eeae3b00f1 Auto-Submitted: auto-generated The branch stable/14 has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=cbf77cecec38dfcdd5fdc24fc1fba9eeae3b00f1 commit cbf77cecec38dfcdd5fdc24fc1fba9eeae3b00f1 Author: Mark Johnston AuthorDate: 2024-12-12 14:06:06 +0000 Commit: Mark Johnston CommitDate: 2025-01-14 14:14:24 +0000 inpcb: Remove bogus SO_REUSEPORT(_LB) checks in in_pcbbind() This check for SO_REUSEPORT was added way back in commit 52b65dbe85faf. Per the commit log, this commit restricted this port-stealing check to unicast addresses, and then only if the existing socket does not have SO_REUSEPORT set. In other words, if there exists a socket bound to INADDR_ANY, and we bind a socket to INADDR_ANY with the same port, then the two sockets need not be owned by the same user if the existing socket has SO_REUSEPORT set. This is a surprising semantic; bugzilla PR 7713 gives some additional context. That PR makes a case for the behaviour described above when binding to a multicast address. But, the SO_REUSEPORT check is only applied when binding to a non-multicast address, so it doesn't really make sense. In the PR the committer notes that "unicast applications don't set SO_REUSEPORT", which makes some sense, but also refers to "multicast applications that bind to INADDR_ANY", which sounds a bit suspicious. OpenBSD performs the multicast check, but not the SO_REUSEPORT check. DragonflyBSD removed the SO_REUSEPORT (and INADDR_ANY) checks back in 2014 (commit 0323d5fde12a4). NetBSD explicitly copied our logic and still has it. The plot thickens: 20 years later, SO_REUSEPORT_LB was ported from DragonflyBSD: this option provides similar semantics to SO_REUSEPORT, but for unicast addresses it causes incoming connections/datagrams to be distributed among all sockets in the group. This commit (1a43cff92a20d) inverted the check for SO_REUSEPORT while adding one for SO_REUSEPORT_LB; this appears to have been inadvertent. However: - apparently no one has noticed that the semantics were changed; - sockets belonging to different users can now be bound to the same port so long as they belong to a single lbgroup bound to INADDR_ANY, which is not correct. Simply remove the SO_REUSEPORT(_LB) checks, as their original justification was dubious and their current implementation is wrong; add some tests. Reviewed by: glebius MFC after: 1 month Sponsored by: Klara, Inc. Sponsored by: Stormshield Differential Revision: https://reviews.freebsd.org/D47832 (cherry picked from commit 4f02a7d739b354eef38e19b25866f64842d69414) --- sys/netinet/in_pcb.c | 4 +- sys/netinet6/in6_pcb.c | 4 +- tests/sys/netinet/socket_afinet.c | 240 +++++++++++++++++++++++++++++++++++++- 3 files changed, 241 insertions(+), 7 deletions(-) diff --git a/sys/netinet/in_pcb.c b/sys/netinet/in_pcb.c index 0add5a7c340b..331804545bee 100644 --- a/sys/netinet/in_pcb.c +++ b/sys/netinet/in_pcb.c @@ -928,9 +928,7 @@ in_pcbbind_avail(struct inpcb *inp, const struct in_addr laddr, (inp->inp_socket->so_type != SOCK_STREAM || in_nullhost(t->inp_faddr)) && (!in_nullhost(laddr) || - !in_nullhost(t->inp_laddr) || - (t->inp_socket->so_options & SO_REUSEPORT) || - (t->inp_socket->so_options & SO_REUSEPORT_LB) == 0) && + !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 de864305b434..04b87819d629 100644 --- a/sys/netinet6/in6_pcb.c +++ b/sys/netinet6/in6_pcb.c @@ -247,9 +247,7 @@ in6_pcbbind_avail(struct inpcb *inp, const struct sockaddr_in6 *sin6, (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) || - (t->inp_socket->so_options & SO_REUSEPORT) || - (t->inp_socket->so_options & SO_REUSEPORT_LB) == 0) && + !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 7076f084719a..ba8c03af46a6 100644 --- a/tests/sys/netinet/socket_afinet.c +++ b/tests/sys/netinet/socket_afinet.c @@ -2,6 +2,7 @@ * SPDX-License-Identifier: BSD-2-Clause * * Copyright (c) 2019 Bjoern A. Zeeb + * Copyright (c) 2024 Stormshield * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -25,11 +26,17 @@ * SUCH DAMAGE. */ -#include +#include #include +#include + #include + #include #include +#include +#include +#include #include @@ -281,6 +288,235 @@ ATF_TC_BODY(socket_afinet_stream_reconnect, tc) ATF_CHECK_EQ(0, rc); } +/* + * Make sure that unprivileged users can't set the IP_BINDANY or IPV6_BINDANY + * socket options. + */ +ATF_TC(socket_afinet_bindany); +ATF_TC_HEAD(socket_afinet_bindany, tc) +{ + atf_tc_set_md_var(tc, "require.user", "unprivileged"); +} +ATF_TC_BODY(socket_afinet_bindany, tc) +{ + int s; + + s = socket(AF_INET, SOCK_STREAM, 0); + ATF_REQUIRE(s >= 0); + ATF_REQUIRE_ERRNO(EPERM, + setsockopt(s, IPPROTO_IP, IP_BINDANY, &(int){1}, sizeof(int)) == + -1); + ATF_REQUIRE(close(s) == 0); + + s = socket(AF_INET, SOCK_DGRAM, 0); + ATF_REQUIRE(s >= 0); + ATF_REQUIRE_ERRNO(EPERM, + setsockopt(s, IPPROTO_IP, IP_BINDANY, &(int){1}, sizeof(int)) == + -1); + ATF_REQUIRE(close(s) == 0); + + s = socket(AF_INET6, SOCK_STREAM, 0); + ATF_REQUIRE(s >= 0); + ATF_REQUIRE_ERRNO(EPERM, + setsockopt(s, IPPROTO_IPV6, IPV6_BINDANY, &(int){1}, sizeof(int)) == + -1); + ATF_REQUIRE(close(s) == 0); + + s = socket(AF_INET6, SOCK_DGRAM, 0); + ATF_REQUIRE(s >= 0); + ATF_REQUIRE_ERRNO(EPERM, + setsockopt(s, IPPROTO_IPV6, IPV6_BINDANY, &(int){1}, sizeof(int)) == + -1); + ATF_REQUIRE(close(s) == 0); +} + +/* + * Bind a socket to the specified address, optionally dropping privileges and + * setting one of the SO_REUSE* options first. + * + * 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) +{ + const char *user; + pid_t child; + + if (unpriv) { + if (!atf_tc_has_config_var(tc, "unprivileged_user")) + atf_tc_skip("unprivileged_user not set"); + user = atf_tc_get_config_var(tc, "unprivileged_user"); + } else { + user = NULL; + } + + child = fork(); + ATF_REQUIRE(child != -1); + if (child == 0) { + int s; + + if (user != NULL) { + struct passwd *passwd; + + passwd = getpwnam(user); + if (seteuid(passwd->pw_uid) != 0) + _exit(1); + } + + s = socket(sa->sa_family, type, 0); + if (s < 0) + _exit(2); + if (bind(s, sa, sa->sa_len) == 0) + _exit(3); + if (errno != EADDRINUSE) + _exit(4); + if (opt != 0) { + if (setsockopt(s, SOL_SOCKET, opt, &(int){1}, + sizeof(int)) != 0) + _exit(5); + } + if (bind(s, sa, sa->sa_len) == 0) + _exit(6); + if (errno != EADDRINUSE) + _exit(7); + _exit(0); + } else { + int status; + + ATF_REQUIRE_EQ(waitpid(child, &status, 0), child); + ATF_REQUIRE(WIFEXITED(status)); + status = WEXITSTATUS(status); + ATF_REQUIRE_MSG(status == 0 || status == 6, + "child exited with %d", status); + return (status == 6); + } +} + +static bool +child_bind_priv(const atf_tc_t *tc, int type, struct sockaddr *sa, int opt) +{ + return (child_bind(tc, type, sa, opt, false)); +} + +static bool +child_bind_unpriv(const atf_tc_t *tc, int type, struct sockaddr *sa, int opt) +{ + return (child_bind(tc, type, sa, opt, true)); +} + +static int +bind_socket(int domain, int type, int opt, bool unspec, struct sockaddr *sa) +{ + socklen_t slen; + int s; + + s = socket(domain, type, 0); + ATF_REQUIRE(s >= 0); + + if (domain == AF_INET) { + struct sockaddr_in sin; + + bzero(&sin, sizeof(sin)); + sin.sin_family = AF_INET; + sin.sin_len = sizeof(sin); + sin.sin_addr.s_addr = htonl(unspec ? + INADDR_ANY : INADDR_LOOPBACK); + sin.sin_port = htons(0); + ATF_REQUIRE(bind(s, (struct sockaddr *)&sin, sizeof(sin)) == 0); + + slen = sizeof(sin); + } else /* if (domain == AF_INET6) */ { + struct sockaddr_in6 sin6; + + bzero(&sin6, sizeof(sin6)); + sin6.sin6_family = AF_INET6; + sin6.sin6_len = sizeof(sin6); + sin6.sin6_addr = unspec ? in6addr_any : in6addr_loopback; + sin6.sin6_port = htons(0); + ATF_REQUIRE(bind(s, (struct sockaddr *)&sin6, sizeof(sin6)) == 0); + + slen = sizeof(sin6); + } + + if (opt != 0) { + ATF_REQUIRE(setsockopt(s, SOL_SOCKET, opt, &(int){1}, + sizeof(int)) == 0); + } + + ATF_REQUIRE(getsockname(s, sa, &slen) == 0); + + return (s); +} + +static void +multibind_test(const atf_tc_t *tc, int domain, int type) +{ + struct sockaddr_storage ss; + int opts[4] = { 0, SO_REUSEADDR, SO_REUSEPORT, SO_REUSEPORT_LB }; + int s; + bool flags[2] = { false, true }; + bool res; + + for (size_t flagi = 0; flagi < nitems(flags); flagi++) { + for (size_t opti = 0; opti < nitems(opts); opti++) { + s = bind_socket(domain, type, opts[opti], flags[flagi], + (struct sockaddr *)&ss); + for (size_t optj = 0; optj < nitems(opts); optj++) { + int opt; + + opt = opts[optj]; + res = child_bind_priv(tc, type, + (struct sockaddr *)&ss, opt); + /* + * Multi-binding is only allowed when both + * sockets have SO_REUSEPORT or SO_REUSEPORT_LB + * set. + */ + if (opts[opti] != 0 && + opts[opti] != SO_REUSEADDR && opti == optj) + ATF_REQUIRE(res); + else + ATF_REQUIRE(!res); + + res = child_bind_unpriv(tc, type, + (struct sockaddr *)&ss, opt); + /* + * 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(close(s) == 0); + } + } +} + +/* + * Try to bind two sockets to the same address/port tuple. Under some + * conditions this is permitted. + */ +ATF_TC(socket_afinet_multibind); +ATF_TC_HEAD(socket_afinet_multibind, 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_multibind, tc) +{ + multibind_test(tc, AF_INET, SOCK_STREAM); + multibind_test(tc, AF_INET, SOCK_DGRAM); + multibind_test(tc, AF_INET6, SOCK_STREAM); + multibind_test(tc, AF_INET6, SOCK_DGRAM); +} + ATF_TP_ADD_TCS(tp) { ATF_TP_ADD_TC(tp, socket_afinet); @@ -289,6 +525,8 @@ ATF_TP_ADD_TCS(tp) ATF_TP_ADD_TC(tp, socket_afinet_poll_no_rdhup); ATF_TP_ADD_TC(tp, socket_afinet_poll_rdhup); 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); return atf_no_error(); }