Resolve CVE-2023-44487

[ahrtr]

What would you like to be added?

Refer to GHSA-qppj-fm5r-hxr3

Bump golang.org/x/net:

• 0.17.0 for 3.4/3.5/main
  • bump golang.org/x/net to v0.17.0, google.golang.org/grpc to v1.58.3 #16739
  • [3.5] Bump golang.org/x/net to v0.17.0 #16743
  • [3.4] Bump golang.org/x/net to v0.17.0 #16746

Bump golang:

• 1.21.3 for main: Bump go to 1.21.3 #16741
• 1.20.10 for 3.4/3.5
  • [3.5] Bump go to v1.20.10 #16745
  • [3.4] bump golang version to 1.20.10 #16744

Bump grpc-go:

• 1.58.3 for main: bump golang.org/x/net to v0.17.0, google.golang.org/grpc to v1.58.3 #16739
• 1.56.3 or 1.57.1 or 1.58.3 for 3.4/3.5.
  • [3.5] Bump grpc-go to1.47 (and fix the connection-string format) #16625
  • [3.5] Upgrade gRPC-go to v1.52.0 #16781
  • [3.5] Upgrade gRPC-go to 1.58.3 #16790
  • [3.4] Upgrade grpc-go to 1.29.1 and Backport "Introduce grpc-1.30+ compatible client/v3/naming API." #16795
  • [3.4] Backport clientv3 naming implementation #16800
  • [3.4] Backport clientv3:get AuthToken gracefully without dialing gRPC with balancer API to get extra connection #16826
  • [3.4] Backport clientv3: Replace balancer with upstream grpc solution #16827
    • Backport [3.4] clientV3: simplify grpc dialer usage. Remove workaround #11184 after bumping grpc to 1.26.0. #16842
    • [3.4] Backport #12671 clientv3: Replace balancer with upstream grpc solution #16844
    • [3.4] backport client: call .Endpoints() in dial() in client/v3/client.go instead of accessing cfg.Endpoints directly #16857
    • [3.4] backport #13359 Fix http2 authority header in single endpoint scenario #16988
  • [3.4] Upgrade grpc to 1.52.0 #16997
  • [3.4] Upgrade grpc 1.58.3 #16999

Clarification

• https://github.com/grpc/grpc-go/pull/6706/files#r1354612788

Why is this needed?

resolve CVE

[tjungblu]

The fix in apiserver seems a little bit more involved than just bumping dependencies:
https://github.com/kubernetes/kubernetes/pull/121120/files

@ahrtr already wrote it down on the OP - sorry!

[ahrtr]

The fix in apiserver seems a little bit more involved than just bumping dependencies:
https://github.com/kubernetes/kubernetes/pull/121120/files

Thanks @tjungblu for pointing this out. (I am not an api-server expert) based on the PR, it seems api-server made two changes:

• In (r *rudimentaryErrorBackoff) OnError(error), do not sleep with the lock, so as not to block all callers;
• Do not allow unauthenticated clients to keep the connections open;

I don't think etcd has similar problems (note K8s doesn't enable auth at all).

Another thing... I noticed that Kubernetes hasn't bumped grpc-go version yet (note latest grpc-go versions have fix to CVE-2023-44487), I guess probably it's because api-server doesn't start grpc server at all.

@liggitt

[chaochn47]

Bump grpc-go:
1.56.3 or 1.57.1 or 1.58.3 for 3.4/3.5.

Just took a quick peak at the gRPC dependency upgrade history on the difference of 3.4 and 3.5:

1. tools: Update tools & go.mod for protos (gogo/protobuf 1.3.1, grpc 1.29.1, grpc-gateway 1.14.6) #12397
2. Update grpc dependency to 1.32. #12709
3. Update dep: github.com/golang/protobuf v1.3.5 ->  v1.5.1, grpc: 1.32->1.36 #12801
4. Add initial Tracing with OpenTelemetry #12919
5. backport-3.5: Update of dependencies:zap, bbolt, grpc, ... #13079
6. [3.5] etcdserver: bump OpenTelemetry to 1.0.1 and gRPC to 1.41.0 #14312

I think goal will be let 3.4 catch up with 3.5 first, ignore other unrelated dependency upgrade and eventually upgrade to 1.58.3, correct?

Is there any major call outs this upgrade could potentially break 3.4 release? @ahrtr @serathius @fuweid

[ahrtr]

I think goal will be let 3.4 catch up with 3.5 first, ignore other unrelated dependency upgrade and eventually upgrade to 1.58.3, correct?

I am good as long as eventually we can resolve the CVE for both 3.5 and 3.4.

