Last Comment Bug 343168 - frame spoofing using document.open()
: frame spoofing using document.open()
Status: VERIFIED FIXED
[sg:low spoof]
: verified1.8.0.7, verified1.8.1
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap) (please use needinfo!)
:
Mentors:
Depends on: 362837 364309
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-29 12:07 PDT by shutdown
Modified: 2007-05-31 18:10 PDT (History)
8 users (show)
dveditz: blocking1.9a1+
mtschrep: blocking1.8.1+
dveditz: blocking1.8.0.7+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (763 bytes, text/html)
2006-06-29 12:09 PDT, shutdown
no flags Details
Something like this (1.39 KB, patch)
2006-06-29 15:19 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Review
Better patch (3.14 KB, patch)
2006-06-30 17:01 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.0.7+
mtschrep: approval1.8.1+
Details | Diff | Review

Description shutdown 2006-06-29 12:07:26 PDT
You can replace framed contents using target.frames[n].document.open().
Something similar to bug 13871, bug 20682, bug 246448, and bug 296850.
Comment 1 shutdown 2006-06-29 12:09:36 PDT
Created attachment 227580 [details]
testcase

works on:

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a3)
  Gecko/20060629 BonEcho/2.0a3
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.0.5)
  Gecko/20060629 Firefox/1.5.0.5
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-06-29 13:00:10 PDT
So I think we should leave document.open/document.close as allAccess, but in those methods do a security check that checks whether the caller can access the our window's frameElement, if there's a non-null frameElement.  If not, throw a security exception.
Comment 3 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-06-29 15:19:27 PDT
Created attachment 227624 [details] [diff] [review]
Something like this
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-06-29 18:56:59 PDT
Comment on attachment 227624 [details] [diff] [review]
Something like this

We need to use GetFrameElement(), not GetFrameElementInternal(); otherwise you can't document.open a document in a toplevel window you just opened via window.open...

And we need a similar check in ::Close().
Comment 5 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-06-30 11:09:20 PDT
(In reply to comment #4)
> (From update of attachment 227624 [details] [diff] [review] [edit])
> We need to use GetFrameElement(), not GetFrameElementInternal(); otherwise you
> can't document.open a document in a toplevel window you just opened via
> window.open...

Oops, I didn't quite understand the difference between GetFrameElement and GetFrameElementInternal. Fixed.

> And we need a similar check in ::Close().

So, why is HTMLDocument.close.get allAccess anyway? What possible use-case is there for closing a generated document that you don't own? It seems that this capability was added for bug 54060 (and previously, somewhere for the XPCDOM landing). Was this simple over-zealousness (it seems that there were problems dealing with javascript: generated pages having bad principals back then, but those should now go away)?
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-06-30 11:33:43 PDT
Hmm.  Good point.  I'd be happy to make close() same-origin (with some testing, etc).
Comment 7 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-06-30 17:01:00 PDT
Created attachment 227766 [details] [diff] [review]
Better patch

I haven't yet tested this, but will tomorrow.
Comment 8 Daniel Veditz [:dveditz] 2006-07-27 17:19:39 PDT
Blake, did you ever test your "Better patch"?
Comment 9 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-07-31 17:51:41 PDT
Comment on attachment 227766 [details] [diff] [review]
Better patch

Yeah, this works.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-07-31 18:23:25 PDT
Comment on attachment 227766 [details] [diff] [review]
Better patch

Looks good!
Comment 11 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-08-01 14:49:45 PDT
Fix checked into trunk.
Comment 12 Mike Schroepfer 2006-08-04 10:03:17 PDT
Comment on attachment 227766 [details] [diff] [review]
Better patch

a=schrep for drivers.  Land away!
Comment 13 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-08-10 11:48:49 PDT
Checked in on the 1.8 branch.
Comment 14 Daniel Veditz [:dveditz] 2006-08-11 11:35:17 PDT
Comment on attachment 227766 [details] [diff] [review]
Better patch

approved for 1.8.0. branch, a=dveditz for drivers
Comment 15 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-08-16 12:10:09 PDT
Fixed on the 1.8.0 branch.
Comment 16 alice nodelman [:alice] [:anode] 2006-08-25 15:13:28 PDT
https://bugzilla.mozilla.org/attachment.cgi?id=227580&action=view shouldn't cause an exploit (no "ALL YOUR FRAMES ARE BELONG TO US")

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060825 BonEcho/2.0b2

verified 1.8.1b2

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060825 Firefox/1.5.0.7pre

verified 1.8.0.7
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-12-05 08:50:11 PST
This caused bug 362837.
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-12-05 11:30:48 PST
So the fixes checked in on 1.8.0 and 1.8/trunk are different.  The 1.8.0 fix does the security check only if !mParser, while the 1.8/trunk fix does it no matter whether mParser is set; hence the difference in behavior in bug 362837.

It seems to me that the checks should be consistent, and I think I would probably go with the 1.8/trunk version so you can't inject content into a document while it's loading, right?
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-12-05 11:41:06 PST
Need support for multiple domains in mochitest to add a test...
Comment 20 Daniel Veditz [:dveditz] 2006-12-05 13:21:06 PST
Blake should make the call, but it looks to me like the 1.8.0 code is the correct one: if we've already opened the document (mParser exists) we don't need to reopen it and no need to check permissions at that point. If the security checks are necessary then they ought to be moved into document.write itself rather than relying on checks in the implied .open() which will only happen if !mParser.
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-12-05 13:38:10 PST
Ah, right.  Since Write() doesn't doesn't call Open() if !mParser, we should indeed not be doing the check in Open() when !mParser.
Comment 26 Ronen Zilberman 2007-05-23 17:38:21 PDT
I've opened a new bug for this: Bug 381300
Comment 27 Daniel Veditz [:dveditz] 2007-05-24 11:36:59 PDT
Comments 22-25 relate to new bug 381300
Comment 28 Jonas Sicking (:sicking) PTO Until July 5th 2007-05-31 18:10:41 PDT
*** Bug 382686 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.