program: discard IPv4 packets with options

[cdammanintopix]

IPv4 header is not always 20 bytes, as it can contain options (i.e. when HeaderLength > 5)

[dthaler]

While this currently addresses the IPv4 issue, the code is still buggy for IPv6. Why not fix it in the same PR? Otherwise, what issue is tracking the fact that the IPv6 code is similarly broken?

[mtfriesen]

This PR is missing support for fragmented XDP buffers - see XdpParseFragmentedFrame in the BufferTooSmall case.

[mtfriesen]

We should add some functional test cases for this new feature.

[mtfriesen]

Need to address outstanding feedback for fragmentation and test cases.

[mtfriesen]

While this currently addresses the IPv4 issue, the code is still buggy for IPv6. Why not fix it in the same PR? Otherwise, what issue is tracking the fact that the IPv6 code is similarly broken?

Perhaps the key question is "why are we adding support for IPv4 options?" - that hasn't been answered in this PR. IPv6 extensions are broken in a slightly different way, since the IPv6 header itself is always a fixed size.

Right now we do not support IPv6 extensions by design, but we also did not support IPv4 options by design. Until now, our trade-off was to expose the bare minimum hand-crafted, unverified parsing code needed to test basic networking scenarios with XDP, plus anything needed to unblock customers, e.g. MsQUIC. Our position has always been that eBPF is the path forward for parsing more complex and arbitrary packet payloads.

[dthaler]

While this currently addresses the IPv4 issue, the code is still buggy for IPv6. Why not fix it in the same PR? Otherwise, what issue is tracking the fact that the IPv6 code is similarly broken?

Perhaps the key question is "why are we adding support for IPv4 options?" - that hasn't been answered in this PR. IPv6 extensions are broken in a slightly different way, since the IPv6 header itself is always a fixed size.

Right now we do not support IPv6 extensions by design, but we also did not support IPv4 options by design. Until now, our trade-off was to expose the bare minimum hand-crafted, unverified parsing code needed to test basic networking scenarios with XDP, plus anything needed to unblock customers, e.g. MsQUIC. Our position has always been that eBPF is the path forward for parsing more complex and arbitrary packet payloads.

I don't follow why we would want to support IPv4 options but not IPv6 extension headers. It's the inconsistency that I'm calling out and it sounds like you agree it's inconsistent in this PR and insufficiently motivated why there should be a difference.

[cdammanintopix]

While this currently addresses the IPv4 issue, the code is still buggy for IPv6. Why not fix it in the same PR? Otherwise, what issue is tracking the fact that the IPv6 code is similarly broken?

In my comprehension, the Offset variable is only used if IpProto == IPPROTO_UDP or IpProto == IPPROTO_TCP. For IPv6, it seems there is not equivalent to the IHL field in IPv4, apart from the NextHeader field, that it used to determine IpProto = Cache->Ip6Hdr->NextHeader. So I don't think there is any bug in IPv6 version of the code.

We should add some functional test cases for this new feature.

Sure, could you please indicate me where to do that. I'm not yet 100% familiar with the code base

While this currently addresses the IPv4 issue, the code is still buggy for IPv6. Why not fix it in the same PR? Otherwise, what issue is tracking the fact that the IPv6 code is similarly broken?

Perhaps the key question is "why are we adding support for IPv4 options?" - that hasn't been answered in this PR. IPv6 extensions are broken in a slightly different way, since the IPv6 header itself is always a fixed size.
Right now we do not support IPv6 extensions by design, but we also did not support IPv4 options by design. Until now, our trade-off was to expose the bare minimum hand-crafted, unverified parsing code needed to test basic networking scenarios with XDP, plus anything needed to unblock customers, e.g. MsQUIC. Our position has always been that eBPF is the path forward for parsing more complex and arbitrary packet payloads.

I don't follow why we would want to support IPv4 options but not IPv6 extension headers. It's the inconsistency that I'm calling out and it sounds like you agree it's inconsistent in this PR and insufficiently motivated why there should be a difference.

Motivation is that we use XDP internally and we encountered IPv4 packets with header length is not the default 20 bytes, so we fixed it in our fork and wanted to share the fix.
The goal of this PR is not to support IPv4 options (nor IPv6 extensions), just to fix the parsing of the UDP and TCP frames if IPv4 header length is not the default 20 bytes.
Right now (without this fix), the parsing of the UDP / TCP headers is not correct, so the user that apply a specific rule will receive packets that he doesn't want / will not receive packets he wants.
The only motivation of this PR is to fix that.