Is there any major call outs this upgrade could potentially break 3.4 release? @ahrtr @serathius @fuweid

No immediate call out in my head for now. We can have more discussion if we run into any issue during the upgrade. thx

[fuweid]

Just quick bump the version in my local for v3.4. It might introduce other changes, such as #12675. We might need to try to bump and get the changes result and then evaluate it.

[ahrtr]

Just quick bump the version in my local for v3.4. It might introduce other changes, such as #12675.

I would suggest not to make such changes if they aren't necessary. Another such example: #12671

[chaochn47]

Just quick bump the version in my local for v3.4. It might introduce other changes, such as #12675.

I would suggest not to make such changes if they aren't necessary. Another such example: #12671

https://github.com/grpc/grpc-go/releases/tag/v1.30.0

1. This release removed the deprecated naming package that etcd 3.4 depends on in https://github.com/etcd-io/etcd/blob/release-3.4/clientv3/naming/grpc.go#L25. etcd client won't compile without backport Cleanup grpc client/v3/naming API #12675.
2. etcd 3.4 balancer is built on top of the removed API balancer.ClientConn.UpdateBalancerState.

I guess we have to check in #12675 and #12671 in release-3.4 in order to bump up gRPC-go version instead of forking the gRPC usage in another release branch to fix the CVE.

[ahrtr]

1. This release removed the deprecated naming package

OK, then we have to backport #12675 to 3.4 in order to bum grpc-go version

2. etcd 3.4 balancer is built on top of the removed API balancer.ClientConn.UpdateBalancerState.

Is there a simple solution to just replace the removed balancer.ClientConn.UpdateBalancerState, instead of replacing the whole client side load balancer solution? Note the grpc.Resolver is still an experimental package.

[chaochn47]

Is there a simple solution to just replace the removed balancer.ClientConn.UpdateBalancerState, instead of replacing the whole client side load balancer solution? Note the grpc.Resolver is still an experimental package.

I need to investigate more to fully understand before adapting any code changes to the release.34 client based on the gRPC update since v1.26.0. ETA is end of this week.

Based on #12124 (comment) and #12652, if we still maintain release-3.4 for a prolonged time, it will bite us again since it's forked.

[fuweid]

etcd 3.4 balancer is built on top of the removed API balancer.ClientConn.UpdateBalancerState

This one seems easy to fix. We just need to replace the API with new type input.

Remove the original balancer plugin API, based on grpc.Balancer, and all related functionality.

From https://github.com/grpc/grpc-go/releases/tag/v1.30.0
REF: grpc/grpc-go#3180

But for the balancer v2 API, grpc/grpc-go#3431 has changed the Balancer API.
I think we can't rewrite all the things to adapt the new API because it needs more test cases to ensure the functionality is working.
We can just backport this clientv3: Replace balancer with upstream grpc solution and then bump the grpc.

---

I try to backport the following pull requests in my local (without test code and testing) and then with their changes, we can bump grpc to v1.52.0 easier. However, each change needs more eyes to check it.

