Build 1999091409 allows frames spoofing using two frames. The idea is: Load the first frame (with TARGET from <A> or <FORM>) with the spoofed location. The location bar changes and it is OK yet. Then load the second frame with its original location - at this moment the location bar changes with that of the original frame and the first frame is spoofed. Advice: Make it as it is in NC 4.61 when TARGET is used another window is opened instead of overwriting an existing frame. The code is: <SCRIPT> a=window.open("http://www.citybank.com"); setTimeout("document.forms[0].submit()",30000); setTimeout("document.forms[1].submit()",35000); </SCRIPT> <FORM ACTION="http://www.mozilla.org/" TARGET="main"> <INPUT TYPE=SUBMIT VALUE="First"> </FORM> <FORM ACTION="http://www.citybank.com/sideThing.htm" TARGET="thing"> <INPUT TYPE=SUBMIT VALUE="Second"> </FORM>

Post-beta for spoofing.

Bulk moving all Browser Security bugs to new Security: General component. The previous Security component for Browser will be deleted.

Changing Qa contact to myself.

Bulk reassigning most of norris's bugs to mstoltz.

Assigning QA to czhang

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

Confirmed. Georgi's testcase no longer works; use the above URL instead. I agree that we should open a new window in this case. Nominating nsbeta2.

Recommend we take Georgi's advice: when a frame which is loaded from a different host as the frameset that contains it does a load with a TARGET attribute, where the target is another frame, we should load in a new window rather than overwriting the frame. This is the behavior 4.x follows. Reassigning to Layout; maybe layout folks know how to do this easier than I.

cc'ing myself.

Reassigning to Eric.

May be a docshell issue because both <A> and <FORMS> generate a link click event, and webshell deals with that, afaik. CC'ing Adam to get his opinion.

Mitchell, per your 2000-06-19 16:37 post, would it be okay to instead of opening up a new window for such a frame, begin searching for the named target frame at children of the highest frame in the hierarchy that is from that host? i.e. Say nyse.com is a frameset website with two frame, "left" and "right" (any similarities between this and existing attacks is purely coincidental ;) ) Say that citibank has links in the frame "left" that target the frame "right". With your scheme, these links will pop open a new window. Instead, if we could target the correct frame "right" we would not need to do that We want citibank to work rationally in this site: INNOCENT SITE mysite.com mysite.com/index.html | ----------------------------- | | "navbar" mysite.com/nav.html "main" citibank.com/index.html | ------------------------------------- | | "left" citibank.com/left.html "right" citibank.com/login.html But not allow this site to grab loads targeted at citibank's "right" frame: MALICIOUS SITE mysite.com mysite.com/index.html | ----------------------------- ------------------------ | | | "navbar" mysite.com/nav.html "main" citibank.com/index.html "right"(hidden) | ------------------------------------- | | "left" citibank.com/left.html "right" citibank.com/login.html One way would be change the way a targetted link in "left" is handled. Instead of beginning the search at the top (mysite.com), travel up the heirarchy from "left" until we hit a frame that has a parent from a different host (we hit "main" in this example). Begin searching for the named target with the children of this frame (children of "main" above). I don't know if this is feasible, but seems to solve the same problem without popping up as many windows. Would this work?

Note that this fix (or any security fix for frame spoofing) is a break with the method recommended in the HTML 4 spec: http://www.w3.org/TR/REC-html40/appendix/notes.html#notes-frames But we have to do it anyway. ;)

Eric, you're right about us violating the HTML 4 spec, but if this problem is exploied, people will blame us and not the W3C. Your proposal sounds fine to me. In the case of the testcase in the URL field above, the attempt to spoof would still cause a new window to be opened, right?

Do you mean the test case here? http://warp.mcom.com/u/mstoltz/bugs/13871a.html This opens up one new window with a window.open, but when the form is submitted, a new window won't be opened. But then, neither do IE 5 or Nav 4.73 :) I think that's the right behaviour. From what I understand: It's okay to submit a form targetted to a frame that you created, regardless of the URL the form is submitted to - this is equivalent to setting the document.location of that frame to something. As long as the frame was created by your host, it should be able to be changed by your host. Again, from what I understand, what wouldn't be okay is for one site to submit a form to a frame that another site created (target=othersitesframename). It also isn't okay for a site (A) to be able to create a frame that captures form submits that another site (B) intended to go to one of it's own (B) frames. Is that right? CC'ing danm because he has worked with the logic that resolves frame names.

