246448 - can spoof framed sites by changing frame contents

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Jesse Ruderman]

This security hole lets an attacker spoof any site that uses frames.

It is like bug 13871 and bug 20682 (in fact, it is a regression of the first). 
I don't know if it regressed due to the fix for bug 52920 or if it regressed
somewhere else.

Comment 1

[Jesse Ruderman]

Attachment: demo 1, using targets

This testcase is based on Guninski's testcase for bug 13871.

Comment 2

[Jesse Ruderman]

Attachment: demo 2, using window.frames[n] to access frames

Comment 3

[Jesse Ruderman]

Lorenzo Colitti suggested these exploits in bug 245406 comment 2.

Comment 4

[chris hofmann]

anyone have thoughts on a fix?

Comment 5

[Brendan Eich [:brendan]]

jst, can you have a look fast, for 1.7 final?

/be

Comment 6

[Jesse Ruderman]

In bug 52920, I advocated disallowing scripts from accessing the frames of a
frameset from another domain.  We found that that policy broke a small number of
sites in which sibling frames needed to talk to each other.

There is a compromise policy: disallow access to frames of a frameset from
another domain in general but allow access to sibling frames (or even all frames
in the same content area as the script).  This compromise policy would prevent
an attacker from spoofing a bank but would not break as many sites.

I still advocate the strict policy, because it's the only way to fix this hole
for the case of webmail sites that display external links in frames.

Comment 7

[chris hofmann]

If I understand things correctly it looks like IE is vulerable as well.  Is that
correct?

Comment 8

[Jesse Ruderman]

IE is vulnerable to both testcases.  Opera is vulnerable to one of them.

Comment 9

[Doug Turner (:dougt)]

what if www.db.com was an https site?  Do you get a mixed content warning message?

Comment 10

[Johnny Stenback (:jst)]

My weekend was overbooked, so I didn't see this yet, and now I've gotta take off
agian... I might be able to have a look tonight, if not, tomorrow...

Comment 11

[Jesse Ruderman]

> what if www.db.com was an https site?  Do you get a mixed content warning 
> message?

I tested with gmail, which supports https and uses frames.

I did get a "mixed secure and non-secure" warning, but only after I toggled a
hidden pref.  Firefox normally only shows that dialog the first time you
encounter mixed content, and I guess I had encountered mixed content at some
point.  The lock did change to a "broken" appearance, though.

Of course, if the attacker owns (in either sense) another https server, there is
no warning and the lock icon retains its "secure" appearance.

Comment 12

[Doug Turner (:dougt)]

Mix content on http sites has been known about for _years_.  Mix content on an
https site displays a dialog.  I do not think that this is a major bug yet.

If the attacker owns an https site, the attacker must have a cert issued by a
trusted ca.  How will the attack play out in this case?

Comment 13

[Jesse Ruderman]

> Mix content on http sites has been known about for _years_.  

Can you elaborate?  What has been known about for years?  Who has known about it?

> Mix content on an https site displays a dialog.

As I said in comment 11, it often does not display a dialog.

> If the attacker owns an https site, the attacker must have a cert issued by a
> trusted ca.  How will the attack play out in this case?

It's not hard to get such a cert.  But if the attacker doesn't want his name
associated with his attack, he'll just break into a random https server on the
web and launch his attack from there.

Finally, it sounds like you would only consider this a major hole if I could
convince you that it affects https sites.  I think it would be nearly as bad
even if it only affected http sites.

Comment 14

[Johnny Stenback (:jst)]

Attachment: Patch that makes mozilla require write permissions to the frameset document in order to change the location of a frame (probably too restrictive).

This patch fixes this bug, but I suspect that it's too restrictive and might
break some "valid" uses of frames/iframes out there. I'll see if I can come up
with something along Jesse's proposal above...

Comment 15

[Johnny Stenback (:jst)]

