From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 4BACA5A0278 for ; Wed, 28 Feb 2024 12:25:33 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1709119522; bh=LC6AQGDm+WaD0uxiQWFHQLvXchTga2Rt9duwgwK3HzU=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=EM6DDnbdjGpdGT/aSmDQps6yKUqVe7qY7MEoDKgieNlPAPo1/vJAY7jr1YLMtL9Vu f853diolLu+KfrKbk7Jj59Dp9TuHFlKHa7LZLqvOsB6lUpGYcWkApdk3qfrJ7GES3L zihe0bj9aCnahOVYCUmpQNb9BFF5/E5nDE0b2LDmyRBa9hoH0AuYJ9Gwd1iX9aeGd8 2ny+b6XjbBg76iQQI7TG7w/C2ZpJ3fmKij+EvGr5p4KxgSurGFewtK25GDRZ2/XVBF o6uQi4CTlzGYkqITLzZrSujT8S1+RJ1IPeoKPgnAQRGbHZVV4NQvQRzkepvwfjwgW6 6nOWdNGno4Oxw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4TlBq66fkKz4wyt; Wed, 28 Feb 2024 22:25:22 +1100 (AEDT) From: David Gibson To: passt-dev@passt.top, Stefano Brivio Subject: [PATCH v3 17/20] tcp: Validate TCP endpoint addresses Date: Wed, 28 Feb 2024 22:25:17 +1100 Message-ID: <20240228112520.2078220-18-david@gibson.dropbear.id.au> X-Mailer: git-send-email 2.43.2 In-Reply-To: <20240228112520.2078220-1-david@gibson.dropbear.id.au> References: <20240228112520.2078220-1-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: FZUIBWQ2C6IJZEWFSRLANRZHXK5K2EFW X-Message-ID-Hash: FZUIBWQ2C6IJZEWFSRLANRZHXK5K2EFW X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: David Gibson X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: TCP connections should typically not have wildcard addresses (0.0.0.0 or ::) nor a zero port number for either endpoint. It's not entirely clear (at least to me) if it's strictly against the RFCs to do so, but at any rate the socket interfaces often treat those values specially[1], so it's not really possible to manipulate such connections. Likewise they should not have broadcast or multicast addresses for either endpoint. However, nothing prevents a guest from creating a SYN packet with such values, and it's not entirely clear what the effect on passt would be. To ensure sane behaviour, explicitly check for this case and drop such packets, logging a debug warning (we don't want a higher level, because that would allow a guest to spam the logs). We never expect such an address on an accept()ed socket either, but just in case, check for it as well. [1] Depending on context as "unknown", "match any" or "kernel, pick something for me" Signed-off-by: David Gibson --- tcp.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 7 deletions(-) diff --git a/tcp.c b/tcp.c index b4d7eec6..2ea985e6 100644 --- a/tcp.c +++ b/tcp.c @@ -284,6 +284,7 @@ #include #include #include +#include #include /* For struct tcp_info */ @@ -1935,27 +1936,59 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, const struct tcphdr *th, const char *opts, size_t optlen, const struct timespec *now) { + in_port_t srcport = ntohs(th->source); + in_port_t dstport = ntohs(th->dest); struct sockaddr_in addr4 = { .sin_family = AF_INET, - .sin_port = th->dest, + .sin_port = htons(dstport), .sin_addr = *(struct in_addr *)daddr, }; struct sockaddr_in6 addr6 = { .sin6_family = AF_INET6, - .sin6_port = th->dest, + .sin6_port = htons(dstport), .sin6_addr = *(struct in6_addr *)daddr, }; const struct sockaddr *sa; struct tcp_tap_conn *conn; union flow *flow; + int s = -1, mss; socklen_t sl; - int s, mss; - - (void)saddr; if (!(flow = flow_alloc())) return; + if (af == AF_INET) { + if (IN4_IS_ADDR_UNSPECIFIED(saddr) || + IN4_IS_ADDR_BROADCAST(saddr) || + IN4_IS_ADDR_MULTICAST(saddr) || srcport == 0 || + IN4_IS_ADDR_UNSPECIFIED(daddr) || + IN4_IS_ADDR_BROADCAST(daddr) || + IN4_IS_ADDR_MULTICAST(daddr) || dstport == 0) { + char sstr[INET_ADDRSTRLEN], dstr[INET_ADDRSTRLEN]; + + debug("Invalid endpoint in TCP SYN: %s:%hu -> %s:%hu", + inet_ntop(AF_INET, saddr, sstr, sizeof(sstr)), + srcport, + inet_ntop(AF_INET, daddr, dstr, sizeof(dstr)), + dstport); + goto cancel; + } + } else if (af == AF_INET6) { + if (IN6_IS_ADDR_UNSPECIFIED(saddr) || + IN6_IS_ADDR_MULTICAST(saddr) || srcport == 0 || + IN6_IS_ADDR_UNSPECIFIED(daddr) || + IN6_IS_ADDR_MULTICAST(daddr) || dstport == 0) { + char sstr[INET6_ADDRSTRLEN], dstr[INET6_ADDRSTRLEN]; + + debug("Invalid endpoint in TCP SYN: %s:%hu -> %s:%hu", + inet_ntop(AF_INET6, saddr, sstr, sizeof(sstr)), + srcport, + inet_ntop(AF_INET6, daddr, dstr, sizeof(dstr)), + dstport); + goto cancel; + } + } + if ((s = tcp_conn_sock(c, af)) < 0) goto cancel; @@ -2006,8 +2039,8 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, sl = sizeof(addr6); } - conn->fport = ntohs(th->dest); - conn->eport = ntohs(th->source); + conn->fport = dstport; + conn->eport = srcport; conn->seq_init_from_tap = ntohl(th->seq); conn->seq_from_tap = conn->seq_init_from_tap + 1; @@ -2736,6 +2769,33 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref, if (s < 0) goto cancel; + if (sa.sa_family == AF_INET) { + const struct in_addr *addr = &sa.sa4.sin_addr; + in_port_t port = sa.sa4.sin_port; + + if (IN4_IS_ADDR_UNSPECIFIED(addr) || + IN4_IS_ADDR_BROADCAST(addr) || + IN4_IS_ADDR_MULTICAST(addr) || port == 0) { + char str[INET_ADDRSTRLEN]; + + err("Invalid endpoint from TCP accept(): %s:%hu", + inet_ntop(AF_INET, addr, str, sizeof(str)), port); + goto cancel; + } + } else if (sa.sa_family == AF_INET6) { + const struct in6_addr *addr = &sa.sa6.sin6_addr; + in_port_t port = sa.sa6.sin6_port; + + if (IN6_IS_ADDR_UNSPECIFIED(addr) || + IN6_IS_ADDR_MULTICAST(addr) || port == 0) { + char str[INET6_ADDRSTRLEN]; + + err("Invalid endpoint from TCP accept(): %s:%hu", + inet_ntop(AF_INET6, addr, str, sizeof(str)), port); + goto cancel; + } + } + if (tcp_splice_conn_from_sock(c, ref.tcp_listen.pif, ref.tcp_listen.port, flow, s, &sa)) return; -- 2.43.2