search

OpenVPN / tap-windows6

Windows TAP driver (NDIS 6)
Other
765 stars 238 forks source link

fix bug: Only send packet when packetlength>=Etherheader(14)+IPv4header(20), but some Ether Packets have no IP header #159

Closed Jimmy01240397 closed 1 year ago

Jimmy01240397 commented 1 year ago

fix bug: Only send packet when packetlength>=Etherheader(14)+IPv4header(20), but some Ether Packets have no IP header

See issues: #158

cron2 commented 1 year ago

@lstipakov what do you think? Is it safe to reduce the size check to "just ethernet header" or do we need to differentiate by ethernet type ("if IPv4, must contain Ethernet + IPv4 header, if PPP ether type, must contain Ethernet + PPP frame, etc")?

lstipakov commented 1 year ago

Interesting, this has been there from the very beginning. I wonder what was the reason for it ("must be IPv4 header"). Sadly we cannot ask Thomas Divine anymore, but maybe James knows.

cron2 commented 1 year ago

Well, for "we only deal with IPv4", it made sense. At some point, IPv6 came along, and the header is bigger so the check never got in the way...

The only reason why I'm worried a bit is "if the tap driver passes on smaller packets at this function, will anything else assume minimum sizes, and crash?" - for OpenVPN, I'm reasonably sure the answer is "no", because everything does size checks on the headers (at least "everything that looks into IPv4 or IPv6 packets", instead of "just pass it on"). For TAP? NDIS? No idea.

lstipakov commented 1 year ago

I tested it with Scapy - created a ethernet frame with a short payload and checked that it has arrived on other side.

The change looks safe - we have additional length checks in tapAdapterTransmit when driver is in tun mode. Plus we have length checks in openvpn.