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

denial of service with long HTTP request header #376

Closed
rivkasegan opened this issue Sep 18, 2023 · 4 comments
Closed

denial of service with long HTTP request header #376

rivkasegan opened this issue Sep 18, 2023 · 4 comments
Assignees
Labels

Comments

@rivkasegan
Copy link

I work on an application that depends on tungstenite and is intended to offer server-side ws:// support to untrusted clients over the public Internet. There are hundreds of instances of the server application operated independently by our community members, and it's not feasible to have other devices (e.g., web application firewalls) protect them. We want to avoid situations where a small number of malicious HTTP requests can devour server CPU resources.

I'm seeing that a single HTTP request (i.e., before the "upgrade: websocket" happens) with any long header can cause request processing to take several minutes or more. For example, testing on a low-cost Ubuntu 23.04 VPS as the server, if the header is 20 million characters, there's more than 99% CPU consumption for five minutes. Similar results have been seen by multiple persons on various Linux systems with tungstenite 0.20.0. Relative to this tungstenite source code

pub fn single_round<Obj: TryParse>(mut self) -> Result<RoundResult<Obj, Stream>> {
trace!("Doing handshake round.");
match self.state {
HandshakeState::Reading(mut buf) => {
let read = buf.read_from(&mut self.stream).no_block()?;
match read {
Some(0) => Err(Error::Protocol(ProtocolError::HandshakeIncomplete)),
Some(_) => Ok(if let Some((size, obj)) = Obj::try_parse(Buf::chunk(&buf))? {
buf.advance(size);
RoundResult::StageFinished(StageResult::DoneReading {
result: obj,
stream: self.stream,
tail: buf.into_vec(),
})
} else {
RoundResult::Incomplete(HandshakeMachine {
I'm seeing more than 4000 calls each to single_round, try_parse, and RoundResult::Incomplete. Looking at buf.len() here
fn try_parse(buf: &[u8]) -> Result<Option<(usize, Self)>> {
let mut hbuffer = [httparse::EMPTY_HEADER; MAX_HEADERS];
I see a value between 100 and 200 on the first call, and the observed value gradually increases until it gets to 20 million after more than 4000 calls, five minutes later. At that point, a client that sent all the required headers gets an "HTTP/1.1 101 Switching Protocols" response with connection, upgrade, and sec-websocket-accept response headers. (If the client didn't send the required request headers, for example sending only My-Long-Header: followed by 20 million characters, the five minutes of CPU time still happens but of course "Switching Protocols" isn't allowed by tungstenite.)

We're able to work around this by checking for a large total header size (and dropping the client's connection) before any of tungstenite's code is called. However, maybe many other crates that depend on tungstenite could experience excessive CPU consumption if people can send long HTTP request headers.

The question is: should this issue be resolved within tungstenite, e.g., by rejecting long header lines (maybe in a configurable way) sooner, by avoiding calls to try_parse until a complete header line (ending with \n) is read, or by making some other change?

I don't think the issue can be attributed to the httparse crate - RFC 7230 3.2.5 allows unlimited header sizes, but tungstenite (in its role as code to implement a server) could choose an upper bound.

Is this a vulnerability in tungstenite, or is tungstenite simply not intended to remain performant when a header has millions of characters?

@agalakhov
Copy link
Member

agalakhov commented Sep 18, 2023

This is definitely a vulnerability, thank you for pointing at it. Too long requests have to be rejected early without trying to parse them.

@carnil
Copy link

carnil commented Sep 21, 2023

This appears to have CVE-2023-43669 assigned.

@agalakhov agalakhov added the bug label Sep 23, 2023
@agalakhov agalakhov self-assigned this Sep 23, 2023
@agalakhov
Copy link
Member

Should be fixed in Tungstenite 0.20.1, please verify.

@rivkasegan
Copy link
Author

thank you! in my application, there's no longer excessive CPU consumption: when attempted, the outcome of check_incoming_packet_size was the first Err(Error::AttackAttempt) with 68232 bytes and 31 packets; the many legitimate users are fine with setting up their WebSocket connections and it's 159 bytes, 1 packet, Ok(())

wez pushed a commit to KumoCorp/kumomta that referenced this issue Sep 25, 2023
I'm not sure if that actually affected us in practice due to
our authentication requirement on that endpoint.

refs: snapview/tungstenite-rs#376
This was referenced Sep 30, 2023
hnez added a commit to hnez/tacd that referenced this issue Oct 4, 2023
See the corresponding tungstenite issue[1] for reference.

[1]: snapview/tungstenite-rs#376

Signed-off-by: Leonard Göhrs <l.goehrs@pengutronix.de>
hnez added a commit to hnez/tacd that referenced this issue Nov 3, 2023
This at least gets rid of one advisory and a new tungstenite issue
(RUSTSEC-2023-0065).

See the corresponding tungstenite issue[1] for reference.

[1]: snapview/tungstenite-rs#376

Signed-off-by: Leonard Göhrs <l.goehrs@pengutronix.de>
gitlab-dfinity pushed a commit to dfinity/ic that referenced this issue Dec 28, 2023
fix: fix multiple advisory warnings and 1 error found by cargo-deny

Openssl is removed (again) from Cargo.toml.

The following warnings are removed from the repository.
```
error[xxx]: Tungstenite allows remote attackers to cause a denial of service
     ┌─ /ic/Cargo.lock:1331:1
     │
1331 │ tungstenite 0.17.3 registry+https://github.com/rust-lang/crates.io-index
     │ ------------------------------------------------------------------------ security xxx detected
     │
     = ID: RUSTSEC-2023-0065
     = Advisory: https://rustsec.org/advisories/RUSTSEC-2023-0065
     = The Tungstenite crate through 0.20.0 for Rust allows remote attackers to cause
       a denial of service (minutes of CPU consumption) via an excessive length of an
       HTTP header in a client handshake. The length affects both how many times a parse
       is attempted (e.g., thousands of times) and the average amount of data for each
       parse attempt (e.g., millions of bytes).
     = Announcement: snapview/tungstenite-rs#376
     = Solution: Upgrade to >=0.20.1 (try `cargo update -p tungstenite`)


warning[unmaintained]: difference is unmaintained
    ┌─ /ic/Cargo.lock:267:1
    │
267 │ difference 2.0.0 registry+https://github.com/rust-lang/crates.io-index
    │ ---------------------------------------------------------------------- unmaintained advisory detected
    │
    = ID: RUSTSEC-2020-0095
    = Advisory: https://rustsec.org/advisories/RUSTSEC-2020-0095
    = The author of the `difference` crate is unresponsive.
      
      Maintained alternatives:
      
      - [`dissimilar`](https://crates.io/crates/dissimilar)
      
      - [`similar`](https://crates.io/crates/similar)
      
      - [`treediff`](https://crates.io/crates/treediff)
      
      - [`diffus`](https://crates.io/crates/diffus)
    = Announcement: johannhof/difference.rs#45
    = Solution: No safe upgrade is available!


warning[unsound]: Unaligned write of u64 on 32-bit and 16-bit platforms
     ┌─ /ic/Cargo.lock:1355:1
     │
1355 │ unsafe-libyaml 0.2.9 registry+https://github.com/rust-lang/crates.io-index
     │ -------------------------------------------------------------------------- unsound advisory detected
     │
     = ID: RUSTSEC-2023-0075
     = Advisory: https://rustsec.org/advisories/RUSTSEC-2023-0075
     = Affected versions allocate memory using the alignment of `usize` and write data
       to it of type `u64`, without using `core::ptr::write_unaligned`. In platforms
       with sub-64bit alignment for `usize` (including wasm32 and x86) these writes
       are insufficiently aligned some of the time.
       
       If using an ordinary optimized standard library, the bug exhibits Undefined
       Behavior so may or may not behave in any sensible way, depending on
       optimization settings and hardware and other things. If using a Rust standard
       library built with debug assertions enabled, the bug manifests deterministically
       in a crash (non-unwinding panic) saying _"ptr::write requires that the pointer
       argument is aligned and non-null"_.
       
       No 64-bit platform is impacted by the bug.
       
       The flaw was corrected by allocating with adequately high alignment on all
       platforms.
     = Announcement: dtolnay/unsafe-libyaml#21
     = Solution: Upgrade to >=0.2.10 (try `cargo update -p unsafe-libyaml`)



warning[yanked]: detected yanked crate (try `cargo update -p ahash`)
   ┌─ /ic/Cargo.lock:20:1
   │
20 │ ahash 0.7.6 registry+https://github.com/rust-lang/crates.io-index
   │ ----------------------------------------------------------------- yanked version

warning[yanked]: detected yanked crate (try `cargo update -p ahash`)
   ┌─ /ic/Cargo.lock:21:1
   │
21 │ ahash 0.8.3 registry+https://github.com/rust-lang/crates.io-index
   │ ----------------------------------------------------------------- yanked version

warning[yanked]: detected yanked crate (try `cargo update -p hermit-abi`)
    ┌─ /ic/Cargo.lock:385:1
    │
385 │ hermit-abi 0.3.1 registry+https://github.com/rust-lang/crates.io-index
    │ ---------------------------------------------------------------------- yanked version
``` 

See merge request dfinity-lab/public/ic!16899
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants