search

varnishcache / varnish-cache

Varnish Cache source code repository
https://www.varnish-cache.org
Other
3.65k stars 374 forks source link

gzip: Heal-the-BREACH mitigation #4131

Open dridi opened 3 months ago

dridi commented 3 months ago

Having the ability to add noise to compressed cache hits was requested a couple times, and after my recent foray into zlib territory I decided to tackle it.

https://en.wikipedia.org/wiki/BREACH

dridi commented 3 months ago

This is lacking test coverage in a sub-request, it should probably be limited to the top one.

dridi commented 3 months ago

The commit message of 724293b013470b6c604d4ff0464ac9a448814f98 says "experimental::gzip_breach" but I meant "feature::gzip_breach". I confused it with the experimental flag introduced in #3873.

dridi commented 3 months ago

Is it a good idea to pad with 1 .. BREACH_PAD?

This is how we get the variable length.

The original paper uses a maximum of HTB = 100, is BREACH_PAD == 256 a good idea?

My understanding was that 100 forced a high enough number of requests to perform the attack. I didn't remember seeing advice against going higher. No objection on picking 100, I simply went with a round number.

Calling VRND_RandomTestable() for each byte looks like significant overhead. Did you consider other options like seeding AES?

I picked what we have to offer in vrnd.h, and I wanted something that would allow test coverage, but I'm open to new solutions.

I wonder if the VDP_PUSH of the gzip header could somehow weaken the mitigation? In other words, do we leak a side channel by informing eve of where the actual data starts?

Yes, I was bothered by that. We could maybe reserve storage in the workspace to avoid this flush.

Similarly, do we leak a timing side channel by generating bytes inline (should we do this upfront?) and depending on the random pad length (should we maybe always generate BREACH_PAD bytes and just send a fraction?)

I guess once we have pre-allocated that space we can avoid that. The problem with the comment is that it needs to be null-terminated. We could of course generate gzip headers with a comment of 0..MAX bytes (plus null character) ahead of time and just randomly pick from that array. It could even be at compile time, no opinion.

dridi commented 3 months ago

bugwash: we should also teach HTB to the gzip VFP for cache misses and passes, and evaluate whether this is appropriate for cache hits.

dridi commented 2 months ago

I revisited the patch series to leave it in a better state in my absence. The mitigation is present in both VFP and VDP, but for a cache miss yielding a beresp gzipped by the backend, we may not have access to OA_GZIPBITS immediately.

The VDP is no longer requiring a flush after the gzip header's random comment, the comment finds stable storage inside the task's workspace.

I did not address the points on constant-time operations or pseudo-random algorithm. We could very well just make a copy of a predefined BREACH_PAD-long string and stick a null character at a random position. I will leave that change to reviewers. :grin:

dridi commented 2 months ago

I forgot to link to the paper author's mitigation:

https://github.com/iit-asi/PAPER-Heal-the-Breach/blob/main/gzip-randomizer.c

It should be noted that when I tried it way back then I ended up with invalid gzip.