From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id 4655A5A0278 for ; Thu, 22 Feb 2024 13:46:23 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1708605982; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=UmOO4rn7K8Ss9C5Jn1mVHxDH7xO/zHpau4KSC5CEDkQ=; b=gyVIO9XdwRZkS1F2hyvem0E1qjG2u2l/a9N45M+Y0I9xK3uNIjFucAfNq3uaNyJFqFR4dx WiI3G1FN8Bk/RfDRx0v9iEgnmKneZSVj/7mZlNZk7a+PXfHoG4YKEpFt0kxI3rVhYNiYZ1 1l78d0JfPkUZlMnoLL3Pari+reOTzKA= Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-363-OweAq7xBOBCNGFimWu0BKg-1; Thu, 22 Feb 2024 07:46:20 -0500 X-MC-Unique: OweAq7xBOBCNGFimWu0BKg-1 Received: by mail-ej1-f71.google.com with SMTP id a640c23a62f3a-a3f56a2936eso90037266b.1 for ; Thu, 22 Feb 2024 04:46:20 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708605979; x=1709210779; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=UmOO4rn7K8Ss9C5Jn1mVHxDH7xO/zHpau4KSC5CEDkQ=; b=iiRgvhjsR2lQL/oOcFpxNTlBpc9Trqh13nnACw9kvq+8BSDbsV4FTo3NRCiCcTH3JF CsPeHHBTu1vdx6dDBzOi312wcL7d4GeW6qbap7JzzWp/D9bwZ9nJJYcQxouUQu0X1OKj cHH5HG2lw2IoYLK81iF6sPtC6XgLtgV7S4CIUKDDDl8x4GoGylmizid2U8uASdAq5hx5 M11JewxDE0r4e6n8FJZzxnXr1kMU7D4jHLNnYfoEu8rE7KoNxTbu0uY135xyYzLuyBlX n7nx/zuPzw8XABumUdCcGtc5R45jv9deSJf7K61mqVzbBTNaczD0Qes4zD7ipWlpmpzO bBSQ== X-Gm-Message-State: AOJu0YwoWfNv7tuRuK8cidtAgJ/OwfuG5c6eLGKTbaw5EpKe3k/Omx66 E7KyMy/VmG9qstWcirISCdYa70z1Rxs+dkSWdAtNwGpz7zcxPsjIUmuJOGW88iYRQR3BokKjEXH lXiKvzDtplrdDvnvuJoWqJFg2rw4JkUga1nV4wZlzdsp5Q1udNXWgja15Ws+K X-Received: by 2002:a17:906:4ecd:b0:a3e:5c67:2883 with SMTP id i13-20020a1709064ecd00b00a3e5c672883mr2198826ejv.22.1708605979077; Thu, 22 Feb 2024 04:46:19 -0800 (PST) X-Google-Smtp-Source: AGHT+IFzX/gx1y9d0afL8zWDrJiuPkVlFt6bUamTGQEHzz2ea9g2Gx/0LuMlreJDm8M+shdPwB+hJQ== X-Received: by 2002:a17:906:4ecd:b0:a3e:5c67:2883 with SMTP id i13-20020a1709064ecd00b00a3e5c672883mr2198812ejv.22.1708605978775; Thu, 22 Feb 2024 04:46:18 -0800 (PST) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id lg9-20020a170906f88900b00a3e94142018sm4062564ejb.132.2024.02.22.04.46.17 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 22 Feb 2024 04:46:18 -0800 (PST) Date: Thu, 22 Feb 2024 13:45:44 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v2 19/22] tcp: Validate TCP endpoint addresses Message-ID: <20240222134544.0c5a8bc3@elisabeth> In-Reply-To: <20240206011734.884138-20-david@gibson.dropbear.id.au> References: <20240206011734.884138-1-david@gibson.dropbear.id.au> <20240206011734.884138-20-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.36; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: PKIA65JL4ZWCHH3E2SAMK6F5NEYXWM4M X-Message-ID-Hash: PKIA65JL4ZWCHH3E2SAMK6F5NEYXWM4M X-MailFrom: sbrivio@redhat.com 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: passt-dev@passt.top 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: On Tue, 6 Feb 2024 12:17:31 +1100 David Gibson wrote: > TCP connections should typically not have wildcard addresses (0.0.0.0 or > ::) nor a zero port number for either endpoint. This is only true for non-local connections, see also: https://archives.passt.top/passt-dev/20240119105630.089c5d34@elisabeth/ Just looking at RFC 6890, which should be sufficient: +----------------------+----------------------------+ | Attribute | Value | +----------------------+----------------------------+ | Address Block | 0.0.0.0/8 | | Name | "This host on this network"| | RFC | [RFC1122], Section 3.2.1.3 | | Allocation Date | September 1981 | | Termination Date | N/A | | Source | True | | Destination | False | | Forwardable | False | | Global | False | | Reserved-by-Protocol | True | +----------------------+----------------------------+ ...it can be used as source, but surely not by a non-loopback interface, so not by the guest. About using it as destination: the table says it can't be done, but: $ nc -l -p 8818 & echo abcd | nc 0.0.0.0 8818 [2] 2624647 abcd and: # tcpdump -ni lo tcp port 8818 tcpdump: verbose output suppressed, use -v[v]... for full protocol decode listening on lo, link-type EN10MB (Ethernet), snapshot length 262144 bytes 13:24:51.379436 IP 127.0.0.1.58574 > 127.0.0.1.8818: Flags [S], seq 3285989360, win 65495, options [mss 65495,sackOK,TS val 1253719321 ecr 0,nop,wscale 7], length 0 13:24:51.379447 IP 127.0.0.1.8818 > 127.0.0.1.58574: Flags [S.], seq 3128422158, ack 3285989361, win 65483, options [mss 65495,sackOK,TS val 1253719321 ecr 1253719321,nop,wscale 7], length 0 13:24:51.379457 IP 127.0.0.1.58574 > 127.0.0.1.8818: Flags [.], ack 1, win 512, options [nop,nop,TS val 1253719321 ecr 1253719321], length 0 13:24:51.379508 IP 127.0.0.1.58574 > 127.0.0.1.8818: Flags [P.], seq 1:6, ack 1, win 512, options [nop,nop,TS val 1253719321 ecr 1253719321], length 5 13:24:51.379513 IP 127.0.0.1.8818 > 127.0.0.1.58574: Flags [.], ack 6, win 512, options [nop,nop,TS val 1253719321 ecr 1253719321], length 0 it's not effectively used, but the kernel understands what I mean by 0.0.0.0: connect(3, {sa_family=AF_INET, sin_port=htons(8818), sin_addr=inet_addr("0.0.0.0")}, 16) = -1 EINPROGRESS (Operation now in progress) So I wouldn't exclude its usage in tcp_listen_handler() -- even though sure, the kernel translates that, so I don't think it can ever become a practical issue. If it annoys you in the perspective of the flow table, maybe we can turn it into a loopback address, when we see it's used as source or destination? If that's problematic as well, I can totally live with this patch, though. About IPv6: +----------------------+---------------------+ | Attribute | Value | +----------------------+---------------------+ | Address Block | ::/128 | | Name | Unspecified Address | | RFC | [RFC4291] | | Allocation Date | February 2006 | | Termination Date | N/A | | Source | True | | Destination | False | | Forwardable | False | | Global | False | | Reserved-by-Protocol | True | +----------------------+---------------------+ ...this is more specific, and its usage as source address is exemplified in RFC 4291, 2.5.2: One example of its use is in the Source Address field of any IPv6 packets sent by an initializing host before it has learned its own address. ...which should never be the case for any flow passt has to handle, so I think it's fine to refuse it in tcp_listen_handler(). The rest of the series, up to 22/22, looks good to me. > 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 31d4e87b..236a8d23 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -284,6 +284,7 @@ > #include > #include > #include > +#include > > #include /* For struct tcp_info */ > > @@ -1930,27 +1931,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; > > @@ -2001,8 +2034,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; > @@ -2732,6 +2765,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; -- Stefano