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

Http: added HTTP header with random length to complicate BREACH attack #1379

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fabik
Copy link
Contributor

@fabik fabik commented Jan 26, 2014

Here's a nice demonstration of BREACH attack: http://resources.infosecinstitute.com/the-breach-attack/

@JanTvrdik
Copy link
Contributor

Corresponding Symfony issue for reference symfony/symfony#8682

@@ -230,12 +230,11 @@ public static function date($time = NULL)
*/
public function __destruct()
{
if (self::$fixIE && isset($_SERVER['HTTP_USER_AGENT']) && strpos($_SERVER['HTTP_USER_AGENT'], 'MSIE ') !== FALSE
&& in_array($this->code, array(400, 403, 404, 405, 406, 408, 409, 410, 500, 501, 505), TRUE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it does not only apply to IE and pages with error status code, but to all browsers and all status codes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Majkl means someone could want disable the breach protection but enable the IE fix. I don't think it's necessary to have both though. According to the comment above it only applies to IE6, I think we can drop support for such old version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's not possible to turn off the protection, because the variable Response::$preventBreachAttack is private. It just prevents sending of the invisible garbage more than once.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really @enumag. I understand IE, I was asking about status codes only. :) But yes, explicit disabling might be good in some cases.

@spaze
Copy link

spaze commented Jan 28, 2014

Just for the record, this approach might kill some HTTP caches on the way, for the two guys out there still caring about them. Is also listed as a second least effective mitigation (out of 7), just sayin'

Also, even the guys themselves say that "Unfortunately, we are unaware of a clean, effective, practical solution to the problem.", so making this a default might not be as great idea as it might seem.

My 2 cents.

@dg
Copy link
Member

dg commented Jan 28, 2014

This is of course not intended for „prevent BREACH attack“ just for „complicate“. Subject is incorrect. And I don't like it much, better should be send HTTP header with random length.

@spaze
Copy link

spaze commented Jan 28, 2014

That's correct, length hiding makes the attack just take longer:

While this measure does make the attack take longer, it does so only slightly.
The countermeasure requires the attacker to issue more requests, and measure the
sizes of more responses, but not enough to make the attack infeasible.

the BREACH paper

@fabik
Copy link
Contributor Author

fabik commented Jan 28, 2014

I rewrited it, so it just sends a header of a random length and on HTTPS only. That should be good, shouldn't it?

@dg
Copy link
Member

dg commented Jan 28, 2014

Better is str_repeat. And length should not be random for the same requests.

@pascal-hofmann
Copy link

With HTTP compression HTTP headers are not compressed, only the HTTP body is. So adding header data does not prevent BREACH. This PR is nonsense and should be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants