Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2972073002:
Mitigate the pushState IPC storm DoS. (Closed)
DescriptionMitigate the pushState IPC storm DoS.
This is nothing like what the ultimate, true solution should look like. It does
seem to work for the narrow problem of calling history.pushState in a tight
loop, however.
BUG=394296
Review-Url: https://codereview.chromium.org/2972073002
Cr-Commit-Position: refs/heads/master@{#485498}
Committed: https://chromium.googlesource.com/chromium/src/+/95558ee76640b366e8b775bd93483b99af379f55
Patch Set 1 #Patch Set 2 : Move the check to |StateObjectAdded|. #
Total comments: 2
Patch Set 3 : Simpler data model. Thanks dcheng! #
Total comments: 9
Patch Set 4 : Fix obvious bugs noted in comments! #
Total comments: 4
Patch Set 5 : Add a LayoutTest; give the bikeshed a bit o' wainscoting #
Total comments: 6
Patch Set 6 : Try testharness.js test instead of old-style. #Patch Set 7 : Make the test work (thanks dcheng!), move the flooding test per kinuko. #
Total comments: 1
Messages
Total messages: 46 (24 generated)
palmer@chromium.org changed reviewers: + dcheng@chromium.org, kinuko@chromium.org
dcheng: core/frame/OWNERS kinuko: FYI Thanks!
The CQ bit was checked by palmer@chromium.org to run a CQ dry run
Description was changed from ========== Mitigate the pushState IPC storm DoS. This is nothing like what the ultimate, true solution should look like. It does seem to work for the narrow solution of calling history.pushState in a tight loop, however. BUG=394296 ========== to ========== Mitigate the pushState IPC storm DoS. This is nothing like what the ultimate, true solution should look like. It does seem to work for the narrow problem of calling history.pushState in a tight loop, however. BUG=394296 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
creis@chromium.org changed reviewers: + creis@chromium.org
Couldn't a page use replaceState to do the same thing and bypass this? And if we take that away, any number of other actions that result in IPCs? I'm a bit skeptical about spending memory or code complexity on a bandaid fix here, since it seems like whack-a-mole. If you just want do something in the short term, though, I won't stop you. (If the replaceState case is equally bad for DoS, we should at least cover that as well.)
The CQ bit was unchecked by palmer@chromium.org
> Couldn't a page use replaceState to do the same thing and bypass this? And if we take that away, any number of other actions that result in IPCs? Yes, and yes. Although I am about to upload a new patchset that moves the check into |StateObjectAdded| so that it covers |replaceState|, too. > I'm a bit skeptical about spending memory or code complexity on a bandaid fix here, since it seems like whack-a-mole. If you just want do something in the short term, though, I won't stop you. (If the replaceState case is equally bad for DoS, we should at least cover that as well.) Yes. Kinuko, who is working on the long-term, real fix, said that a quick bandaid would help. The issue is being exploited in the wild by e.g. tech support scammers, so mitigating the issue may provide some relief. Spending a bit of memory is a bummer, though. But the issue has been around for a long time, and it will be easy to remove this code when we don't need it anymore.
The CQ bit was checked by palmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
https://codereview.chromium.org/2972073002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.h (right): https://codereview.chromium.org/2972073002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.h:114: mutable HostLimits host_limits; As history is tied to the Window object, shall we just store a counter + WTF::TimeTicks? A navigation will naturally reset this, and we don't have to worry about purging old entries in the map. (It's a bit counterintuitive since the constructor takes a LocalFrame*... but that's just how it is.)
https://codereview.chromium.org/2972073002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.h (right): https://codereview.chromium.org/2972073002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.h:114: mutable HostLimits host_limits; > As history is tied to the Window object, shall we just store a counter + WTF::TimeTicks? A navigation will naturally reset this, and we don't have to worry about purging old entries in the map. > > (It's a bit counterintuitive since the constructor takes a LocalFrame*... but that's just how it is.) Do you mean we don't need to keep track of the hostname, and we can have just: mutable int state_update_count; mutable TimeTicks last_udpate; that would apply for all hostnames? I'm a little concerned about host A eating up the available budget and hurting well-behaved host B. Does that make sense?
On 2017/07/06 23:53:06, palmer wrote: > https://codereview.chromium.org/2972073002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/History.h (right): > > https://codereview.chromium.org/2972073002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/History.h:114: mutable HostLimits > host_limits; > > As history is tied to the Window object, shall we just store a counter + > WTF::TimeTicks? A navigation will naturally reset this, and we don't have to > worry about purging old entries in the map. > > > > (It's a bit counterintuitive since the constructor takes a LocalFrame*... but > that's just how it is.) > > Do you mean we don't need to keep track of the hostname, and we can have just: > > mutable int state_update_count; > mutable TimeTicks last_udpate; > > that would apply for all hostnames? I'm a little concerned about host A eating > up the available budget and hurting well-behaved host B. Does that make sense? Right, we don't need the hostname. I'll try to clarify since my explanation wasn't very good. Every time we navigate to a new page, we create a new Document. Each Document receives a new Window object*. The History object is tied to the Window object: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Loc... So each C++ History object corresponds to exactly one Document, so it's not possible for different Documents (and origins) to share the same History object. Of course, a same-origin window could reach into another same-origin window and mess with its History object--but they're same origin, so it won't end up penalizing an unrelated origin. * since it's the web, there's an exception of course... navigating away from the initial empty document to a page that's same-origin to the initial empty document will reuse the Window object. Meh.
Awesome, thanks. I'll send up a new, simplified patchset today.
https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.cpp (right): https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:126: constexpr auto kStateUpdateLimitResetInterval = TimeDelta::FromSeconds(10); Nit: static https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:130: state_flood_guard.last_updated = now; Do we want to allow in this case, since > 10 seconds has elapsed? https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.h (right): https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.h:106: bool IsFloodingState(const String& hostname) const; Nit: remove the arg which is no longer needed. Nit 2: how about calling this IsThrottlingStateObjectChanges()
lukasza@chromium.org changed reviewers: + lukasza@chromium.org
Some drive-by comments... https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.cpp (right): https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:122: bool History::IsFloodingState(const String& hostname) const { |hostname| parameter is unused now and can be removed, right? https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:130: state_flood_guard.last_updated = now; TL;DR: Do you need a |return false| statement above? What happens if a well-behaved page pushes state changes every 10 seconds. We don't want to loose any user data in this case, right? If I understand the code correctly, then in this the legitimate/slow 50-th push would get here and would be classified as flooding and dropped - this seems undesirable.
https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.cpp (right): https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:122: bool History::IsFloodingState(const String& hostname) const { > |hostname| parameter is unused now and can be removed, right? Done. https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:126: constexpr auto kStateUpdateLimitResetInterval = TimeDelta::FromSeconds(10); On 2017/07/07 at 21:04:09, dcheng wrote: > Nit: static Done. https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:130: state_flood_guard.last_updated = now; On 2017/07/07 at 21:04:09, dcheng wrote: > Do we want to allow in this case, since > 10 seconds has elapsed? Yep, done. https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.h (right): https://codereview.chromium.org/2972073002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.h:106: bool IsFloodingState(const String& hostname) const; > Nit: remove the arg which is no longer needed. Done. > Nit 2: how about calling this IsThrottlingStateObjectChanges() Done.
The CQ bit was checked by palmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM with a naming nit Sorry I'm bad at thinking of names =( https://codereview.chromium.org/2972073002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.h (right): https://codereview.chromium.org/2972073002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.h:106: bool IsThrottlingStateObjectChanges() const; Sorry. I thought about this slightly more and I think "ShouldThrottle" is better than "IsThrottling" =P I would also just not mark it const (since it really isn't) https://codereview.chromium.org/2972073002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.h:109: mutable struct { Then we won't need mutable!
Oh... sorry... one other thing I didn't think of--is it easy to add a unit test for this?
The CQ bit was unchecked by palmer@chromium.org
https://codereview.chromium.org/2972073002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.h (right): https://codereview.chromium.org/2972073002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.h:106: bool IsThrottlingStateObjectChanges() const; > Sorry. I thought about this slightly more and I think "ShouldThrottle" is better than "IsThrottling" =P OK. In the previous patchset I thought about calling it "ShouldThrottle" but convinced myself that your name made sense as a reflection of the object's internal policy-deciding state. But, whatever. :) I'll change it to ShouldThrottle. > I would also just not mark it const (since it really isn't) OK. https://codereview.chromium.org/2972073002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.h:109: mutable struct { > Then we won't need mutable! OK.
> Oh... sorry... one other thing I didn't think of--is it easy to add a unit test for this? This latest patchset has one.
The CQ bit was checked by palmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2972073002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/loader/stateobjects/spam-pushstate-then-throttled.html (right): https://codereview.chromium.org/2972073002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/loader/stateobjects/spam-pushstate-then-throttled.html:11: function log(txt) It's generally preferred to write tests using the test framework in testharness.js these days. Would that make sense here? There's test boilerplate that helps with setting up async tests as well. Some documentation: - Github: https://github.com/w3c/web-platform-tests/tree/master/resources - Actual documentation: http://web-platform-tests.org/writing-tests/testharness-api.html - Tutorial: http://darobin.github.com/test-harness-tutorial/docs/using-testharness.html https://codereview.chromium.org/2972073002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/loader/stateobjects/spam-pushstate-then-throttled.html:43: -Makes kStateUpdateLimit calls to pushState() Dare I suggest <ul><li></li></ul> =P
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2972073002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/loader/stateobjects/spam-pushstate-then-throttled.html (right): https://codereview.chromium.org/2972073002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/loader/stateobjects/spam-pushstate-then-throttled.html:43: -Makes kStateUpdateLimit calls to pushState() Maybe we should make it clear that it's UA-specific, chromium-only test? https://codereview.chromium.org/2972073002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.cpp (right): https://codereview.chromium.org/2972073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:243: } nit: no { } needed Do we want to have this after the CanChangeToUrl check below?
https://codereview.chromium.org/2972073002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/loader/stateobjects/spam-pushstate-then-throttled.html (right): https://codereview.chromium.org/2972073002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/loader/stateobjects/spam-pushstate-then-throttled.html:43: -Makes kStateUpdateLimit calls to pushState() > Maybe we should make it clear that it's UA-specific, chromium-only test? But it tests behavior in History, which is core to Blink, so all callers will behave this way. Right? https://codereview.chromium.org/2972073002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/History.cpp (right): https://codereview.chromium.org/2972073002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/History.cpp:243: } > nit: no { } needed Done. > Do we want to have this after the CanChangeToUrl check below? Done.
The CQ bit was checked by palmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, new test LGTM
The CQ bit was unchecked by palmer@chromium.org
The CQ bit was checked by palmer@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2972073002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/loader/stateobjects/spam-pushstate-then-throttled.html (right): https://codereview.chromium.org/2972073002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/loader/stateobjects/spam-pushstate-then-throttled.html:17: history.pushState("DiscardedSpamItem", "51"); I wanted to say this discarsion is impl-specific behavior, which is not spec'ed.
On 2017/07/11 00:38:04, kinuko wrote: > https://codereview.chromium.org/2972073002/diff/120001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/fast/loader/stateobjects/spam-pushstate-then-throttled.html > (right): > > https://codereview.chromium.org/2972073002/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/loader/stateobjects/spam-pushstate-then-throttled.html:17: > history.pushState("DiscardedSpamItem", "51"); > I wanted to say this discarsion is impl-specific behavior, which is not spec'ed. (basically lgtm/2, regardless)
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1499728130108820,
"parent_rev": "15ea4a1a425c1fb5c29e9cc35709403d7e4d37a8", "commit_rev":
"95558ee76640b366e8b775bd93483b99af379f55"}
Message was sent while issue was closed.
Description was changed from ========== Mitigate the pushState IPC storm DoS. This is nothing like what the ultimate, true solution should look like. It does seem to work for the narrow problem of calling history.pushState in a tight loop, however. BUG=394296 ========== to ========== Mitigate the pushState IPC storm DoS. This is nothing like what the ultimate, true solution should look like. It does seem to work for the narrow problem of calling history.pushState in a tight loop, however. BUG=394296 Review-Url: https://codereview.chromium.org/2972073002 Cr-Commit-Position: refs/heads/master@{#485498} Committed: https://chromium.googlesource.com/chromium/src/+/95558ee76640b366e8b775bd9348... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/95558ee76640b366e8b775bd9348... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
