Last Comment Bug 296850 - (sa15601) Frame injection spoofing with targets (bug 246448 returns - SA15601)
(sa15601)
: Frame injection spoofing with targets (bug 246448 returns - SA15601)
Status: RESOLVED FIXED
[sg:fix] workaround: force new window...
: fixed-aviary1.0.5, fixed1.7.9, regression
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
: Adam Lock
: Andrew Overholt [:overholt]
Mentors:
http://secunia.com/multiple_browsers_...
: 296736 297820 299189 (view as bug list)
Depends on:
Blocks: 254921 289204
  Show dependency treegraph
 
Reported: 2005-06-06 11:03 PDT by Daniel Veditz [:dveditz]
Modified: 2007-04-01 15:08 PDT (History)
41 users (show)
dveditz: blocking1.7.9+
dveditz: blocking‑aviary1.0.5+
dveditz: blocking1.8b3+
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
sidebar testcase (671 bytes, text/html)
2005-06-06 22:15 PDT, bugzilla
no flags Details
Move the JS context pushing code into windowwatcher (13.74 KB, patch)
2005-06-07 11:09 PDT, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: review+
brendan: superreview+
brendan: approval‑aviary1.0.5+
brendan: approval1.7.9+
brendan: approval1.8b3+
Details | Diff | Splinter Review
aviary branch version of the above. (11.07 KB, patch)
2005-06-07 16:00 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
1.7 branch version of the above. (10.76 KB, patch)
2005-06-07 16:29 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
fix mozilla_1_7_branch's fire (1.62 KB, patch)
2005-06-07 20:37 PDT, Ginn Chen
no flags Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2005-06-06 11:03:12 PDT
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 2005-06-06 11:10:24 PDT
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 [:bz] (still a bit busy) 2005-06-06 14:35:17 PDT
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 [:bz] (still a bit busy) 2005-06-06 16:39:16 PDT
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 [:bz] (still a bit busy) 2005-06-06 19:13:31 PDT
Or perhaps IsCapabilityEnabled should get the principal from cx like
GetSubjectPrincipal does for cases when fp is null?
Comment 5 bugzilla 2005-06-06 22:15:58 PDT
Created attachment 185528 [details]
sidebar testcase

This is uncommon example but produces the same results.
Comment 6 Taral 2005-06-07 09:14:08 PDT
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 [:bz] (still a bit busy) 2005-06-07 10:00:07 PDT
> Could this be a config option protecting me?

Yes.
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2005-06-07 11:09:13 PDT
Created attachment 185581 [details] [diff] [review]
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 [:bz] (still a bit busy) 2005-06-07 11:42:56 PDT
Comment on attachment 185581 [details] [diff] [review]
Move the JS context pushing code into windowwatcher

Excellent.
Comment 10 Brendan Eich [:brendan] 2005-06-07 14:57:10 PDT
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, jst@mozilla.com) 2005-06-07 16:00:08 PDT
Created attachment 185617 [details] [diff] [review]
aviary branch version of the above.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2005-06-07 16:03:22 PDT
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, jst@mozilla.com) 2005-06-07 16:29:28 PDT
Created attachment 185620 [details] [diff] [review]
1.7 branch version of the above.
Comment 14 Ginn Chen 2005-06-07 20:37:31 PDT
Created attachment 185636 [details] [diff] [review]
fix mozilla_1_7_branch's fire

jst's patch burned Mozilla 1.7 branch
Comment 15 Ginn Chen 2005-06-07 20:46:21 PDT
Comment on attachment 185636 [details] [diff] [review]
fix mozilla_1_7_branch's fire

a=#developers
Comment 16 Haydn Haines 2005-06-08 03:18:11 PDT
(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) 2005-06-08 07:55:54 PDT
Setting this pref in all.js for the Suite worked for me ...

  pref("browser.frame.validate_origin", true); 
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2005-06-08 09:37:19 PDT
Pete, that pref defaults to true....  See
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#3359
Comment 19 Pete Collins (MDG) 2005-06-08 14:15:31 PDT
(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) 2005-06-08 14:41:44 PDT
> 
> 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, jst@mozilla.com) 2005-06-08 14:57:00 PDT
Fixed on trunk and branches.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2005-06-08 17:53:44 PDT
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) 2005-06-09 07:41:04 PDT
(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 [:bz] (still a bit busy) 2005-06-09 08:28:21 PDT
Ah, yes.  In 1.6 the pref defaulted to false.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2005-06-09 17:43:30 PDT
*** Bug 296736 has been marked as a duplicate of this bug. ***
Comment 26 Daniel Veditz [:dveditz] 2005-06-15 13:35:02 PDT
*** Bug 297820 has been marked as a duplicate of this bug. ***
Comment 27 Daniel Veditz [:dveditz] 2005-06-30 01:35:58 PDT
*** Bug 299189 has been marked as a duplicate of this bug. ***
Comment 28 Jeremy C. Reed 2005-07-06 22:44:35 PDT
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) 2005-07-07 05:01:12 PDT
>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 [:bz] (still a bit busy) 2005-07-07 09:56:21 PDT
> 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] 2005-07-08 23:39:23 PDT
(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

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