1314912 - (CVE-2020-26963) history.pushState - Firefox Hangs and then iterating script error

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[sachin.raste]

Attachment: index.html

User Agent: Mozilla/5.0 (Windows NT 5.2; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20161019084923

Steps to reproduce:

Opened up the attached html file which contains 

     jQuery(window).load(function(){
                    var total = "";
                    for(var i=0; i<100000; i++){
                        total = total + i.toString();
                        history.pushState(0, 0, total);
                    } });




Actual results:

Firefox Hangs , CPU usage crosses 50% , after some time, get script error .

Havent checked this issue on my phone / tablet, however I believe it should. 


Expected results:

Redirect / Stop gracefully

Comment 1

[sachin.raste]

OS : Windows 2003

Comment 2

[YF (Yang)]

WFM in 52.0a1 (2016-11-02) (32-bit) on Win10 x64.

Comment 3

[Andrew Overholt [:overholt]]

Nika, can you see if this is easy to fix?

Comment 4

[Stuart Philp :sphilp]

Couple of profiles that may or may not be useful

STR: Take the script from comment 0, edit the iterations to a smaller number (i.e. 1000 and 3000), run in scratchpad

Here's a loop of 1000 times in a profile https://perfht.ml/2BrMBrF - the 3 jumps on cp4 is the script running. This was just minor hangs, browser still basically usable.

And then https://perfht.ml/2BsHibB which is 3000 loops and locked up the browser for 15 seconds or so

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Add a rate limit to calls to pushState/replaceState

This looks like it was fixed by Chromium in
https://bugs.chromium.org/p/chromium/issues/detail?id=394296. It was fixed by
adding a rate limit to calls to pushState and replaceState.

I figure we might as well do the same thing - this patch adds a similar rate
limit to pushState and replaceState in Gecko.

MozReview-Commit-ID: IThztDz1xKI

Comment 6

[Nika Layzell [:nika] (ni? for response)]

I should note that I'm not sure how I would add a reliable automated test for this - so I haven't done so.

Comment 7

[Daniel Veditz [:dveditz]]

bug 1246773 is the same issue (and has 3 other dupes). Even with multi-process this hangs the parent/browser in addition to the child. Manually killing the child process eventually led to me getting my browser back, but took a while (30-60s) before the parent became responsive again.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Please see bug 1380305 comment 22 for some concerns about this approach.

Comment 13

[Samael Wang [:freesamael] (away for now)]

Nika, are you interested in continuing the work here? (if so I will resolve duplicated bug 1380305)

Looks like we do need to handle location.hash. The latest Chrome Canary / Safari Tech Preview are also suffering from location.hash flooding, BTW. Not sure if there're other cases we need to worry about.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

(In reply to Samael Wang [:freesamael] from comment #13)
> Nika, are you interested in continuing the work here? (if so I will resolve
> duplicated bug 1380305)
> 
> Looks like we do need to handle location.hash. The latest Chrome Canary /
> Safari Tech Preview are also suffering from location.hash flooding, BTW. Not
> sure if there're other cases we need to worry about.

Your patch looks more polished than mine but I can finish this bug up if you'd like. Handling location.hash should be possible too, we can approach it in the same way.

Comment 16

[Samael Wang [:freesamael] (away for now)]

Then I will continue my work here :)

Comment 17

[Samael Wang [:freesamael] (away for now)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from bug 1380305 comment #23)
> Is this a problem for replaceState too, not just pushState?  I guess that
> would still have the IPC traffic, but not the nonstop growth in data we need
> to store in the parent process, right?
> 

There wasn't much to store in the parent process, and the session history length is limited to 50 entries so there shouldn't be much to store in the child process either. The issue was mainly that the frequency of sendAsyncMessage from the child is a few times faster than the parent can consume, which keeps the parent process busy and the chrome UI extremely non-responsive. 

Besides, it seems the parent process was too busy to schedule GC, so the memory size keeps growing. According to the DMD output in bug 1380305 comment #16 I believe it was the URI generated in RemoteWebNavigation which consumes the memory:
https://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/toolkit/modules/RemoteWebProgress.jsm#251

On my Linux machine, it can cause thrashing in a few minutes and make the whole system be non-responsive. It seems anything causes infinite OnLocationChange has the same effect. I just realized I can also reproduce it with a few `pushState` and then a simple `history.back` + `history.forward` loop. Now I'm thinking that we should probably throttle all LOCATION_CHANGE_SAME_DOCUMENT navigation.

Comment 18

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1314912 - Part 1: Throttle same document navigation caused by content scripts, if there have been too many LOCATION_CHANGE_SAME_DOCUMENT caused by content scripts in a short time frame.

The patch records the number of successive LOCATION_CHANGE_SAME_DOCUMENT when
the incumbent global exists and it's not system principal, which should
basically indicate content scripts are using History or Location APIs.

When the number of successive LOCATION_CHANGE_SAME_DOCUMENT recorded exceeds
the limit, it throws in InternalLoad and AddState if the caller is from content
scripts and it's making same document navigation.

Review commit: https://reviewboard.mozilla.org/r/206138/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206138/

Comment 19

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1314912 - Part 2: Add a few test cases.

Review commit: https://reviewboard.mozilla.org/r/206140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206140/

Comment 20

[Samael Wang [:freesamael] (away for now)]

I'm proposing a more aggressive solution that would require another review.

Comment 21

[Samael Wang [:freesamael] (away for now)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from bug 1380305 comment #23)
> The algorithm in this function is bit nondeterministic from the point of
> view of a web page.  It might be able to do 98 pushstate calls in a given
> 10-second period (do one call, wait 9.5 seconds, do 48 calls, wait one
> second, do 49 calls, for a total of 97 calls in a period starting 5s after
> that first call).  Or it might not, depending on exactly how its calls align
> with the timer.  That might be OK, as long as we're very clear in our
> documentation, but seems like it could lead to pages that randomly work some
> times but not others...  Is this basically the algorithm Chrome and Safari
> are using?
> 

Yes I think that's also what Chrome & Safari are doing.

Chromium:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/History.cpp?l=144-161

Webkit:
https://github.com/WebKit/webkit/blob/679b410c541bdccac78ef40c873ea497a613243a/Source/WebCore/page/History.cpp#L201-L211

Comment 22

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8935253 [details]
Bug 1314912 - Part 2: Add a few test cases.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206140/diff/1-2/

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8935252 [details]
Bug 1314912 - Part 1: Throttle same document navigation caused by content scripts, if there have been too many LOCATION_CHANGE_SAME_DOCUMENT caused by content scripts in a short time frame.

https://reviewboard.mozilla.org/r/206138/#review212020

::: commit-message-cb909:4
(Diff revision 1)
> +Bug 1314912 - Part 1: Throttle same document navigation caused by content scripts, if there have been too many LOCATION_CHANGE_SAME_DOCUMENT caused by content scripts in a short time frame. r?bz
> +
> +The patch records the number of successive LOCATION_CHANGE_SAME_DOCUMENT when
> +the incumbent global exists and it's not system principal, which should

What happens if the page does a bunch of click() on anchors that point to the page url, but with different refs?  That comes via OnLinkClickEvent.  Does the AutoJSAPI there give us an incumbent global?  I would expect it does not, and that situation won't be caught by this code.

Ideally, we would explicitly check whatever would become the triggering principal for the load in the load case...

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8935253 [details]
Bug 1314912 - Part 2: Add a few test cases.

https://reviewboard.mozilla.org/r/206140/#review212022

::: docshell/test/navigation/test_bug1314912_back_forward.html:43
(Diff revision 2)
> +    }
> +    for (let i = 0; i < 30; i++) {
> +      history.forward();
> +    }
> +
> +    // The next history navigation should throw.

Hmm.  So this will mean that if too many history.back() happen in a row, they could start failing?

But why should we block back() or forward() ever?  It's _adding_ things to the history that's a problem, no?

::: docshell/test/navigation/test_bug1314912_back_forward.html:47
(Diff revision 2)
> +
> +    // The next history navigation should throw.
> +    SimpleTest.doesThrow(
> +      () => history.back(), "check history.back does throw");
> +
> +    if (hasRepeated) {

!hasRepeated, I would think.

::: docshell/test/navigation/test_bug1314912_back_forward.html:48
(Diff revision 2)
> +    // The next history navigation should throw.
> +    SimpleTest.doesThrow(
> +      () => history.back(), "check history.back does throw");
> +
> +    if (hasRepeated) {
> +      // Repeat the test again after 5 seconds.

Where are we testing that once we've reached the cap other attemptes within the 3s timeout will fail too?

::: docshell/test/navigation/test_bug1314912_hash_change.html:42
(Diff revision 2)
> +    // The next hash change should throw.
> +    SimpleTest.doesThrow(
> +      () => location.hash = ref,
> +      "check setting location.hash does throw");
> +
> +    if (hasRepeated) {

!hasReated.

::: docshell/test/navigation/test_bug1314912_hash_change.html:43
(Diff revision 2)
> +    SimpleTest.doesThrow(
> +      () => location.hash = ref,
> +      "check setting location.hash does throw");
> +
> +    if (hasRepeated) {
> +      // Repeat the test again after 5 seconds.

Again, we should test the within-3-second thing, I would think.

::: docshell/test/navigation/test_bug1314912_pushState.html:42
(Diff revision 2)
> +    // The next pushState should throw.
> +    SimpleTest.doesThrow(
> +      () => history.pushState(null, "test", location.href + "#" + ref),
> +      "check history.pushState does throw");
> +
> +    if (hasRepeated) {

!hasRepeated, and test within the 3s timeout...

Comment 25

[Samael Wang [:freesamael] (away for now)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #24)
> Comment on attachment 8935253 [details]
> Bug 1314912 - Part 2: Add a few test cases.
> 
> https://reviewboard.mozilla.org/r/206140/#review212022
> 
> ::: docshell/test/navigation/test_bug1314912_back_forward.html:43
> (Diff revision 2)
> > +    }
> > +    for (let i = 0; i < 30; i++) {
> > +      history.forward();
> > +    }
> > +
> > +    // The next history navigation should throw.
> 
> Hmm.  So this will mean that if too many history.back() happen in a row,
> they could start failing?
> 
> But why should we block back() or forward() ever?  It's _adding_ things to
> the history that's a problem, no?
> 

Not really. Anything that causes excessive location changes can DoS the parent process and hang the chrome UI. See comment 17.

> ::: docshell/test/navigation/test_bug1314912_back_forward.html:48
> (Diff revision 2)
> > +    // The next history navigation should throw.
> > +    SimpleTest.doesThrow(
> > +      () => history.back(), "check history.back does throw");
> > +
> > +    if (hasRepeated) {
> > +      // Repeat the test again after 5 seconds.
> 
> Where are we testing that once we've reached the cap other attemptes within
> the 3s timeout will fail too?
> 

I'm worrying about intermittent failures on try server if we do another `setTimeut` within 3 seconds. Bug 1362410 shows that 1 second ExpirationTracker can take more than 3 seconds to run on try server.

Comment 26

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8935253 [details]
Bug 1314912 - Part 2: Add a few test cases.

https://reviewboard.mozilla.org/r/206140/#review212022

> Hmm.  So this will mean that if too many history.back() happen in a row, they could start failing?
> 
> But why should we block back() or forward() ever?  It's _adding_ things to the history that's a problem, no?

Not really. Anything that causes excessive location changes can DoS the parent process and hang the chrome UI. See comment 17.

> Where are we testing that once we've reached the cap other attemptes within the 3s timeout will fail too?

I'm worrying about intermittent failures on try server if we do another `setTimeut` within 3 seconds. Bug 1362410 shows that 1 second ExpirationTracker can take more than 3 seconds to run on try server.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> I'm worrying about intermittent failures on try server if we do another `setTimeut` within 3 seconds

Sure.  But when the timer fires you can check how much time has _actually_ passed and decide whether to actually try to assert anything about the state at that point if too much time has passed.

Comment 28

[Samael Wang [:freesamael] (away for now)]

For anyone interested in this bug, feel free to take the patch and continue the work.

Comment 32

[Liam England]

This is actually not fixed in Chromium, open bug there: https://bugs.chromium.org/p/chromium/issues/detail?id=1038223

Comment 35

[Paul Zühlcke [:pbz]]

Attachment: Bug 1314912 - Rate limit calls to History and Location interfaces. r=smaug

This adds a rate limit to all methods of the History interface and
the location.hash setter for non-system callers.
The rate limit is counted per BrowsingContextGroup, per process and can be controlled by prefs.

This patch is based on the original rate limit patch by :freesamael.

Comment 36

[Paul Zühlcke [:pbz]]

Attachment: Bug 1314912 - Added test for location change rate limit. r=smaug

Depends on D90136

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efcefed227f3
Rate limit calls to History and Location interfaces. r=smaug
https://hg.mozilla.org/integration/autoland/rev/ff5164e4aec8
Added test for location change rate limit. r=smaug

Comment 38

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/efcefed227f3
https://hg.mozilla.org/mozilla-central/rev/ff5164e4aec8

Comment 39

[Tom Ritter [:tjr]]

Attachment: advisory.txt