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

Brotli DoS #7738

Open
swankjesse opened this issue Mar 7, 2023 · 10 comments · Fixed by #8227
Open

Brotli DoS #7738

swankjesse opened this issue Mar 7, 2023 · 10 comments · Fixed by #8227
Labels
bug Bug in existing code

Comments

@swankjesse
Copy link
Member

swankjesse commented Mar 7, 2023

From a Bugcrowd submission (Block-internal link, Block-internal Bugcrowd link) we’ve received, OkHttp is vulnerable to a Brotli decompression bomb DoS.

Receive a request body like this one can exhaust OkHttp:
https://github.com/bones-codes/bombs/raw/master/http/10GB/10GB.html.br.bz2

@swankjesse swankjesse added the bug Bug in existing code label Mar 7, 2023
@yschimke
Copy link
Collaborator

yschimke commented Mar 7, 2023

There isn't an easy API for BrotliInputStream to enforce this, and it's hard to ask users to do it.

https://github.com/square/okhttp/blob/master/okhttp-brotli/src/main/kotlin/okhttp3/brotli/internal/brotli.kt

We could change BrotliInterceptor (deprecate and add) to an instance class, with some reasonable default maximum size, or some ratio compared to compressed.

object BrotliInterceptor : Interceptor {

Any good patterns suggested?

@yschimke
Copy link
Collaborator

@swankjesse I'm tempted to fix in Okio via some DecompressionLimit(ratio: Float?, limit: Long?) type, and insert around and inside (if ratio is applied).

Apply to GzipSource and a new BrotliSource.

Also make it possible to create BrotliInterceptor instances and pass in a non default DecompressionLimit, but put something sensible but tolerant in for the object form.

Is it worth contributing the a Brotli Source to okio?

Envoy has some 100x ratio check envoyproxy/envoy@d4c39e6#diff-88b327a1e72d55d1bb686b3b1f28f594b6b08139968304e6804a808fbb375ff0R26

Another library has max size https://github.com/python-pillow/Pillow/pull/674/files, same for golang? https://stackoverflow.com/questions/56629115/how-to-protect-service-from-gzip-bomb/56629623#56629623

@srmish-jfrog
Copy link

Hello, as per our disclosure policy, more than 120 days have passed and we plan to disclosure this issue publicly. Can you please share if this issue was fixed in some version of OkHttp so that we may refer to the fixed version in our disclosure?

@yschimke
Copy link
Collaborator

No current fix is in place in OkHttp or Okio.

cc @swankjesse do you want to move quickly, or accept as a risk since clients are connecting to known servers and deliberately opting into Brotli.

The current options would be

  1. Stop using Brotli - it's an extra optional dependency anyway, not on by default.
  2. Ensure you only use for trusted servers.

@srmish-jfrog can you ensure it only flags with the okhttp-brotli library? This is already public, so it's mainly about automated tools flagging this.

@srmish-jfrog
Copy link

To be clear, we don't want to cause trouble and highlight an issue if there is no fix yet and you are planning to fix the issue.
I'm trying to understand if this is something you are planning to fix, or if we should just document this issue in its current status and be done with it.

In any case, of course we can flag it with the specific library you requested

@yschimke
Copy link
Collaborator

yschimke commented Jul 17, 2023

It's no trouble for us. It's a real issue, we'll probably have to change API to address, so it won't happen this week. Apps can remove the dependency and copy the very small implementation if they have a clear strategy. Or just remove the dependency.

Please mark it against this dependency instead of the entire project as this is optional.

com.squareup.okhttp3:okhttp-brotli:4.11.0

@barchetta
Copy link

barchetta commented Jul 28, 2023

Update: the CVE has been corrected and the cpe now states squareup:okhttp-brotli

@srmish-jfrog The NVD is currently identifying this against squareup:okhttp:*:*:*:*:*:*:*:* so it is getting flagged by scans by any use of okhttp. Would save a lot of headaches across a lot of projects if this could be narrowed to okhttp-brotli.

https://nvd.nist.gov/vuln/detail/CVE-2023-3782

@srmish-jfrog
Copy link

@barchetta I don't understand why this happened, I specifically wrote "com.squareup.okhttp3:okhttp-brotli" both in the CVE JSON and our reference page.
I will ask them to change it right now, sorry about this.

@srmish-jfrog
Copy link

srmish-jfrog commented Aug 3, 2023

OK great they changed it to cpe:2.3:a:squareup:okhttp-brotli:*:*:*:*:*:*:*:* -
https://nvd.nist.gov/vuln/detail/CVE-2023-3782

swankjesse added a commit that referenced this issue Jan 29, 2024
This uses the same ratio technique as Envoy. It's a quick hack
and it gets out of the way.

Closes: #7738
swankjesse added a commit that referenced this issue Jan 29, 2024
This uses the same ratio technique as Envoy. It's a quick hack
and it gets out of the way.

Closes: #7738
swankjesse added a commit that referenced this issue Jan 29, 2024
* Defend against brotli decompression bombs

This uses the same ratio technique as Envoy. It's a quick hack
and it gets out of the way.

Closes: #7738

* Code review feedback
@yschimke
Copy link
Collaborator

Reverting in #8229

@yschimke yschimke reopened this Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug in existing code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants