ip: Defer checks for an unspecified dstaddr until after pfil hooks

[markj]

Summary

To comply with Common Criteria certification requirements, it may be
necessary to ensure that packets to 0.0.0.0/::0 are dropped and logged
by the system firewall. Currently, such packets are dropped by
ip_input() and ip6_input() before reaching pfil hooks; let's defer the
checks slightly to give firewalls a chance to drop the packets
themselves, as this gives better observability. Add some regression
tests for this with pf+pflog.

Note that prior to commit 713264f6b8b, v4 packets to the unspecified
address were not dropped by the IP stack at all.

Note that ip_forward() and ip6_forward() ensure that such packets are
not forwarded; they are passed back unmodified.

[glebius]

On second thought this probably needs to be mentioned in Relnotes and not MFC-ed. Cause it might be that somebody was relying on the stack to drop those packets before firewall.

[markj]

In D48163#1098248, @glebius wrote:

On second thought this probably needs to be mentioned in Relnotes and not MFC-ed. Cause it might be that somebody was relying on the stack to drop those packets before firewall.

Well, prior to commit 713264f6b8bc5f they weren't, and that commit is not very old. A RELNOTES entry is reasonable.

Note that our stack will drop such packets even if forwarding is enabled. I guess the concern is that a firewall redirect might turn a packet to 0.0.0.0 into a valid packet?

[zlei]

In D48163#1098396, @markj wrote:

In D48163#1098248, @glebius wrote:

On second thought this probably needs to be mentioned in Relnotes and not MFC-ed. Cause it might be that somebody was relying on the stack to drop those packets before firewall.

Well, prior to commit 713264f6b8bc5f they weren't, and that commit is not very old. A RELNOTES entry is reasonable.

Note that our stack will drop such packets even if forwarding is enabled. I guess the concern is that a firewall redirect might turn a packet to 0.0.0.0 into a valid packet?

I thinks that is a general question, that is, does a firewall want to see all known invalid packets? For example, the above logic to check IN_LOOPBACK on the wire.

[franco_opnsense.org]

The Common Criteria aspects are documented here https://docs.vmware.com/en/VMware-SD-WAN/6.0/VMware-SD-WAN-Administration-Guide/GUID-DF592A36-E680-44CE-ABFC-ACE19B55B448.html

Quoting them here since the site is going to be offline at the end of the year:

The following packets are automatically dropped, counted, or logged:

Packets with invalid fragments or fragments which cannot be completely re-assembled that are destined to the Edge.
Packets where the source address is defined as being on either broadcast network, multicast network, or loopback address.
Packets with the IP options: Loose Source Routing, Strict Source Routing, or Record Route specified.
Packets which have the source or destination address as unspecified or reserved for future.
Packets where the source address is equal to the address of the network interface where the network packet was received.
Packets where the source address does not belong to the networks reachable via the network interface where the network packet was received.
Packets where the source or destination address of the network packet is defined as being unspecified (i.e. 0.0.0.0) or an address “reserved for future use” (i.e. 240.0.0.0/4) as specified in RFC 5735 for IPv4.
Packets where the source or destination address of the network packet is defined as an “unspecified address” or an address “reserved for future definition and use” (i.e. unicast addresses not in this address range: 2000::/3) as specified in RFC 3513 for IPv6.

The CC Firewall settings are applied to all the Edges associated with the Profile.

If anything beyond this is of value here to individual use cases that haven't existed before may be an academic question.

The biggest problem in handling invalid packets this way is pf route-to by the way, which sends packets out the wire... for which D8877 was proposed a long time ago.

[ae]

In D48163#1098396, @markj wrote:

I guess the concern is that a firewall redirect might turn a packet to 0.0.0.0 into a valid packet?

I can think of such a use, but even if such a packet is redirected to localhost, it will result in invalid response with an unspecified source address, because original source and destination will be used in PCB.

[markj]

In D48163#1098579, @zlei wrote:

I thinks that is a general question, that is, does a firewall want to see all known invalid packets? For example, the above logic to check IN_LOOPBACK on the wire.

I am not sure about this. I can see an argument in either direction. For this change my justification is that, until recently, the v4 stack did not check at all for dst_addr == INADDR_ANY. So, really I'm trying to restore the old behaviour here, and keeping IPv6 the same for consistency.

I could be convinced to defer more invalid packet checks to after the pfil hook. Does anyone see any (dis)advantages to that?

In D48163#1098612, @ae wrote:

In D48163#1098396, @markj wrote:

I guess the concern is that a firewall redirect might turn a packet to 0.0.0.0 into a valid packet?

I can think of such a use, but even if such a packet is redirected to localhost, it will result in invalid response with an unspecified source address, because original source and destination will be used in PCB.

I don't quite follow - why is the source address necessarily unspecified? The modified check is looking only at the destination address. An unspecified source address can arise in practice, e.g., DHCP clients will generate such packets.

[ae]

In D48163#1098658, @markj wrote:

I don't quite follow - why is the source address necessarily unspecified? The modified check is looking only at the destination address. An unspecified source address can arise in practice, e.g., DHCP clients will generate such packets.

I meant the case when ipfw is used as transparent proxy. In this case it adds fwd tag to a mbuf and ip*_input can handle packets that are not destined for the local host. In this case the reply, that is generated by TCP will have source address that was destination address in forwarded packet. Anyway, I think it is right that we drop such packets.