From 119c3c82c1c22337ceaffea0cdca5b127a139a1d Mon Sep 17 00:00:00 2001
From: Shaun Crampton <shaun@tigera.io>
Date: Mon, 6 Nov 2017 10:01:18 +0000
Subject: [PATCH] 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       | 11 +++++++----
 lib/backend/model/rule_test.go  |  8 +++++++-
 lib/validator/validator_test.go | 14 ++++++++++++++
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/lib/backend/model/rule.go b/lib/backend/model/rule.go
index dd4ebf478..3953ea6dc 100644
--- a/lib/backend/model/rule.go
+++ b/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"`
diff --git a/lib/backend/model/rule_test.go b/lib/backend/model/rule_test.go
index a9fd96759..81145b2d4 100644
--- a/lib/backend/model/rule_test.go
+++ b/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},
diff --git a/lib/validator/validator_test.go b/lib/validator/validator_test.go
index bb31e1b08..1706e937e 100644
--- a/lib/validator/validator_test.go
+++ b/lib/validator/validator_test.go
@@ -69,6 +69,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),