Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: limit the amount of pending-accept reset streams #668

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

seanmonstar
Copy link
Member

Streams that have been received by the peer, but not accepted by the user, can also receive a RST_STREAM. This is a legitimate pattern: one could send a request and then shortly after, realize it is not needed, sending a CANCEL.

However, since those streams are now "closed", they don't count towards the max concurrent streams. So, they will sit in the accept queue, using memory.

In most cases, the user is calling accept in a loop, and they can accept requests that have been reset fast enough that this isn't an issue in practice.

But if the peer is able to flood the network faster than the server accept loop can run (simply accepting, not processing requests; that tends to happen in a separate task), the memory could grow.

So, this introduces a maximum count for streams in the pending-accept but remotely-reset state. If the maximum is reached, a GOAWAY frame with the error code of ENHANCE_YOUR_CALM is sent, and the connection marks itself as errored.

Closes hyperium/hyper#2877 (we assume)

@seanmonstar
Copy link
Member Author

I started to make the limit configurable, but then paused, wanting to get something working faster. If it seems worth the delay, I can add in the public API to set these limits yourself.

@LPardue
Copy link
Contributor

LPardue commented Apr 13, 2023

Since this seems like something that occurs due to several dynamic factors or behaviours, a single hard coded limit on paper seems too restrictive. Not least because it isn't immediately obvious where the value 20 comes from. This might be an entirely reasonable value but without some wider testing, who knows? A default value that can be tweaked via API seems like a decent middleground to me.

Copy link
Contributor

@Noah-Kennedy Noah-Kennedy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of merging this now as is, but we should in the future make the threshold configurable.

@seanmonstar
Copy link
Member Author

Added another commit making the options part of the public API.

src/client.rs Outdated Show resolved Hide resolved
@Noah-Kennedy
Copy link
Contributor

I'm summarizing a conversation @seanmonstar and I had in Discord here:

There is a non-zero chance that the threshold we set does trip on real (and non-malicious) traffic. We have a log line in here for if this goes off, but it might be worthwhile for us to expose an actual metric for these GOAWAYs so that users of the library can better understand how frequently this trips.

@seanmonstar
Copy link
Member Author

Given that I've been at this for 14 hours, I'm going to go knock out. Since the assumed problem is quite hard to trigger, and that the fix here is a guess, and that there's always some amount of risk when publishing new versions, I'm opting for leaving this open for the night.

@Noah-Kennedy
Copy link
Contributor

This will also give our peers outside of North America a chance to look at this change before we release it.

If anyone has any concerns about this change, please make them heard.

Copy link
Contributor

@nox nox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with one ask.

src/proto/streams/recv.rs Show resolved Hide resolved
Streams that have been received by the peer, but not accepted by the
user, can also receive a RST_STREAM. This is a legitimate pattern: one
could send a request and then shortly after, realize it is not needed,
sending a CANCEL.

However, since those streams are now "closed", they don't count towards
the max concurrent streams. So, they will sit in the accept queue, using
memory.

In most cases, the user is calling `accept` in a loop, and they can
accept requests that have been reset fast enough that this isn't an
issue in practice.

But if the peer is able to flood the network faster than the server
accept loop can run (simply accepting, not processing requests; that
tends to happen in a separate task), the memory could grow.

So, this introduces a maximum count for streams in the pending-accept
but remotely-reset state. If the maximum is reached, a GOAWAY frame with
the error code of ENHANCE_YOUR_CALM is sent, and the connection marks
itself as errored.

ref CVE-2023-26964
ref GHSA-f8vr-r385-rh5r

