diff --git a/wgengine/filter/filter.go b/wgengine/filter/filter.go index d2298a531..5e18fb0cf 100644 --- a/wgengine/filter/filter.go +++ b/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 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) { + 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. 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 diff --git a/wgengine/packet/packet.go b/wgengine/packet/packet.go index 2e20c0c5d..0481cf9d1 100644 --- a/wgengine/packet/packet.go +++ b/wgengine/packet/packet.go @@ -94,8 +94,10 @@ func (ipp *IP) UnmarshalJSON(b []byte) error { } const ( - EchoReply uint8 = 0x00 - EchoRequest uint8 = 0x08 + 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()