Last Comment Bug 537862 - (CVE-2010-0172) asyncAuthPrompt can attach to wrong DOM window.
(CVE-2010-0172)
: asyncAuthPrompt can attach to wrong DOM window.
Status: RESOLVED FIXED
[3.6.x][sg:low spoof]
: regression, verified1.9.2
Product: Toolkit
Classification: Components
Component: Password Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.3a2
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
Depends on:
Blocks: 475053 499233
  Show dependency treegraph
 
Reported: 2010-01-04 19:05 PST by Justin Dolske [:Dolske]
Modified: 2010-04-12 14:32 PDT (History)
10 users (show)
mbeltzner: blocking1.9.2-
mbeltzner: wanted1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.2-fixed
unaffected


Attachments
Patch v.1 (4.22 KB, patch)
2010-01-04 19:05 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.2 (9.24 KB, patch)
2010-01-06 20:13 PST, Justin Dolske [:Dolske]
honzab.moz: review+
gavin.sharp: review+
dveditz: approval1.9.2.2+
Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2010-01-04 19:05:10 PST
Created attachment 420023 [details] [diff] [review]
Patch v.1

Over in bug 499233 I'm adding some code to trigger nsLoginManagerPromper's _doAsyncPrompt from an observer in the factory... [The patch suppresses HTTP auth prompts until a master password has been entered.] I noticed there wasn't a way to map prompts queued in the factory's _asyncPrompts to actual prompter instances (where _doAsyncPrompt lives). And then realized this could be fairly bad. :(

Then prompter initializes a LoginManagerPrompter, it includes the DOMWindow for which the prompt is to be bound. But when _doAsyncPrompt runs, it pulls an arbitrary queued prompt from _asyncPrompts, and processes it in the current prompter instance. If the prompt was queued from a different prompter, it will be attached to the wrong DOM window (ie, tab).

I don't *think* this is directly exploitable, in that that promptAuth() uses the provided channel (not window) for login manager lookups, so we won't end up sending/saving passwords for the wrong host.

But it does mean that there's the potential for evil.com to get it's HTTP auth prompt attached to bank.com's tab, and then an inattentive user will likely type in their bank.com username and password.
Comment 1 Justin Dolske [:Dolske] 2010-01-04 19:07:20 PST
Comment on attachment 420023 [details] [diff] [review]
Patch v.1

This has a little more cleanup than strictly necessary, the key points are to add a .prompter property to the |asyncPrompt| object, and to use that when calling promptAuth().
Comment 2 Justin Dolske [:Dolske] 2010-01-04 19:12:41 PST
Comment on attachment 420023 [details] [diff] [review]
Patch v.1

Haven't extensively tested this yet, but it does pass existing tests.
Comment 3 Mike Beltzner [:beltzner, not reading bugmail] 2010-01-04 20:55:14 PST
Do we know if this affects other branches, too?

(not blocking release, marking for stability release triage)
Comment 4 Justin Dolske [:Dolske] 2010-01-04 21:35:57 PST
This does not affect branches (1.9.2 and down), the relevant code was added for 3.6 in bug 475053.
Comment 5 Justin Dolske [:Dolske] 2010-01-06 20:13:56 PST
Created attachment 420482 [details] [diff] [review]
Patch v.2

Move _doAsyncPrompt() into the factory. Honza suggested this, and I had been thinking about it anyway, so it's done. As a side effect, this fixes a bug where the factory's observer calls this.log(), but there wasn't a log method on the factory.

Also noticed and fixed a potential bug where the observer clears _asyncPrompts... If we ever end up with multiple factories (which shouldn't happen?), we'd want to clear the object on the prototype, not just mask it with an empty object on 1 instance.

Patch is larger because of the shifting of code, but it's otherwise functionally the same as v.1.
Comment 6 Honza Bambas (:mayhemer) 2010-01-12 02:28:42 PST
Comment on attachment 420482 [details] [diff] [review]
Patch v.2

r=honzab

Sorry for the delay.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-01-15 12:34:19 PST
Comment on attachment 420482 [details] [diff] [review]
Patch v.2

>diff --git a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

> LoginManagerPromptFactory.prototype = {

>     _asyncPrompts : {},

I'd prefer just setting this on the instance, in the constructor, to avoid having to refer to __proto__ in observe(). Unless there's some reason multiple factories would actually want to share a list of prompts, but I don't think that makes sense - I don't even know why we'd have multiple factory instances to begin with.

>     getPrompt : function (aWindow, aIID) {

>+        this._debug = prefBranch.getBoolPref("debug");

Do we really need to read the pref for each invocation of getPrompt (not sure whether other prefs are similarly "live")?
Comment 8 Justin Dolske [:Dolske] 2010-01-15 13:17:06 PST
(In reply to comment #7)

> >     _asyncPrompts : {},
> 
> I'd prefer just setting this on the instance

Ditto, I just wasn't sure if we always have a singleton factory... Seemed easier to just bandaid how it works now than to prove that. [If we did end up with multiple factories, we'd want them to share this state.]

> >+        this._debug = prefBranch.getBoolPref("debug");
> 
> Do we really need to read the pref for each invocation of getPrompt (not sure
> whether other prefs are similarly "live")?

Just being lazy again, the patch has already grown enough that I should just cut'n'paste the observer code as used elsewhere.
Comment 9 Justin Dolske [:Dolske] 2010-02-18 13:56:06 PST
Pushed http://hg.mozilla.org/mozilla-central/rev/9f933c33b56d
Comment 10 Mike Beltzner [:beltzner, not reading bugmail] 2010-02-18 14:49:31 PST
Comment on attachment 420482 [details] [diff] [review]
Patch v.2

This is pretty big, so I think I'm gonna let it bake for a while. Dolske, if you can say smart things about relative safety of this patch, this is your cue:
Comment 11 Daniel Veditz [:dveditz] 2010-02-26 14:01:29 PST
Comment on attachment 420482 [details] [diff] [review]
Patch v.2

Approved for 1.9.2.2, a=dveditz for release-drivers
Comment 12 Daniel Veditz [:dveditz] 2010-02-26 14:01:51 PST
We need some tests for the parenting aspects of this change.
Comment 14 Al Billings [:abillings] 2010-03-15 17:13:14 PDT
Is there anything to be done here to test this change?
Comment 15 Justin Dolske [:Dolske] 2010-03-22 13:32:48 PDT
Any HTTP auth prompt usage goes through this path, so that's the the main thing to test. The particular bug here had to do with multiple concurrent HTTP auth requests from different sites. Starting up a bunch of authenticated tabs (for different sites) with session restore and ensuring the prompts are attached to the right tab would exercise this fix.
Comment 16 Al Billings [:abillings] 2010-03-22 14:27:40 PDT
I've been dogfooding 3.6 nightlies during the last few weeks and I have auth in tabs all the time. I haven't seen any issues at all. 

I'm marking this as verified1.9.2.

Dan, you still want tests from a dev for this, right?
Comment 17 Carsten Book [:Tomcat] - PTO-back Sept 4th 2010-03-22 16:04:12 PDT
also all fine with Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2 and multiple logins like recommend from dolske

Note You need to log in before you can comment on or make changes to this bug.