ts_dataplane, ts_tunnel: fix reserved_zeroes check in could_be_wireguard - #415
Conversation
npry
left a comment
There was a problem hiding this comment.
I'd prefer to not fragment the packet_ident module? In my mental model could_be_wireguard doesn't really have to do with ts_tunnel, it's about exterior packet identification, where ts_tunnel is about the protocol internals. If you feel strongly though no worries
Regardless, the fix lgtm, ty!
Yep, 100% agree...the move was to avoid making
Reverted the second commit and kept the first commit with the fix, ty! |
…kt len `could_be_wireguard()` was expecting four bytes of zeroes in valid WireGuard packets starting at idx 1; the `reserved_zeroes` field in every WireGuard packet is _three_ bytes of zeroes, starting at idx 1. This was misclassifying any WireGuard packet with a non-zero byte at idx 4 (top byte of the receiver/sender index field, depending on message type) as not WireGuard. Impact was minimal, as we currently process `PacketType::Unknown` and `PacketType::WireGuard` identically anyway in `DataPlane::process_inbound()`. Also increases the min packet size to 16 bytes, since no valid WireGuard packet will be smaller. Signed-off-by: Dylan Bargatze <dylan@tailscale.com>
905161b to
0e30f2f
Compare
Split into two commits; first commit is the bugfix, second is a potential minor improvement.
could_be_wireguard()was expecting four bytes of zeroes in valid WireGuard packets starting at idx 1; thereserved_zeroesfield in every WireGuard packet is three bytes of zeroes, starting at idx 1. This was misclassifying any WireGuard packet with a non-zero byte at idx 4 (top byte of the receiver/sender index field, depending on message type) as not WireGuard. Impact was non-existent, as we currently processPacketType::UnknownandPacketType::WireGuardidentically anyway inDataPlane::process_inbound(). Also increases the min packet size to 16 bytes, since no valid WireGuard packet will be smaller.The second commit adds tighter length-based checks to
could_be_wireguard()by moving it tots_tunneland checking minimum WireGuard packet lengths per-message-type; may or may not be worth the extra ~2-4 branches on the hot path, thus the separate commit.Updates #cleanup.