Disable remote proxy patching except AWS Lambda

To avoid unintended tampering with remote downstreams via service config, refactor BasicEnvoyExtender and RuntimeConfig to disallow typical Envoy extensions from being applied to non-local proxies. Continue to allow this behavior for AWS Lambda and the read-only Validate builtin extensions. Addresses CVE-2023-2816.
hashicorp · May 23, 2023 · ba18381 · ba18381
1 parent e2a81aa
commit ba18381
Show file tree

Hide file tree

Showing 35 changed files with 1,284 additions and 661 deletions.
diff --git a/.changelog/17415.txt b/.changelog/17415.txt
@@ -0,0 +1,7 @@
+```release-note:security
+extensions: Disable remote downstream proxy patching by Envoy Extensions other than AWS Lambda. Previously, an operator with service:write ACL permissions for an upstream service could modify Envoy proxy config for downstream services without equivalent permissions for those services. This issue only impacts the Lua extension. [[CVE-2023-2816](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-2816)]
+```
+
+```release-note:breaking-change
+extensions: The Lua extension now targets local proxy listeners for the configured service's upstreams, rather than remote downstream listeners for the configured service, when ListenerType is set to outbound in extension configuration. See [CVE-2023-2816](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2023-2816) changelog entry for more details.
+```
diff --git a/agent/envoyextensions/builtin/aws-lambda/aws_lambda.go b/agent/envoyextensions/builtin/aws-lambda/aws_lambda.go
@@ -42,7 +42,7 @@ func Constructor(ext api.EnvoyExtension) (extensioncommon.EnvoyExtender, error)
 	if err := a.fromArguments(ext.Arguments); err != nil {
 		return nil, err
 	}
-	return &extensioncommon.BasicEnvoyExtender{
+	return &extensioncommon.UpstreamEnvoyExtender{
 		Extension: &a,
 	}, nil
 }
