ayourtch / nat46

OpenWRT feed with stateless NAT46 kernel module
34 stars 28 forks source link

Zero checksum #18

Closed ayourtch closed 3 years ago

ayourtch commented 3 years ago

@dedeckeh @cpackham - upon further discussion with the person who has reported the issue, seems like the below might be a better approach... So, please take out your review bats and let me know what you think :)

detobate commented 3 years ago

Thanks @ayourtch .

Context is, we see some MAP Border Relays, upon receiving an IPv4 packet with UDP checksum == zero, will translate to IPv6 leaving the UDP6 checksum also zero. This causes the current nat46 module to calculate an invalid UDP4 checksum when translating back.

This issue can be seen when using Windows 10's IPSec client, once the key exchange is completed, and it attempts to send UDP-encapsulated ESP.

I think it's unclear if these MAP Border Relays are violating RFC8200's requirement for UDP6 checksum, or if RFC6936 allows for it.

Either way, this patch resolves the issue with the Windows IPSec client.

ejordangottlieb commented 3 years ago

I think the patch is a good approach. Some context and feedback. RFC7915 discusses the UDP checksum 0 case. There are BR implementations that choose to not implement the SHOULD checksum calculation on checksum 0 as only incremental checksum are supported on the platform (significant performance implications). In my opinion, there is no value in doing the calculation on the BR as the packet could have been corrupted in transit in the IPv4 domain before arriving on the BR. When translating for IPv4 UE NAT46 should always perform the checksum calc on UDP checksum 0 before it translates to IPv6 as the endpoint could be a IPv4-mapped IPv6 address. There is an epic corner case where an IPv4-mapped IPv6 originated packet could arrive at the MAP-T CE with checksum 0 value (corrupted in transit) when using the module argument to skip checksum calculation.

ayourtch commented 3 years ago

All right, so you all might give this exact patch a run and post the feedback on this comment in the form of 👍 if it fully work and 👎 if you can spot anything that needs improvement/fixing, and I will keep this open for a couple of weeks or so, and then if there is no thumbs down and only thumbs up, will merge it ? (I have been dealing with mostly CI-enabled systems for the past few years so in the absence of it would rather be conservative :-)

detobate commented 3 years ago

There's another case NEXTHDR_UDP further down that we need a break clause to avoid the csumming functions:

https://github.com/ayourtch/nat46/blob/zero-checksum/nat46/modules/nat46-core.c#L1577-L1583

detobate commented 3 years ago

We tested this patch, along with the additional change I mentioned above, using Windows 10 IPSec client and a Windows Server 2019 IPSec server.

Without the patches, IPSec successfully exchanges keys, but fails to complete setup.

With the patches, the UDP-encapsulated ESP packets arrive with a zero checksum and the IPSec tunnel establishes.

ayourtch commented 3 years ago

Cool, would you like to make a PR with a full patch that you have tested ? I can then merge it.

ayourtch commented 3 years ago

Thanks a lot for testing! merging :)

ayourtch commented 3 years ago

Thanks a lot for testing! merging :)

(And for the additional fix!)