Closed Bug 672485 Opened 13 years ago Closed 13 years ago

Holding enter allows arbitrary extension installation.

Categories

(Firefox :: Security, defect)

5 Branch
x86
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 9
Tracking Status
firefox5 - wontfix
firefox6 - wontfix
firefox7 + fixed
firefox8 + fixed
firefox9 + fixed
status1.9.2 --- unaffected

People

(Reporter: marius.mlynski, Assigned: Unfocused)

References

Details

(Keywords: verified-aurora, verified-beta, Whiteboard: [sg:critical][qa!])

Attachments

(4 files, 1 obsolete file)

It is possible to install an arbitrary XPI without user's consent if the user holds Enter on a malicious website containing a specially crafted JavaScript code.

There are 2 layers of protection against an unauthorized installation of extensions:

1) The principal of the opener is checked against whitelisted domains that are allowed to download the plugin without asking. If the domain is not trusted, the user is asked to allow to download the plugin.
2) When the plugin is downloaded, the user is asked to confirm the installation.

The first protection can be circumvented by creating a hidden "Embed" element containing an arbitrary XPI as its "pluginspage" parameter. The attacker can focus this element while the user holds Enter, causing a number of "Plugin Finder Service" windows to appear. The first window focuses the "Cancel" button and will just close, but all the subsequent ones will set focus on the "Manual Install" button directing to the malicious XPI. As soon as the user releases the key, the browser will start launching multiple windows with the provided URL. The windows will have a ChromeWindow object as their opener, so the user will not be asked to allow to download a plugin.

The second protection can be bypassed due to a logic error in amWebInstallListener.js. When no window-watcher is registered in Services, this will throw:

Services.ww.openWindow(this.window, "chrome://mozapps/content/xpinstall/xpinstallConfirm.xul",null, "chrome,modal,centerscreen", args);

with the following message in the console:

Warning: WARN addons.manager: InstallListener threw exception when calling onDownloadEnded: TypeError: Services.ww is undefined
Source File: resource://gre/components/amWebInstallListener.js
Line: 170

When the confirmation dialog fails to open, the installation will commence as though the user clicked "Install".

Getting the browser into a state in which Services.ww is permanently undefined can be achieved with no user interaction by using a modified version of https://bugzilla.mozilla.org/show_bug.cgi?id=616659 on a signed website. Exploiting it locally will likely bring about corrupt windows and a lot of errors in the console, but Services.ww will be defined. On a remote signed website, with the try-catch-inside-eval recursion performed during page load, after an alert dialog which prevents "too much recursion" error from occuring, it fails silently and Services.ww remains undefined for the whole browsing session, ie. Firefox will not ask to confirm a plugin installation until the user restarts the browser. The only mitigating factor is that if Services.ww was defined earlier in the browser session, the exploit may fail and corrupt windows will appear (hence point #2 in the steps described below, after opening windows to import the certificate). This was noticed by Zach Hoffman in https://bugzilla.mozilla.org/show_bug.cgi?id=616659#c37 but the implications there were reverse.

There may be other ways to wind up with Services.ww undefined, but those should be safe as long as the plugin installation is stopped when xpinstallConfirm.xul fails to open.

Reproducible: always (as long as Services.ww wasn't defined earlier in the browsing session)

Steps to reproduce:
1. Import x509.cert for software makers identification.
2. Close the browser and reopen after the process shuts down.
3. Open jar:http://x.x.x.x/mozilla/signed.jar!exploit.htm (see comment 1)
4. Press and hold enter.

Actual results:
The plugin is automatically downloaded and installed.

Expected results:
The plugin should neither be downloaded nor installed without user's consent.

Verified to work with Firefox 5.0.1 on Windows 7 SP1 64-bit and Windows XP SP3 32-bit.
Ah, one more thing: if the exploit fails, ie. Services.ww happens to be already defined in the browsing session causing display of corrupt windows after loading the page, and you deny privileges to stop windows from popping-up, then make sure to remove these lines from prefs.js before retrying:

user_pref("capability.principal.certificate.pX.denied", "UniversalXPConnect");
user_pref("capability.principal.certificate.pX.id", "BF:94:EA:8A:E8:55:BC:EE:59:0C:4B:0B:6D:3B:09:80:9D:9D:F7:0E");
user_pref("capability.principal.certificate.pX.subjectName", "CN=a,O=a,OU=a,ST=a,C=aa,UID=a,E=a");

Also, I found that "pluginspage" windows open a bit slow with with ChromeBug 1.7.2 installed, so be advised to turn it off if you have it enabled.
This is a pretty confused bug report. If you give any website UniversalXPConnect, you've lost (there are much easier ways to exploit a user once you've got UniversalXPConnect). We're removing support for enablePrivilege, so I don't think that part of the bug is a concern.

