Bug 28688 – REGRESSION(r24994): Cannot create a frame with a javascript URL

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Alexey Proskuryakov]

The following works in other engines, but doesn't work in WebKit:

    var ifr = document.createElement("iframe");
    ifr.setAttribute("src", "javascript:'PASS'");
    document.body.appendChild(ifr);

This regressed in <http://trac.webkit.org/changeset/24994>.

[Alexey Proskuryakov]

Created attachment 38501 [details]
proposed fix

[Sam Weinig]

Comment on attachment 38501 [details]
proposed fix

There are a few more case this probably needs to patch.  See JSHTMLIFrameElementCustom.cpp, JSHTMLFrameElementCustom.cpp and JSAttrCustom.cpp.  There may be more I am missing, so it is probably best to go back to the initial checkin and see what we did.

[Adam Barth]

Why isn't checkNodeSecurity passing?  If it's a blank document, it should inherit its SecurityOrigin from its parent...  Maybe the issue is that it isn't attached yet?

We should test document.writing into the detached blank frame as well.

[Alexey Proskuryakov]

Created attachment 38505 [details]
fix more copy/paste checks

[Alexey Proskuryakov]

Created attachment 38507 [details]
remove unrelated changes

[Darin Adler]

Comment on attachment 38507 [details]
remove unrelated changes

> -            if (!checkNodeSecurity(exec, static_cast<HTMLFrameElementBase*>(ownerElement)->contentDocument()))
> +            HTMLFrameElementBase* frameElement = static_cast<HTMLFrameElementBase*>(ownerElement);
> +            if (frameElement->contentDocument() && !checkNodeSecurity(exec, frameElement->contentDocument()))
>                  return;

I think it would be better to put the contentDocument into a local variable instead of the frame.

>          HTMLFrameElementBase* frame = static_cast<HTMLFrameElementBase*>(element);
> -        if (!checkNodeSecurity(exec, frame->contentDocument()))
> +        if (frame->contentDocument() && !checkNodeSecurity(exec, frame->contentDocument()))
>              return false;

Ditto.

> -        if (!checkNodeSecurity(exec, imp->contentDocument()))
> +        if (imp->contentDocument() && !checkNodeSecurity(exec, imp->contentDocument()))
>              return false;

I think this might read slightly better if the contentDocument was in a local variable. I had trouble seeing the relationship between the two.

> -        if (!checkNodeSecurity(exec, imp->contentDocument()))
> +        if (imp->contentDocument() && !checkNodeSecurity(exec, imp->contentDocument()))
>              return;

Ditto.

It would be better if there was some shared code rather than this null check having to be everywhere. Maybe a checkDocumentSecurity function that takes a HTMLFrameElementBase* or Document*?

r=me

[Alexey Proskuryakov]

(In reply to comment #3)
> Why isn't checkNodeSecurity passing?  If it's a blank document, it should
> inherit its SecurityOrigin from its parent...

There is no document in this case yet (hence the added null checks). Sam actually wonders if "node &&" in checkNodeSecurity() is doing what it's supposed to do.

> We should test document.writing into the detached blank frame as well.

This sounds like something that's at best tangentially related to this bug to me (I'm also not sure what to document.write).

[Sam Weinig]

Comment on attachment 38507 [details]
remove unrelated changes

This seems fine, but we really should come back to this at some point to address the horribly named checkNodeSecurity (My first attempt at a better name is allowsAccessForNode, which kind of still sucks). It is unclear why the current implementation returns false for null nodes, but I am concerned of the ramifications of changing that without more thought. r=me, but please look into Adam's concern about document.write() if you are able to.

[Sam Weinig]

Comment on attachment 38507 [details]
remove unrelated changes

This seems fine, but we really should come back to this at some point to address the horribly named checkNodeSecurity (My first attempt at a better name is allowsAccessForNode, which kind of still sucks). It is unclear why the current implementation returns false for null nodes, but I am concerned of the ramifications of changing that without more thought. r=me, but please look into Adam's concern about document.write() if you are able to.

[Sam Weinig]

I also want to note that the test probably doesn't need to be in http

[Adam Barth]

(In reply to comment #7)
> There is no document in this case yet (hence the added null checks). Sam
> actually wonders if "node &&" in checkNodeSecurity() is doing what it's
> supposed to do.

I see.  I think you're right that this is secure, but reasoning that out is
quite complicated.  I need to spend some more time thinking about the security
model of detached frames.  For example, do they have window objects?  Can their
window objects become visible to other origins?

I don't think we need to answer those questions before moving forward with this
patch though.

> This sounds like something that's at best tangentially related to this bug to
> me (I'm also not sure what to document.write).

I think it's fine to look into this in another bug.

[Alexey Proskuryakov]

Committed revision 47734.

> I think it would be better to put the contentDocument into a local variable
> instead of the frame.

Fixed.

> I also want to note that the test probably doesn't need to be in http

The rest of javaScriptURL tests (as landed in r24994) are in http/security, I just wanted to have this next to them.

> I need to spend some more time thinking about the security
> model of detached frames.  For example, do they have window objects?

I think I see what you're saying now. My understanding is that they have both window and document objects, so this patch shouldn't have the behavior for them.

My reason to make this patch was to fix layout test in bug 28374, so I want to be done with it quickly. It indeed seems that more thought is required to verify whether checkNodeSecurity is doing the right thing - maybe a better fix would be to correct that instead.