mfontanini / libtins

High-level, multiplatform C++ network packet sniffing and crafting library.
http://libtins.github.io/
BSD 2-Clause "Simplified" License
1.91k stars 377 forks source link

one too many byte swaps of IP::ip_header::tot_len on BSD/Apple #479

Closed bwillcox closed 2 years ago

mfontanini commented 2 years ago

I vaguely remember fixing this. Can you expand on how this issue manifests? The fix itself was for the issue when sending a packet where to lowest PDU is IP, for some reason on a Mac I needed those bytes swapped. This line of code was definitely not added randomly so while I'm okay with removing it if it's causing an issue, I'd need some justification to do that.

bwillcox commented 2 years ago

I vaguely remember fixing this. Can you expand on how this issue manifests? The fix itself was for the issue when sending a packet where to lowest PDU is IP, for some reason on a Mac I needed those bytes swapped. This line of code was definitely not added randomly so while I'm okay with removing it if it's causing an issue, I'd need some justification to do that.

See line 449. That sets tot_len that also does a byte swap. So in the BSD/Apple case on little endian hardware, the bytes are swapped to big endian inside the ifdef, then they are byte swapped immediately again after the #endif.

Is it possible you ran BSD/Apple on big endian hardware? Then both these calls would be no-ops and everything would work fine. I'm running this code on x86 mac.

mfontanini commented 2 years ago

Right, so I think for some reason the total length in mac had to be in little endian. I remember wtf'ing a bit when I came across this. Just to be clear, this manifests when sending a packet, right? As in, without the extra byte flipping line that's in master, when you send a packet where the lowest layer is an IP, the packet on the wire is going to be corrupted as the length is backwards.

Is it possible you ran BSD/Apple on big endian hardware?

No, this was a x86 one.

bwillcox commented 2 years ago

Right, so I think for some reason the total length in mac had to be in little endian.

Do you mean little endian on wire (i.e. low order bits sent first)? This would be out of spec, but this appears to be what the extra swap appears to do.

Just to be clear, this manifests when sending a packet, right?

When constructing and serializing an IP packet, yes, like the test does.

As in, without the extra byte flipping line that's in master, when you send a packet where the lowest layer is an IP, the packet on the wire is going to be corrupted as the length is backwards.

This is where we are not on the same page. What I'm seeing is the extra byte flipping corrupting the packet by placing the low order byte (0x33) of tot_len at the lower addressed byte (byte 2) of the serialized packet. The low order byte of tot_len should be byte 3 of the serialized packet.

mfontanini commented 2 years ago

Just to be clear, this manifests when sending a packet, right?

When constructing and serializing an IP packet, yes, like the test does.

That's not the same :). "sending" means taking the serialized bytes and put them on the wire. You are correct in that serializing that field as little endian is out of spec. However, for some reason mac expects the total length to be in little endian format, hence the extra swap. Keep in mind there's 2 aspects to this:

Now this takes us to the problem where you want to make a distinction between "I want to just serialize this" and "I want to serialize this to put it in the wire, so please apply any non standard platform-specific breakages". I don't think there's a way of distinguishing this in libtins, as it's not something you would really expect in the first place, so the decision at the time was to assume you want to dump the packet on the wire and apply the extra swap every time. Are you currently having the issue that you just want to serialize packets, but not send them, and you're being affected by the extra flip?

bwillcox commented 2 years ago

I think the BSD kernel may assume the length field is in host format when the kernel is handling the packet from the user's socket send call. Then the kernel presumably swaps the bytes prior to putting the packet on the wire.

I'm only using this in a unit test, and parsing the serialized packet is all that matters, so I'll leave my workaround in place, that is to swap the bytes a third time :^)