firewall reject packets: cleanup error cases #957
Conversation
We found a few types of packets that could cause a panic here, live if a non-TCP packet has TCP header options set. These change should shore up any edge cases and ensure we reserve the maximum amount of buffer space we need to create the reject packet.
There was a problem hiding this comment.
This PR works great!
A lot of the calculations are due to the double-use of the 'out' buffer as both input and output. It may be simpler (or it may be more confusing!) to swap the 'out' buffer and 'packet' buffer usage after the packet is decrypted. Meaning, once the packet is decrypted in decryptToTun, in the case of a rejectOutside, send the 'packet' variable as the output buffer and 'out' as the input buffer. rejectOutside would also need to be updated to ensure the two different buffers are used in the call to f.sendNoMetrics.
Since both slices are already allocated, re-using the packet slice would avoid a reallocation, which I believe is the goal of re-using the out slice when constructing the reject packet. The advantage of this approach is that two completely different slices would be used in the call to f.sendNoMetrics, ensuring no overlap ever occurs. It may also be more confusing to swap the usage of the two slices, though.
All that said, the PR looks like it'll work great. There is enough room reserved in the buffer to handle the larger header lengths, and I've manually verified it works in the ICMP case I identified that triggered the issue.
iputil/packet.go
Outdated
| if ihl > 60 { | ||
| // invalid | ||
| return nil | ||
| } |
There was a problem hiding this comment.
FWIW, this is literally the largest number you can get from the above calculation. This condition will never be hit, unless the above code changes.
There was a problem hiding this comment.
yes true! I was trying to be extra careful the no-one could craft a packet that would crash these, even if the packet was invalid. But you are correct that this check is redundant.
I think I see what you are saying here, it might be clearer to do this instead of trying to do it all in the out buffer. Let me try it. |
|
Updated to re-use the packet []byte to build the reject packet |
We found a few types of packets that could cause a panic here, like if a non-TCP packet has IP header options set. These changes should shore up any edge cases and ensure we reserve the maximum amount of buffer space we need to create the reject packet.