Closes hyperium/hyper#2877
The new option is available to both client and server `Builder`s.
@seanmonstar seanmonstar merged commit d3f37e9 into master Apr 13, 2023
@seanmonstar seanmonstar deleted the sean/reset-streams-growing branch April 13, 2023 14:15
Noah-Kennedy added a commit to Noah-Kennedy/hyper that referenced this pull request Apr 13, 2023
This allows users to set the configuration option from hyperium/h2#668.
seanmonstar pushed a commit to hyperium/hyper that referenced this pull request Apr 13, 2023
…ion (#3201)

This allows users to set the configuration option from hyperium/h2#668.
jiangliu added a commit to jiangliu/image-service that referenced this pull request Apr 21, 2023
error[vulnerability]: Resource exhaustion vulnerability in h2 may lead to Denial of Service (DoS)
   ┌─ /github/workspace/Cargo.lock:68:1
   │
68 │ h2 0.3.13 registry+https://github.com/rust-lang/crates.io-index
   │ --------------------------------------------------------------- security vulnerability detected
   │
   = ID: RUSTSEC-2023-0034
   = Advisory: https://rustsec.org/advisories/RUSTSEC-2023-0034
   = If an attacker is able to flood the network with pairs of `HEADERS`/`RST_STREAM` frames, such that the `h2` application is not able to accept them faster than the bytes are received, the pending accept queue can grow in memory usage. Being able to do this consistently can result in excessive memory use, and eventually trigger Out Of Memory.

     This flaw is corrected in [hyperium/h2#668](hyperium/h2#668), which restricts remote reset stream count by default.

Signed-off-by: Jiang Liu <[email protected]>
jiangliu added a commit to jiangliu/image-service that referenced this pull request Apr 21, 2023
error[vulnerability]: Resource exhaustion vulnerability in h2 may lead to Denial of Service (DoS)
   ┌─ /github/workspace/Cargo.lock:68:1
   │
68 │ h2 0.3.13 registry+https://github.com/rust-lang/crates.io-index
   │ --------------------------------------------------------------- security vulnerability detected
   │
   = ID: RUSTSEC-2023-0034
   = Advisory: https://rustsec.org/advisories/RUSTSEC-2023-0034
   = If an attacker is able to flood the network with pairs of `HEADERS`/`RST_STREAM` frames, such that the `h2` application is not able to accept them faster than the bytes are received, the pending accept queue can grow in memory usage. Being able to do this consistently can result in excessive memory use, and eventually trigger Out Of Memory.

     This flaw is corrected in [hyperium/h2#668](hyperium/h2#668), which restricts remote reset stream count by default.

Signed-off-by: Jiang Liu <[email protected]>
jiangliu added a commit to jiangliu/image-service that referenced this pull request Apr 22, 2023
error[vulnerability]: Resource exhaustion vulnerability in h2 may lead to Denial of Service (DoS)
   ┌─ /github/workspace/Cargo.lock:68:1
   │
68 │ h2 0.3.13 registry+https://github.com/rust-lang/crates.io-index
   │ --------------------------------------------------------------- security vulnerability detected
   │
   = ID: RUSTSEC-2023-0034
   = Advisory: https://rustsec.org/advisories/RUSTSEC-2023-0034
   = If an attacker is able to flood the network with pairs of `HEADERS`/`RST_STREAM` frames, such that the `h2` application is not able to accept them faster than the bytes are received, the pending accept queue can grow in memory usage. Being able to do this consistently can result in excessive memory use, and eventually trigger Out Of Memory.

     This flaw is corrected in [hyperium/h2#668](hyperium/h2#668), which restricts remote reset stream count by default.

Signed-off-by: Jiang Liu <[email protected]>
jiangliu added a commit to jiangliu/image-service that referenced this pull request Apr 22, 2023
error[vulnerability]: Resource exhaustion vulnerability in h2 may lead to Denial of Service (DoS)
   ┌─ /github/workspace/Cargo.lock:68:1
   │
68 │ h2 0.3.13 registry+https://github.com/rust-lang/crates.io-index
   │ --------------------------------------------------------------- security vulnerability detected
   │
   = ID: RUSTSEC-2023-0034
   = Advisory: https://rustsec.org/advisories/RUSTSEC-2023-0034
   = If an attacker is able to flood the network with pairs of `HEADERS`/`RST_STREAM` frames, such that the `h2` application is not able to accept them faster than the bytes are received, the pending accept queue can grow in memory usage. Being able to do this consistently can result in excessive memory use, and eventually trigger Out Of Memory.

     This flaw is corrected in [hyperium/h2#668](hyperium/h2#668), which restricts remote reset stream count by default.

Signed-off-by: Jiang Liu <[email protected]>
imeoer pushed a commit to dragonflyoss/nydus that referenced this pull request Apr 23, 2023
error[vulnerability]: Resource exhaustion vulnerability in h2 may lead to Denial of Service (DoS)
   ┌─ /github/workspace/Cargo.lock:68:1
   │
68 │ h2 0.3.13 registry+https://github.com/rust-lang/crates.io-index
   │ --------------------------------------------------------------- security vulnerability detected
   │
   = ID: RUSTSEC-2023-0034
   = Advisory: https://rustsec.org/advisories/RUSTSEC-2023-0034
   = If an attacker is able to flood the network with pairs of `HEADERS`/`RST_STREAM` frames, such that the `h2` application is not able to accept them faster than the bytes are received, the pending accept queue can grow in memory usage. Being able to do this consistently can result in excessive memory use, and eventually trigger Out Of Memory.

     This flaw is corrected in [hyperium/h2#668](hyperium/h2#668), which restricts remote reset stream count by default.

Signed-off-by: Jiang Liu <[email protected]>
mofishzz pushed a commit to mofishzz/image-service that referenced this pull request Jun 2, 2023
error[vulnerability]: Resource exhaustion vulnerability in h2 may lead to Denial of Service (DoS)
   ┌─ /github/workspace/Cargo.lock:68:1
   │
68 │ h2 0.3.13 registry+https://github.com/rust-lang/crates.io-index
   │ --------------------------------------------------------------- security vulnerability detected
   │
   = ID: RUSTSEC-2023-0034
   = Advisory: https://rustsec.org/advisories/RUSTSEC-2023-0034
   = If an attacker is able to flood the network with pairs of `HEADERS`/`RST_STREAM` frames, such that the `h2` application is not able to accept them faster than the bytes are received, the pending accept queue can grow in memory usage. Being able to do this consistently can result in excessive memory use, and eventually trigger Out Of Memory.

     This flaw is corrected in [hyperium/h2#668](hyperium/h2#668), which restricts remote reset stream count by default.

Signed-off-by: Jiang Liu <[email protected]>
jiangliu added a commit to dragonflyoss/nydus that referenced this pull request Jun 12, 2023
error[vulnerability]: Resource exhaustion vulnerability in h2 may lead to Denial of Service (DoS)
   ┌─ /github/workspace/Cargo.lock:68:1
   │
68 │ h2 0.3.13 registry+https://github.com/rust-lang/crates.io-index
   │ --------------------------------------------------------------- security vulnerability detected
   │
   = ID: RUSTSEC-2023-0034
   = Advisory: https://rustsec.org/advisories/RUSTSEC-2023-0034
   = If an attacker is able to flood the network with pairs of `HEADERS`/`RST_STREAM` frames, such that the `h2` application is not able to accept them faster than the bytes are received, the pending accept queue can grow in memory usage. Being able to do this consistently can result in excessive memory use, and eventually trigger Out Of Memory.

     This flaw is corrected in [hyperium/h2#668](hyperium/h2#668), which restricts remote reset stream count by default.

Signed-off-by: Jiang Liu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants