Add support for header chain validation on IPv6 Fragments (RFC7112)

[thj]

Summary

RFC7112 requires that the first fragment in a fragment chain contains a valid
header.
"When a host fragments an IPv6 datagram, it MUST include the entire
IPv6 Header Chain in the First Fragment."

Check that the first fragment (frag offset = 0) has a next header which is an
upper layer header and check that there is enough payload in the fragment to
evaluate the upper layer header.

Drop any fragments that have already arrived for this fragment chain if header
validation fails.

Test Plan

I have a scapy packet generator here[1] that will create fragments for UDP,
TCP, and v6 in v6. It creates two fragments for TCP and v6 in v6 and sends them
first fragment first and first fragment second, to test flushing the fragment
chain on a bad first fragment.

Because fragments other than the final one have to be a multiple of 8 bytes
UDP, UDP-Lite, SCTP, ICMP6 and ESP will have to have valid headers to reach
this check.

[1]: https://people.freebsd.org/~thj/tests/sendfrag.py

There is a fragment accounting issue I still need to track down, I suspect that short fragments aren't being accounted for and I am not accounting for the fragments in the existing chain when I clear it.

[bz]

sys/netinet6/frag6.c

339

doesn't have doesn't have to have a have. doesn't contain a full upper maybe?

340

Sentences end in a .

346

R..emove ..... ... chain.

989

Empty line.

[thj]

• Add support for ipv4 in ipv6, gre and dccp fragments
• Unstatic definition
• address language issues in comments

I still need to update the test script, getting scapy to generate dccp
fragments is less straight forward than expected. There are no regressions
against the previous test script.

[thj]

• Correctly locate structs in mbuf chain

[tuexen]

sys/netinet/dccp.h

43 ↗

