296850 - (sa15601) Frame injection spoofing with targets (bug 246448 returns - SA15601)

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Daniel Veditz [:dveditz]]

Frame injection spoofing came back. It was fixed by bug 246448 in Mozilla 1.7
and remained fixed through Firefox 1.0.2

Firefox 1.0.3 and Mozilla 1.7.7 are vulnerable again (as is the trunk/Deer Park).

Comment 1

[Arthur]

Can an automated regression test for this bug be added to the build process so
it won't rear its head again?

Comment 2

[Boris Zbarsky [:bzbarsky]]

URL to test with for now:

data:text/html,<a
href="http://web.mit.edu/bzbarsky/www/testcases/security/frameset.html"
target="_blank">Click here first</a><a href="http://www.mozilla.org"
target="frameUniqueNameNoOneWillThinkOf">Click here second</a>

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK, this is a regression from bug 289204.  Here's what happens:

When we try to do the targeted load, we look for an existing named target.  The
search fails, since there is no target we can access.  We then go ahead and open
a new named window via the window watcher.  Now opening a named window actually
just calls nsWindowWatcher::OpenWindowJS.  This, since bug 289204 was fixed,
pushes the JSContext of the window on the stack, since the caller is C++ code
and thus IsCallerChrome tests true.

So we go ahead and push this JSContext on the stack, then try to open the
window.  But first, we look for an existing window by this name (since
window.open doesn't do such a search itself and all -- this is where the search
happens).  Now when we end up in nsDocShell::ValidateOrigin, we have a JSContext
on the stack.  So the call to GetSubjectPrincipal returns a principal (it
returns null before the change for bug 289204!).  So we do a IsCapabilityEnabled
check for UniversalBrowserWrite (since we have a principal).  But
IsCapabilityEnabled returns PR_TRUE if there is no stack frame (which there
isn't, in this case), no matter whether a JSContext is on the stack or not.  So
we think we have permission to access the docshell and we do the load.

As a stopgap, we could make IsCapabilityEnabled return !cx instead of PR_TRUE
for the !fp case...  Not sure what else that would affect, though.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Or perhaps IsCapabilityEnabled should get the principal from cx like
GetSubjectPrincipal does for cases when fp is null?

Comment 5

[u115577]

Attachment: sidebar testcase

This is uncommon example but produces the same results.

Comment 6

[Taral]

This doesn't work for me. The attempted injection loads in a new tab. Could this
be a config option protecting me?

(Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050601
Firefox/1.0+)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Could this be a config option protecting me?

Yes.

Comment 8

[Johnny Stenback (:jst)]

Attachment: Move the JS context pushing code into windowwatcher

This fixes this bug by moving the JS context pushing code from
nsGlobalWindow::OpenInternal() into the windowwatcher code that does the actual
opening. By doing this we can delay the pushing of the callee windows context
onto the stack to past the place where we look for an existing window, and that
way things work as they should again.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 185581 [details] [diff] [review]
Move the JS context pushing code into windowwatcher

Excellent.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 185581 [details] [diff] [review]
Move the JS context pushing code into windowwatcher

sr+a=me for trunk and both branches.

/be

Comment 11

[Johnny Stenback (:jst)]

Attachment: aviary branch version of the above.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 185581 [details] [diff] [review]
Move the JS context pushing code into windowwatcher

For the record, there's an unused nsCOMPtr<nsIJSContextStack> stack variable in
this trunk patch, I'll remove that before checking in (already removed in the
branch version of this diff)

Comment 13

[Johnny Stenback (:jst)]

Attachment: 1.7 branch version of the above.

Comment 14

[Ginn Chen]

Attachment: fix mozilla_1_7_branch's fire

jst's patch burned Mozilla 1.7 branch

Comment 15

[Ginn Chen]

Comment on attachment 185636 [details] [diff] [review]
fix mozilla_1_7_branch's fire

a=#developers

Comment 16

[Haydn Haines]

(In reply to comment #6)
> This doesn't work for me. The attempted injection loads in a new tab. Could this
> be a config option protecting me?
> 
> (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050601
> Firefox/1.0+)

FYI: The TabMix extension fixes this bug, I assume this is because
target="_blank"'s are opened in new tabs instead of new windows.

Haydn.

Comment 17

[Pete Collins (MDG)]

Setting this pref in all.js for the Suite worked for me ...

  pref("browser.frame.validate_origin", true); 

Comment 18

[Boris Zbarsky [:bzbarsky]]

Pete, that pref defaults to true....  See
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#3359

Comment 19

[Pete Collins (MDG)]

(In reply to comment #18)
> Pete, that pref defaults to true....  See
> http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#3359

Yes, but I believe because the pref doesn't exist, the call to

  rv = mPrefs->GetBoolPref("browser.frames.enabled", &tmpbool);

returns a failure code, in turn mAllowSubframes is not set to true.

See:

  http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#3350

It has already been verified having this pref set to true fixes this issue on
the suite (we applied to 1.6 branch for Linspire). 

I have verified it with binary distributions. 

Mozilla Suite: 1.6, 1.7.5, 1.7.6 - that's as far as I tested.

It doesn't seem to work w/ Firefox (not sure why). 

Comment 20

[Pete Collins (MDG)]

> 
> See:
> 
>   http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#3350

Scratch that, I was looking at the wrong per there ...

Anyway, the point being it is a simple fix for the suite ...

Comment 21

[Johnny Stenback (:jst)]

Fixed on trunk and branches.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Pete, Mozilla 1.7.5 and 1.7.6, are not vulnerable to this bug -- this bug was a
regression starting with 1.7.7.  See comment 0.  So I'm not sure what you were
really testing, but it wasn't this bug...

Comment 23

[Pete Collins (MDG)]

(In reply to comment #22)
> Pete, Mozilla 1.7.5 and 1.7.6, are not vulnerable to this bug -- this bug was a
> regression starting with 1.7.7.  See comment 0.  So I'm not sure what you were
> really testing, but it wasn't this bug...

I used the pref to fix 1.6 which is vulnerable by this bug. Sorry about the 1.7
mixup.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  In 1.6 the pref defaulted to false.

Comment 25

[Boris Zbarsky [:bzbarsky]]

*** Bug 296736 has been marked as a duplicate of this bug. ***

Comment 26

[Daniel Veditz [:dveditz]]

*** Bug 297820 has been marked as a duplicate of this bug. ***

Comment 27

[Daniel Veditz [:dveditz]]

*** Bug 299189 has been marked as a duplicate of this bug. ***

Comment 28

[Jeremy C. Reed]

Is there a specific patch for this bug fix?

Are the only files fixed related to this security issue:
mozilla/dom/src/base/nsGlobalWindow.cpp and
mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp ?

What are the cvs revision numbers for the fixed files? Does mozilla offer any
webbased cvs (cvsweb) so I can review diffs?

Does this fix also cover http://secunia.com/advisories/15489/ ?

(I am working on updating firefox package for pkgsrc.)

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

>What are the cvs revision numbers for the fixed files? Does mozilla offer any
>webbased cvs (cvsweb) so I can review diffs?

you can use http://bonsai.mozilla.org, querying by date should hopefully give
you the revision numbers.

Comment 30

[Boris Zbarsky [:bzbarsky]]

> Is there a specific patch for this bug fix?

Yes, it's attached to this bug.

For the rest, see lxr.mozilla.org and bonsai.mozilla.org

Comment 31

[Daniel Veditz [:dveditz]]

(In reply to comment #28)
> Does this fix also cover http://secunia.com/advisories/15489/ ?

No, that's a completely unrelated issue. See bug 298934