It's also not clear to me: are you saying that bug 616659 is not fixed?

Nevertheless, there are a few bits of this report that do concern me:

* embed pluginspage= can bypass the XPI whitelist (an annoyance hole, but not strictly a security bug)
* the pluginspage frame opens with a ChromeWindow opener (a security bug, but one I think we will have fixed in FF6 or 7)
* If the confirmation dialog fails, we should just stop, rather than continuing
OK, let me rephrase:

If you run the following code:

alert('Welcome aboard!');
function xpl(){
	eval("try{xpl()}catch(b){netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect')}");
}
try{xpl()}catch(e){};

then, provided that Services.ww wasn't defined earlier in the browsing session, no windows asking for an elevated privilege will appear. The browser will fail to display any windows, and the user will not be asked for an elevated privilege! Instead, the browser will be in a state in which Services.ww is not defined, which allows to install a plug-in without asking due to a bug in resource://gre/components/amWebInstallListener.js. My report has nothing to do with granting UniversalXPConnect privileges, nor it is required that the user grants any elevated privileges, please reread it.

This bug: https://bugzilla.mozilla.org/show_bug.cgi?id=616659 is fixed in that corrupt windows that may appear (if Services.ww was defined earlier in the browsing session) will not evaluate to true when closed. While fiddling with that code, I found that if the windows fail to display, Services.ww is undefined, which allows to install plugins without any confirmation.

I can provide the video of running the exploit if needed.
(In reply to comment #7)
> This bug: https://bugzilla.mozilla.org/show_bug.cgi?id=616659 is fixed in
> that corrupt windows that may appear (if Services.ww was defined earlier in
> the browsing session) will not evaluate to true when closed. While fiddling
> with that code, I found that if the windows fail to display, Services.ww is
> undefined, which allows to install plugins without any confirmation.

It's not clear to me how you're making Services.ww undefined. Is there some example code that does that?
As stated in the first post:

"On a remote signed website, with the try-catch-inside-eval recursion performed during page load, after an alert dialog which prevents "too much recursion" error from occuring, it fails silently and Services.ww remains undefined for the whole browsing session"

Where "it" is the following code:

function xpl(){
	eval("try{xpl()}catch(b){netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect')}");
}
try{xpl()}catch(e){};

Thus, the effective script is:

<script>
alert('anything');
function xpl(){
	eval("try{xpl()}catch(b){netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect')}");
}
try{xpl()}catch(e){};
</script>

If it's run on a remote signed website before any other windows (not tabs) were launched in the browsing session, then Services.ww remains undefined for the rest of this browsing session, which in turn allows to install plugins without confirming.
Btw, jar:http:...signed.jar!/exploit.htm linked in comment 5 does exactly that on load, after closing the pop-up. On all computers (Win XP/7) I've tried, Services.ww was permanently undefined after following the first 3 steps from comment 0 (+closing the "welcome" alert), and Firefox did not ask to confirm installation (xpinstallConfirm.xul) after I held and released Enter or navigated from the exploit page to AMO and clicked any of the "add to Firefox" buttons. With a malicious third party with an access to a proper certificate from a trusted CA, just steps 3 and 4 are necessary to have an arbitrary extension installed in the browser.
So far on OSX the only result of running this code is a stalled browser that won't respond to input, no extension install.
On Windows 7 the testcase did something more, after holding enter for a while the install missing plugins bar appeared and it attempted to stop me leaving the page, but no add-on got downloaded that I can see, Services.ww was showing as defined after that.
Dave, what version did you check? The proof of concept is suited for the current public release (5.0.1).

I've made 2 short videos to show how the proof of concept behaves here in v5.0.1 (select HD and fullscreen for the best quality):

http://www.youtube.com/watch?v=hPy2Ivq3MxQ - Windows 7 SP1, the key was imported before recording the video.
http://www.youtube.com/watch?v=I96N-iZJoe8 - Windows XP SP3, shows the full process including the key import. This works a little bit slower due to a combination of video capturing and running things in a VM.

Enter was released as soon as the counter froze completely.
> * embed pluginspage= can bypass the XPI whitelist (an annoyance hole, but
> not strictly a security bug)

I filed this as a security bug a few months ago ;) Bug 655916.

> Getting the browser into a state in which Services.ww is permanently undefined