Oh, and out of those changes, only the nsDocShell.cpp changes are needed, the
rest is cleanup and error code propoagation fixes that aren't needed to fix this
bug, but we'd want those on the trunk at least, IMO.

Comment 16

[Johnny Stenback (:jst)]

Attachment: Always permit loads in a frame from a frame in the same toplevel window.

This should be safer, along the Jesse's lines, though not quite the same. I
think this should be ok, with this patch, Mozilla lets anything be loaded into
a frame, as long as the request for the load comes from the same top-level
window, if not, same-origin rules apply, the caller needs to be in the same
origin as the callee's parent (i.e. the frameset, or iframe host document).

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 150729 [details] [diff] [review]
Always permit loads in a frame from a frame in the same toplevel window.

Thoughts? Again, only the nsDocShell changes are needded to fix this bug, the
rest is secondary changes...

Comment 18

[Asa Dotzler [:asa]]

Just to clarify on the mixed content warning, Firefox's behavior is slightly
different from Seamonkey's. It does not have the "show me this warning every
time" checkbox checked so the first time you dismiss it is the last time you'll
see it. SeaMonkey does have this show me again box checked so you'll get the
warning until you actually say "don't show me again". See bugs 206863 and bug
172091.

Comment 19

[Johnny Stenback (:jst)]

Attachment: Same as above, but with a pref that lets us disable these checks

Comment 20

[chris hofmann]

jesse, can you apply the patch and start testing?

I guess we need some focused testing and then a round up of sites that use
frames in strange ways that we might possibly break...   

doron, can you help find some sites that do some interesting things with frames?

Comment 21

[Jesse Ruderman]

Comment on attachment 150732 [details] [diff] [review]
Same as above, but with a pref that lets us disable these checks

The patch doesn't fix the first testcase if you click the buttons manually
(rather than with JS on a timer).

With the first testcase, nothing appears in the JS console indicating that
something was blocked.	Something should appear in the JS console (regardless
of whether JS is used) like we do for other security measures.

Are the changes in nsHTMLFormElement.cpp part of the fix for this bug?

Comment 22

[Doron Rosenberg (IBM)]

I tried applying this to the 1.7 branch, and got:

nsDocShell.cpp: In member function `nsresult
   nsDocShell::CheckLoadingPermissions()':
nsDocShell.cpp:4941: error: no matching function for call to `
   nsDerivedSafe<nsIScriptObjectPrincipal>::GetPrincipal()'
../../dist/include/dom/nsIScriptObjectPrincipal.h:59: error: candidates are:
   virtual nsresult nsIScriptObjectPrincipal::GetPrincipal(nsIPrincipal**)
nsDocShell.cpp: In member function `virtual nsresult
   nsDocShell::InternalLoad(nsIURI*, nsIURI*, nsISupports*, int, const
   PRUnichar*, const char*, nsIInputStream*, nsIInputStream*, unsigned int,
   nsISHEntry*, int, nsIDocShell**, nsIRequest**)':
nsDocShell.cpp:5046: warning: enumeral mismatch in conditional expression: `
   nsIContentPolicy::<anonymous enum>' vs `nsIContentPolicy::<anonymous enum>'
gmake[3]: *** [nsDocShell.o] Error 1

Comment 23

[Johnny Stenback (:jst)]