[nibanks]

Motivation is that we use XDP internally and we encountered IPv4 packets with header length is not the default 20 bytes, so we fixed it in our fork and wanted to share the fix.
The goal of this PR is not to support IPv4 options (nor IPv6 extensions), just to fix the parsing of the UDP and TCP frames if IPv4 header length is not the default 20 bytes.
Right now (without this fix), the parsing of the UDP / TCP headers is not correct, so the user that apply a specific rule will receive packets that he doesn't want / will not receive packets he wants.
The only motivation of this PR is to fix that.

This all makes sense to me, and I think this is reasonable to take. I do also think adding some explicit tests (@mtfriesen to give the pointer) would be a good idea.

[mtfriesen]

I agree, this is the minimal change to parse variable-length IPv4 headers without wading into parsing specific options/extensions. Makes sense to take it.

Re: tests, I'd start perhaps with the GenericRxHeaderFragments test case and add support for extending the IPv4 header in the PktBuildUdpFrame and PktBuildTcpFrame helpers.

[dthaler]

While this currently addresses the IPv4 issue, the code is still buggy for IPv6. Why not fix it in the same PR? Otherwise, what issue is tracking the fact that the IPv6 code is similarly broken?

In my comprehension, the Offset variable is only used if IpProto == IPPROTO_UDP or IpProto == IPPROTO_TCP. For IPv6, it seems there is not equivalent to the IHL field in IPv4, apart from the NextHeader field, that it used to determine IpProto = Cache->Ip6Hdr->NextHeader. So I don't think there is any bug in IPv6 version of the code.

There is definitely a problem in the IPv6 code equivalent to the IPv4 problem before, which is that it incorrectly assumes that the UDP header can only appear immediately after the IPv6 header. Such an assumption is equivalent to an assumption that an IPv4 packet has no options.

While this currently addresses the IPv4 issue, the code is still buggy for IPv6. Why not fix it in the same PR? Otherwise, what issue is tracking the fact that the IPv6 code is similarly broken?

Perhaps the key question is "why are we adding support for IPv4 options?" - that hasn't been answered in this PR. IPv6 extensions are broken in a slightly different way, since the IPv6 header itself is always a fixed size.
Right now we do not support IPv6 extensions by design, but we also did not support IPv4 options by design. Until now, our trade-off was to expose the bare minimum hand-crafted, unverified parsing code needed to test basic networking scenarios with XDP, plus anything needed to unblock customers, e.g. MsQUIC. Our position has always been that eBPF is the path forward for parsing more complex and arbitrary packet payloads.

Either don't support skipping over IPv4 options, or support skipping over IPv6 extension headers. It makes no sense to support one but not the other.

I don't follow why we would want to support IPv4 options but not IPv6 extension headers. It's the inconsistency that I'm calling out and it sounds like you agree it's inconsistent in this PR and insufficiently motivated why there should be a difference.

Motivation is that we use XDP internally and we encountered IPv4 packets with header length is not the default 20 bytes, so we fixed it in our fork and wanted to share the fix.

By that same motivation you should support IPv6 packets with intermediate headers.

The goal of this PR is not to support IPv4 options (nor IPv6 extensions), just to fix the parsing of the UDP and TCP frames if IPv4 header length is not the default 20 bytes. Right now (without this fix), the parsing of the UDP / TCP headers is not correct, so the user that apply a specific rule will receive packets that he doesn't want / will not receive packets he wants. The only motivation of this PR is to fix that.

Same motivation means should fix IPv6 to skip over intermediate headers. Otherwise you'll see the exact same problem in IPv6.

[mtfriesen]

I agree that on a conceptual level that we're solving a problem in IPv4 that has an IPv6 equivalent. However, our consistency in this XDP parsing module has been very simple: we solve only the problems that actual customers need us to solve until we can provide them with eBPF, which is the superior, general solution.

Since this code is intended to be removed eventually, we aim to avoid unnecessary development costs, minimize our attack surface, etc. unless a specific customer requires increasing the size and complexity of this codebase. In other words, it is intentionally incomplete and incorrect, except in specific cases where customers care about narrowly defined "correct" behavior.

[cdammanintopix]

