Closed Bug 537862 (CVE-2010-0172) Opened 14 years ago Closed 14 years ago

asyncAuthPrompt can attach to wrong DOM window.

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a2
Tracking Status
status1.9.2 --- .2-fixed
status1.9.1 --- unaffected

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

(Keywords: regression, verified1.9.2, Whiteboard: [3.6.x][sg:low spoof])

Attachments

(1 file, 1 obsolete file)

Attached patch Patch v.1 (obsolete) — Splinter Review
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.
Flags: blocking1.9.2?
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().
Attachment #420023 - Attachment is patch: true
Attachment #420023 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 420023 [details] [diff] [review]
Patch v.1

Haven't extensively tested this yet, but it does pass existing tests.
Attachment #420023 - Flags: review?(honzab.moz)
Do we know if this affects other branches, too?

(not blocking release, marking for stability release triage)
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Whiteboard: [3.6.x]
This does not affect branches (1.9.2 and down), the relevant code was added for 3.6 in bug 475053.
Whiteboard: [3.6.x] → [3.6.x][sg:low spoof]
Attached patch Patch v.2Splinter Review
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.
Attachment #420023 - Attachment is obsolete: true
Attachment #420482 - Flags: review?(honzab.moz)
Attachment #420023 - Flags: review?(honzab.moz)
Attachment #420482 - Flags: review?(honzab.moz) → review+
Comment on attachment 420482 [details] [diff] [review]
Patch v.2

r=honzab

Sorry for the delay.
Attachment #420482 - Flags: review?(gavin.sharp)
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")?
Attachment #420482 - Flags: review?(gavin.sharp) → review+
(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.
Pushed http://hg.mozilla.org/mozilla-central/rev/9f933c33b56d
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a2
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:
Attachment #420482 - Flags: approval1.9.2.2?
Comment on attachment 420482 [details] [diff] [review]
Patch v.2

Approved for 1.9.2.2, a=dveditz for release-drivers
Attachment #420482 - Flags: approval1.9.2.2? → approval1.9.2.2+
We need some tests for the parenting aspects of this change.
Is there anything to be done here to test this change?
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.
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?
Keywords: verified1.9.2
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
Alias: CVE-2010-0172
Group: core-security
Blocks: 555472
No longer blocks: 555472
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: