Page MenuHomeFreeBSD

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

Authored by thj on Aug 22 2018, 7:07 PM.

Details

Reviewers
jtl
bz
Group Reviewers
transport
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.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 24425
Build 23238: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
thj edited the test plan for this revision. (Show Details)Aug 22 2018, 7:08 PM
thj added reviewers: jtl, transport.
tuexen added a subscriber: tuexen.Aug 22 2018, 9:40 PM
tuexen added inline comments.
sys/netinet6/frag6.c
1010

I'm not sure here. Why is it enough to stop here? Don't you need to look into the IPv6 payload?

1023

TCP_MAXHLEN is the maximum TCP header length (60), which is not the correct check here. I guess you want to check against sizeof(struct tcphdr).

tuexen added inline comments.Aug 22 2018, 9:43 PM
sys/netinet6/frag6.c
1010

Looked it up in RFC 7112. It is fine.

ae added a comment.Aug 23 2018, 10:47 AM

Also, I think ipfw/pf needs an additional look to correctly handle this too.

sys/netinet6/frag6.c
1010

I think IPPROTO_IPV4 and IPPROTO_GRE also can be added here.

ae added a comment.Aug 23 2018, 10:50 AM
In D16851#359108, @ae wrote:

Also, I think ipfw/pf needs an additional look to correctly handle this too.

What you think, if we make non-static ip6_checkfirstfrag() function, that can also be used by ipfw/pf/ipfil etc.?

thj added a comment.Aug 23 2018, 11:00 AM
In D16851#359110, @ae wrote:
In D16851#359108, @ae wrote:

Also, I think ipfw/pf needs an additional look to correctly handle this too.

What you think, if we make non-static ip6_checkfirstfrag() function, that can also be used by ipfw/pf/ipfil etc.?

I understand from conversations during BSDCam that ipfw relies on frag6 for fragment input processing.

I have no problem exposing this function for other users.

sys/netinet6/frag6.c
1023

My intention is to require the maximum TCP header length be carried in the fragment. That is how I read the definition:

sys/netinet/tcp.h:150

#define TCP_MAXHLEN (0xf<<2) /* max length of header in bytes */
#define TCP_MAXOLEN (TCP_MAXHLEN - sizeof(struct tcphdr))

					/* max space left for options */
ae added a comment.EditedAug 23 2018, 11:05 AM
In D16851#359111, @thj wrote:

What you think, if we make non-static ip6_checkfirstfrag() function, that can also be used by ipfw/pf/ipfil etc.?

I understand from conversations during BSDCam that ipfw relies on frag6 for fragment input processing.
I have no problem exposing this function for other users.

Actually ipfw does not reassemble IPv6 fragments, but it has the code that handles IPPROTO_FRAGMENT. So, I think we can drop invalid fragments at this time.

Also I'll take a look to nat64 code.

tuexen added inline comments.Aug 23 2018, 11:14 AM
sys/netinet6/frag6.c
1023

I see. Then put a comment there, that you require more bytes to be there than you normally have a in a TCP header. Most of the times, you above code requires even user data to be there.
Especially, you'll drop a packet which contains the complete TCP header in the first fragment, but the payload on follow-up packets. The RFC considers this OK, your code does not...

ae added inline comments.Aug 23 2018, 11:18 AM
sys/netinet6/frag6.c
1023

Eventually, you can get the TCP header length from a packet and check against it.

tuexen added inline comments.Aug 23 2018, 11:33 AM
sys/netinet6/frag6.c
1023

I agree. However, this requires changing the signature of the function. The result does not depend only on the protocol being used but also on the actual header.

bz added a subscriber: bz.Aug 24 2018, 10:12 PM
sys/netinet6/frag6.c
342

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

343

Sentences end in a .

349

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

1004

Empty line.

bz added a reviewer: bz.Aug 24 2018, 10:13 PM
thj updated this revision to Diff 48828.Oct 6 2018, 1:09 PM
thj marked 11 inline comments as done.
thj edited the test plan for this revision. (Show Details)
  • 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 updated this revision to Diff 48833.Oct 6 2018, 4:07 PM
  • Correctly locate structs in mbuf chain
tuexen added inline comments.Oct 6 2018, 4:35 PM
sys/netinet/dccp.h
44

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....

46

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

sys/netinet6/frag6.c
1005

Why are you initialising this to 0?

1030

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

1051

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

1052

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 marked an inline comment as done.Oct 6 2018, 4:41 PM
thj added inline comments.
sys/netinet6/frag6.c
1030

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

tuexen added inline comments.Oct 6 2018, 4:43 PM
sys/netinet6/frag6.c
1052

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 added a comment.Oct 6 2018, 4:59 PM

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 marked 3 inline comments as done.Oct 6 2018, 5:00 PM
thj added inline comments.
sys/netinet/dccp.h
44

I missed a reserved byte in the header.

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

46

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 added inline comments.Oct 6 2018, 5:14 PM
sys/netinet/dccp.h
46

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 updated this revision to Diff 48835.Oct 6 2018, 6:07 PM
thj marked an inline comment as done.
  • Teach netinet/in.h about dccp
  • Make sure length bytes are in the first mbuf
thj updated this revision to Diff 48903.Oct 8 2018, 7:34 PM
  • Use (ip+exthdr len) + length when pulling up mbuf
tuexen added inline comments.Oct 8 2018, 7:44 PM
sys/netinet6/frag6.c
1014

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 updated this revision to Diff 48909.Oct 8 2018, 8:28 PM
  • Do pullup per protcol that requires it
jtl requested changes to this revision.Jan 11 2019, 10:01 PM
jtl added inline comments.
sys/netinet/dccp.h
40

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
42

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
342

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

349

This comment still needs to be a full sentence.

791

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

1035

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.

1052

Again, doublefree. See above.

1064

Doublefree. See above.

1077

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").
1079

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
415

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

This revision now requires changes to proceed.Jan 11 2019, 10:01 PM
thj marked 6 inline comments as done.Feb 26 2019, 8:20 PM
thj added inline comments.
sys/netinet6/ip6_var.h
415

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

thj updated this revision to Diff 57210.May 9 2019, 8:04 AM
thj marked an inline comment as done.

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 updated this revision to Diff 57763.May 23 2019, 1:50 PM

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
Harbormaster completed remote builds in B24425: Diff 57763.
bz added a comment.Jun 17 2019, 9:28 AM

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 requested changes to this revision.Jul 18 2019, 2:46 PM
jtl added inline comments.
sys/netinet/dccp.h
46

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
347

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.

347

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.

350

Comments need to be a full sentence.

This revision now requires changes to proceed.Jul 18 2019, 2:46 PM
thj marked 4 inline comments as done.Jul 21 2019, 10:06 AM
thj added inline comments.
sys/netinet6/frag6.c
347

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 requested changes to this revision.Jul 31 2019, 2:45 PM

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

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

sys/netinet6/frag6.c
344

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.

1001

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.