615761 - window.external.AddSearchProvider loop DoS

Status: RESOLVED FIXED

(Firefox :: Search, defect, P3)

Description

[federico lanusse]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12

window.external.AddSearchProvider loop DoS

javascript loop that atemp to load a SearchProvider will open appmodal that locks firefox until it crashes.

Reproducible: Always

Steps to Reproduce:
1.Set a page with following code
	<script>
	var engineURL = "http://localhost/001.xml"	
	while (true==true){window.external.AddSearchProvider(engineURL)}
	</script>
2. engineURL file must exist, but don't need to be a valid search engine
3. Open page at step1
Actual Results:  
Application will crash, this can vary from time to time, and engineURL  content, but it will always crash sooner or later

Expected Results:  
application should not crash, popup should be windowsmodal so the user have any chance to exit the loop

able to reproduce in windows 7(64x), and XP(32x)

Comment 1

[WBT]

Should this be marked as a security bug, because it's revealing an easy attack on FF users?

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

DoS is commonly considered as sg:low.
Can you provide a Stacktrace against a recent 3.6 or 4.0 Build?
https://developer.mozilla.org/En/How_to_get_a_stacktrace_for_a_bug_report

Comment 4

[Johann Hofmann [:johannh]]

This is another window modal prompt in our way to freedom from window modal prompts. Florian, do you think we could make this prompt tab-modal?

What we would need to do that are references to the originating content window here: https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/toolkit/components/processsingleton/MainProcessSingleton.js#47 and https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/toolkit/components/search/nsSearchService.js#1557

I'm not sure how to get that content window, to be honest. browser.contentWindow seems to be null in all cases.

Comment 5

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #4)
> This is another window modal prompt in our way to freedom from window modal
> prompts. Florian, do you think we could make this prompt tab-modal?
> 
> What we would need to do that are references to the originating content
> window here:
> https://searchfox.org/mozilla-central/rev/
> d03ad8843e3bf2e856126bc53b0475c595e5183b/toolkit/components/processsingleton/
> MainProcessSingleton.js#47 and
> https://searchfox.org/mozilla-central/rev/
> d03ad8843e3bf2e856126bc53b0475c595e5183b/toolkit/components/search/
> nsSearchService.js#1557
> 
> I'm not sure how to get that content window, to be honest.
> browser.contentWindow seems to be null in all cases.

Pretty sure it's in the wrong process. You'd need to send this back to the content process it came from and then show the alert there, then forward the user response back (where necessary). That or adapt our prompting infrastructure so we can do prompts on tabs without having the tab content care about the prompt, which I don't know is possible right now (but maybe it is and I've never looked closely enough) -- the tab prompts live in the parent process anyway, I believe.

Comment 6

[Florian Quèze [:florian]]

(In reply to Johann Hofmann [:johannh] from comment #4)
> This is another window modal prompt in our way to freedom from window modal
> prompts. Florian, do you think we could make this prompt tab-modal?

It would be nicer, yes. Or we could also make it a doorhanger.

It's also possible that in the future we could deprecate and eventually remove this old API.

Comment 7

[Johann Hofmann [:johannh]]

Note that we have deprecated this function in bug 1503551 and made it a dummy. I can still reproduce this issue in Nightly, which suggests that the dummy is still making its way up to the parent process. We should stop it from doing so or completely remove the function.

Comment 8

[Mike Kaply [:mkaply]]

It is not still a dummy. We undid that because it breaks many things.

Comment 11

[Emma Zühlcke [:emz]]

I'm working on a patch for this, based on Bug 1271842. We can make this prompt async and tab modal. That means it does not steal focus from the user and cannot be spammed anymore.

Comment 12

[Emma Zühlcke [:emz]]

Attachment: Bug 615761 - Made window.external.AddSearchProvider prompts tab modal. r=johannh,standard8

Comment 13

[Emma Zühlcke [:emz]]

Attachment: Bug 615761 - Promise chain AddSearchProvider window actor calls to prevent IPC flooding. r=standard8!,johannh

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8afe4a315797
Made window.external.AddSearchProvider prompts tab modal. r=johannh,Standard8
https://hg.mozilla.org/integration/autoland/rev/2388eb7ac460
Promise chain AddSearchProvider window actor calls to prevent IPC flooding. r=johannh,Standard8

Comment 15

[Andreea Pavel [:apavel]]

Backed out for failing bc at browser_policy_search_engine.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=2388eb7ac4602c04b45bf5956d627bead1641e7d&selectedJob=298886999

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298886999&repo=autoland&lineNumber=9988

Backout: https://hg.mozilla.org/integration/autoland/rev/6da10e626704988f14184d236b1119efaef5244e

Comment 16

[Emma Zühlcke [:emz]]

Attachment: Bug 615761 - Updated enterprise policy search engine test. r=johannh!

Depends on D70870

Comment 17

[Emma Zühlcke [:emz]]

Fixed the test and will land this again with a fix for Bug 1632805, which has a review pending.

Comment 18

[Emma Zühlcke [:emz]]

Given that the feature will be disabled by pref in 78 (Bug 1632447), I think we can drop these patches. They'd only provide value for one cycle if we uplifted to 77. I haven't observed abuse of this in the wild so we can probably tolerate it for 77.

Comment 19

[Mathew Hodson]

Is this fixed now that bug 1632448 landed?

Comment 20

[Mark Banner (:standard8)]

(In reply to Mathew Hodson from comment #19)

Is this fixed now that bug 1632448 landed?

Yes, thanks for highlighting this, I thought I'd already closed all of these.

Fixed by code removal via bug 1632448.