Closed
Bug 246448
Opened 21 years ago
Closed 21 years ago
can spoof framed sites by changing frame contents
Categories
(Core :: Security, defect)
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)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
dveditz
:
review-
brendan
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
dveditz
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
application/octet-stream
|
Details | |
(deleted),
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.
Reporter | ||
Comment 2•21 years ago
|
||
Reporter | ||
Updated•21 years ago
|
Summary: attacker can change contents of another site's frames → can spoof framed sites by changing frame contents
Comment 5•21 years ago
|
||
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
Reporter | ||
Comment 6•21 years ago
|
||
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•21 years ago
|
||
If I understand things correctly it looks like IE is vulerable as well. Is that correct?
Reporter | ||
Comment 8•21 years ago
|
||
IE is vulnerable to both testcases. Opera is vulnerable to one of them.
Comment 9•21 years ago
|
||
what if www.db.com was an https site? Do you get a mixed content warning message?
Assignee | ||
Comment 10•21 years ago
|
||
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...
Reporter | ||
Comment 11•21 years ago
|
||
> 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•21 years ago
|
||
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?
Reporter | ||
Comment 13•21 years ago
|
||
> 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.
Assignee | ||
Comment 14•21 years ago
|
||
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...
Assignee | ||
Comment 15•21 years ago
|
||
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.
Assignee | ||
Comment 16•21 years ago
|
||
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).
Assignee | ||
Comment 17•21 years ago
|
||
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)
Comment 18•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Attachment #150729 -
Flags: superreview?(brendan)
Attachment #150729 -
Flags: review?(dveditz)
Assignee | ||
Updated•21 years ago
|
Attachment #150732 -
Flags: superreview?(brendan)
Attachment #150732 -
Flags: review?(dveditz)
Comment 20•21 years ago
|
||
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?
Reporter | ||
Comment 21•21 years ago
|
||
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•21 years ago
|
||
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
Assignee | ||
Comment 23•21 years ago
|
||
(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.
Assignee | ||
Comment 24•21 years ago
|
||
(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•21 years ago
|
||
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.
Assignee | ||
Comment 26•21 years ago
|
||
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
Assignee | ||
Updated•21 years ago
|
Attachment #150763 -
Flags: superreview?(brendan)
Attachment #150763 -
Flags: review?(dveditz)
Assignee | ||
Updated•21 years ago
|
Attachment #150732 -
Flags: superreview?(brendan)
Attachment #150732 -
Flags: review?(dveditz)
Assignee | ||
Comment 27•21 years ago
|
||
Reporter | ||
Comment 28•21 years ago
|
||
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•21 years ago
|
||
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+
Comment 30•21 years ago
|
||
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•21 years ago
|
||
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-
Comment 32•21 years ago
|
||
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)
Assignee | ||
Comment 33•21 years ago
|
||
(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.
Assignee | ||
Comment 34•21 years ago
|
||
Assignee | ||
Comment 35•21 years ago
|
||
Comment 37•21 years ago
|
||
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•21 years ago
|
||
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+
Comment 39•21 years ago
|
||
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•21 years ago
|
||
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.
Reporter | ||
Comment 41•21 years ago
|
||
I notified Opera about this bug yesterday: bugs.opera.com 145283. I notified Microsoft about this bug today.
Comment 42•21 years ago
|
||
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: 21 years ago
Keywords: fixed1.7
Resolution: --- → FIXED
Whiteboard: [sg:fix] fixed-aviary1.0
Comment 43•21 years ago
|
||
Adding Jon Granrose to CC list to help round up QA resources for verification
Reporter | ||
Comment 45•21 years ago
|
||
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?
Assignee | ||
Comment 46•21 years ago
|
||
The patch in bug 246923 fixes that issue and should fix all those bugs except for the one where multiple windows are involved.
Reporter | ||
Comment 48•21 years ago
|
||
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
Comment 50•21 years ago
|
||
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 54•21 years ago
|
||
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•21 years ago
|
||
Bug 247070 has been closed as DUP of Bug 248753. Thaks a lot, Johnny Stenback. My concern is Bug 249751 only now.
Comment 56•21 years ago
|
||
We've found that Bug 249751 has no relation to fix by this bug. Johnny Stenback, thanks again for your help.
Comment 57•21 years ago
|
||
Comment 58•21 years ago
|
||
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)
Assignee | ||
Comment 59•21 years ago
|
||
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+
Comment 60•20 years ago
|
||
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•20 years ago
|
||
(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•20 years ago
|
||
(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 64•20 years ago
|
||
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+
Comment 66•20 years ago
|
||
Note: The Common Vulnerabilities and Exposures project (cve.mitre.org) has assigned the name CAN-2004-0718 to this issue.
Comment 67•20 years ago
|
||
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
Updated•19 years ago
|
Flags: testcase+
Updated•18 years ago
|
Flags: in-testsuite+ → in-testsuite?
![]() |
||
Comment 68•17 years ago
|
||
Collin, Adam, do your tests for bug 408052 cover all the issues in this bug? Or does this still need more testcases?
Comment 69•17 years ago
|
||
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•17 years ago
|
||
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.
Reporter | ||
Updated•11 years ago
|
Keywords: csec-sop,
csec-spoof
You need to log in
before you can comment on or make changes to this bug.
Description
•