server: prohibit more than MaxConcurrentStreams handlers from running at once

[dfawley]

This addresses CVE-2023-44487 by not allowing more server handlers to be running than the HTTP/2 MAX_CONCURRENT_STREAMS setting.

This will need to be backported to the last 3 releases.

RELEASE NOTES:

• server: prohibit more than MaxConcurrentStreams handlers from running at once (CVE-2023-44487) -- in addition to this change, applications should ensure they do not leave running tasks behind related to the RPC before returning from method handlers, or should enforce appropriate limits on any such work.

[codecov]

Codecov Report

Merging #6703 (f4e4574) into master (59f57b1) will increase coverage by 0.34%.
Report is 1 commits behind head on master.
The diff coverage is 81.08%.

Additional details and impacted files

[dfawley]

Forcing merge because Mergeable isn't running for some reason.

[dfawley]

Where do you limit that (I could not find it)? AFAIK, the block from Serve seems to create a new goroutine for each new connection:

Here

https://github.com/grpc/grpc-go/pull/6703/files#diff-366e46a40f6f60b4f7614eb0976bb51820364bf5ca6ccc4787eb49d7bdbef3e6R982

You're pointing at per-connection logic, not per-method.

I updated the attacker to use multiple connections

The vulnerability in question is regarding a single connection.

If you want to limit connections, you can use something like https://pkg.go.dev/golang.org/x/net/netutil#LimitListener

[ReToCode]

Yes the attack is on a single connection. I guess what I'm trying to say is, using this attack, a malicious client can create a lot of load on a single connection (not enough to bring the server down, though). But a client (or multiple) can also just use more than one connection and do the same on every connection without hitting the connection or maxConcurrentStream limits.

To be more specific, using the attack, we never get to https://github.com/grpc/grpc-go/pull/6703/files#diff-366e46a40f6f60b4f7614eb0976bb51820364bf5ca6ccc4787eb49d7bdbef3e6R982 (as the connections are already reset inside the st.HandleStreams function. But this still causes a lot of goroutines to be spawned (at the handleRawConn and also at serveStreams and puts the server under some load).

To summarize, because of

We don't close connections due to RST_STREAM operations; we just limit the active worker goroutines.

I think there is still some attack vector open for a server created with grpc-go compared to a vanilla golang server. There probably should be some protection mechanisms around that, even if requests are not valid GRPC requests.

[dfawley]

Two things:

1. You're not looking at the client's resources spent in order to incur these costs. When I run your tests, I see 200% CPU for the server binary but also 177% CPU for the attacker binary.
2. You have a sleep in the go http/2 server's handler, but not in the grpc server's method handlers. This hardly seems fair.

The go HTTP server will kill a connection that queues up RPCs at a ratio of 4:1. gRPC-Go does not have this type of protection. Instead, if there is any stream queued, we stop reading from the connection until a method handler returns to allow the stream to be processed. As a result, you will not see gRPC-Go kill connections due to this kind of attack.

The go heuristic (fixed 4:1 ratio) has some potential weaknesses, in that it could detect a responsible client as a malicious one, especially with low values of MAX_CONCURRENT_STREAMS (e.g. 1) and while under heavy server load. This could cause cascading problems, though the value (4x) does seem pretty reasonable and such problems are probably very unlikely.

[ReToCode]

Thanks for your reply. I do agree with your summary 👍 Just some notes for the two bullet points for completeness of the discussion:

1. I'm aware of the CPU overhead the client attack has. I was just looking at Mitigate http/2 attacks by authenticated clients kubernetes/kubernetes#121197 (comment) which indicates that there are possibly better attack methods than the one I used, which would shift the load more to the server-side. But I agree with you that the whole attack is a balance between resources of client + server.

2. You are right about that. The sleep was only there to proof a point. As attackers calls are not valid GRPC calls, there was no comparable way to add the same to the GRPC server without modifying the GRPC source.

[dfawley]

You are right about that. The sleep was only there to proof a point. As attackers calls are not valid GRPC calls, there was no comparable way to add the same to the GRPC server without modifying the GRPC source.

True, if you are calling a method that does not exist, that is the case. You could also call a real method that is registered on the server, but obviously an attacker will pick whatever method works best.

There is still more we can do here, that is not directly related to this vulnerability, and I hope we can prioritize some of that work soon.