-
Notifications
You must be signed in to change notification settings - Fork 377
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
gzip: Heal-the-BREACH mitigation #4131
base: master
Are you sure you want to change the base?
Conversation
This is lacking test coverage in a sub-request, it should probably be limited to the top one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First and foremost: Thank you for this addition, it would be nice to see this smart idea land in varnish-cache.
On the implementation, I have some questions:
- Is it a good idea to pad with
1 .. BREACH_PAD
? - The original paper uses a maximum of
HTB = 100
, isBREACH_PAD == 256
a good idea? - Calling
VRND_RandomTestable()
for each byte looks like significant overhead. Did you consider other options like seeding AES? - 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? - 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?)
if (len <= 0) | ||
return (len); | ||
|
||
if (br->skip > len) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this not be >=
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
This is how we get the variable length.
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.
I picked what we have to offer in
Yes, I was bothered by that. We could maybe reserve storage in the workspace to avoid this flush.
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. |
bugwash: we should also teach HTB to the gzip VFP for cache misses and passes, and evaluate whether this is appropriate for cache hits. |
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 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 |
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. |
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