Frame spoofing using loading two frames

VERIFIED FIXED

Status

()

defect
P1
normal
VERIFIED FIXED
20 years ago
11 years ago

People

(Reporter: joro, Assigned: pollmann)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm++][nsbeta3-][pdtp1] fix in hand, URL)

Attachments

(7 attachments)

(Reporter)

Description

20 years ago
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>

Updated

20 years ago
Status: NEW → ASSIGNED
Target Milestone: M14

Comment 1

20 years ago
Post-beta for spoofing.

Updated

20 years ago
Summary: Frame spoofing using loading two frames → [Feature] Frame spoofing using loading two frames

Updated

20 years ago
Blocks: 16565

Updated

20 years ago
Target Milestone: M14 → M16

Comment 2

19 years ago
Bulk moving all Browser Security bugs to new Security: General component.  The 
previous Security component for Browser will be deleted.
Component: Security → Security: General

Updated

19 years ago
Summary: [Feature] Frame spoofing using loading two frames → Frame spoofing using loading two frames
Target Milestone: M16 → M17

Comment 3

19 years ago
Changing Qa contact to myself.
QA Contact: dshea → junruh
Bulk reassigning most of norris's bugs to mstoltz.
Assignee: norris → mstoltz
Status: ASSIGNED → NEW

Comment 5

19 years ago
Assigning QA to czhang
QA Contact: junruh → 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.
Group: netscapeconfidential?
Status: NEW → ASSIGNED
Keywords: 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.
Assignee: mstoltz → clayton
Status: ASSIGNED → NEW
Component: Security: General → Layout
QA Contact: czhang → petersen
cc'ing myself.
Reassigning to Eric.
Assignee: clayton → pollmann
(Assignee)

Comment 11

19 years ago
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.
(Assignee)

Comment 12

19 years ago
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?
(Assignee)

Comment 13

19 years ago
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?
(Assignee)

Comment 15

19 years ago
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.
(Assignee)

Comment 17

19 years ago
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?
(Assignee)

Comment 18

19 years ago
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?

Comment 20

19 years ago
Putting on [nsbeta2-] radar. Not critical to beta2. 
Whiteboard: [nsbeta2-]
(Assignee)

Comment 21

19 years ago
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.
Target Milestone: M17 → Future
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.
Keywords: nsbeta2nsbeta3
Whiteboard: [nsbeta2-]
(Assignee)

Comment 23

19 years ago
Removing from Future list per above recommendation
Target Milestone: Future → M19
Marking nsbeta3+
Whiteboard: [nsbeta3+]
Setting priority to P1
Priority: P3 → P1

Comment 26

19 years ago
PDT agrees P1

Comment 27

19 years ago
Putting [pdtp1] in whiteboard
Whiteboard: [nsbeta3+] → [nsbeta3+][pdtp1]
(Assignee)

Comment 28

19 years ago
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.
Status: NEW → ASSIGNED
Whiteboard: [nsbeta3+][pdtp1] → [nsbeta3+][pdtp1] may be worksforme
(Assignee)

Comment 29

19 years ago
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.
Whiteboard: [nsbeta3+][pdtp1] may be worksforme → [nsbeta3+][pdtp1]
(Assignee)

Comment 30

19 years ago
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...

Comment 31

19 years ago
PDT marking nsbeta3-, nominating for rtm
Keywords: rtm
Whiteboard: [nsbeta3+][pdtp1] → [nsbeta3-][pdtp1]
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?
(Assignee)

Comment 33

19 years ago
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.
(Assignee)

Comment 34

19 years ago
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.
(Assignee)

Comment 36

19 years ago
> 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.
(Assignee)

Comment 38

19 years ago
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+.
Whiteboard: [nsbeta3-][pdtp1] → [rtm+][nsbeta3-][pdtp1]
Marking [rtm need info] Waiting for patch to be attached, review + super-review.
Whiteboard: [rtm+][nsbeta3-][pdtp1] → [rtm need info][nsbeta3-][pdtp1]
(Assignee)

Comment 43

19 years ago
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.)
Component: Layout → Security: General
OS: Windows 95 → All
Hardware: PC → All
(Assignee)

Comment 44

19 years ago
(Assignee)

Comment 45

19 years ago
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) ;)
(Assignee)

Updated

19 years ago
Whiteboard: [rtm need info][nsbeta3-][pdtp1] → [rtm need info][nsbeta3-][pdtp1] fix in hand
(Assignee)

Comment 46

19 years ago
Rick Potts was kind enough to give me a review and an r= for this patch.
(Assignee)

Comment 47

19 years ago
(Assignee)

Comment 48

19 years ago
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.
(Assignee)

Comment 49

19 years ago
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.
(Assignee)

Comment 50

19 years ago
> 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.
(Assignee)

Comment 51

19 years ago
(Assignee)

Comment 52

19 years ago
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.
(Assignee)

Comment 54

19 years ago
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.
(Assignee)

Comment 56

19 years ago
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?

Comment 58

19 years ago
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. 

Comment 59

19 years ago
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. 
(Assignee)

Comment 60

19 years ago
> 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...
(Assignee)

Comment 61

19 years ago
> 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.

Comment 63

19 years ago
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. 
(Assignee)

Comment 64

19 years ago
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...
Whiteboard: [rtm need info][nsbeta3-][pdtp1] fix in hand → [rtm+][nsbeta3-][pdtp1] fix in hand
(Assignee)

Comment 66

19 years ago
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...
(Assignee)

Updated

19 years ago
Whiteboard: [rtm+][nsbeta3-][pdtp1] fix in hand → [rtm need info][nsbeta3-][pdtp1] fix in hand
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.
(Assignee)

Comment 70

19 years ago
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)

Comment 71

19 years ago
sr=mscott on the latest patch 4. 
(Assignee)

Comment 72

19 years ago
Thanks for the r= and sr=.  I'm marking it rtm+ now so PDT can decide if this
will go into the branch.
Whiteboard: [rtm need info][nsbeta3-][pdtp1] fix in hand → [rtm+][nsbeta3-][pdtp1] fix in hand

Comment 73

19 years ago
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.
Whiteboard: [rtm+][nsbeta3-][pdtp1] fix in hand → [rtm need info][nsbeta3-][pdtp1] fix in hand
(Assignee)

Comment 74

19 years ago
Checked into the trunk to allow testing.
(Assignee)

Comment 75

19 years ago
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)
Whiteboard: [rtm need info][nsbeta3-][pdtp1] fix in hand → [rtm+][nsbeta3-][pdtp1] fix in hand

Comment 77

19 years ago
rtm++ for the simplified patch
Whiteboard: [rtm+][nsbeta3-][pdtp1] fix in hand → [rtm++][nsbeta3-][pdtp1] fix in hand
(Assignee)

Comment 78

19 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED

Comment 79

19 years ago
Fixed in the branch 0ct 24 builds.

Updated

19 years ago
Keywords: vtrunk

Comment 80

19 years ago
Verified on the 11/27 Mac, Linux and Win32 trunk builds.
Status: RESOLVED → VERIFIED
Group: netscapeconfidential?
(Assignee)

Comment 81

18 years ago
*** 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.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.