@@ -65,7 +65,7 @@ func (a *awsLambda) validate() error {
 // CanApply returns true if the kind of the provided ExtensionConfiguration matches
 // the kind of the lambda configuration
 func (a *awsLambda) CanApply(config *extensioncommon.RuntimeConfig) bool {
-	return config.Kind == config.OutgoingProxyKind()
+	return config.Kind == config.UpstreamOutgoingProxyKind()
 }
 
 // PatchRoute modifies the routing configuration for a service of kind TerminatingGateway. If the kind is
@@ -75,6 +75,11 @@ func (a *awsLambda) PatchRoute(r *extensioncommon.RuntimeConfig, route *envoy_ro
 		return route, false, nil
 	}
 
+	// Only patch outbound routes.
+	if extensioncommon.IsRouteToLocalAppCluster(route) {
+		return route, false, nil
+	}
+
 	for _, virtualHost := range route.VirtualHosts {
 		for _, route := range virtualHost.Routes {
 			action, ok := route.Action.(*envoy_route_v3.Route_Route)
@@ -95,6 +100,11 @@ func (a *awsLambda) PatchRoute(r *extensioncommon.RuntimeConfig, route *envoy_ro
 
 // PatchCluster patches the provided envoy cluster with data required to support an AWS lambda function
 func (a *awsLambda) PatchCluster(_ *extensioncommon.RuntimeConfig, c *envoy_cluster_v3.Cluster) (*envoy_cluster_v3.Cluster, bool, error) {
+	// Only patch outbound clusters.
+	if extensioncommon.IsLocalAppCluster(c) {
+		return c, false, nil
+	}
+
 	transportSocket, err := extensioncommon.MakeUpstreamTLSTransportSocket(&envoy_tls_v3.UpstreamTlsContext{
 		Sni: "*.amazonaws.com",
 	})
@@ -156,7 +166,12 @@ func (a *awsLambda) PatchCluster(_ *extensioncommon.RuntimeConfig, c *envoy_clus
 
 // PatchFilter patches the provided envoy filter with an inserted lambda filter being careful not to
 // overwrite the http filters.
-func (a *awsLambda) PatchFilter(_ *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter) (*envoy_listener_v3.Filter, bool, error) {
+func (a *awsLambda) PatchFilter(_ *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter, isInboundListener bool) (*envoy_listener_v3.Filter, bool, error) {
+	// Only patch outbound filters.
+	if isInboundListener {
+		return filter, false, nil
+	}
+
 	if filter.Name != "envoy.filters.network.http_connection_manager" {
 		return filter, false, nil
 	}

diff --git a/agent/envoyextensions/builtin/aws-lambda/aws_lambda_test.go b/agent/envoyextensions/builtin/aws-lambda/aws_lambda_test.go
@@ -86,7 +86,7 @@ func TestConstructor(t *testing.T) {
 
 			if tc.ok {
 				require.NoError(t, err)
-				require.Equal(t, &extensioncommon.BasicEnvoyExtender{Extension: &tc.expected}, e)
+				require.Equal(t, &extensioncommon.UpstreamEnvoyExtender{Extension: &tc.expected}, e)
 			} else {
 				require.Error(t, err)
 			}
@@ -323,10 +323,11 @@ func TestPatchFilter(t *testing.T) {
 		return v
 	}
 	tests := map[string]struct {
-		filter       *envoy_listener_v3.Filter
-		expectFilter *envoy_listener_v3.Filter
-		expectBool   bool
-		expectErr    string
+		filter          *envoy_listener_v3.Filter
+		isInboundFilter bool
+		expectFilter    *envoy_listener_v3.Filter
+		expectBool      bool
+		expectErr       string
 	}{
 		"invalid filter name is ignored": {
 			filter:       &envoy_listener_v3.Filter{Name: "something"},
@@ -416,6 +417,36 @@ func TestPatchFilter(t *testing.T) {
 			},
 			expectBool: true,
 		},
+		"inbound filter ignored": {
+			filter: &envoy_listener_v3.Filter{
+				Name: "envoy.filters.network.http_connection_manager",
+				ConfigType: &envoy_listener_v3.Filter_TypedConfig{
+					TypedConfig: makeAny(&envoy_http_v3.HttpConnectionManager{
+						HttpFilters: []*envoy_http_v3.HttpFilter{
+							{Name: "one"},
+							{Name: "two"},
+							{Name: "envoy.filters.http.router"},
+							{Name: "three"},
+						},
+					}),
+				},
+			},
+			expectFilter: &envoy_listener_v3.Filter{
+				Name: "envoy.filters.network.http_connection_manager",
+				ConfigType: &envoy_listener_v3.Filter_TypedConfig{
+					TypedConfig: makeAny(&envoy_http_v3.HttpConnectionManager{
+						HttpFilters: []*envoy_http_v3.HttpFilter{
+							{Name: "one"},
+							{Name: "two"},
+							{Name: "envoy.filters.http.router"},
+							{Name: "three"},
+						},
+					}),
+				},
+			},
+			isInboundFilter: true,
+			expectBool:      false,
+		},
 	}
 
 	for name, tc := range tests {
@@ -425,7 +456,7 @@ func TestPatchFilter(t *testing.T) {
 				PayloadPassthrough: true,
 				InvocationMode:     "asynchronous",
 			}
-			f, ok, err := l.PatchFilter(nil, tc.filter)
+			f, ok, err := l.PatchFilter(nil, tc.filter, tc.isInboundFilter)
 			require.Equal(t, tc.expectBool, ok)
 			if tc.expectErr == "" {
 				require.NoError(t, err)

diff --git a/agent/envoyextensions/builtin/http/localratelimit/ratelimit.go b/agent/envoyextensions/builtin/http/localratelimit/ratelimit.go
@@ -97,9 +97,7 @@ func (r *ratelimit) validate() error {
 
 // CanApply determines if the extension can apply to the given extension configuration.
 func (p *ratelimit) CanApply(config *extensioncommon.RuntimeConfig) bool {
-	// rate limit is only applied to the service itself since the limit is
-	// aggregated from all downstream connections.
-	return string(config.Kind) == p.ProxyType && !config.IsUpstream()
+	return string(config.Kind) == p.ProxyType
 }
 
 // PatchRoute does nothing.
@@ -114,7 +112,13 @@ func (p ratelimit) PatchCluster(_ *extensioncommon.RuntimeConfig, c *envoy_clust
 
 // PatchFilter inserts a http local rate_limit filter at the head of
 // envoy.filters.network.http_connection_manager filters
-func (p ratelimit) PatchFilter(_ *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter) (*envoy_listener_v3.Filter, bool, error) {
+func (p ratelimit) PatchFilter(_ *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter, isInboundListener bool) (*envoy_listener_v3.Filter, bool, error) {
+	// rate limit is only applied to the inbound listener of the service itself
+	// since the limit is aggregated from all downstream connections.
+	if !isInboundListener {
+		return filter, false, nil
+	}
+
 	if filter.Name != "envoy.filters.network.http_connection_manager" {
 		return filter, false, nil
 	}

diff --git a/agent/envoyextensions/builtin/lua/lua.go b/agent/envoyextensions/builtin/lua/lua.go
@@ -64,11 +64,11 @@ func (l *lua) validate() error {
 
 // CanApply determines if the extension can apply to the given extension configuration.
 func (l *lua) CanApply(config *extensioncommon.RuntimeConfig) bool {
-	return string(config.Kind) == l.ProxyType && l.matchesListenerDirection(config)
+	return string(config.Kind) == l.ProxyType
 }
 
-func (l *lua) matchesListenerDirection(config *extensioncommon.RuntimeConfig) bool {
-	return (config.IsUpstream() && l.Listener == "outbound") || (!config.IsUpstream() && l.Listener == "inbound")
+func (l *lua) matchesListenerDirection(isInboundListener bool) bool {
+	return (!isInboundListener && l.Listener == "outbound") || (isInboundListener && l.Listener == "inbound")
 }
 
 // PatchRoute does nothing.
@@ -82,7 +82,12 @@ func (l *lua) PatchCluster(_ *extensioncommon.RuntimeConfig, c *envoy_cluster_v3
 }
 
 // PatchFilter inserts a lua filter directly prior to envoy.filters.http.router.
-func (l *lua) PatchFilter(_ *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter) (*envoy_listener_v3.Filter, bool, error) {
+func (l *lua) PatchFilter(_ *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter, isInboundListener bool) (*envoy_listener_v3.Filter, bool, error) {
+	// Make sure filter matches extension config.
+	if !l.matchesListenerDirection(isInboundListener) {
+		return filter, false, nil
+	}
+
 	if filter.Name != "envoy.filters.network.http_connection_manager" {
 		return filter, false, nil
 	}

diff --git a/agent/envoyextensions/builtin/wasm/structs.go b/agent/envoyextensions/builtin/wasm/structs.go
@@ -193,7 +193,7 @@ func (p *pluginConfig) asyncDataSource(rtCfg *extensioncommon.RuntimeConfig) (*e
 	// fetch the data from the upstream source.
 	remote := &p.VmConfig.Code.Remote
 	clusterSNI := ""
-	for service, upstream := range rtCfg.LocalUpstreams {
+	for service, upstream := range rtCfg.Upstreams {
 		if service == remote.HttpURI.Service {
 			for sni := range upstream.SNI {
 				clusterSNI = sni

diff --git a/agent/envoyextensions/builtin/wasm/wasm.go b/agent/envoyextensions/builtin/wasm/wasm.go
@@ -68,12 +68,13 @@ func (w *wasm) fromArguments(args map[string]any) error {
 
 // CanApply indicates if the WASM extension can be applied to the given extension configuration.
 // Currently the Wasm extension can be applied if the extension configuration is for an inbound
-// listener on the a local connect-proxy.
-// It does not patch extensions for service upstreams.
+// listener (checked below) on a local connect-proxy.
 func (w wasm) CanApply(config *extensioncommon.RuntimeConfig) bool {
-	return config.IsLocal() && w.wasmConfig.ListenerType == "inbound" &&
-		config.Kind == w.wasmConfig.ProxyType
+	return config.Kind == w.wasmConfig.ProxyType
+}
 
+func (w wasm) matchesConfigDirection(isInboundListener bool) bool {
+	return isInboundListener && w.wasmConfig.ListenerType == "inbound"
 }
 
 // PatchRoute does nothing for the WASM extension.
@@ -88,7 +89,11 @@ func (w wasm) PatchCluster(_ *extensioncommon.RuntimeConfig, c *envoy_cluster_v3
 
 // PatchFilter adds a Wasm filter to the HTTP filter chain.
 // TODO (wasm/tcp): Add support for TCP filters.
-func (w wasm) PatchFilter(cfg *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter) (*envoy_listener_v3.Filter, bool, error) {
+func (w wasm) PatchFilter(cfg *extensioncommon.RuntimeConfig, filter *envoy_listener_v3.Filter, isInboundListener bool) (*envoy_listener_v3.Filter, bool, error) {
+	if !w.matchesConfigDirection(isInboundListener) {
+		return filter, false, nil
+	}
+
 	if filter.Name != "envoy.filters.network.http_connection_manager" {
 		return filter, false, nil
 	}

diff --git a/agent/envoyextensions/builtin/wasm/wasm_test.go b/agent/envoyextensions/builtin/wasm/wasm_test.go
@@ -33,21 +33,24 @@ import (
 func TestHttpWasmExtension(t *testing.T) {
 	t.Parallel()
 	cases := map[string]struct {
-		extName      string
-		canApply     bool
-		args         func(bool) map[string]any
-		rtCfg        func(bool) *extensioncommon.RuntimeConfig
-		inputFilters func() []*envoy_http_v3.HttpFilter
-		expFilters   func(tc testWasmConfig) []*envoy_http_v3.HttpFilter
-		errStr       string
-		debug        bool
+		extName         string
+		canApply        bool
+		args            func(bool) map[string]any
+		rtCfg           func(bool) *extensioncommon.RuntimeConfig
+		isInboundFilter bool
+		inputFilters    func() []*envoy_http_v3.HttpFilter
+		expFilters      func(tc testWasmConfig) []*envoy_http_v3.HttpFilter
+		expPatched      bool
+		errStr          string
+		debug           bool
 	}{
 		"http remote file": {
-			extName:      api.BuiltinWasmExtension,
-			canApply:     true,
-			args:         func(ent bool) map[string]any { return makeTestWasmConfig(ent).toMap(t) },
-			rtCfg:        func(ent bool) *extensioncommon.RuntimeConfig { return makeTestRuntimeConfig(ent) },
-			inputFilters: makeTestHttpFilters,
+			extName:         api.BuiltinWasmExtension,
+			canApply:        true,
+			args:            func(ent bool) map[string]any { return makeTestWasmConfig(ent).toMap(t) },
+			rtCfg:           func(ent bool) *extensioncommon.RuntimeConfig { return makeTestRuntimeConfig(ent) },
+			isInboundFilter: true,
+			inputFilters:    makeTestHttpFilters,
 			expFilters: func(tc testWasmConfig) []*envoy_http_v3.HttpFilter {
 				return []*envoy_http_v3.HttpFilter{
 					{Name: "one"},
@@ -65,6 +68,7 @@ func TestHttpWasmExtension(t *testing.T) {
 					{Name: "three"},
 				}
 			},
+			expPatched: true,
 		},
 		"local file": {
 			extName:  api.BuiltinWasmExtension,
@@ -76,8 +80,9 @@ func TestHttpWasmExtension(t *testing.T) {
 				cfg.PluginConfig.VmConfig.Code.Local.Filename = "plugin.wasm"
 				return cfg.toMap(t)
 			},
-			rtCfg:        func(ent bool) *extensioncommon.RuntimeConfig { return makeTestRuntimeConfig(ent) },
-			inputFilters: makeTestHttpFilters,
+			rtCfg:           func(ent bool) *extensioncommon.RuntimeConfig { return makeTestRuntimeConfig(ent) },
+			isInboundFilter: true,
+			inputFilters:    makeTestHttpFilters,
 			expFilters: func(tc testWasmConfig) []*envoy_http_v3.HttpFilter {
 				return []*envoy_http_v3.HttpFilter{
 					{Name: "one"},
@@ -95,18 +100,38 @@ func TestHttpWasmExtension(t *testing.T) {
 					{Name: "three"},
 				}
 			},
+			expPatched: true,
+		},
+		"inbound filters ignored": {
+			extName:         api.BuiltinWasmExtension,
+			canApply:        true,
+			args:            func(ent bool) map[string]any { return makeTestWasmConfig(ent).toMap(t) },
+			rtCfg:           func(ent bool) *extensioncommon.RuntimeConfig { return makeTestRuntimeConfig(ent) },
+			isInboundFilter: false,
+			inputFilters:    makeTestHttpFilters,
+			expFilters: func(tc testWasmConfig) []*envoy_http_v3.HttpFilter {
+				return []*envoy_http_v3.HttpFilter{
+					{Name: "one"},
+					{Name: "two"},
+					{Name: "envoy.filters.http.router"},
+					{Name: "three"},
+				}
+			},
+			expPatched: false,
 		},
 		"no cluster for remote file": {
 			extName:  api.BuiltinWasmExtension,
 			canApply: true,
 			args:     func(ent bool) map[string]any { return makeTestWasmConfig(ent).toMap(t) },
 			rtCfg: func(ent bool) *extensioncommon.RuntimeConfig {
 				rt := makeTestRuntimeConfig(ent)
-				rt.LocalUpstreams = nil
+				rt.Upstreams = nil
 				return rt
 			},
-			inputFilters: makeTestHttpFilters,
-			errStr:       "no upstream found for remote service",
+			isInboundFilter: true,
+			inputFilters:    makeTestHttpFilters,
+			errStr:          "no upstream found for remote service",
+			expPatched:      false,
 		},
 	}
 
@@ -140,10 +165,10 @@ func TestHttpWasmExtension(t *testing.T) {
 				require.NoError(t, err)
 
 				inputHttpConMgr := makeHttpConMgr(t, c.inputFilters())
-				obsHttpConMgr, patched, err := w.PatchFilter(c.rtCfg(enterprise), inputHttpConMgr)
+				obsHttpConMgr, patched, err := w.PatchFilter(c.rtCfg(enterprise), inputHttpConMgr, c.isInboundFilter)
 				if c.errStr == "" {
 					require.NoError(t, err)
-					require.True(t, patched)
+					require.Equal(t, c.expPatched, patched)
 
 					cfg := testWasmConfigFromMap(t, c.args(enterprise))
 					expHttpConMgr := makeHttpConMgr(t, c.expFilters(cfg))
@@ -156,6 +181,7 @@ func TestHttpWasmExtension(t *testing.T) {
 
 					prototest.AssertDeepEqual(t, expHttpConMgr, obsHttpConMgr)
 				} else {
+					require.Error(t, err)
 					require.Contains(t, err.Error(), c.errStr)
 				}
 
@@ -554,7 +580,7 @@ func makeTestRuntimeConfig(enterprise bool) *extensioncommon.RuntimeConfig {
 	return &extensioncommon.RuntimeConfig{
 		Kind:        api.ServiceKindConnectProxy,
 		ServiceName: api.CompoundServiceName{Name: "test-service"},
-		LocalUpstreams: map[api.CompoundServiceName]*extensioncommon.UpstreamData{
+		Upstreams: map[api.CompoundServiceName]*extensioncommon.UpstreamData{
 			{
 				Name:      "test-file-server",
 				Namespace: acl.NamespaceOrDefault(ns),