From da1111d61cc34b355f15a231743b7efe653cf46b Mon Sep 17 00:00:00 2001 From: Jacob Cukjati Date: Tue, 3 Jan 2023 09:08:57 -0600 Subject: [PATCH] v1.13-route-validation (#7616) --- changelog/v1.13.2/validate-routes.yaml | 7 ++ .../gloo/api/v1/options/hcm/hcm.proto.sk.md | 2 +- projects/gloo/api/v1/options/hcm/hcm.proto | 2 +- .../gloo/pkg/api/v1/options/hcm/hcm.pb.go | 2 +- projects/gloo/pkg/plugins/hcm/plugin.go | 9 +- projects/gloo/pkg/plugins/hcm/plugin_test.go | 13 +++ projects/gloo/pkg/translator/route_config.go | 79 +++++++++++++- .../gloo/pkg/translator/route_config_test.go | 77 +++++++++++++ .../gloo/pkg/translator/translator_test.go | 103 ++++++++++++++++++ 9 files changed, 288 insertions(+), 6 deletions(-) create mode 100644 changelog/v1.13.2/validate-routes.yaml create mode 100644 projects/gloo/pkg/translator/route_config_test.go diff --git a/changelog/v1.13.2/validate-routes.yaml b/changelog/v1.13.2/validate-routes.yaml new file mode 100644 index 00000000000..7ee989c63af --- /dev/null +++ b/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 diff --git a/docs/content/reference/api/github.com/solo-io/gloo/projects/gloo/api/v1/options/hcm/hcm.proto.sk.md b/docs/content/reference/api/github.com/solo-io/gloo/projects/gloo/api/v1/options/hcm/hcm.proto.sk.md index 2c568138405..528ddfa5f49 100644 --- a/docs/content/reference/api/github.com/solo-io/gloo/projects/gloo/api/v1/options/hcm/hcm.proto.sk.md +++ b/docs/content/reference/api/github.com/solo-io/gloo/projects/gloo/api/v1/options/hcm/hcm.proto.sk.md @@ -120,7 +120,7 @@ v3 documents https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filte | `pathWithEscapedSlashesAction` | [.hcm.options.gloo.solo.io.HttpConnectionManagerSettings.PathWithEscapedSlashesAction](../hcm.proto.sk/#pathwithescapedslashesaction) | Action to take when request URL path contains escaped slash sequences (%2F, %2f, %5C and %5c). The default value can be overridden by the :ref:`http_connection_manager.path_with_escaped_slashes_action` runtime variable. The :ref:`http_connection_manager.path_with_escaped_slashes_action_sampling` runtime variable can be used to apply the action to a portion of all requests. | | `codecType` | [.hcm.options.gloo.solo.io.HttpConnectionManagerSettings.CodecType](../hcm.proto.sk/#codectype) | Supplies the type of codec that the connection manager should use. 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#extensions-filters-network-http-connection-manager-v3-httpconnectionmanager. | | `mergeSlashes` | [.google.protobuf.BoolValue](https://developers.google.com/protocol-buffers/docs/reference/csharp/class/google/protobuf/well-known-types/bool-value) | Determines if adjacent slashes in the path are merged into one before any processing of requests by HTTP filters or routing. 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. | -| `normalizePath` | [.google.protobuf.BoolValue](https://developers.google.com/protocol-buffers/docs/reference/csharp/class/google/protobuf/well-known-types/bool-value) | Should paths be normalized according to RFC 3986 before any processing of requests by HTTP filters or routing? 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. | +| `normalizePath` | [.google.protobuf.BoolValue](https://developers.google.com/protocol-buffers/docs/reference/csharp/class/google/protobuf/well-known-types/bool-value) | 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. | | `uuidRequestIdConfig` | [.hcm.options.gloo.solo.io.HttpConnectionManagerSettings.UuidRequestIdConfigSettings](../hcm.proto.sk/#uuidrequestidconfigsettings) | | | `http2ProtocolOptions` | [.protocol.options.gloo.solo.io.Http2ProtocolOptions](../../protocol/protocol.proto.sk/#http2protocoloptions) | Additional HTTP/2 settings that are passed directly to the HTTP/2 codec. | | `internalAddressConfig` | [.hcm.options.gloo.solo.io.HttpConnectionManagerSettings.InternalAddressConfig](../hcm.proto.sk/#internaladdressconfig) | Configuration of internal addresses. | diff --git a/projects/gloo/api/v1/options/hcm/hcm.proto b/projects/gloo/api/v1/options/hcm/hcm.proto index 7cac0fe2562..9b55f9714e3 100644 --- a/projects/gloo/api/v1/options/hcm/hcm.proto +++ b/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; diff --git a/projects/gloo/pkg/api/v1/options/hcm/hcm.pb.go b/projects/gloo/pkg/api/v1/options/hcm/hcm.pb.go index c15e0e476dd..b20bdc51179 100644 --- a/projects/gloo/pkg/api/v1/options/hcm/hcm.pb.go +++ b/projects/gloo/pkg/api/v1/options/hcm/hcm.pb.go @@ -391,7 +391,7 @@ type HttpConnectionManagerSettings struct { // Determines if adjacent slashes in the path are merged into one before any processing of requests by HTTP filters or routing. // 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 MergeSlashes *wrappers.BoolValue `protobuf:"bytes,29,opt,name=merge_slashes,json=mergeSlashes,proto3" json:"merge_slashes,omitempty"` - // 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 NormalizePath *wrappers.BoolValue `protobuf:"bytes,30,opt,name=normalize_path,json=normalizePath,proto3" json:"normalize_path,omitempty"` UuidRequestIdConfig *HttpConnectionManagerSettings_UuidRequestIdConfigSettings `protobuf:"bytes,37,opt,name=uuid_request_id_config,json=uuidRequestIdConfig,proto3" json:"uuid_request_id_config,omitempty"` diff --git a/projects/gloo/pkg/plugins/hcm/plugin.go b/projects/gloo/pkg/plugins/hcm/plugin.go index 1899eb6fc1f..64bacef3edd 100644 --- a/projects/gloo/pkg/plugins/hcm/plugin.go +++ b/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{ diff --git a/projects/gloo/pkg/plugins/hcm/plugin_test.go b/projects/gloo/pkg/plugins/hcm/plugin_test.go index 464e4bff220..7c92ff755ac 100644 --- a/projects/gloo/pkg/plugins/hcm/plugin_test.go +++ b/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() { diff --git a/projects/gloo/pkg/translator/route_config.go b/projects/gloo/pkg/translator/route_config.go index adce8b9c879..bfede32d9ed 100644 --- a/projects/gloo/pkg/translator/route_config.go +++ b/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 +} diff --git a/projects/gloo/pkg/translator/route_config_test.go b/projects/gloo/pkg/translator/route_config_test.go new file mode 100644 index 00000000000..b0ca90f2c9a --- /dev/null +++ b/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()) + } + }) + +}) diff --git a/projects/gloo/pkg/translator/translator_test.go b/projects/gloo/pkg/translator/translator_test.go index bce206c7da7..a605072bb18 100644 --- a/projects/gloo/pkg/translator/translator_test.go +++ b/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() {