422025 - iframe.contentDocument should use XOW (was: iframe.contentDocument should be restricted like window.document)

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[Jesse Ruderman]

Anne van Kesteren points out that Firefox trunk doesn't allow window.document cross-domain but does allow iframe.contentDocument cross-domain.  This inconsistency seems like a bug.

http://tc.labs.opera.com/apis/same-origin/001.htm
http://tc.labs.opera.com/apis/same-origin/002.htm

Comment 1

[Johnny Stenback (:jst)]

Depends on your definition of a bug. It's per design, really, IMO. XOW's protect us here, so there's really no security concern here, but we could special case this in the DOM classinfo code and check whether the caller and the document it's trying to access through iframe.contentDocument is same origin or not, and throw if it's not.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #1)
> Depends on your definition of a bug. It's per design, really, IMO. XOW's
> protect us here

Except not any more... We would used to wrap all documents, but not anymore. We still have security checks in place that prevent this from being *really* terrible (i.e. no contentDocument.cookie), but we do need to fix this somehow.

Comment 3

[Johnny Stenback (:jst)]

But we still always unconditionally wrap [i]frame elements in XOWs don't we? Doesn't that still mean we end up wrapping [i]frame.contentDocument in a XOW too if that document is from a different origin?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #3)
> But we still always unconditionally wrap [i]frame elements in XOWs don't we?

Yes.

> Doesn't that still mean we end up wrapping [i]frame.contentDocument in a XOW
> too if that document is from a different origin?

Unfortunately not because of a bizarro path we take from XPCWrapper::NewResolve to XPC_WN_GetterSetter.

Comment 5

[Daniel Veditz [:dveditz]]

In Firefox 2 this wouldn't be a problem because we _know_ people can get to the document and protect (most of) its properties. When we implemented XOW did we remove those checks, meaning we're now more vulnerable than before?

Comment 6

[moz_bug_r_a4]

Bug 369334 is fixed by XOW.  But, this bug can be used to circumvent that fix.

Comment 8

[moz_bug_r_a4]

(In reply to comment #5)
> In Firefox 2 this wouldn't be a problem because we _know_ people can get to the
> document and protect (most of) its properties. When we implemented XOW did we
> remove those checks, meaning we're now more vulnerable than before?
> 

"we're now more vulnerable than before" seems to be true.  Now, it's possible
to set expando properties or override idl methods on a cross-origin document.

  var d = iframe.contentDocument;
  d.x = x;
  d.getElementById = function() {
    return y;
  };

Comment 10

[Boris Zbarsky [:bzbarsky]]

Blake, mind explaining the reason that the wrapping doesn't quite work (last part of comment 4)?  Just trying to understand what XOW does and doesn't do for me; need it for some other code.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So there seems to be two bugs discussed here:

1. Comment 0 suggests that we should not let iframe.contentDocument be accessed
   at all for cross-domain iframes.

2. Getting iframe.contentDocument should return a XOW for cross domain iframes.
   Comment 2 suggests that doesn't happen.


Ideally I'd like to fix 1. However we likely can't make accessing the property throw for cross-domain iframes. People often enumerate all properties of their own DOM objects, and while it's as important what we return for the various properties, throwing on any access breaks the whole enumeration.

This has been particularly true for the window object.

So we currently never throw on any property access on objects from your own DOM. Unfortunately I don't know what we could do other than throw if we want to deny access. Returning null seems very weird too.


Fixing 2 we should definitely do though.

Comment 12

[Johnny Stenback (:jst)]

Bumping priority and taking bug per discussion in today's Firefox meeting, mrbkap, the only way I've figured out how to sanely fix this so far is to always wrap objects in a XOW when doing cross-scope access (from content), i.e. any time you reach into another frame we'll wrap the resulting object in a XOW. Seems to work, passes mochitests etc. I'll attach a patch, but if you have other ideas on this please do let me know. If we go with my approach, I'd really prefer to have this in a beta. Patch coming up.

Comment 13

[Johnny Stenback (:jst)]

Attachment: Possible fix.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 311626 [details] [diff] [review]
Possible fix.

Yeah, let's do this.

>+++ b/js/src/xpconnect/src/xpcconvert.cpp
>@@ -1219,6 +1219,24 @@ XPCConvert::NativeInterface2JSObject(XPCCallContext& ccx,
...
>+                    // Reaching a cross scopes from content code. Wrap

"across".

Comment 15

[Mike Schroepfer]

Comment on attachment 311626 [details] [diff] [review]
Possible fix.

If this needs beta exposure let's get it in.

Comment 16

[Reed Loden [:reed]]

Checking in js/src/xpconnect/src/XPCWrapper.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/XPCWrapper.cpp,v  <--  XPCWrapper.cpp
new revision: 1.27; previous revision: 1.26
done
Checking in js/src/xpconnect/src/xpcconvert.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcconvert.cpp,v  <--  xpcconvert.cpp
new revision: 1.128; previous revision: 1.127
done