[WireGuard] [PATCH] go test: add ICMP ping

Jason A. Donenfeld Jason at zx2c4.com
Thu Jul 7 12:20:06 CEST 2016


This is awesome. What fun it was making this yesterday. A couple nits, however:

On Thu, Jul 7, 2016 at 4:57 AM, Jonathan Rudenberg
<jonathan at titanous.com> wrote:
> +               Checksum: 0xa15b, // the packet is always the same, hard-code checksum

Isn't there some Go lib we can use to calculate this for us?

> +       // read ICMP Echo Reply packet
> +       replyPacket := make([]byte, 128)

Is this 128 exact?

> +       replyPacket = replyPacket[:n]

I guess not, since you truncate it to n here.

> +       replyHeaderLen := int(replyPacket[0]&0x0f) << 2
> +       replyLen := binary.BigEndian.Uint16(replyPacket[2:])
> +       replyMessage, err := icmp.ParseMessage(1, replyPacket[replyHeaderLen:replyLen])

I realize this is Go, where nobody cares about indices because the
runtime does checks for you, but... this pops out at me as ugly and
dangerous. replyHeaderLen and replyLen are attacker controlled!

> +       if echo.ID != 1 || echo.Seq != 1 || string(echo.Data) != "WireGuard" {

Can we set ID and Seq to random numbers, and check that we get them back here?


More information about the WireGuard mailing list