square
Brotli DoS
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
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?
@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 https://github.com/envoyproxy/envoy/commit/d4c39e635603e2f23e1e08ddecf5a5fb5a706338#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
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?
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
- Stop using Brotli - it's an extra optional dependency anyway, not on by default.
- 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.
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
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
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
@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.
OK great they changed it to cpe:2.3:a:squareup:okhttp-brotli:*:*:*:*:*:*:*:* -
https://nvd.nist.gov/vuln/detail/CVE-2023-3782