Jeepers. This is why we need separate recursion limits for chrome & content.  And why we need to NS_RUNTIMEABORT when chrome js hits OOM or TMR -- we can't possibly audit our chrome js against the possibility that at any bytecode instruction the script could just stop executing.
(In reply to comment #13)
> Dave, what version did you check? The proof of concept is suited for the
> current public release (5.0.1).

I was testing the latest nightly builds.
(In reply to comment #14)
> > Getting the browser into a state in which Services.ww is permanently undefined
> 
> Jeepers. This is why we need separate recursion limits for chrome & content.
> And why we need to NS_RUNTIMEABORT when chrome js hits OOM or TMR -- we
> can't possibly audit our chrome js against the possibility that at any
> bytecode instruction the script could just stop executing.

I think the key for me is that, yes we could amend amWebInstallListener.js to behave in the case where Services.ww is undefined, but if things like that can be undefined then code all over the place is going to break. We should fix whatever is causing that.
Whiteboard: [sg:critical]
(In reply to comment #15)
> I was testing the latest nightly builds.

I've checked the latest nightly build and I think the reason why you hadn't seen any windows while holding Enter is as follows:

In Firefox 5.0.1, the Embed element for an unsupported plugin looks like this: http://i.imgur.com/FTlKX.png -- the entire box links to Plugin Finder Service, it's also sufficient to focus the Embed element to get PFS windows while just holding Enter.

Nightly (8.0a1 2011-07-21) implements this feature as shown here: http://i.imgur.com/BiVtr.png -- the link is nested deep inside the XUL box, that's why focusing the Embed element isn't sufficient. However, it's possible to obtain a reference to the link if the user clicks within the boundaries of the XUL box (essentially anywhere on the page, as you can follow the cursor with the XUL element). The link can be focused subsequently. I will attach a simple test that shows how it's done.

Btw, from what I'm seeing, Services.ww can end up in the undefined state in the Nightly build (ie. the implications of a successful attempt are exactly the same), although there are more failures (broken windows after the try-catch recursion) where 5.0.1 would have a 100% success rate. It looks that the exploit works more reliably with the release build.
Dave, is this something you can own? If not, can you help find an appropriate owner here? Thanks!
Assignee: nobody → dtownsend
Blair could you put together the simple fix here? Just wrap the Services.ww call in a try...catch and if it fails cancel the installs. No idea how you'd test that though
Assignee: dtownsend → bmcbride
Attached patch Patch v1 (obsolete) — Splinter Review
This fixes up the Services.ww issue, and tests it (test fails without the fix). As a bonus, it applies cleanly to the various branches.
Attachment #550934 - Flags: review?(jst)
Attachment #550934 - Flags: review?(dtownsend)
Attachment #550934 - Flags: review?(jst) → review+
Attachment #550934 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/mozilla-central/rev/84ce41f8cec7
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Backed out due to a leak (that I don't yet understand)
https://hg.mozilla.org/mozilla-central/rev/aa9f527a4928
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Blair, any updates here? Need help with tracking this down, or you think you're making progress here?
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #25)
> Blair, any updates here? Need help with tracking this down, or you think
> you're making progress here?

Ugh, sorry, I've been struck down with the flu for a decent amount of the week. I could indeed use some help trying to figure out the leak - I'm kinda stumped.
Target Milestone: --- → Firefox 8
Andrew, could you help Blair look into the leak that the patch in this bug introduced?
What test causes the leak?  General browsing, the test you added, something else?
(In reply to Andrew McCreight [:mccr8] from comment #28)
> What test causes the leak?

The test added in this patch.
I appear to have fixed the leak, though I don't understand how (but I'd like to!). This leaked:

delete Services.ww;
... do stuff ...
Services.ww = Cc["..."].getService(Ci.nsIWindowWatcher);


This doesn't:

var gWindowWatcher = Services.ww;
delete Services.ww;
... do stuff ...
Services.ww = gWindowWatcher;



However... I found another issue, where it was sending the addon-install-failed notification with an empty array of addons. And fixing that broke a bunch of assumptions the addons manager (and it's tests) make about how installs are expected to fail. Working on a new patch.
Attached patch Patch v1.1Splinter Review
Waiting on Try results, just because I'm paranoid. But testing locally, the leak is gone, and the tests pass.

I looked into making the install actually fail (rather than just be canceled), but it would require adding a new API.
Attachment #550934 - Attachment is obsolete: true
Attachment #555021 - Flags: review?(dtownsend)
Comment on attachment 555021 [details] [diff] [review]
Patch v1.1

Scratch that. Try says it still leaks :(
Attachment #555021 - Flags: review?(dtownsend)
I can reproduce this locally with the first version of the patch, but not the second, so I looked at the first version to see if that might help.  I used the steps described in 
   https://bugzilla.mozilla.org/show_bug.cgi?id=561359#c60
There are two nsGlobalWindows being kept alive in the cycle collector via ChromeWindows:

0x11a913c58 [JS Object (ChromeWindow) (global=11a913c58)]
        --[xpc_GetJSPrivate(obj)]-> 0x11ac15ca0 [XPCWrappedNative (ChromeWindow)]
        --[mIdentity]-> 0x11ac14748 [nsGlobalWindow]

0x1238a1e50 [JS Object (ChromeWindow) (global=1238a1e50)]
        --[xpc_GetJSPrivate(obj)]-> 0x11bd43080 [XPCWrappedNative (ChromeWindow)]
        --[mIdentity]-> 0x11bd41f38 [nsGlobalWindow]
These ChromeWindows have been marked as reachable by the JS GC.

Looking at the GC dump, one of the ChromeWindows (0x11a913c58) seems to be kept alive because it is on the machine stack.  I only have one path for the other ChromeWindow, which says that it is being held alive via the first ChromeWindow, but there may be another path, too.

I think being stored on the machine stack means it is being kept in a local variable somehow, but I'm not sure.  I'll see if I can get more information about how that is happening, but given that it uses a conservative stack scanner I am not too hopeful.

Maybe this will help...
The leak didn't go away when I undid the changes in amWebInstallListener.js, so I guess it is a problem with the test itself?
Dao has a lot of experience with fixing tests that leak windows, maybe he can help here.
I don't know about the leak (xpinstall tests are somewhat hard to follow because of the custom "Harness"), but I'd like to point out that messing with Services.ww like the patch does isn't nice at all, since you're sharing the browser with all other browser-chrome tests. E.g. if some chrome code runs simultaneously with your test and breaks, this might cause issues for all subsequent tests.
(In reply to Andrew McCreight [:mccr8] from comment #34)
> The leak didn't go away when I undid the changes in amWebInstallListener.js,
> so I guess it is a problem with the test itself?

AFAICT, yea. Which confuses me more, since it's a simple test using a harness that's already used by many other tests, without leaks.


(In reply to Dão Gottwald [:dao] from comment #36)
> but I'd like to point out that messing
> with Services.ww like the patch does isn't nice at all, since you're sharing
> the browser with all other browser-chrome tests. E.g. if some chrome code
> runs simultaneously with your test and breaks, this might cause issues for
> all subsequent tests.

I know. But that's the whole point of the test. So the alternative is to not have the test (which would make solving the leak a lot easier...).
(In reply to Blair McBride (:Unfocused) from comment #37)
> (In reply to Dão Gottwald [:dao] from comment #36)
> > but I'd like to point out that messing
> > with Services.ww like the patch does isn't nice at all, since you're sharing
> > the browser with all other browser-chrome tests. E.g. if some chrome code
> > runs simultaneously with your test and breaks, this might cause issues for
> > all subsequent tests.
> 
> I know. But that's the whole point of the test.

Affecting random other code isn't point of the test...

> So the alternative is to not
> have the test (which would make solving the leak a lot easier...).

If you're confident that the leak isn't caused by your code, sure. You should probably file a bug on the leak, though, in case this issue already exists and your test just unveils it.
If we believe that the fix for this is good and doesn't leak by itself, could we take the fix for this security bug w/o the leaking test to get this security fix out to our users sooner?
Depends on: 682410
Comment on attachment 555021 [details] [diff] [review]
Patch v1.1

r+ to land without the testcase
Attachment #555021 - Flags: review+
Landed sans test:
https://hg.mozilla.org/mozilla-central/rev/b379d2c614d0

And filed bug 682410 to figure out what's going on with that leak.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
As a side note, the other issue highlighted in the exploit (non-whitelisted site can trigger xpinstall from pluginspage due to a ChromeWindow opener) is not resolved yet. With amWebInstallListener.js fixed by Blair to stop when xpinstallConfirm.xul fails to open, it shouldn't be possible any more to use it to do anything scary (at least without user's consent), but it may be worth fixing.
jst / Mossop: What say you to comment 42?
Comment on attachment 555021 [details] [diff] [review]
Patch v1.1

This didn't explode.
Attachment #555021 - Flags: approval-mozilla-beta?
Attachment #555021 - Flags: approval-mozilla-aurora?
Can we get this landed on m-c asap? We're running out of time for 7 here, and this needs to land on m-c before we approve it for branches.
According to comment 41, it is on M-C, just sans test.
Depends on: 655916
Attachment #555021 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 555021 [details] [diff] [review]
Patch v1.1

Denied for mozilla-beta. We'll let it bubble up from mozilla-aurora naturally. Please renominate if you disagree.
Attachment #555021 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 555021 [details] [diff] [review]
Patch v1.1

This is essentially a drive-by install (a little bit of easily lured user interaction). Why in the world would we not take this simple patch to prevent that? Patch and testcase checked into trunk/aurora raises the risk of someone else figuring this out.

Renominating for Firefox 7 beta
Attachment #555021 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
Comment on attachment 555021 [details] [diff] [review]
Patch v1.1

On further review we will take this on mozilla-beta. Please land asap.
Attachment #555021 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Target Milestone: Firefox 8 → Firefox 9
QA tracking this for verification across branches.
Whiteboard: [sg:critical] → [sg:critical],[qa+]
Attachment #547601 - Attachment mime type: text/plain → text/html
Not sure if this applies to Firefox 3.6.x or not. The patch doesn't, there's no equivalent use of Services.ww in the old code. I don't think anything was done here about the reporters first point in comment 0, but I did see that effect in any of the testcases attached or linked-to by this bug.
(In reply to Daniel Veditz from comment #52)
> Not sure if this applies to Firefox 3.6.x or not.

It doesn't, the old code defines its own window watcher:

var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].getService(Ci.nsIWindowWatcher);
ww.openWindow(null, URI_XPINSTALL_CONFIRM_DIALOG,"", "chrome,centerscreen,modal,dialog,titlebar", ifptr);
if (!dpb.GetInt(0)) {// User said OK - install items

I see no way it could fail and proceed with the installation at the same time.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0) Gecko/20100101 Firefox/7.0 ID:20110922153450

I'm not sure if this is fixed or if I am testing this wrong. Here is what I did:

1) Import the attached cert
2) Close Firefox and restart after the process is closed
3) Go the the website in comment 1 (404'd so I'm guessing this is broken)
4) Go to attached Proof-of-Concept
5) Click the button on the page
6) Press ENTER key

Result:
Plugin installer dialog appears

Am I doing this wrong or is this not fixed on Firefox 7?
Whiteboard: [sg:critical],[qa+] → [sg:critical],[qa?]
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #54)
> 3) Go the the website in comment 1 (404'd so I'm guessing this is broken)

It's up and running (a slash was missing, see comment 5), although it didn't take account of the different "missing plugin" XUL box in the newer versions of Firefox (see comment 18). I've now repacked signed.jar to include a new file exploitn.htm, which uses the pluginspage link focus method from attachment 547601 [details], so you can just replace "signed.jar!/exploit.htm" with "signed.jar!/exploitn.htm" and proceed as described if you're not using Firefox 5.

Btw, check out the last paragraph in comment 18. If it happens to throw corrupt windows after the alert dialog, kill the process and retry.

> 6) Press ENTER key

And hold it down until the counter freezes.

Firefox 7.0b6, Windows 7 SP1, using a fairly fresh user profile: http://www.youtube.com/watch?v=044-OTl0TZw (xpinstallConfirm.xul fails to open, but the extension is not installed)
Summary: Holding enter allows arbitrary plugin installation. → Holding enter allows arbitrary extension installation.
Website is still 404 for me...
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #56)
> Website is still 404 for me...

I've checked the logs and the last link you were trying to access (with a slash after "!" and "exploitn.htm" as the internal filename) was correct, you just missed the "jar:" prefix before "http:", hence 404.
Attached image Screenshot: Test
Okay, I reran the steps with the correct URL and here is my result.

Firefox 9.0a2 on Ubuntu 11.04 64-bit. Is this expected given the bug is "resolved"?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #58)
> Firefox 9.0a2 on Ubuntu 11.04 64-bit. Is this expected given the bug is
> "resolved"?

Yes, it seems that the trick to keep Services.ww unregistered failed, so there's no sign of any incorrect behaviour (except the thing mentioned in comment 42: the ChromeWindow opener allows the confirm dialog to appear without asking), thus it's all good. It may well be that clobbering enablePrivilege in the callback to keep ww from being defined is a Windows-specific thing -- I've never tested it on Unix/Mac, and on Windows in the most recent Firefox versions it's either a complete failure or a complete success (up to the point of Blair's fix, which silently cancels the installation).
So does that mean this bug can be marked VERIFIED FIXED?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #60)
> So does that mean this bug can be marked VERIFIED FIXED?

Yes, I think so, there's no way it can be exploited in FF7+, and the problem described in comment 42 will be resolved with the fix to bug 655916.
Okay, marking verified. I confirm the same behaviour in Firefox 7.0, 8.0b1, and 9.0a2.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical],[qa?] → [sg:critical][qa!]
Group: core-security
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: