wgengine/filter: allow ICMP response packets.

Longer term, we should probably update the packet filter to be fully
stateful, for both TCP and ICMP. That is, only ICMP packets related to
a session *we* initiated should be allowed back in. But this is
reasonably secure for now, since wireguard is already trimming most
traffic. The current code would not protect against eg. Ping-of-Death style
attacks from VPN nodes.

Fixes tailscale/tailscale#290.

Signed-off-by: Avery Pennarun <apenwarr@tailscale.com>

wgengine/filter/filter.go

@@ -166,12 +166,15 @@ func (f *Filter) RunOut(b []byte, q *packet.QDecode, rf RunFlags) Response {
 func (f *Filter) runIn(q *packet.QDecode) (r Response, why string) {
 	switch q.IPProto {
 	case packet.ICMP:
+		if q.IsEchoResponse() || q.IsError() {
+			// ICMP responses are allowed.
+			// TODO(apenwarr): consider using conntrack state.
+			//  We could choose to reject all packets that aren't
+			//  related to an existing ICMP-Echo, TCP, or UDP
+			//  session.
+			return Accept, "icmp response ok"
+		} else if matchIPWithoutPorts(f.matches, q) {
 			// If any port is open to an IP, allow ICMP to it.
-		// TODO(apenwarr): allow ICMP packets on existing sessions.
-		//   Right now ICMP Echo Response doesn't always work, and
-		//   probably important ICMP responses on TCP sessions
-		//   also get blocked.
-		if matchIPWithoutPorts(f.matches, q) {
 			return Accept, "icmp ok"
 		}
 	case packet.TCP:
@@ -209,7 +212,6 @@ func (f *Filter) runIn(q *packet.QDecode) (r Response, why string) {
 }
 
 func (f *Filter) runOut(q *packet.QDecode) (r Response, why string) {
-	// TODO(apenwarr): create sessions on ICMP Echo Request too.
 	if q.IPProto == packet.UDP {
 		t := tuple{q.DstIP, q.SrcIP, q.DstPort, q.SrcPort}
 		var ti interface{} = t // allocate once, rather than twice inside mutex

wgengine/packet/packet.go

@@ -96,6 +96,8 @@ func (ipp *IP) UnmarshalJSON(b []byte) error {
 const (
 	EchoReply    uint8 = 0x00
 	EchoRequest  uint8 = 0x08
+	Unreachable  uint8 = 0x03
+	TimeExceeded uint8 = 0x0B
 )
 
 const (
@@ -315,6 +317,18 @@ func (q *QDecode) IsTCPSyn() bool {
 	return (q.TCPFlags & SynAck) == Syn
 }
 
+// For a packet that has already been decoded, check if it's an IPv4 ICMP
+// "Error" packet.
+func (q *QDecode) IsError() bool {
+	if q.IPProto == ICMP && len(q.b) >= q.subofs+8 {
+		switch q.b[q.subofs] {
+		case Unreachable, TimeExceeded:
+			return true
+		}
+	}
+	return false
+}
+
 // For a packet that has already been decoded, check if it's an IPv4 ICMP
 // Echo Request.
 func (q *QDecode) IsEchoRequest() bool {
@@ -324,6 +338,15 @@ func (q *QDecode) IsEchoRequest() bool {
 	return false
 }
 
+// For a packet that has already been decoded, check if it's an IPv4 ICMP
+// Echo Response.
+func (q *QDecode) IsEchoResponse() bool {
+	if q.IPProto == ICMP && len(q.b) >= q.subofs+8 {
+		return q.b[q.subofs] == EchoReply && q.b[q.subofs+1] == 0
+	}
+	return false
+}
+
 func (q *QDecode) EchoRespond() []byte {
 	icmpid := binary.BigEndian.Uint16(q.Sub(4, 2))
 	b := q.Trim()