Eric, When you say, "a frame that another site created," are you including the case where site A does a window.open("site B"), and site B is a frameset containing multiple frames? A should not be able to change the location of one of B's frames in this case, even though A opened the window.

Yes. That's included in what I meant by "a frame that another site created" This is a little different from http://warp.mcom.com/u/mstoltz/bugs/13871a.html because http://warp.mcom.com/u/mstoltz/bugs/13871a.html opens up a window containing a document that is also on warp.mcom.com. If it contained a document on another host, then I agree that http://warp.mcom.com/u/mstoltz/bugs/13871a.html should not be able to update the frames inside of it, including via form submission. It seems that a summary is that when searching for a named frame, we should not cross host-change boundaries in walking upwards to the root frame, or searching downwards from the root frame for the named frame. And as usual, if the named frame is not found, we should open up a new window. Sound right?

Is that policy too restrictive? :S

Whoops, my mistake. The target page for 13871a.html is now on a different server. Your explanatiopn sounds right, Eric. Can we implement it that way?

Putting on [nsbeta2-] radar. Not critical to beta2.

This bug has been marked "future" because the original netscape engineer working on this is over-burdened. If you feel this is an error, that you or another known resource will be working on this bug,or if it blocks your work in some way -- please attach your concern to the bug for reconsideration.

Nominating nsbeta3 - please reconsider this bug. This is a security exploit by which a malicious site could replace a frame on another site with a "spoofed" frome from the malicious site, potentially inducing a user to give up a password or other sensitive data. Netscape's exposure on this issue could be significant.

Removing from Future list per above recommendation

Marking nsbeta3+

Setting priority to P1

PDT agrees P1

Putting [pdtp1] in whiteboard

This may already be fixed. I've found 'the' frame spoofing bug that caused Nav 4.51 to go out: http://home13.inet.tele.dk/kruse/frame_spoofing.htm Unfortunately, Citibank's site no longer uses framesets, so this bug can not be demonstrated there. I recreated the old test case here: http://blueviper.mcom.com/frames/spoof.html When we try to do the illegal frame updating, we get this on the console: JavaScript error: line 0: uncaught exception: [Exception... "Access to property denied" code: "1010" nsresult: "0x805303f2 (NS_ERROR_DOM_PROP_ACCESS_DENIED)" location: "http://blueviper.mcom.com/frames/spoof.html Line: 14"] I'll look around to find other spoofing exploits we may be susceptible to.

Oh, yeah - the site in the URL above is still getting spoofed. :) This may only be a problem for the form submit/link click case and not through dom access to the frames[] array.

See the following bugsplat bugs for more info on the fix for 4.5/4.51: https://scopus.mcom.com/bugsplat/show_bug.cgi?id=338737 https://scopus.mcom.com/bugsplat/show_bug.cgi?id=336170 One thing I've learned from looking these over is that the fix went in and was revised several times because it broke existing sites. The sites were broken because the original fix was too restrictive. The fix was relaxed to allow different hosts from the same domain to update each others frames. I've also pinned down Nav's exact behaviour with some test cases, and narrowed down the location where the security check should probably occur. On the way to a fix...

PDT marking nsbeta3-, nominating for rtm

Just want to point out that if this is implemented such that different hosts form the same domain can access each other's frames, this behavior is inconsistent with that of cross-frame DOM access, which is granted only to pages from the same host, unless both pages set document.domain to their common factor. Eric, how do you determine "domain?" Are two hosts in .co.uk or mountainview.ca.us considered to be in the same domain?

