From 751e637199edce4affafe8b040e73a5a02156fab Mon Sep 17 00:00:00 2001 From: Nathan Fudenberg Date: Mon, 15 Jul 2024 13:51:09 -0400 Subject: [PATCH] 1.17: Fix/enforce regex inutils (#9735) (#9762) * regexutils: Add poc for how we can validate regex, does not include all locations just for cors * cors: trailing } * cors: testing with invalid regex * docs: update transforms to actually work. Note routes already use regex compile and transforms use envoy validation * changelogs * changelogs * Adding changelog file to new location * Deleting changelog file from old location * plugins/cors/test: add bad RE2 * projects/pkg/cors: regex * regexutils: naming update --------- Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> --- changelog/v1.17.0/cors-regex-safety.yaml | 7 ++++ .../transformations/_index.md | 2 +- pkg/utils/regexutils/regex.go | 40 ++++++++++++++++--- projects/gloo/pkg/plugins/cors/plugin.go | 8 +++- .../pkg/plugins/cors/route_plugin_test.go | 11 ++++- .../pkg/plugins/cors/vhost_plugin_test.go | 15 ++++++- 6 files changed, 72 insertions(+), 11 deletions(-) create mode 100644 changelog/v1.17.0/cors-regex-safety.yaml diff --git a/changelog/v1.17.0/cors-regex-safety.yaml b/changelog/v1.17.0/cors-regex-safety.yaml new file mode 100644 index 00000000000..f610e1a1d2d --- /dev/null +++ b/changelog/v1.17.0/cors-regex-safety.yaml @@ -0,0 +1,7 @@ +changelog: + - type: FIX + issueLink: https://github.com/solo-io/gloo/issues/7524 + resolvesIssue: false + description: >- + Fix regex safety check for CORS allowed origins. + Other instances in gloo already checked the regex either through compile or envoy validate. diff --git a/docs/content/guides/traffic_management/request_processing/transformations/_index.md b/docs/content/guides/traffic_management/request_processing/transformations/_index.md index 98fb7c3ffed..02f3b08652f 100644 --- a/docs/content/guides/traffic_management/request_processing/transformations/_index.md +++ b/docs/content/guides/traffic_management/request_processing/transformations/_index.md @@ -51,7 +51,7 @@ virtualHost: regular: requestTransforms: - matcher: - - prefix: '/parent' + prefix: '/parent' requestTransformation: transformationTemplate: headers: diff --git a/pkg/utils/regexutils/regex.go b/pkg/utils/regexutils/regex.go index cfb33e5745c..139bf503aaa 100644 --- a/pkg/utils/regexutils/regex.go +++ b/pkg/utils/regexutils/regex.go @@ -2,6 +2,7 @@ package regexutils import ( "context" + "regexp" envoy_type_matcher_v3 "github.com/envoyproxy/go-control-plane/envoy/type/matcher/v3" wrappers "github.com/golang/protobuf/ptypes/wrappers" @@ -11,22 +12,49 @@ import ( "github.com/solo-io/solo-kit/pkg/errors" ) -func NewRegex(ctx context.Context, regex string) *envoy_type_matcher_v3.RegexMatcher { +// NewCheckedRegex creates a new regex matcher with the given regex. +// It is tightly coupled to envoy's implementation of regex. +func NewCheckedRegex(ctx context.Context, candidateRegex string) (*envoy_type_matcher_v3.RegexMatcher, error) { + if err := CheckRegexString(candidateRegex); err != nil { + return nil, err + } + return NewRegex(ctx, candidateRegex), nil +} + +// CheckRegexString to make sure the string is a valid RE2 expression +func CheckRegexString(candidateRegex string) error { + // https://github.com/envoyproxy/envoy/blob/v1.30.0/source/common/common/regex.cc#L19C8-L19C14 + // Envoy uses the RE2 library for regex matching in google's owned c++ impl. + // go has https://pkg.go.dev/regexp which implements RE2 with a single caveat. + _, err := regexp.Compile(candidateRegex) + return err +} + +// NewRegex creates a new regex matcher with the given regex. +// It is tightly coupled to envoy's implementation of regex. +// Wraps NewRegexFromSettings which wraps NewRegexWithProgramSize which leads to the tight coupling. +// NOTE: Call this after having checked regex with CheckRegexString. +func NewRegex(ctx context.Context, candidateRegex string) *envoy_type_matcher_v3.RegexMatcher { settings := settingsutil.MaybeFromContext(ctx) - return NewRegexFromSettings(settings, regex) + return NewRegexFromSettings(settings, candidateRegex) } -func NewRegexFromSettings(settings *v1.Settings, regex string) *envoy_type_matcher_v3.RegexMatcher { +// NewRegexFromSettings wraps NewRegexWithProgramSize with the program size from the settings. +// NOTE: Call this after having checked regex with CheckRegexString. +func NewRegexFromSettings(settings *v1.Settings, candidateRegex string) *envoy_type_matcher_v3.RegexMatcher { var programsize *uint32 if settings != nil { if max_size := settings.GetGloo().GetRegexMaxProgramSize(); max_size != nil { programsize = &max_size.Value } } - return NewRegexWithProgramSize(regex, programsize) + return NewRegexWithProgramSize(candidateRegex, programsize) } -func NewRegexWithProgramSize(regex string, programsize *uint32) *envoy_type_matcher_v3.RegexMatcher { +// NewRegexWithProgramSize creates a new regex matcher with the given program size. +// This means its tightly coupled to envoy's implementation of regex. +// NOTE: Call this after having checked regex with CheckRegexString. +func NewRegexWithProgramSize(candidateRegex string, programsize *uint32) *envoy_type_matcher_v3.RegexMatcher { var maxProgramSize *wrappers.UInt32Value if programsize != nil { @@ -39,7 +67,7 @@ func NewRegexWithProgramSize(regex string, programsize *uint32) *envoy_type_matc EngineType: &envoy_type_matcher_v3.RegexMatcher_GoogleRe2{ GoogleRe2: &envoy_type_matcher_v3.RegexMatcher_GoogleRE2{MaxProgramSize: maxProgramSize}, }, - Regex: regex, + Regex: candidateRegex, } } diff --git a/projects/gloo/pkg/plugins/cors/plugin.go b/projects/gloo/pkg/plugins/cors/plugin.go index b575048e831..9633899ebee 100644 --- a/projects/gloo/pkg/plugins/cors/plugin.go +++ b/projects/gloo/pkg/plugins/cors/plugin.go @@ -76,7 +76,7 @@ func (p *plugin) ProcessVirtualHost( p.filterRequiredForListener[params.HttpListener] = struct{}{} corsPolicy := &envoy_config_cors_v3.CorsPolicy{} if err := p.translateCommonUserCorsConfig(params.Ctx, corsPlugin, corsPolicy); err != nil { - return err + return errors.Join(errors.New("invalid cors config"), err) } return pluginutils.SetVhostPerFilterConfig(out, wellknown.CORS, corsPolicy) @@ -134,8 +134,12 @@ func (p *plugin) translateCommonUserCorsConfig( }) } for _, ao := range in.GetAllowOriginRegex() { + regexStruct, err := regexutils.NewCheckedRegex(ctx, ao) + if err != nil { + return err + } out.AllowOriginStringMatch = append(out.GetAllowOriginStringMatch(), &envoy_type_matcher_v3.StringMatcher{ - MatchPattern: &envoy_type_matcher_v3.StringMatcher_SafeRegex{SafeRegex: regexutils.NewRegex(ctx, ao)}, + MatchPattern: &envoy_type_matcher_v3.StringMatcher_SafeRegex{SafeRegex: regexStruct}, }) } out.AllowMethods = strings.Join(in.GetAllowMethods(), ",") diff --git a/projects/gloo/pkg/plugins/cors/route_plugin_test.go b/projects/gloo/pkg/plugins/cors/route_plugin_test.go index 465eebbfcce..652399ce5e8 100644 --- a/projects/gloo/pkg/plugins/cors/route_plugin_test.go +++ b/projects/gloo/pkg/plugins/cors/route_plugin_test.go @@ -30,7 +30,8 @@ var _ = Describe("Route Plugin", func() { // values used in first example allowOrigin1 = []string{"solo.io", "github.com"} - allowOriginRegex1 = []string{`*\.solo\.io`, `git.*\.com`} + allowOriginRegex1 = []string{`.*\.solo\.io`, `git.*\.com`} + badOriginRegex1 = []string{`*\.solo\.io`, `git.*\.com`} // has a * at the front which is invalid allowMethods1 = []string{"GET", "POST"} allowHeaders1 = []string{"allowH1", "allow2"} exposeHeaders1 = []string{"exHeader", "eh2"} @@ -135,6 +136,14 @@ var _ = Describe("Route Plugin", func() { Expect(outRoute.TypedPerFilterConfig).To(Equal(expected.TypedPerFilterConfig)) }) + It("should reject bad CORS", func() { + inRoute := routeWithCors(&cors.CorsPolicy{ + AllowOriginRegex: badOriginRegex1, + }) + outRoute := basicEnvoyRoute() + err := plugin.(plugins.RoutePlugin).ProcessRoute(params, inRoute, outRoute) + Expect(err).To(HaveOccurred(), "any regex starting with a * should not be RE2 compliant") + }) It("should process empty specification", func() { inRoute := routeWithCors(&cors.CorsPolicy{}) outRoute := basicEnvoyRoute() diff --git a/projects/gloo/pkg/plugins/cors/vhost_plugin_test.go b/projects/gloo/pkg/plugins/cors/vhost_plugin_test.go index f852c6118b5..11fce6a75bd 100644 --- a/projects/gloo/pkg/plugins/cors/vhost_plugin_test.go +++ b/projects/gloo/pkg/plugins/cors/vhost_plugin_test.go @@ -27,7 +27,8 @@ var _ = Describe("VirtualHost Plugin", func() { // values used in first example allowOrigin1 = []string{"solo.io", "github.com"} - allowOriginRegex1 = []string{`*\.solo\.io`, `git.*\.com`} + allowOriginRegex1 = []string{`.*\.solo\.io`, `git.*\.com`} + badOriginRegex1 = []string{`*\.solo\.io`, `git.*\.com`} // has a * at the front which is invalid allowMethods1 = []string{"GET", "POST"} allowHeaders1 = []string{"allowH1", "allow2"} exposeHeaders1 = []string{"exHeader", "eh2"} @@ -134,6 +135,18 @@ var _ = Describe("VirtualHost Plugin", func() { } Expect(out).To(Equal(envoy1min)) }) + It("should reject bad CORS virtual hosts - minimal specification", func() { + out := &envoy_config_route_v3.VirtualHost{} + inRoute := &v1.VirtualHost{ + Options: &v1.VirtualHostOptions{ + Cors: &cors.CorsPolicy{ + AllowOriginRegex: badOriginRegex1, + }, + }, + } + err := plugin.(plugins.VirtualHostPlugin).ProcessVirtualHost(params, inRoute, out) + Expect(err).To(HaveOccurred(), "any regex starting with a * should not be RE2 compliant") + }) It("should process virtual hosts - empty specification", func() { out := &envoy_config_route_v3.VirtualHost{} inRoute := &v1.VirtualHost{