Fix validation for ICMP type and code in backend rules.

- 0 (ping reply) should have been allowed.
- 255 (unused) should have been foridden due to lack of kernel support.

lib/backend/model/rule.go

@@ -31,10 +31,13 @@ type Rule struct {
 	Protocol    *numorstring.Protocol `json:"protocol,omitempty" validate:"omitempty"`
 	NotProtocol *numorstring.Protocol `json:"!protocol,omitempty" validate:"omitempty"`
 
-	ICMPType    *int `json:"icmp_type,omitempty" validate:"omitempty,gte=1,lte=255"`
-	ICMPCode    *int `json:"icmp_code,omitempty" validate:"omitempty,gte=1,lte=255"`
-	NotICMPType *int `json:"!icmp_type,omitempty" validate:"omitempty,gte=1,lte=255"`
-	NotICMPCode *int `json:"!icmp_code,omitempty" validate:"omitempty,gte=1,lte=255"`
+	// ICMP validation notes: 0 is a valid (common) ICMP type and code.  Type = 255 is not assigned
+	// to any protocol and the Linux kernel doesn't support matching on it so we validate against
+	// it.
+	ICMPType    *int `json:"icmp_type,omitempty" validate:"omitempty,gte=0,lt=255"`
+	ICMPCode    *int `json:"icmp_code,omitempty" validate:"omitempty,gte=0,lte=255"`
+	NotICMPType *int `json:"!icmp_type,omitempty" validate:"omitempty,gte=0,lt=255"`
+	NotICMPCode *int `json:"!icmp_code,omitempty" validate:"omitempty,gte=0,lte=255"`
 
 	SrcTag      string             `json:"src_tag,omitempty" validate:"omitempty,tag"`
 	SrcNet      *net.IPNet         `json:"src_net,omitempty" validate:"omitempty"`

lib/backend/model/rule_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2016 Tigera, Inc. All rights reserved.
+// Copyright (c) 2016-2017 Tigera, Inc. All rights reserved.
 
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -35,6 +35,8 @@ var icmpProto = numorstring.ProtocolFromString("icmp")
 var intProto = numorstring.ProtocolFromInt(123)
 var icmpType = 10
 var icmpCode = 6
+var icmpTypeZero = 0
+var icmpCodeZero = 0
 var portRange, _ = numorstring.PortFromRange(10, 20)
 var ports = []numorstring.Port{
 	numorstring.SinglePort(1234),
@@ -59,6 +61,10 @@ var ruleStringTests = []ruleTest{
 		"deny icmp type 10"},
 	{model.Rule{Protocol: &icmpProto, ICMPType: &icmpType, ICMPCode: &icmpCode},
 		"allow icmp type 10 code 6"},
+	{model.Rule{Action: "deny", Protocol: &icmpProto, ICMPType: &icmpTypeZero},
+		"deny icmp type 0"},
+	{model.Rule{Protocol: &icmpProto, ICMPType: &icmpTypeZero, ICMPCode: &icmpCodeZero},
+		"allow icmp type 0 code 0"},
 	// And negations of packet-wide matches.
 	{model.Rule{Action: "allow", NotProtocol: &tcpProto}, "allow !tcp"},
 	{model.Rule{Action: "deny", Protocol: &icmpProto, NotICMPType: &icmpType},

lib/validator/validator_test.go

@@ -79,6 +79,20 @@ func init() {
 		// Empty rule is valid, it means "allow all".
 		Entry("empty rule (m)", model.Rule{}, true),
 
+		// (Backend model) ICMP.
+		Entry("should accept ICMP 0/any (m)", model.Rule{ICMPType: &V0}, true),
+		Entry("should accept ICMP 0/0 (m)", model.Rule{ICMPType: &V0, ICMPCode: &V0}, true),
+		Entry("should accept ICMP 128/0 (m)", model.Rule{ICMPType: &V128, ICMPCode: &V0}, true),
+		Entry("should accept ICMP 254/255 (m)", model.Rule{ICMPType: &V254, ICMPCode: &V255}, true),
+		Entry("should reject ICMP 255/255 (m)", model.Rule{ICMPType: &V255, ICMPCode: &V255}, false),
+		Entry("should reject ICMP 128/256 (m)", model.Rule{ICMPType: &V128, ICMPCode: &V256}, false),
+		Entry("should accept not ICMP 0/any (m)", model.Rule{NotICMPType: &V0}, true),
+		Entry("should accept not ICMP 0/0 (m)", model.Rule{NotICMPType: &V0, NotICMPCode: &V0}, true),
+		Entry("should accept not ICMP 128/0 (m)", model.Rule{NotICMPType: &V128, NotICMPCode: &V0}, true),
+		Entry("should accept not ICMP 254/255 (m)", model.Rule{NotICMPType: &V254, NotICMPCode: &V255}, true),
+		Entry("should reject not ICMP 255/255 (m)", model.Rule{NotICMPType: &V255, NotICMPCode: &V255}, false),
+		Entry("should reject not ICMP 128/256 (m)", model.Rule{NotICMPType: &V128, NotICMPCode: &V256}, false),
+
 		// (Backend model) Actions.
 		Entry("should accept allow action (m)", model.Rule{Action: "allow"}, true),
 		Entry("should accept deny action (m)", model.Rule{Action: "deny"}, true),