Closed Bug 246448 Opened 20 years ago Closed 20 years ago

can spoof framed sites by changing frame contents

Categories

(Core :: Security, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.7final

People

(Reporter: jruderman, Assigned: jst)

References

Details

(5 keywords, Whiteboard: [sg:fix] fixed-aviary1.0)

Attachments

(10 files, 3 obsolete files)

467 bytes, text/html
Details
273 bytes, text/html
Details
8.66 KB, patch
Details | Diff | Splinter Review
22.84 KB, patch
dveditz
: review-
brendan
: superreview+
Details | Diff | Splinter Review
22.35 KB, patch
Details | Diff | Splinter Review
1.73 KB, patch
Details | Diff | Splinter Review
20.63 KB, patch
dveditz
: review+
Details | Diff | Splinter Review
20.74 KB, patch
Details | Diff | Splinter Review
2.34 KB, application/octet-stream
Details
18.69 KB, patch
caillon
: approval1.4.3+
Details | Diff | Splinter Review
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.
Attached file demo 1, using targets
This testcase is based on Guninski's testcase for bug 13871.
Flags: blocking1.7?
Keywords: testcase
Summary: attacker can change contents of another site's frames → can spoof framed sites by changing frame contents
Lorenzo Colitti suggested these exploits in bug 245406 comment 2.
anyone have thoughts on a fix?
jst, can you have a look fast, for 1.7 final?

/be
Assignee: security-bugs → jst
Flags: blocking1.7? → blocking1.7+
Target Milestone: --- → mozilla1.7final
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.
If I understand things correctly it looks like IE is vulerable as well.  Is that
correct?
IE is vulnerable to both testcases.  Opera is vulnerable to one of them.
what if www.db.com was an https site?  Do you get a mixed content warning message?
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...
> 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.
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?
> 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.
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...
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.
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 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...
Attachment #150729 - Flags: superreview?(brendan)
Attachment #150729 - Flags: review?(dveditz)
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.
Attachment #150729 - Attachment is obsolete: true
Attachment #150729 - Flags: superreview?(brendan)
Attachment #150729 - Flags: review?(dveditz)
Attachment #150732 - Flags: superreview?(brendan)
Attachment #150732 - Flags: review?(dveditz)
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 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?
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
(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.
(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.
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.
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...
Attachment #150732 - Attachment is obsolete: true
Attachment #150763 - Flags: superreview?(brendan)
Attachment #150763 - Flags: review?(dveditz)
Attachment #150732 - Flags: superreview?(brendan)
Attachment #150732 - Flags: review?(dveditz)
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 on attachment 150763 [details] [diff] [review]
Fix targeted links too.

sr=brendan@mozilla.org, pending r= from dveditz, darin, or both!

/be
Attachment #150763 - Flags: superreview?(brendan) → superreview+
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 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 :-(
Attachment #150763 - Flags: review?(dveditz) → review-
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)
(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.
Fixed on both 1.7 branch and trunk now...
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 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.
Attachment #150784 - Flags: review+
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.
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.
I notified Opera about this bug yesterday: bugs.opera.com 145283.

I notified Microsoft about this bug today.
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.
Status: NEW → RESOLVED
Closed: 20 years ago
Keywords: fixed1.7
Resolution: --- → FIXED
Whiteboard: [sg:fix] fixed-aviary1.0
Adding Jon Granrose to CC list to help round up QA resources for verification
adding tracy to verify on 1.7
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?
The patch in bug 246923 fixes that issue and should fix all those bugs except
for the one where multiple windows are involved.
verified
Status: RESOLVED → VERIFIED
Keywords: fixed1.7verified1.7
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.
Group: security
Well, they found the hole in IE, and then tested lots of other browsers.
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! :-)
supposedly this regressed on trunk, see bug 249770
sorry, nevermind me, 1.8a1 is too old to contain this patch.
*** Bug 249770 has been marked as a duplicate of this bug. ***
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?   
Bug 247070 has been closed as DUP of Bug 248753.
Thaks a lot, Johnny Stenback. 
My concern is Bug 249751 only now.
We've found that Bug 249751 has no relation to fix by this bug.
Johnny Stenback, thanks again for your help.
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.
Attachment #152724 - Flags: superreview?(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.
Attachment #152724 - Flags: superreview?(jst) → superreview+
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.

(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.
(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 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
Attachment #154465 - Flags: approval1.4.3+
Blocks: 253961
Note: The Common Vulnerabilities and Exposures project (cve.mitre.org) has
assigned the name CAN-2004-0718 to this issue.
Blocks: sa15601
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.
No longer blocks: sa15601
Flags: testcase+
Flags: in-testsuite+ → in-testsuite?
Collin, Adam, do your tests for bug 408052 cover all the issues in this bug?  Or does this still need more testcases?
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).
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.
Keywords: csec-sop, csec-spoof
You need to log in before you can comment on or make changes to this bug.