(On Diff #48828)

I have a hard time to match the above structure to what is in RFC4340. The structure has a length of 15 bytes, whereas the header is either 12 or 16 bytes long. Maybe just define the short header and make clear in the name that it is the short header....

45 ↗

(On Diff #48828)

I think the definition of IPPROTO_DCCP should go into netinet/in.h.

sys/netinet6/frag6.c

990

Why are you initialising this to 0?

1015

Don't you need mtod() here? Are are sure you have at least 13 consecutive bytes in the mbuf?

1036

Don't you need mtod() here? Are are sure you have at least 1 byte in the mbuf?

1037

I guess you want to use the corresponding field of ip on the right side. Without initialising hdrlen to 0, your compiler might have catched this...

[thj]

sys/netinet6/frag6.c

1015

Yes, I realised this was incorrect as soon I left to catch the bus. Should be fixed in the later rev

[tuexen]

sys/netinet6/frag6.c

1037

When I was putting in the comment, the code looked different. One question remains: Are you sure you have enough consecutive bytes in the mbuf?

[tuexen]

Hi Tom,

something went wrong when I entered my comments. They showed up at the wrong line and refer to code looking differently from the patch how...
So forget about them, except three comments:

• You might consider moving IPPROTO_DCCP to netinet/in.h.
• Are you sure you have enough consecutive bytes to be able to do pointer arithmetic of access the fields in the transport header?
• Why are you initialising hdrlen to 0 instead of just declaring it?

[thj]

sys/netinet/dccp.h

43 ↗

(On Diff #48828)

I missed a reserved byte in the header.

Do you suggest two structures for DCCP, a long and a short one?

45 ↗

(On Diff #48828)

There is already an entry in the DCCP slot (33), IPPROTO_SEP.

Question, should I put an entry for DCCP next to IPPROTO_SEP, put DCCP elsewhere in the file or replace the SEP entry with DCCP?

IPPROTO 33 is DCCP in the IANA Registry.
A grep of the tree only turns up a single reference to IPPROTO_SEP

https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml

The commit that brought in IPPROTO_SEP references rfc1700 and suggests there might be some tuning of names required. The reference in rfc1700 for SEP is '[JC120] <mystery contact>'

I am leaning towards dropping this entry, but don't have any idea what effect removing this symbol will have.

[jtl]

sys/netinet/dccp.h

45 ↗

(On Diff #48828)

It looks like this was added about 20 years ago in r33804. From the commit log, it looks like the entries were added straight from RFC1700, without regard to whether they were used.

As RFC1700 is no longer the authoritative list of internet numbers, I think it is safe to change the list in the header file to conform to the actual authoritative source.

(Random thought to ponder: what will people think in 2038 when they read my commits from 2018?)

[thj]

• Teach netinet/in.h about dccp
• Make sure length bytes are in the first mbuf

[thj]

• Use (ip+exthdr len) + length when pulling up mbuf

[tuexen]

sys/netinet6/frag6.c

999

Doesn't this require the complete fragment to be in contiguous memory? I'm not sure this is always possible. I think it would be better to require only the minimum amount of memory to access the fields to be contiguous, which would be protocol specific.

[thj]

• Do pullup per protcol that requires it

[jtl]

sys/netinet/dccp.h

39 ↗

(On Diff #48909)

Technically, this is both ccval and cscov. It probably needs something similar to this excerpt from struct tcphdr:

#if BYTE_ORDER == LITTLE_ENDIAN
        u_char  th_x2:4,                /* (unused) */
                th_off:4;               /* data offset */
#endif
#if BYTE_ORDER == BIG_ENDIAN
        u_char  th_off:4,               /* data offset */
                th_x2:4;                /* (unused) */
#endif

41 ↗

(On Diff #48909)

These last few fields don't exactly match the RFC. Because this header could be used by others in the future, I think its important that what you commit match the RFC. (OTOH, I think it is fine to just define the common part [through the "X" field] and add a placeholder for what follows.)

sys/netinet6/frag6.c

339

This still isn't fixed: "doesn't have an upper layer..." or "doesn't contain an upper layer..."

346

This comment still needs to be a full sentence.

779

This isn't the right statistic for this error. We might need to teach frag6_freef to deal with different error codes.

1020

In a failure, this results in a double-free.

Even though this resets m, it doesn't update the caller's concept of m. To do that you need to change the prototype to something like:
int ip6_validhdrchain(struct mbuf **mp, ...)

Then, you can do this early in the code:

struct mbuf *m;

m = *mp;

Then, after this failure, you would do:
*mp = NULL;

Alternatively, you can change the semantics of the return value such that a certain return value always means the mbuf is freed.

1037

Again, doublefree. See above.

1049

Doublefree. See above.

1062

I can see a possibility that this will cause strange problems in two cases:

1. Down the road, when someone adds a new protocol, they may forget to add it to this list. For that reason, I think we need to do similar checking on unfragmented packets -- at least, for INVARIANTS builds. In fact, there is no reason you couldn't use exactly this same function to check unfragmented packets.
2. In cases where someone uses raw sockets to listen for a new protocol. There are two options for dealing with this that immediately come to mind. The easy option is to have the default behavior controlled by a sysctl. The harder option is to have the default behavior vary based on the protocols for which raw sockets are listening. For version 1, I would suggest the "easy" way (sysctl, and with a default of "allow").

1064

This is unnecessary. I think you should either have a default clause in the switch statement or this, but not both.

sys/netinet6/ip6_var.h

405 ↗

(On Diff #48909)

As implemented, why is this not just a static function?

[thj]

sys/netinet6/ip6_var.h

405 ↗

(On Diff #48909)

ae@ requested it be exposed for other users (pf/ipfil...)

[thj]

Address review comments

• don't leak mbufs in validhdrchain
• Add new icmpv6 param problem error for fragment chain bad
• return new param problem when header chain validation fails
• add new param problem to icmpv6 error stat histogram
• teach netstat about this new error counter
• fix accounting when dropping already queued fragments
• fully specify dccp header

This doesn't

• call validhdrchain for all packets with invariants
• add a sysctl to control use

[thj]

Respond to review comments

• add function to test if proto is upper layer header
• add validhdr check in ip6_input with INVARIANTS
• add sysctl to stop check on unknown upper layer headers

[bz]

I have a kind-of stupid question: where in all this do you actually walk an entire header chain to the end rather than just checking the next one? Are there no cases when this might be needed?

[jtl]

sys/netinet/dccp.h

46 ↗

(On Diff #57763)

Due to alignment, it seems likely that an extra byte will be added before d_cksum. The easiest way to get around this is to turn this into a 2-byte array (e.g. uint8_t d_cksum[2]).

sys/netinet6/frag6.c

344

The indentation looks funny here; however, that could be a Phabricator display issue. Please verify the continuation line is tabbed to line up with the if + 4 spaces more. See style(9) for more complete details.

344

Let me illustrate my concern with this change by way of example: Assume I have a packet with this set of headers: IP6, frag, dstopts, tcp. As far as I can tell, this change will not verify that the tcp header is in the first fragment.

To counteract this, I think you need to add some sort of loop to recursively check headers until it reaches a ULP header, exceeds some limit (e.g. the same loop limit we have in the main header processing code), or reaches the end of the packet.

347

Comments need to be a full sentence.

[thj]

sys/netinet6/frag6.c

344

Only allowing fragments in the form [frag ulp] was deliberate, we were aiming for a simpler change.

We have everything now to do this correctly (searching the entire chain). I will update to allow more extension headers between the frag header and the ulp

[bz]

In addition to the other comments I still nowhere see a loop that walks the entire extension header chain. If you don't do that you cannot fulfil the requirement you are trying to solve. What am I missing?

sys/netinet/in.h

173 ↗

(On Diff #57763)

If this is to be done, it's a separate commit which could just happen right away.

sys/netinet6/frag6.c

341

I would even reference RFC 8200 (pg 18/19):

The first fragment packet is composed of:
..
    (3)  Extension headers, if any, and the Upper-Layer header.  These
         headers must be in the first fragment.  Note: This restricts
         the size of the headers through the Upper-Layer header to the
         MTU of the path to the packet's destinations(s).

I've just worked my way through frag6.c and I start to understand what I didn't like on the current approach:

(1) one call to a function would be enough the "ip6_upperlayerhdr(ip6f->ip6f_nxt)" could be done there.
(2) the V_ip6_allow_unknown_upperlayerhdr is something with this generic name I'd love to have inside that one function (or a helper function called from there) but not in the code itself. It is not a property of frag6.c to decide if this policy is acceptable or not but of the checking code.

So I guess I am asking to collapse the initial check and the additional full search into one KPI (making the other two functions possible helper functions, though only if that makes sense code/loop wise). I haven't looked yet.

Then lastly I'll probably go back and revise where/when the check is done but let's leave that for the moment as frag6.c is likely to change in the meantime.

986

This code really should not live in frag6.c. There's a few more places in the source tree which can re-use this. Having done one of the first loops in the tree in ipfw2 after v6 support was merged and people having added more protocols to it since, please make sure these two are in sync, compatible, if not even able to share code.

[thj]

updated

[dheerajk_netapp.com]

Can you share your plan on when this code will be committed? I dont see the code changes in the GIT repository of FreeBSD

[thj]

Update to building version

[dheerajk_netapp.com]

sys/netinet6/ip6_input.c

959 ↗

(On Diff #64442)

For an IPv6 fragment with no upper-layer header in the first fragment, wouldn't this allow the fragment without discarding it?
ip6_upperlayerhdr will return true(1) only if the header that is checked is a upper layer protocol.

[thj]

sys/netinet6/ip6_input.c

959 ↗

(On Diff #64442)

Looks to me that this is only checking the upper layer header will be vailid if it is present and ensuring that one is in the first fragment.

[bdcom.dragon_gmail.com]

sys/netinet6/ip6_input.c

934 ↗

(On Diff #109960)

what if the packet (first frag) looks like this [ip6_hdr] + [frag_hdr] + [dest_opt_hdr] + [icmp6_hdr]
this code will not validate those

IPv6 Ready has a test case for this requirement (1.3.6), so the code needs to PASS this test case.
https://www.ipv6ready.org/docs/Core_Conformance.pdf

[bdcom.dragon_gmail.com]

I think we just need to check first fragments in frag6_input() and check whole header chain everytime:

if (fragoff == 0)
{
    if (!ip6_valid_hdrchain(&m, off, nxt, frgpartlen))
    {
         icmp6_error(m, ICMP6_PARAM_PROB, ICMP6_PARAMPROB_MISSING_HDR, 0);
         return (IPPROTO_DONE);
     }
}

This will conform with RFC 8200, page 21, last paragraph.

[thj]

In D16851#1066818, @bdcom.dragon_gmail.com wrote:

I think we just need to check first fragments in frag6_input() and check whole header chain everytime:

if (fragoff == 0)
{
    if (!ip6_valid_hdrchain(&m, off, nxt, frgpartlen))
    {
         icmp6_error(m, ICMP6_PARAM_PROB, ICMP6_PARAMPROB_MISSING_HDR, 0);
         return (IPPROTO_DONE);
     }
}

This will conform with RFC 8200, page 21, last paragraph.

It has been some number of years since I started doing this and also since I last looked at it. Are you interested in providing a patch to match the testcases you mention? I'm happy to review any suggestions, but this is quite low on my list right now.