v1.13-route-validation (#7616)

changelog/v1.13.2/validate-routes.yaml

@@ -0,0 +1,7 @@
+changelog:
+  - type: FIX
+    description: |
+      Add route validation to reject and sanitize invalid paths.
+      Set the normalized_path attribute for the [Envoy HTTP Connection Manager](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto.html?highlight=hcm) to true as the default value.
+    issueLink: https://github.com/solo-io/solo-projects/issues/4336
+    resolvesIssue: false

projects/gloo/api/v1/options/hcm/hcm.proto

@@ -194,7 +194,7 @@ message HttpConnectionManagerSettings {
     // See here for more information: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto
     google.protobuf.BoolValue merge_slashes = 29;
 
-    // Should paths be normalized according to RFC 3986 before any processing of requests by HTTP filters or routing?
+    // Should paths be normalized according to RFC 3986 before any processing of requests by HTTP filters or routing? Defaults to True.
     // See here for more information: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto
     google.protobuf.BoolValue normalize_path = 30;
 

projects/gloo/pkg/plugins/hcm/plugin.go

@@ -18,6 +18,7 @@ import (
 	"github.com/solo-io/gloo/projects/gloo/pkg/utils"
 	"github.com/solo-io/go-utils/contextutils"
 	"google.golang.org/protobuf/types/known/anypb"
+	"google.golang.org/protobuf/types/known/wrapperspb"
 )
 
 var (
@@ -63,7 +64,13 @@ func (p *plugin) ProcessHcmNetworkFilter(params plugins.Params, _ *v1.Listener,
 	out.PathWithEscapedSlashesAction = envoyhttp.HttpConnectionManager_PathWithEscapedSlashesAction(in.GetPathWithEscapedSlashesAction())
 	out.CodecType = envoyhttp.HttpConnectionManager_CodecType(in.GetCodecType())
 	out.MergeSlashes = in.GetMergeSlashes().GetValue()
-	out.NormalizePath = in.GetNormalizePath()
+
+	// default to true as per RFC 3986
+	if in.GetNormalizePath() == nil {
+		out.NormalizePath = &wrapperspb.BoolValue{Value: true}
+	} else {
+		out.NormalizePath = in.GetNormalizePath()
+	}
 
 	if in.GetAcceptHttp_10().GetValue() {
 		out.HttpProtocolOptions = &envoycore.Http1ProtocolOptions{

projects/gloo/pkg/plugins/hcm/plugin_test.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1"
+	"google.golang.org/protobuf/types/known/wrapperspb"
 
 	envoycore "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
 	envoyhttp "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/network/http_connection_manager/v3"
@@ -77,6 +78,7 @@ var _ = Describe("Plugin", func() {
 			DrainTimeout:          prototime.DurationToProto(time.Hour),
 			DelayedCloseTimeout:   prototime.DurationToProto(time.Hour),
 			ServerName:            &wrappers.StringValue{Value: "ServerName"},
+			NormalizePath:         &wrapperspb.BoolValue{Value: false},
 
 			AcceptHttp_10: &wrappers.BoolValue{Value: true},
 			HeaderFormat: &hcm.HttpConnectionManagerSettings_ProperCaseHeaderKeyFormat{
@@ -262,6 +264,17 @@ var _ = Describe("Plugin", func() {
 		Expect(cfg.GetServerHeaderTransformation()).To(Equal(envoyhttp.HttpConnectionManager_PASS_THROUGH))
 	})
 
+	Context("Default Values", func() {
+		It("should contain the default value for NormalizePath", func() {
+			T := &wrapperspb.BoolValue{Value: true}
+			settings = &hcm.HttpConnectionManagerSettings{}
+			cfg := &envoyhttp.HttpConnectionManager{}
+			err := processHcmNetworkFilter(cfg)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(cfg.NormalizePath).To(Equal(T))
+		})
+	})
+
 	Context("Http2 passed", func() {
 		//validation uses the same shared code so no need to validate large and small for all fields
 		It(" should not accept connection streams that are too small", func() {

projects/gloo/pkg/translator/route_config.go

@@ -3,6 +3,7 @@ package translator
 import (
 	"context"
 	"fmt"
+	"regexp"
 	"strings"
 	"unicode"
 
@@ -29,9 +30,28 @@ import (
 )
 
 var (
-	NoDestinationSpecifiedError = errors.New("must specify at least one weighted destination for multi destination routes")
+	// invalidPathSequences are path sequences that should not be contained in a path
+	invalidPathSequences = []string{"//", "/./", "/../", "%2f", "%2F", "#"}
+	// invalidPathSuffixes are path suffixes that should not be at the end of a path
+	invalidPathSuffixes = []string{"/..", "/."}
+	// validCharacterRegex pattern based off RFC 3986 similar to kubernetes-sigs/gateway-api implementation
+	// for finding "pchar" characters = unreserved / pct-encoded / sub-delims / ":" / "@"
+	validPathRegexCharacters = "^(?:([A-Za-z0-9/:@._~!$&'()*+,:=;-]*|[%][0-9a-fA-F]{2}))*$"
+
+	NoDestinationSpecifiedError       = errors.New("must specify at least one weighted destination for multi destination routes")
+	SubsetsMisconfiguredErr           = errors.New("route has a subset config, but the upstream does not")
+	CompilingRoutePathRegexError      = errors.Errorf("error compiling route path regex: %s", validPathRegexCharacters)
+	ValidRoutePatternError            = errors.Errorf("must only contain valid characters matching pattern %s", validPathRegexCharacters)
+	PathContainsInvalidCharacterError = func(s, invalid string) error {
+		return errors.Errorf("path [%s] cannot contain [%s]", s, invalid)
+	}
+	PathEndsWithInvalidCharactersError = func(s, invalid string) error {
+		return errors.Errorf("path [%s] cannot end with [%s]", s, invalid)
+	}
+)
 
-	SubsetsMisconfiguredErr = errors.New("route has a subset config, but the upstream does not")
+var (
+	validPathRegex *regexp.Regexp
 )
 
 type RouteConfigurationTranslator interface {
@@ -153,6 +173,7 @@ func (h *httpRouteConfigurationTranslator) envoyRoutes(
 
 	for i := range out {
 		h.setAction(params, routeReport, in, out[i])
+		validateEnvoyRoute(out[i], routeReport)
 	}
 
 	return out
@@ -199,6 +220,23 @@ func initRoutes(
 	return out
 }
 
+// validateEnvoyRoute will validate all paths on a route. Any path, rewrite, or redirect of the path
+// will be validated.
+func validateEnvoyRoute(r *envoy_config_route_v3.Route, routeReport *validationapi.RouteReport) {
+	match := r.GetMatch()
+	route := r.GetRoute()
+	re := r.GetRedirect()
+	name := r.GetName()
+	validatePath(match.GetPath(), name, routeReport)
+	validatePath(match.GetPrefix(), name, routeReport)
+	validatePath(match.GetPathSeparatedPrefix(), name, routeReport)
+	validatePath(route.GetPrefixRewrite(), name, routeReport)
+	validatePath(re.GetPrefixRewrite(), name, routeReport)
+	validatePath(re.GetPathRedirect(), name, routeReport)
+	validatePath(re.GetHostRedirect(), name, routeReport)
+	validatePath(re.GetSchemeRedirect(), name, routeReport)
+}
+
 // utility function to transform gloo matcher to envoy route matcher
 func GlooMatcherToEnvoyMatcher(ctx context.Context, matcher *matchers.Matcher) envoy_config_route_v3.RouteMatch {
 	match := envoy_config_route_v3.RouteMatch{
@@ -797,3 +835,40 @@ func isWarningErr(err error) bool {
 		return false
 	}
 }
+
+func validatePath(path, name string, routeReport *validationapi.RouteReport) {
+	if err := ValidateRoutePath(path); err != nil {
+		validation.AppendRouteError(routeReport, validationapi.RouteReport_Error_ProcessingError, err.Error(), name)
+	}
+}
+
+// ValidateRoutePath will validate a string for all characters according to RFC 3986
+// "pchar" characters = unreserved / pct-encoded / sub-delims / ":" / "@"
+// https://www.rfc-editor.org/rfc/rfc3986/
+func ValidateRoutePath(s string) error {
+	if s == "" {
+		return nil
+	}
+	if validPathRegex == nil {
+		re, err := regexp.Compile(validPathRegexCharacters)
+		if err != nil {
+			validPathRegex = nil
+			return CompilingRoutePathRegexError
+		}
+		validPathRegex = re
+	}
+	if !validPathRegex.Match([]byte(s)) {
+		return ValidRoutePatternError
+	}
+	for _, invalid := range invalidPathSequences {
+		if strings.Contains(s, invalid) {
+			return PathContainsInvalidCharacterError(s, invalid)
+		}
+	}
+	for _, invalid := range invalidPathSuffixes {
+		if strings.HasSuffix(s, invalid) {
+			return PathEndsWithInvalidCharactersError(s, invalid)
+		}
+	}
+	return nil
+}

projects/gloo/pkg/translator/route_config_test.go

@@ -0,0 +1,77 @@
+package translator_test
+
+import (
+	"strconv"
+	"strings"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
+	. "github.com/onsi/gomega"
+	"github.com/solo-io/gloo/projects/gloo/pkg/translator"
+)
+
+var _ = Describe("Route Configs", func() {
+
+	DescribeTable("validate route path", func(path string, expectedValue bool) {
+		if expectedValue {
+			Expect(translator.ValidateRoutePath(path)).ToNot(HaveOccurred())
+		} else {
+			Expect(translator.ValidateRoutePath(path)).To(HaveOccurred())
+		}
+	},
+		Entry("Hex", "%af", true),
+		Entry("Hex Camel", "%Af", true),
+		Entry("Hex num", "%00", true),
+		Entry("Hex double", "%11", true),
+		Entry("Hex with valid", "%af801&*", true),
+		Entry("valid with hex", "801&*%af", true),
+		Entry("valid with hex and valid", "801&*%af719$@!", true),
+		Entry("Hex single", "%0", false),
+		Entry("unicode chars", "ĩ", false),
+		Entry("unicode chars", "¥¨˚∫", false),
+		Entry("//", "hello/something//", false),
+		Entry("/./", "hello/something/./", false),
+		Entry("/../", "hello/something/../", false),
+		Entry("hex slash upper", "hello/something%2F", false),
+		Entry("hex slash lower", "hello/something%2f", false),
+		Entry("hash", "hello/something#", false),
+		Entry("/..", "hello/../something", false),
+		Entry("/.", "hello/./something", false),
+	)
+
+	It("Should validate all seperate characters", func() {
+		// must allow all "pchar" characters = unreserved / pct-encoded / sub-delims / ":" / "@"
+		// https://www.rfc-editor.org/rfc/rfc3986
+		// unreserved
+		// alpha Upper and Lower
+		for i := 'a'; i <= 'z'; i++ {
+			Expect(translator.ValidateRoutePath(string(i))).ToNot(HaveOccurred())
+			Expect(translator.ValidateRoutePath(strings.ToUpper(string(i)))).ToNot(HaveOccurred())
+		}
+		// digit
+		for i := 0; i < 10; i++ {
+			Expect(translator.ValidateRoutePath(strconv.Itoa(i))).ToNot(HaveOccurred())
+		}
+		unreservedChars := "-._~"
+		for _, c := range unreservedChars {
+			Expect(translator.ValidateRoutePath(string(c))).ToNot(HaveOccurred())
+		}
+		// sub-delims
+		subDelims := "!$&'()*+,;="
+		Expect(len(subDelims)).To(Equal(11))
+		for _, c := range subDelims {
+			Expect(translator.ValidateRoutePath(string(c))).ToNot(HaveOccurred())
+		}
+		// pchar
+		pchar := ":@"
+		for _, c := range pchar {
+			Expect(translator.ValidateRoutePath(string(c))).ToNot(HaveOccurred())
+		}
+		// invalid characters
+		invalid := "<>?\\|[]{}\"^%#"
+		for _, c := range invalid {
+			Expect(translator.ValidateRoutePath(string(c))).To(HaveOccurred())
+		}
+	})
+
+})

projects/gloo/pkg/translator/translator_test.go

@@ -353,6 +353,15 @@ var _ = Describe("Translator", func() {
 		return report
 	}
 
+	translateWithInvalidRoutePath := func() *validation.ProxyReport {
+		_, errs, report := translator.Translate(params, proxy)
+		err := errs.Validate()
+		ExpectWithOffset(1, err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("cannot contain [/../]"))
+		return report
+
+	}
+
 	translateWithBuggyHasher := func() *validation.ProxyReport {
 		buggyHasher := func(resources []envoycache.Resource) (uint64, error) {
 			return 0, errors.New("This is a buggy hasher error")
@@ -657,6 +666,100 @@ var _ = Describe("Translator", func() {
 		})
 	})
 
+	Context("invalid route paths", func() {
+		It("should report an invalid path redirect", func() {
+			glooRoute := &v1.Route{
+				Action: &v1.Route_RedirectAction{
+					RedirectAction: &v1.RedirectAction{
+						PathRewriteSpecifier: &v1.RedirectAction_PathRedirect{
+							// invalid sequence
+							PathRedirect: "/../../home/secretdata",
+						},
+					},
+				},
+			}
+			routes[0] = glooRoute
+			translateWithInvalidRoutePath()
+		})
+		It("should report an invalid prefix rewrite", func() {
+			glooRoute := &v1.Route{
+				Action: &v1.Route_RedirectAction{
+					RedirectAction: &v1.RedirectAction{
+						PathRewriteSpecifier: &v1.RedirectAction_PrefixRewrite{
+							PrefixRewrite: "home/../secret",
+						},
+					},
+				},
+			}
+			routes[0] = glooRoute
+			translateWithInvalidRoutePath()
+		})
+
+		It("should report an invalid host redirect", func() {
+			glooRoute := &v1.Route{
+				Action: &v1.Route_RedirectAction{
+					RedirectAction: &v1.RedirectAction{
+						HostRedirect: "otherhost/../secret",
+					},
+				},
+			}
+			routes[0] = glooRoute
+			translateWithInvalidRoutePath()
+		})
+
+		It("should report an invalid prefix rewrite", func() {
+			glooRoute := &v1.Route{
+				Action: &v1.Route_RouteAction{
+					RouteAction: &v1.RouteAction{
+						Destination: &v1.RouteAction_Single{
+							Single: &v1.Destination{
+								DestinationType: &v1.Destination_Upstream{
+									Upstream: &core.ResourceRef{
+										Name:      "somename",
+										Namespace: "someNamespace",
+									},
+								},
+							},
+						},
+					},
+				},
+				Options: &v1.RouteOptions{
+					PrefixRewrite: &wrapperspb.StringValue{Value: "/home/../secret"},
+				},
+			}
+			routes[0] = glooRoute
+			translateWithInvalidRoutePath()
+		})
+
+		It("should report an invalid prefix", func() {
+			glooRoute := &v1.Route{
+				Matchers: []*matchers.Matcher{
+					{
+						PathSpecifier: &matchers.Matcher_Prefix{
+							Prefix: "home/../secret",
+						},
+					},
+				},
+			}
+			routes[0] = glooRoute
+			translateWithInvalidRoutePath()
+		})
+
+		It("should report an invalid prefix", func() {
+			glooRoute := &v1.Route{
+				Matchers: []*matchers.Matcher{
+					{
+						PathSpecifier: &matchers.Matcher_Exact{
+							Exact: "home/../secret",
+						},
+					},
+				},
+			}
+			routes[0] = glooRoute
+			translateWithInvalidRoutePath()
+		})
+	})
+
 	Context("route header match", func() {
 		It("should translate header matcher with no value to a PresentMatch", func() {