Closed Bug 343168 Opened 14 years ago Closed 13 years ago
frame spoofing using document
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:18.104.22.168) Gecko/20060629 Firefox/22.214.171.124
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.
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
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().
Hmm. Good point. I'd be happy to make close() same-origin (with some testing, etc).
I haven't yet tested this, but will tomorrow.
Attachment #227624 - Attachment is obsolete: true
Whiteboard: [sg:low spoof]
Blake, did you ever test your "Better patch"?
Flags: blocking126.96.36.199? → blocking188.8.131.52+
Whiteboard: [sg:low spoof] → [sg:low spoof][at risk]
Comment on attachment 227766 [details] [diff] [review] Better patch Yeah, this works.
Comment on attachment 227766 [details] [diff] [review] Better patch Looks good!
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #227766 - Flags: approval1.8.1?
Whiteboard: [sg:low spoof][at risk] → [sg:low spoof][at risk][baking until 8/3]
Comment on attachment 227766 [details] [diff] [review] Better patch a=schrep for drivers. Land away!
Attachment #227766 - Flags: approval1.8.1? → approval1.8.1+
Checked in on the 1.8 branch.
Whiteboard: [sg:low spoof][at risk][baking until 8/3] → [sg:low spoof][at risk]
Comment on attachment 227766 [details] [diff] [review] Better patch approved for 1.8.0. branch, a=dveditz for drivers
Attachment #227766 - Flags: approval184.108.40.206? → approval220.127.116.11+
Fixed on the 1.8.0 branch.
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:18.104.22.168pre) Gecko/20060825 Firefox/22.214.171.124pre verified 126.96.36.199
Whiteboard: [sg:low spoof][at risk] → [sg:low spoof]
This caused bug 362837.
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?
Need support for multiple domains in mochitest to add a test...
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.
Ah, right. Since Write() doesn't doesn't call Open() if !mParser, we should indeed not be doing the check in Open() when !mParser.
I've opened a new bug for this: Bug 381300
Comments 22-25 relate to new bug 381300
Duplicate of this bug: 382686
You need to log in before you can comment on or make changes to this bug.