Check for an overly large fragment.
ClosedPublic
Actions

Authored by markj on Nov 8 2018, 11:46 PM.

Details

Reviewers

emaste
jtl
glebius
bz

Commits

rS340313: Ensure that IP fragments do not extend beyond IP_MAXPACKET.

Summary

Packets exceeding this limit can trigger an overflow in the code which
inserts the fragment into its list; this can be used to violate the
invariant that fragments are sorted by increasing ip_off.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Nov 8 2018, 11:46 PM

Harbormaster completed remote builds in B20712: Diff 50186.Nov 8 2018, 11:46 PM

markj added reviewers: emaste, bz, jtl, glebius.Nov 8 2018, 11:47 PM

emaste added inline comments.Nov 9 2018, 12:04 AM

sys/netinet/ip_reass.c
209 ↗	(On Diff #50186)	you could `& IP_OFFMASK` in host order

Yeah, I saw this shortly before seeing this review. I agree we should fix it.

sys/netinet/ip_reass.c
209 ↗	(On Diff #50186)	We really should convert all of these to host order once on receipt and then just leave them that way until we are ready to reassemble. (I'm not suggesting we make that part of the same commit, but its yet one more piece of cleanup.)
209 ↗	(On Diff #50186)	Have you confirmed that this comparison will be promoted to something wider than 16 bits by the compiler? (In other words, will the comparison itself be safe from overflow on all of our compilers?) I don't see anything in here that is specifically wider than 16 bits that would trigger that.
209 ↗	(On Diff #50186)	To be pedantic, this could do "the wrong thing" if the IP header of the first fragment had no options, but subsequent fragments did have options. With this check, you could end up discarding a fragment (which came with options) at the end of the packet, even though that amount of data would fit in the reassembled IP packet (if the first fragment had no options). You could avoid this by moving the check below the place where we adjust ip_len. (Note that I personally think such a scenario is both highly unusual and broken.)

I'm writing a regression test for this to stick under tests/sys/netinet. I'll see if I can add test cases for D17922 as well.

sys/netinet/ip_reass.c
209 ↗	(On Diff #50186)	I agree that we could use some cleanup here, I just didn't want to pollute the review. Regarding type widths, yes, everything should be promoted to int before the comparison. We depend on this on line 368 as well. I'll move the check below the adjustment.

In D17914#382797, @markj wrote:

I'm writing a regression test for this to stick under tests/sys/netinet. I'll see if I can add test cases for D17922 as well.

Thanks! That's great; it will be very helpful!

Sorry; getting IPv4 fragments into my head is not a good idea.