(In reply to comment #21)
> (From update of attachment 150732 [details] [diff] [review])
> The patch doesn't fix the first testcase if you click the buttons manually
> (rather than with JS on a timer).

Ah, true. Looking...

> With the first testcase, nothing appears in the JS console indicating that
> something was blocked.	Something should appear in the JS console (regardless
> of whether JS is used) like we do for other security measures.
> 
> Are the changes in nsHTMLFormElement.cpp part of the fix for this bug?

Yes, the nsHTMLFormElement.cpp changes make the error appear on the JS console
in the first testcase as well, though only when the form is submitted from JS.

Comment 24

[Johnny Stenback (:jst)]

(In reply to comment #22)
> I tried applying this to the 1.7 branch, and got:
> 
> nsDocShell.cpp: In member function `nsresult
>    nsDocShell::CheckLoadingPermissions()':
> nsDocShell.cpp:4941: error: no matching function for call to `
>    nsDerivedSafe<nsIScriptObjectPrincipal>::GetPrincipal()'
> ../../dist/include/dom/nsIScriptObjectPrincipal.h:59: error: candidates are:
>    virtual nsresult nsIScriptObjectPrincipal::GetPrincipal(nsIPrincipal**)
> nsDocShell.cpp: In member function `virtual nsresult
>    nsDocShell::InternalLoad(nsIURI*, nsIURI*, nsISupports*, int, const
>    PRUnichar*, const char*, nsIInputStream*, nsIInputStream*, unsigned int,
>    nsISHEntry*, int, nsIDocShell**, nsIRequest**)':
> nsDocShell.cpp:5046: warning: enumeral mismatch in conditional expression: `
>    nsIContentPolicy::<anonymous enum>' vs `nsIContentPolicy::<anonymous enum>'
> gmake[3]: *** [nsDocShell.o] Error 1
> 

Hmm, yeah, API differences. On the 1.7 branch, try changing the lines:

    nsIPrincipal *parentPrincipal;
    if (!sop || !(parentPrincipal = sop->GetPrincipal())) {

to:

    nsCOMPtr<nsIPrincipal> parentPrincipal;
    if (!sop || NS_FAILED(sop->GetPrincipal(getter_AddRefs(parentPrincipal))) ||
        !parentPrincipal) {

If that doesn't work, let me know and I'll whip up a patch that applies cleanly
and works on the 1.7 branch.

Comment 25

[Daniel Veditz [:dveditz]]

Hard to find real-world complicated frameset sites these days (like the one for
which the change in bug 52920 was made). Good, in that if we regress those it
probably won't affect many popular sites. Bad because these sites live on inside
corporate intranets but we won't know about it until a regression drives these
people out of the woodwork.

Comment 26

[Johnny Stenback (:jst)]

Attachment: Fix targeted links too.

This fixes all the known spoofs (using JS, targeted form submissions, and
targeted links). So far I haven't found any frameset sites that are broken
because of this...

Comment 27

[Johnny Stenback (:jst)]

Attachment: Same thing for the aviary branch.

Comment 28

[Jesse Ruderman]

I tested the new trunk patch.  It works.

I see the JS console error with the first testcase now ("access to property
denied").  My problem was that I didn't run |make| in the layout directory.

Nothing happens when I click the buttons with target= manually in the first
testcase.  I should get a JS console error or a new window (as if the frameset
page didn't exist at all). This problem isn't worth holding up releases and I
wouldn't mind filing a new bug for it.

Does the patch for the aviary branch also work on the 1.7 branch?

Comment 29

[Brendan Eich [:brendan]]

Comment on attachment 150763 [details] [diff] [review]
Fix targeted links too.

sr=[e-mail], pending r= from dveditz, darin, or both!

/be

Comment 30

[Daniel Veditz [:dveditz]]

In addition to form submits failing silently, <a> links do as well. That's
probably obvious but I wanted to be explicit about it. Looks pretty good so far
but I'm still trying a few more things.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 150763 [details] [diff] [review]
Fix targeted links too.

r- about:plugins and about:about are broken

>   ResetToURI(uri, aLoadGroup);
> 
>-  if (aChannel) {
>-    nsCOMPtr<nsISupports> owner;
>-    aChannel->GetOwner(getter_AddRefs(owner));
>+  if (uri) {
>+    // Use the channel owner as our principal if we're loading a
>+    // javascript:, data:, or chrome: URI.
>+    PRBool useOwner = PR_FALSE;
>+    if (NS_FAILED(uri->SchemeIs("javascript", &useOwner)) || useOwner ||
>+        NS_FAILED(uri->SchemeIs("data", &useOwner)) || useOwner ||
>+        NS_FAILED(uri->SchemeIs("chrome", &useOwner)) || useOwner) {
>+      nsCOMPtr<nsISupports> owner;
>+      aChannel->GetOwner(getter_AddRefs(owner));

Looks like you went down the same path in nsDocument.cpp I tried for the
xpinstall whitelisting (so I didn't have to treat ftp links different than
http). I'm happy to see Chatzilla working after your change, somehow my version
managed to break it.

I think I explicitly added about: to the list of schemes where you test for js,
data, and chrome in order to get about:plugins working.

Earlier in that routine resource: is lumped with chrome: so I threw that in
too, but I didn't really look into whether that was necessary or safe since by
that time I had nearly given up on the approach.

continuing review and testing, but wanted to let you know the bad news :-(

Comment 32

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: fix about:plugins

basically we need to also exempt resource: uris and resource: uris embedded in
jar uris, so get the real url segment from inside the uri if it's a jar uri
(recursively if necessary) and check to see if that's one of the allowed sets
(which now includes resource  this is secure since it points itno the app bin
dir)

Comment 33

[Johnny Stenback (:jst)]

(In reply to comment #31)
> Looks like you went down the same path in nsDocument.cpp I tried for the
> xpinstall whitelisting (so I didn't have to treat ftp links different than
> http). I'm happy to see Chatzilla working after your change, somehow my version
> managed to break it.

Yeah, it was based on our discussions about your changes that I went down this
path, and only now did I realize that I don't need those changes at all. IOW,
the patch, w/o any changes to nsDocument.cpp is what we want, pending any other
problems.

The reason I don't need the nsDocument changes is that even though I pass in an
owner to the docshell loading code more often now, the docshell still only
passes the owner to the channel if the URI is a javascript: or data: URI, and
that's what matters for the code in nsDocument::Reset().

This twisted mess is badly in need of some cleanup, once the branches are taken
care of, I'll leave this bug open to clean this up, or I'll file a followup bug
on the cleanup.

Comment 34

[Johnny Stenback (:jst)]

Attachment: Updated patch for the trunk.

Comment 35

[Johnny Stenback (:jst)]

Attachment: Same thing for the 1.7 branch.

Comment 36

[Johnny Stenback (:jst)]

Fixed on both 1.7 branch and trunk now...

Comment 37

[Daniel Veditz [:dveditz]]

Attachment: frames testcases (.zip)

To use this testcase as is you need to run a web server on localhost, and add
the following aliases in your hosts file: search.localhost, content.localhost
and www.localhost

There are two testcases with the same basic structure, a top "menu bar" frame,
a left navigation frame, and a content frame. The navigation and content frames
themselves each contain two frames. http://localhost/framespoof/topframe.html
loads the top frame from localhost, and the left and right frames from
content.localhost.  http://localhost/framespoof/topmulti.html is mostly the
same except the left navigation frame is loaded from search.localhost

All links work prior to this patch. They also work in IE which can be spoofed
by both of Jesse's demos.

After the patch with topframe.html:
 - left and right frames can target each other
 - top frame can target the left and right outer frames but not
   any of the subframes

with topmulti.html
 - same restrictions on the top frame
 - left and right frames cannot target each other

As an added bonus load either testcase using www.localhost as the host and now
the top frame can't even target the outer left or right frames.

This frame arrangement doesn't seem overly complicated or unreasonable, I'm
sure I've used sites that worked much like these examples. We're going to get a
lot of squawking on this (mostly corporate and university intranet users I
suspect).

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 150784 [details] [diff] [review]
Updated patch for the trunk.

r=dveditz for the code (thanks for fixing the null checks), but I think we'll
be pretty unhappy with the number of resulting regressions.

Comment 39

[chris hofmann]

rachel has rounded up a few more test cases of frame content generated out of
Front Page and some general sites.   testing looks good so far in the things she
has checked.  more results and test cases on the way.

Comment 40

[chris hofmann]

bob may also be able to help with testing ideas and the search for public sites
the might be affected.   It might also be good to pull together a evangelism
blurb to describe how any affected sites can change for the required
compatibility needed for the security protection offered by this fix.

Comment 41

[Jesse Ruderman]

I notified Opera about this bug yesterday: bugs.opera.com 145283.

I notified Microsoft about this bug today.

Comment 42

[Daniel Veditz [:dveditz]]

Bug has been fixed on trunk and the 1.7 and aviary branches (in time for ff0.9).
Any future refinements to address as-yet-undiscovered site regressions should go
in a new bug.

Comment 43

[Daniel Veditz [:dveditz]]

Adding Jon Granrose to CC list to help round up QA resources for verification

Comment 44

[Jon Granrose]

adding tracy to verify on 1.7

Comment 45

[Jesse Ruderman]

I searched bugzilla for comments containing docshell.frameloadcheck.disabled.  I
found three bugs:

bug 248138 (sbroker.de), which uses a pop-up "transaction area"
bug 246844 (PHPMyAdmin), sounds like it's all in one window
bug 247016 (some framed site), sounds like it's all in one window

Only sbroker.de uses separate windows.  The others should be fixed once jst
figures out how to make targeted link loads follow the same policy as script
loads.  Is there a bug filed for that?

Comment 46

[Johnny Stenback (:jst)]

The patch in bug 246923 fixes that issue and should fix all those bugs except
for the one where multiple windows are involved.

Comment 47

[Tracy Walker [:tracy]]

verified

Comment 48

[Jesse Ruderman]

This hole is public now:
http://developers.slashdot.org/article.pl?sid=04/07/01/1741243
http://secunia.com/advisories/11978/

The Slashdot article makes it sound like someone found the hole independently
rather than by reverse-engineering the Mozilla patch.

Comment 49

[alanjstr]

Well, they found the hole in IE, and then tested lots of other browsers.

Comment 50

[Lorenzo Colitti]

HAH!

> The following browsers are not affected:
> * Mozilla Firefox 0.9 for Windows
> * Mozilla Firefox 0.9.1 for Windows
> * Mozilla 1.7 for Windows
> * Mozilla 1.7 for Linux

... and that's all. We done good! :-)

Comment 51

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

supposedly this regressed on trunk, see bug 249770

Comment 52

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

sorry, nevermind me, 1.8a1 is too old to contain this patch.

Comment 53

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

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

Comment 54

[WADA:World Anti-bad-Duping Agency]

Two bug are reported which seems to be affected by fix for this bug.
 - Bug 247070(FRAME case) and Bug 249751(IFRAME case)
Are these new bugs? Or normal?
Or result of incomplete fix for this bug on release build of Firefox 0.9/Firefox
0.9.1? 

What is expected result of Bug 249751?
When I tried to re-create Bug 249751 problem on Trunk Nighlty of Mozilla Suites,
problem could not be recreated.
Was the fix for this bug checked-in on Mozilla Suite?
How can we know whether the fix for this bug is applied or not on my using
Moziila?   

Comment 55

[WADA:World Anti-bad-Duping Agency]

Bug 247070 has been closed as DUP of Bug 248753.
Thaks a lot, Johnny Stenback. 
My concern is Bug 249751 only now.

Comment 56

[WADA:World Anti-bad-Duping Agency]

We've found that Bug 249751 has no relation to fix by this bug.
Johnny Stenback, thanks again for your help.

Comment 57

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Backport to 1.4 branch

Comment 58

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 152724 [details] [diff] [review]
Backport to 1.4 branch

Jst, care to have a look at this?  The patch should be good -- it even works
against the supplied testcases -- but I'd like you to sign off on it anyway
since there were some changes due to conflict resolution, different APIs, etc.

Comment 59

[Johnny Stenback (:jst)]

Comment on attachment 152724 [details] [diff] [review]
Backport to 1.4 branch

sr=jst, but this caused regression bug 246923, which backs out part of this
change and makes some other changes. I wouldn't take this w/o intrgrating the
fix for the regression, and I'd take them both as one combined patch (easier to
review too), so that's my suggestion, don't don't land this as is, merge in the
fix for the regression.

Comment 60

[Jim Burrill]

Having some difficulty with this fix when some frames are in the same domain,
but are using multiple servers in the same domain for different frames.

Example:  A frameset with the parent comming from www1.domain.com, left subframe
comming from www1.domain.com, and right subframe comming from www2.domain.com. 
Using a target <a href="www1.domain.com" target="right"> located in the left
frame is not being allowed to re-populate the right frame under this fix.  If
the original right frame was loaded with a page from www1.domain.com, then it
works until the right frame is loaded with www2.domain.com, then it does not.

Comment 61

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #60)

> Example:  A frameset with the parent comming from www1.domain.com, left subframe
> comming from www1.domain.com, and right subframe comming from www2.domain.com. 
> Using a target <a href="www1.domain.com" target="right"> located in the left
> frame is not being allowed to re-populate the right frame under this fix.  If
> the original right frame was loaded with a page from www1.domain.com, then it
> works until the right frame is loaded with www2.domain.com, then it does not.

Does setting document domain to same one("domain.com") work?
(Issue document.domain="domain.com"; at both sites) 
This is normal way to avoid denial of "Cross Site Scripting" by "Same Origin
Policy" when multiple servers in a domain are used.

Comment 62

[Jim Burrill]

(In reply to comment #61)
> (In reply to comment #60)
> 
> > Example:  A frameset with the parent comming from www1.domain.com, left subframe
> > comming from www1.domain.com, and right subframe comming from www2.domain.com. 
> > Using a target <a href="www1.domain.com" target="right"> located in the left
> > frame is not being allowed to re-populate the right frame under this fix.  If
> > the original right frame was loaded with a page from www1.domain.com, then it
> > works until the right frame is loaded with www2.domain.com, then it does not.
> 
> Does setting document domain to same one("domain.com") work?
> (Issue document.domain="domain.com"; at both sites) 
> This is normal way to avoid denial of "Cross Site Scripting" by "Same Origin
> Policy" when multiple servers in a domain are used.


Yes it does, sorry to bother.  Thank you for your assistance!

Comment 63

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: 1.4 backport with regression fixes

Comment 64

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 154465 [details] [diff] [review]
1.4 backport with regression fixes

i had jst take a quick look and he said this looks good: "ship it".

a=blizzard for 1.4.3

Comment 65

[Christopher Aillon (sabbatical, not receiving bugmail)]

Landed for 1.4.3

Comment 66

[Mark Cox]

Note: The Common Vulnerabilities and Exposures project (cve.mitre.org) has
assigned the name CAN-2004-0718 to this issue.

Comment 67

[Daniel Veditz [:dveditz]]

This bug has returned in Firefox 1.0.3, Mozilla 1.7.7, and the trunk. Opened bug
296850 for the regression to avoid confusion with old discussion and patches.

Comment 68

[Boris Zbarsky [:bzbarsky]]

Collin, Adam, do your tests for bug 408052 cover all the issues in this bug?  Or does this still need more testcases?

Comment 69

[Adam Barth]

This is covered by test_bug13871.html, which tests for the same things as demo 1 and demo 2 (as well as two other methods of navigating the frame).

Comment 70

[Adam Barth]

Oh, I forgot this was the bug with the big zip file test case.  I think we cover all the cases there, but the tests are spread out among a bunch of tests and we don't have a test case with such a complex frame hierarchy.  One possibility is to copy the zip file as a giant test case, but it's probably more useful to keep the more focused tests we have now.