Mitch, that's exactly what I was wondering! Here's strange, but somehow reasonable logic from XP_ValidateDomain() (Nova branch, internal lxr): http://lxr/nova/source/lib/xp/xp_cntxt.c#269 (origin_host is the full origin host name, e.g. "bugzilla.mozilla.org") ... host_len = strlen(origin_host); // e.g. 20 ... (target_domain is the full target host name, e.g. "mozilla.org") ... int domain_len = strlen(target_domain); // e.g. 11 if((host_len > domain_len) && !(XP_STRCASECMP((origin_host+host_len-domain_len), target_domain)) && (origin_host[host_len-domain_len-1] == '.' || origin_host[host_len-domain_len-1] == '/')){ If I'm reading this right, domain A can change frames in domain B if it is a proper subdomain of B, i.e., bugzilla.mozilla.org can change frames owned by mozilla.org, but NOT vice versa, and bugzilla.mozilla.org can not change frames owned by tinderbox.mozilla.org. Also, that very last line above seems unnecessary: origin_host[host_len-domain_len-1] == '/')){ Seems to say that http://mozilla.org can change frames owned by http://mozilla.org - should be taken care of by the direct comparison above it AND the XP_ValidateOrigin call that preceeds XP_ValidateDomain... There's also extra logic in this routine to allow access if both URL's are file: URL's, and otherwise disallow access unless they are the same protocol. Also, to allow access if EITHER the target frame OR the target frame's parent matches exactly, or matches as the same domain of the origin frame. Does this sound reasonable? I'm not sure exactly what web sites out there we would be breaking if we just turned off access to other hosts period versus this somewhat more relaxed check.

FWIW, I'm not too sure I like this fix from a security standpoint - but I also don't have all the info that the people in 4.5x did when spinning above bugfixes. For example, this means that subdomain providers can still be spoofed: http://spoofer.dyndns.org Can still spoof: http://dyndns.org Mitch, from what you've seen, do you think setting document.domain is widely known enough to put this fix in without the "domain" relaxation above and not break too many sites? I can't think of an easy way to scan the web to see how many sites try to update other named frames as targets of links/form submits where the target and origin are not from the same host... :S

The logic you quoted above has a flaw: I think it will see "server.britishbank.co.uk" and "server.hackersite.co.uk" as being in the same domain. There's no way to determine what portion of an address constitutes a domain name under the control of a single entity. That's why we restrict access to a single host, with the document.domain exception that requires the cooperation of both pages. Document.domain won't work in this case, becuase setting that property changes the document's principal but not the document's URL. You'd have to do this access check based on principals, not URLs, which may be easier anyway because you can just call principal.Equals() to do the origin comparison. As for your last question, I'm not sure how widely used document.domain is, or whether it's ever been applied to this particular issue. I'm not sure how many sites would break by using a strict same-origin policy either. Vidur may have some idea.

> The logic you quoted above has a flaw: I think it will see > "server.britishbank.co.uk" and "server.hackersite.co.uk" as being in the same > domain. There's no way to determine what portion of an address constitutes a > domain name under the control of a single entity. That's why we restrict I agree completely that there is no way to determine what portion of an address constitutes... :) Just a minor nit though - server.hackersite.co.uk and server.britishbank.co.uk will not be able to modify each other's domain names using the logic in: http://lxr/nova/source/lib/xp/xp_cntxt.c#269 Though server.hackersite.co.uk is a longer string by one character than server.britishbank.co.uk, when we do a strcmp starting at the second character of server.hackersite.co.uk there will not be a match. Also, the first extra character is not a dot. An example that demonstrantes the relaxation is: foo.server.britishbank.co.uk ---- can target ----> server.britishbank.co.uk It's important to note that foo.server.britishbank.co.uk is a longer hostname than server.britishbank.co.uk, and has "something dot" before server.britishbank.co.uk, which is what the logic there checks for.

So, If I'm understanding you, my.netscape.com could reload a frame owned by netscape.com, and vice versa perhaps, but my.netscape.com cannot reload a frame oned by webmail.netscape.com? One is not a subdomain of the other. Is this what you're intending? It seems like an incomplete solution.

That's right. my.netscape.com can reload a frame owned by netscape.com, but not vice versa. Sorry if my wording indicates that this is what we *should* do for Netscape 6 - that's not what I meant to say. :) I was just investigating and stating what Netscape 4.5x+ does. I defer to folks with a broader view on security to what the correct behavoiur of Netscape 6 should be. (And it sounds like you lean towards a strict host comparison, right?)

Eric, Yes, but I'm paranoid. Breaking a lot of sites is not going to help our competitiveness. I think we need another opinion. The "subdomain" solution seems like it should be secure, but maybe overly restrictive - home.netscape.com would be denied access to webmail.netscape.com, for example. But then again, since we can't really determine what part of the URL constitutes the 'domain,' maybe this is reasonable. I'd be okay with the subdomain solution.