Same motivation means should fix IPv6 to skip over intermediate headers. Otherwise you'll see the exact same problem in IPv6.

In IPv6, there is no bug: the UDP / TCP headers will just not be parsed , so the user will not see any packets whatever the applied rule. This is a limitation, but not a bug.
In IPv4, the UDP / TCP headers will be incorrectly parsed, leading to incorrect packets filtering. This is a bug, not a limitation.
If you prefer, we could also fix this bug by adding if (Cache->Ip4Hdr->HeaderLength < 5) { return; } to ensure that UDP / TCP headers are not parsed, and have the exact same behaviour as in IPv6 (but I personally think it is quite easy to fix the bug and skip the options part, as there is a simple HeaderLength field in the IPv4 header)

[mtfriesen]

To be fair, both IPv4 and IPv6 were known limitations: we intentionally did not bother with the IPv4 header length field. We've taken an extremely minimalist approach to parsing headers: touch only the fields we must to support specific customer scenarios.

[cdammanintopix]

I agree that on a conceptual level that we're solving a problem in IPv4 that has an IPv6 equivalent. However, our consistency in this XDP parsing module has been very simple: we solve only the problems that actual customers need us to solve until we can provide them with eBPF, which is the superior, general solution.

Since this code is intended to be removed eventually, we aim to avoid unnecessary development costs, minimize our attack surface, etc. unless a specific customer requires increasing the size and complexity of this codebase. In other words, it is intentionally incomplete and incorrect, except in specific cases where customers care about narrowly defined "correct" behavior.

Talking about the attack surface: with the code right now (without the fix), if someone uses XDP to, let's say, drop any packet that matches a specific rule for a security purpose, it's really easy for an attacker to just specify an IPv4 HeaderLength > 5, and write fake UDP / TCP headers that will just bypass the rule intended. Packets will then be passed to the other layers, meaning that the security purpose intended is not achieved.

As explained in the motivations, we just wanted to share with you this simple fix. If you intend to remove this part of the code, no problem for me not to merge this PR. Just be aware that it may cause some security issues and could be used by an attacker.

[mtfriesen]

Right, however, we don't have any DDoS/filtering customers at the moment. If we did, however, we'd need to add the IPv6 extension-skipping logic for exactly the same reason as the IPv4 example. There is also a long tail for DDoS, e.g. fragmentation attacks, that are currently entirely unsupported in our L4+ filters.

[mtfriesen]

Basically, we've very intentionally defined a narrow scope to these parsing rules because eBPF is the long-term answer. We know from experience that parsing packets in an RFC-compliant, performant, and secure manner has nontrivial development and ongoing maintenance costs.

[dthaler]

If you prefer, we could also fix this bug by adding if (Cache->Ip4Hdr->HeaderLength < 5) { return; } to ensure that UDP / TCP headers are not parsed, and have the exact same behaviour as in IPv6

I agree that would be consistent. I'm only arguing for consistency. So I'd be fine with that, or with adding IPv6-extension skipping logic (if security is important for this legacy code which sounds like it'll be deprecated/removed once eBPF logic is in).

[cdammanintopix]

If you prefer, we could also fix this bug by adding if (Cache->Ip4Hdr->HeaderLength < 5) { return; } to ensure that UDP / TCP headers are not parsed, and have the exact same behaviour as in IPv6

I agree that would be consistent. I'm only arguing for consistency. So I'd be fine with that, or with adding IPv6-extension skipping logic (if security is important for this legacy code which sounds like it'll be deprecated/removed once eBPF logic is in).

Just done that and re-pushed.I agree this is the best way to have consistency between IPv4 & IPv6, i.e. discard packets with options / extensions explicitly.

[mtfriesen]

We're hitting build failures:

D:\a\xdp-for-windows\xdp-for-windows\src\xdp\program.c(462): error C26451: Arithmetic overflow: Using operator '<<' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '<<' to avoid overflow (io.2). [D:\a\xdp-for-windows\xdp-for-windows\src\xdp\xdp.vcxproj]
D:\a\xdp-for-windows\xdp-for-windows\src\xdp\program.c(695): error C26451: Arithmetic overflow: Using operator '<<' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '<<' to avoid overflow (io.2). [D:\a\xdp-for-windows\xdp-for-windows\src\xdp\xdp.vcxproj]

[cdammanintopix]

Casting to UINT64 seems to fix the issue