Closed
Bug 343168
Opened 19 years ago
Closed 19 years ago
frame spoofing using document.open()
Categories
(Core :: Security, defect, P2)
Core
Security
Tracking
()
VERIFIED
FIXED
mozilla1.9alpha1
People
(Reporter: sync2d, Assigned: mrbkap)
References
Details
(Keywords: verified1.8.0.7, verified1.8.1, Whiteboard: [sg:low spoof])
Attachments
(2 files, 1 obsolete file)
763 bytes,
text/html
|
Details | |
3.14 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
dveditz
:
approval1.8.0.7+
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
You can replace framed contents using target.frames[n].document.open().
Something similar to bug 13871, bug 20682, bug 246448, and bug 296850.
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•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Comment 3•19 years ago
|
||
Assignee: dveditz → mrbkap
Status: NEW → ASSIGNED
Attachment #227624 -
Flags: superreview?(bzbarsky)
Attachment #227624 -
Flags: review?(bzbarsky)
Comment 4•19 years ago
|
||
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().
Attachment #227624 -
Flags: superreview?(bzbarsky)
Attachment #227624 -
Flags: superreview-
Attachment #227624 -
Flags: review?(bzbarsky)
Attachment #227624 -
Flags: review-
Assignee | ||
Comment 5•19 years ago
|
||
(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•19 years ago
|
||
Hmm. Good point. I'd be happy to make close() same-origin (with some testing, etc).
Assignee | ||
Comment 7•19 years ago
|
||
I haven't yet tested this, but will tomorrow.
Assignee | ||
Updated•19 years ago
|
Attachment #227624 -
Attachment is obsolete: true
Updated•19 years ago
|
Flags: blocking1.9a1+
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Whiteboard: [sg:low spoof]
Updated•19 years ago
|
Flags: blocking1.8.1? → blocking1.8.1+
Comment 8•19 years ago
|
||
Blake, did you ever test your "Better patch"?
Flags: blocking1.8.0.6? → blocking1.8.0.6+
Whiteboard: [sg:low spoof] → [sg:low spoof][at risk]
Assignee | ||
Comment 9•19 years ago
|
||
Comment on attachment 227766 [details] [diff] [review]
Better patch
Yeah, this works.
Attachment #227766 -
Flags: superreview?(bzbarsky)
Attachment #227766 -
Flags: review?(bzbarsky)
Comment 10•19 years ago
|
||
Comment on attachment 227766 [details] [diff] [review]
Better patch
Looks good!
Attachment #227766 -
Flags: superreview?(bzbarsky)
Attachment #227766 -
Flags: superreview+
Attachment #227766 -
Flags: review?(bzbarsky)
Attachment #227766 -
Flags: review+
Assignee | ||
Comment 11•19 years ago
|
||
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•19 years ago
|
Attachment #227766 -
Flags: approval1.8.1?
Updated•19 years ago
|
Whiteboard: [sg:low spoof][at risk] → [sg:low spoof][at risk][baking until 8/3]
Comment 12•19 years ago
|
||
Comment on attachment 227766 [details] [diff] [review]
Better patch
a=schrep for drivers. Land away!
Attachment #227766 -
Flags: approval1.8.1? → approval1.8.1+
Assignee | ||
Comment 13•18 years ago
|
||
Checked in on the 1.8 branch.
Keywords: fixed1.8.1
Whiteboard: [sg:low spoof][at risk][baking until 8/3] → [sg:low spoof][at risk]
Updated•18 years ago
|
Attachment #227766 -
Flags: approval1.8.0.7?
Comment 14•18 years ago
|
||
Comment on attachment 227766 [details] [diff] [review]
Better patch
approved for 1.8.0. branch, a=dveditz for drivers
Attachment #227766 -
Flags: approval1.8.0.7? → approval1.8.0.7+
Comment 16•18 years ago
|
||
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
Status: RESOLVED → VERIFIED
Updated•18 years ago
|
Group: security
Whiteboard: [sg:low spoof][at risk] → [sg:low spoof]
Comment 17•18 years ago
|
||
This caused bug 362837.
Comment 18•18 years ago
|
||
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•18 years ago
|
||
Need support for multiple domains in mochitest to add a test...
Flags: in-testsuite?
Comment 20•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
I've opened a new bug for this: Bug 381300
Comment 27•18 years ago
|
||
Comments 22-25 relate to new bug 381300
You need to log in
before you can comment on or make changes to this bug.
Description
•