Marking rtm+.

Marking [rtm need info] Waiting for patch to be attached, review + super-review.

CC'ing Rick because the spoofing fix is entirely in the URI loader (possibly the validate origin calls will be in the security manager). I've attached a really rough outline of what I plan to do above. I'll need to implement the validateOrigin routine, but it will be a near cut and paste of the one in the 4.x codebase (allowing subdomains to target frames from this domain.)

Okay, I implemented a patch and did a bit of testing - it turns out that Nav's behaviour is not what I had gotten from reading the source code for the Nova codebase. I sat there and ran the two browsers side by side (4.x versus 6 with this patch), and lo and behold... Mitch, you were right all along, Nav actually only allows one frame to update another if there is the two hosts are an exact match. :) I implemented this behaviour. (Tried the more complex 'subdomain' stuff - it was all wrong) This patch is controllable by a pref and off by default, to turn it on, you must add this line to your prefs.js: user_pref("browser.frame.validate_origin", true); This is identical to the pref in 4.x. (I think, again, I got this from reading source in the 4.x base...) I did lots of testing with frames nested inside of frames, popping open windows, submitting forms, setting timers, file: urls. I have not yet tested document.domain and other protocols (https, ftp, javascript, ...) At this point, I'd love any comments, suggestions, testing, and if you like the patch, reviews, super reviews, approvals. It's not that big of a change - about 120 lines (many are comments) that break down to two new static methods in nsURILoader, changes in nsURILoader::GetTarget, and very minor changes in OpenURLVia and the constructor. (compare with the length of my ramblings on this bug report) ;)

Rick Potts was kind enough to give me a review and an r= for this patch.

Sometime while sleeping last night, I realized that the way I was comparing hosts yesterday (PL_strcmp of the hostname) was not only adding needless complexity, but also was not as correct as just calling Equals() on the principal. This new patch fixes a bug with my previous patch, and makes it simpler at the same time. The bug was that if one frame comes from https on host foo.bar.com and tries to update a frame from http on foo.bar.com, Nav 4.x does *not* allow the update but we did with patch 1. patch2 fixes this problem. The only thing I still need to test is document.domain setting to make sure that works.

Here's more info on the fix - I attempted to retract my post to the newsgroup, but at any rate, here it is: Summary: In a targetted load <a target=foo>, we already do a search for the frame or window that has that name. Now, once we've found it: - Compare the principals of the origin and target frames. - If they are not the same, send the load to a new window. - This fix is controllable by a pref (same as in Nav 4.x). - I defaulted the pref to false (insecure, like IE) but Nav defaults to true (secure). This is to minimize risk, but can be easily changed. Details: 1) Preference checking: Add member variable ValidateOrigin to URILoader This is a boolean variable that will represent the state of the pref "browser.frame.validate_origin" This is a pref with no UI, so it must be manually set by editing prefs.js. It is the same pref that Nav 4.x uses to turn on and off this behaviour (I have verified this with 4.73) Initialize the member variable in the URILoader constructor. My patch currently defaults to false, so if either the prefs service can not be obtained, or this pref is not manually set in the prefs.js file, we will default to insecure behaviour. This is easy to change (one line) to default to secure if that is preferred. I chose to default to false because there is some risk associated with this fix. It is possible that some sites are relying on the insecure default behaviour of Internet Explorer to load pages into frames that are owned by a sister site. With this fix turned on by default, we would not be breaking these sites, but forcing the insecure loads into a new window. This is possibly unexpected behaviour for a user of Internet Explorer, but does not limit functionality and is the same as the behaviour as Nav 4.x 2) Doing the security check: Add an extra security check to URILoader::GetTarget() In GetTarget, if we find a target frame with given name, and the pref is set, we will now do an additional security check. If we pass the check we allow the load to continue normally. If we fail the check, we change the target to "_blank" to send the load to a new window. The check is straight forward: Compare principals of origin and target frames? Same: check passed. Different: get parent of target frame? No parent found: target is toplevel frame, check passed. Parent found: Compare principals of origin and target parent frame? Same: check passed. Different: check failed. If the check succeeds, we allow the load to proceed into the target frame as usual. If the check fails, we return the window context of the origin frame. Next, we set the target name to "_blank". These two changes cause the load to be treated exactly as if it had been originally targeted a "_blank" and it will go to a new window, just as Nav 4.x does. When getting the parent of the target frame, I call GetSameTypeParent to avoid going up into chrome. 4) Allowing the load to be retargetted at a new window: Changed the arguments to GetTarget to allow modification of the target name Before my change, in OpenURIVia a const char* was passed to GetTarget. GetTarget would then set the window context as appropriate. OpenURIVia would then send this same const char* to the loader's Open method, along with the found window context. After my change, we pass an nsCAutoString into GetTarget. GetTarget can then modify the string as described above, to load "_blank". OpenURIVia passes this possibly modified string to the loader's Open method, along with the found window context. 5) Two new static helper functions in nsURILoader.cpp: Bool ValidateOrigin(DocShellTreeItem origin, DocShellTreeItem target) originPrincipal = GetPrincipal(origin) targetPrincipal = GetPrincipal(taret) return targetPrincipal->Equals(originPrincipal); GetPrincipalFromTreeItem(DocShellTreeItem treeItem, Principal principal) DOMdocument = treeItem->GetInterface(nsIDOMDocument) document = DOMdocument->QueryInterface(nsIDocument) document->GetPrincipal(principal) Testing: I've nearly completed some exhaustive testing, which I'll send an email about later. If you'd like to check out the various test cases, they can be found at: http://302easyst.net/~pollmann/bugs/spoof To enable leveraging of the testcases there, I mapped *.302easyst.net and dsl.pollmann.net to the same IP address, and am also running an https server on that machine, so you can try many permutations on the test cases coming from various hosts, domains, subdomains, and protocols. For far all testing has been successful. With the pref false we act as we did before (same as IE), and with the pref true, we act the same as Nav 4.x.

