Details

Reviewers

tuexen
rscheff

Group Reviewers

transport

Commits

rS361752: We should never allow either the broadcast or IN_ADDR_ANY to be

Summary

Address 0.0.0.0 and 255.255.255.255 really should not be allowed to be sent
to in TCP but somehow this is overlooked. This patch fixes that.

Test Plan

Make sure we can no longer try to connect to or send to 0.0.0.0 or 255.255.255.255 with pkt-drill or any other favorite tooling

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rrs requested review of this revision.May 15 2020, 12:41 PM

rrs created this revision.

rscheff added a subscriber: rscheff.May 15 2020, 1:45 PM

rscheff added inline comments.

tcp_usrreq.c
547 ↗	(On Diff #71820)	Do you want to use INADDR_ANY and INADDR_BROADCAST here instead of the hardcoded numericals? (Why is the reviews context missing when you generate a Diff, rrs@?) In the inet6 case below, should IN6_IS_ADDR_UNSPECIFIED (::) also be excluded?

This revision was not accepted when it landed; it landed in state Needs Review.May 15 2020, 2:00 PM

Closed by commit rS361080: This fixes several skyzaller issues found with the (authored by rrs). · Explain Why

This revision was automatically updated to reflect the committed changes.

rrs added a commit: rS361080: This fixes several skyzaller issues found with the.

tuexen added inline comments.May 15 2020, 2:00 PM

tcp_usrreq.c
547 ↗	(On Diff #71820)	I guess using the constants if preferred. Regarding the IPv6 question. The wirdcard address gets automagically transformed to the loopback address. See in6_pcb.c:378. I guess the missing context is due to the usage of simply svn diff without requesting the context.

Herald added 1 blocking reviewer(s): transport. · View Herald TranscriptMay 15 2020, 2:00 PM

Herald added 1 blocking reviewer(s): transport. · View Herald Transcript

Herald added subscribers: melifaro, imp. · View Herald Transcript

rscheff added a reviewer: rscheff.May 15 2020, 7:39 PM

A commit (rS361080) referenced this Diff due to a mistype, binding the wrong commit here., when D24826 should have been targeted. Reopening, so this actual issue doesn't drop to the floor.

rscheff removed a commit: rS361080: This fixes several skyzaller issues found with the.May 15 2020, 7:43 PM

@rrs can you please re-upload the Diff, with the defined constants from in.h instead of hardcoded numbers.

I don't know how to bind the older (actual) Diff back here, when the commit bound this to a different Diff...

This revision now requires changes to proceed.May 15 2020, 7:46 PM

0.0.0.0 and :: are unicast addresses, and it's a bit unclear to me why transport protocol would want to decide that some underlying addresses are invalid. They also work in some other operating systems.
255.255.255.255 is indeed broadcast and not intended to use with TCP, but the same argument can be made around multicast address range. Do we want to include it as well?

Could you please add a reference to any standard explaining why these should be prohibited?

TCP is strictly unicast connection oriented. There are other protocols to implement reliable transport over multicast (eg RFC3208). TCP simply does not work with a multicast address as either source or destination, since there are no mechanisms to arbitrate between one-to-many and then many-to-one responses.

Multicast is already restricted in the code (see https://reviews.freebsd.org/source/src/browse/head/sys/netinet/tcp_usrreq.c$323) for both IPv4 and IPv6.

Incoming packets should never contain the placeholder value INADDR_ANY (0.0.0.0 or :: ) but always be resolved to an actual (unicast) address.

For some reason, https://reviews.freebsd.org/D24852?id=71820 does not show any context though.