[chttp2] Rollup of fixes for CVE-2023-44487 #34763
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Really minimal change to make the output buffer for chttp2 be a `grpc_core::SliceBuffer` so that we can start mixing in the new framer code. --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>
Isolate ping callback tracking to its own file. Also takes the opportunity to simplify keepalive code by applying the ping timeout to all pings. Adds an experiment to allow multiple pings outstanding too (this was originally an accidental behavior change of the work, but one that I think may be useful going forward). --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>
Force http/2 rapid reset attacks to read data by randomly sending a ping on some percentage of reset streams received at the server.
Previously chttp2 would allow infinite requests prior to a settings ack - as the agreed upon limit for requests in that state is infinite. Instead, after MAX_CONCURRENT_STREAMS requests have been attempted, start blanket cancelling requests until the settings ack is received. This can be done efficiently without allocating request state structures.
Cap requests per read, rst_stream handled per read. If these caps are exceeded, offload processing of the connection to a backing thread pool, and allow other connections to make progress.
Previously our settings changes carried no timeout, fix that. Default timeout starts at keepalive_timeout*2.
If a request is invalid, take a random amount of time before sending the RST_STREAM, so that MAX_CONCURRENT_STREAMS remaining becomes unpredictable.
Experiment 1: On RST_STREAM: reduce MAX_CONCURRENT_STREAMS for one round trip. Experiment 2: If a settings frame is outstanding with a lower MAX_CONCURRENT_STREAMS than is configured, and we receive a new incoming stream that would exceed the new cap, randomly reject it. --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>
…c#34589) We probably shouldn't count the time it takes us to write out data as part of the ping timeout
The `TickForDuration()` method was using `grpc_core::Timestamp::Now()` to get the current time, but that was not in sync with the `now_` value inside the Fuzzing EE itself, with the result that after two subsequent 250ms increments, timers were not being properly fired. I've added a test that demonstrates this failure without the fix.
Fix b/304114403 - adds a new experimental tracer useful for diagnosing ping timeout failures in unit tests - adds a pair of experimental tracers for fuzzing event engine - fix the behavior of FuzzingEventEngine so that a RunAfter(0, ...) runs in the same tick - up the rate of sends (reduce the send delay) so we guarantee to be able to send 200kb/sec in fuzzed e2e unit tests --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>
…outs (grpc#34647) Just seeing data flowing in after a ping is enough to establish liveness of a connection, and so we can limit keepalive timeouts to that. Ping timeouts are necessary for protocol correctness, but may be stuck behind other traffic, so give them a little more of a grace period. --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>
…34665) Instead of fixing a target size for writes, try to adapt it a little to observed bandwidth. The initial algorithm tries to get large writes within 100-1000ms maximum delay - this range probably wants to be tuned, but let's see. The hope here is that on slow connections we can not back buffer so much and so when we need to send a ping-ack it's possible without great delay.
…rpc#34697) Cancel streams if we have too much concurrency on a single channel to allow that server to recover. There seems to be a convergence in the HTTP2 community about this being a reasonable thing to do, so I'd like to try it in some real scenarios. If this pans out well then I'll likely drop the `red_max_concurrent_streams` and the `rstpit` experiments in preference to this. I'm also considering tying in resource quota so that under high memory pressure we just default to this path. --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>
ctiller
requested review from
jtattermusch,
veblush and
gnossen
as code owners
ctiller
requested review from
yashykt
and removed request for
gnossen and
jtattermusch
grpc-checks
bot
added
per-call-memory/neutral
per-channel-memory/neutral
bloat/medium
labels
veblush
approved these changes
Oct 20, 2023
gnossen
added
the
release notes: yes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.