> The only thing I still need to test is document.domain setting to make sure > that works. I just found one difference between Nav 6 with the patch and Nav 4.x Specifically, if we have this: Origin host is bar.foo.com Target host is baz.foo.com, document.domain='foo.com' Nav will allow origin host to change target host in this case - this is where the "subdomain" stuff I found in the 4.x codebase comes in - it is only used if you set document.domain, hmmm... This means with the patch we are more strict in the way we handle an explicitly set document.domain than Nav 4.x is. Since I don't know why Nav chose to do this, I can only guess it is because some sites required it. I'll look into a fix.

Updated patch includes one change. Before, in GetTarget, if the two document principals were not equal, we would force the load into a new window. Now, we check to see if document.domain is set in the target. If it is, we check to see if the origin document comes from a subdomain of that domain. If it does we allow the load. If it is not from a subdomain, we still force the load into a new window. This makes us behave like Nav 4.x in the above mentioned case. I've also retested all of the other cases (well most of them, there are hundreds if testing all permutations) and we're still the same as Nav 4.x in those as well.

Patch looks good to me. r=mstoltz. Hope it isn't too late to get this into rtm, considering all the work eric put into it. It it is too late, let's make sure this gets into the trunk.

Mitch, per your review, would you recommend the default for this pref be false (insecure, as it is now, like IE), or true (secure, like nav)?

Eric, Since they pay me to be paranoid, I would recommend the check be turned on by default. I guess it really depends on how many sites we break, and whether that breakage is worse than the potential for spoofing. Do you have any idea about this? Do any top sites depend on this behavior? I'll take a look myself.

Well, if they work in Nav they *should* work in NS 6 with the fix. Every test case I came up with works just like Nav with the fix turned on.

Right - so if we're reimplementing 4.x behavior with the pref turned on, then why don't we turn it on by default?

Couple random comments, one of which I asked in email form already: 1) You added some special case code for imap and mailbox urls. Do we need to include a case for news urls as well? 2) Chrome urls --> chrome urls run through the uriloader too. Will changing GetTarget to now perform this validate origin check be triggered for every chrome url? If so, is there a way (and is it okay) to circumvent this code for local chrome? I'm thinking of the things like the throbber icon image which get loaded over and over again.

Eric, can you send me the diffs (-u) directly? For some reason, saving the attachment to my drive and applying the patch keeps failing. I'm not sure why yet.

