8000 Http: added HTTP header with random length to complicate BREACH attack by fabik · Pull Request #1379 · nette/nette · GitHub
[go: up one dir, main page]

Skip to content

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

Open
fabik wants to merge 1 commit intonette:masterfrom
fabik:patch-18
Open

Http: added HTTP header with random length to complicate BREACH attack#1379
fabik wants to merge 1 commit intonette:masterfrom
fabik:patch-18

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

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.

< 8000 /div>

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.

@dg dg force-pushed the master branch 2 times, most recently from 489cca2 to 0b969cd Compare May 10, 2018 20:14
@dg dg force-pushed the master branch 3 times, most recently from 09a7d92 to b9698a8 Compare February 10, 2019 23:23
@dg dg force-pushed the master branch 3 times, most recently from 5a8c108 to 3aa3147 Compare February 26, 2019 14:39
@dg dg force-pushed the master branch 2 times, most recently from 5feee0e to 3fc1e40 Compare January 5, 2021 15:23 8000
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.

7 participants

0