• clientv3: replace deprecated UpdateBalancerState/Picker API (Part of update grpc-go version to v1.32.0 which has some breaking api changes #12398)
• Introduce grpc-1.30+ compatible client/v3/naming API. 5d7c1db
• endpoints: implement Update method for EndpointManager. 571ed50
• *: implement Endpoint Watch and new Resolver 8feb55f
• Get rid of legacy client/v3/naming API. Update grpcproxy to use the new abstractions. a836a80
• clientv3:get AuthToken gracefully without extra connection. (clientv3:get AuthToken gracefully without extra connection. #12165) 8050881
• Bump to grpc-1.29.1
• Replace client/v3/balancer with standard components: resolver + round_robin LB 0b75fed

[dfawley]

cc @dfawley for opinion about upgrading grpc from 1.26 to 1.56.3 to fix CVE-2023-44487

Was there something specific you wanted information about?

Generally speaking, that CVE isn't very important if you are only handling traffic from trusted clients. In other circumstances, we would highly recommend using a frontend for (D)DOS protection/etc, which could also be used to mitigate that CVE.

That said, 1.26 is very old, and I'd highly recommend upgrading versions on a far more frequent basis to get other bug and security fixes.

[enj]

I did some basic http2 DOS testing against 19bf472, etcd held up well against a single connection, but would still consume 5+ GB of RAM (and basically all the CPU on my machine) once I upped the connection count to ~25. The attacking client was using less than 75% of a single core and less than 150 MB of RAM. I could have trivially bumped the connection count number until etcd OOM'd.

[ahrtr]

I did some basic http2 DOS testing against 19bf472

How many data (K/V pairs) roughly are there in the etcd db file? What's the HTTP2 request you submit to etcd? It's a known issue that etcd may consume huge memory in expensive range request.

FYI. #13458

[enj]

How many data (K/V pairs) roughly are there in the etcd db file?

0 I think, it is just a blank local server running via:

etcd --advertise-client-urls https://127.0.0.1:PORT --data-dir DIR --listen-client-urls https://127.0.0.1:PORT --log-level=warn --cert-file CRT --key-file KEY --auto-tls=true --peer-auto-tls=true

What's the HTTP2 request you submit to etcd?

Basically a GET /v3/. The client in unauthenticated. I know I could make the attack consume more memory by making a more well formed request that would take etcd longer to process.

[tjungblu]

The majority of operators we had fix this CVE by downgrading to http/1 when unauthenticated, think of health probes and metrics endpoints.

[ahrtr]

Basically a GET /v3/. The client in unauthenticated.

Somehow I did not reproduce the issue you described. I used Jmeter to create 100 concurrent threads(users), and kept sending GET /v3/ requests to https://127.0.0.1:2379. The CPU usage is around 300%, and the memory is around 70M. The client side was using about 200% CPU, and 1G memory. Note there is no any data in the etcd db.

Note there is 32G memory in total and 10 Cores in my local environment. There was still some idle CPU and unused memory during the test.

Would you mind share your detailed steps?

[ahrtr]

The majority of operators we had fix this CVE by downgrading to http/1 when unauthenticated, think of health probes and metrics endpoints.

It seems there isn't much we can do in etcd. Also Kubernetes only accesses etcd via gRPC instead of HTTP. K8s/kubelet only accesses the /health, /livez and /readyz over HTTP.

See

etcd/server/embed/serve.go

Lines 353 to 380 in 19bf472

	func (sctx *serveCtx) createMux(gwmux *gw.ServeMux, handler http.Handler) *http.ServeMux {
	httpmux := http.NewServeMux()
	for path, h := range sctx.userHandlers {
	httpmux.Handle(path, h)
	}
	if gwmux != nil {
	httpmux.Handle(
	"/v3/",
	wsproxy.WebsocketProxy(
	gwmux,
	wsproxy.WithRequestMutator(
	// Default to the POST method for streams
	func(_ *http.Request, outgoing *http.Request) *http.Request {
	outgoing.Method = "POST"
	return outgoing
	},
	),
	wsproxy.WithMaxRespBodyBufferSize(0x7fffffff),
	wsproxy.WithLogger(wsProxyZapLogger{sctx.lg}),
	),
	)
	}
	if handler != nil {
	httpmux.Handle("/", handler)
	}
	return httpmux
	}

and (note: the gwmux returned by registerGateway is passed to createMux as the first parameter)

etcd/server/embed/serve.go

Lines 287 to 339 in 19bf472

	func (sctx *serveCtx) registerGateway(dial func(ctx context.Context) (*grpc.ClientConn, error)) (*gw.ServeMux, error) {
	ctx := sctx.ctx
	conn, err := dial(ctx)
	if err != nil {
	return nil, err
	}
	// Refer to https://grpc-ecosystem.github.io/grpc-gateway/docs/mapping/customizing_your_gateway/
	gwmux := gw.NewServeMux(
	gw.WithMarshalerOption(gw.MIMEWildcard,
	&gw.HTTPBodyMarshaler{
	Marshaler: &gw.JSONPb{
	MarshalOptions: protojson.MarshalOptions{
	UseProtoNames: true,
	EmitUnpopulated: false,
	},
	UnmarshalOptions: protojson.UnmarshalOptions{
	DiscardUnknown: true,
	},
	},
	},
	),
	)
	handlers := []registerHandlerFunc{
	etcdservergw.RegisterKVHandler,
	etcdservergw.RegisterWatchHandler,
	etcdservergw.RegisterLeaseHandler,
	etcdservergw.RegisterClusterHandler,
	etcdservergw.RegisterMaintenanceHandler,
	etcdservergw.RegisterAuthHandler,
	v3lockgw.RegisterLockHandler,
	v3electiongw.RegisterElectionHandler,
	}
	for _, h := range handlers {
	if err := h(ctx, gwmux, conn); err != nil {
	return nil, err
	}
	}
	go func() {
	<-ctx.Done()
	if cerr := conn.Close(); cerr != nil {
	sctx.lg.Warn(
	"failed to close connection",
	zap.String("address", sctx.l.Addr().String()),
	zap.Error(cerr),
	)
	}
	}()
	return gwmux, nil
	}

[enj]

Would you mind share your detailed steps?

Unfortunately I cannot share exploit code. You can see if something like https://github.com/secengjeff/rapidresetclient helps to reproduce the issue better, but the approach you are taking is not sufficient.

It seems there isn't much we can do in etcd. Also Kubernetes only accesses etcd via gRPC instead of HTTP. K8s/kubelet only accesses the /health, /livez and /readyz over HTTP.

You can tear down unauthenticated http2 connections in the same way as kubernetes/kubernetes#121120. This will allow you to keep http2 enabled without being at risk from DOS (the assumption being that an authenticated attacker will be more interested in reading the contents of etcd instead of DOS).

[ahrtr]

if something like https://github.com/secengjeff/rapidresetclient helps

Yes, it's much better now, but it still can't reproduce the situation you mentioned. It could increase the cpu usage to around 600%, and memory to 2GB+ with 100 concurrently connections, and each kept sending HEADER and RST_STREAM.

You can tear down unauthenticated http2 connections in the same way as kubernetes/kubernetes#121120.

If I understood it correctly, K8s depends on the Authorization header to identify unauthenticated connections. Note that K8s doesn't enable etcd's auth, and there is no Authorization header at all.

But etcd supports verify client's certificate. Please add flag --client-cert-auth when starting etcd. Please let me know if you still see the issue.

[enj]

Yes, it's much better now, but it still can't reproduce the situation you mentioned.

A more robust exploit will work far better than that repo. You should assume the worse case is easily 10x-1000x what you see from that tool.

If I understood it correctly, K8s depends on the Authorization header to identify unauthenticated connections.

No, Kube supports many different forms of auth, including cert based auth. But they are all resolved via the http.Handler which is why I injected the connection close logic at that layer.

Note that K8s doesn't enable etcd's auth, and there is no Authorization header at all.

Kube's usage of etcd isn't really relevant because etcd isn't exposed to external users in the Kube scenario.

But etcd supports verify client's certificate. Please add flag --client-cert-auth when starting etcd.

Yeah, this will prevent the issue because it stops the TLS connection from starting in the first place, so no http2 based attack can be attempted. This should be the documented recommendation to limit abuse of http2 for unauthenticated clients. You will have to decide if you want to do anything further to limit authenticated clients (and I do not know if there exist environments where setting --client-cert-auth is not practical).

[ahrtr]

A more robust exploit will work far better than that repo. You should assume the worse case is easily 10x-1000x what you see from that tool.

YES, it also ran out of the CPU & memory after removing the sleep between sending HEADER and RST_STREAM. For anyone reference, this is my rafactored version main.go.

Starting etcd with command below,

./bin/etcd   --cert-file ./tests/fixtures/server2.crt --key-file ./tests/fixtures/server2.key.insecure --listen-client-urls https://127.0.0.1:2379 --advertise-client-urls https://127.0.0.1:2379

Run the DoS test,

./rapidresetclient -delay 0 -con 30

This should be the documented recommendation to limit abuse of http2 for unauthenticated clients.

Agreed. But it should not be a big problem. Usually etcd isn't a public service, In K8s use case, etcd is supposed to be only accessed by apiserver. In Non-K8s use case, etcd should run behind a loadbalancer. But anyway, agree to update doc.

and I do not know if there exist environments where setting --client-cert-auth is not practical

I suppose it (disabling client cert authentication) is only useful in dev or test environment. In TKG, it's enabled by default. @tjungblu @chaochn47 @fuweid @serathius Could you feedback whether Openshift, EKS, AKS and GKE enable --client-cert-auth?

[jmhbnz]

Could you feedback whether Openshift, EKS, AKS and GKE enable --client-cert-auth?

Based on https://github.com/openshift/cluster-etcd-operator/blob/a060517d1452b3756b230de1d1393e3573de4f45/docs/etcd-tls-assets.md?plain=1#L349 yes OpenShift has --client-cert-auth enabled out of the box.

[serathius]

Could you feedback whether Openshift, EKS, AKS and GKE enable --client-cert-auth?

Yes, GKE enables --client-cert-auth.

[fuweid]

Could you feedback whether Openshift, EKS, AKS and GKE enable --client-cert-auth?

Yes, AKS enables --client-cert-auth. As far as I know, the go stdlib enables http2 to https by default.
Ideally, the --peer-client-cert-auth should be true as well.

[ahrtr]

Thanks all for the feedback and help.

This big task should have been resolved.

[ahrtr]

Oops, we haven't bumped grpc-go version yet:(

[ahrtr]

All done!

[ahrtr]

Thanks all again, especially @chaochn47 and @fuweid !