> 1) You added some special case code for imap and mailbox urls. Do we need to > include a case for news urls as well? From my email reply: Good question - I'm not familiar with the reasons behind that check. I think Mitch might be able to answer your question. If we do decide to add "news" urls, we should add them to both places. Really, that would be orthogonal to the spoofing bugfix...

> 2) Chrome urls --> chrome urls run through the uriloader too. Will changing > GetTarget to now perform this validate origin check be triggered for every > chrome url? If so, is there a way (and is it okay) to circumvent this code for > local chrome? I'm thinking of the things like the throbber icon image which > get loaded over and over again. I set a breakpoint in GetTarget and loaded a few pages. I never see it being called until I click on a <A HREF TARGET="foo">. I can't tell you for certain that there are no targetted links in Chrome, but I haven't seen any - they seem to be unusual.

The imap/mailbox check is explained by the comment on the following line: Unlike http URLs, each mail message should be considered a distinct trust domain, so instead of checking only scheme://host:port we need to check the whole url. Yes, this should apply to news as well.

I'm going to go ahead and give the sr for this change. I've been running with Eric's diffs and I'm not seeing any regressions caused by them. Mail still behaves okay and so does the browser for normal stuff. Eric, can you add a check for "news:" as well? mstoltz, I wasn't able to trigger this new code path at all for mail. What can I do to test that path out? I guess I don't see any mail urls that retarget to another window.

Oh, didn't see this was already sr'd. Thanks, Scott! I'll add the "news:" check (per Scott's review) and turn on by default (per Mitch's recommendation). I'll attach a final cvs diff -u in a moment...

Drat, just found a problem with that patch... We are being too strict if the *origin frame* set it's document.domain :S I am working on a fix. Sorry all...

Try loading the testcase page above, mailing it to yourself using "Send Page," and reading the mail message. That should test the mail case.

r=mstoltz on patch #4.

Patch 4 addresses 2 additional edge cases to reduce chances of regressions: 1) Origin frame sets document.domain We were comparing the origin frame's principal URI with the target frame's principal URI. We now instead compare the origin frame's document URI with the target frame's principal URI. This is what Nav does and prevents, e.g. myhost.netscape.com from being able to target a frame from http://netscape.com just by setting document.domain to netscape.com 2) Target document sets document.domain to same host as the target document This is what Nav does and allows, e.g. target document http://netscape.com to set document.domain="netscape.com" to allow myhost.netscape.com to target it's frame (before I was comparing principalURI with documentURI, which would see they were the same and not do the subdomain check)

sr=mscott on the latest patch 4.

Thanks for the r= and sr=. I'm marking it rtm+ now so PDT can decide if this will go into the branch.

This patch is pretty big to land on the branch directly. If you think it's *really critical*, maybe you could try it on the trunk and then do some serious testing. Marking [rtm need info] until trunk test results are available.

Checked into the trunk to allow testing.

This has been cooking on the trunk for a few days with no reported adverse effects. Resubmitting to PDT for consideration. A simplified, equivalent fix is also available, which I will attach (removes DOM changes, reviewed by Johnny and Vidur)

rtm++ for the simplified patch

Fix checked into the branch and trunk. To verify, go to the above test case or: http://302easyst.net/~pollmann/bugs/spoof/6 A new window will open up. If you click on either of the buttons in the original window (any of the top four on 302easyst), the load should open up an additional new window instead of going to the existing window that popped up. Clicking on the fourth button at 302easyst.net should load into the window that popped up. There are more test cases here: http://302easyst.net/~pollmann/bugs/spoof/ https://302easyst.net/~pollmann/bugs/spoof/ http://media.302easyst.net/~pollmann/bugs/spoof/ http://big.media.302easyst.net/~pollmann/bugs/spoof/ http://server.302easyst.net/~pollmann/bugs/spoof/ http://dsl.pollmann.net/~pollmann/bugs/spoof/ Basically, walking through each of these test cases should do the same thing in Mozilla as it does in 4.75, if you view the pages in both browsers side by side. The one exception I noticed was loading a file: URL from an http: URL does nothing in Mozilla, which is also the case in builds that don't include the change.

Fixed in the branch 0ct 24 builds.

Verified on the 11/27 Mac, Linux and Win32 trunk builds.

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

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

Test for this got added in bug 408052.