Make error messages for cross-domain access OOPIF-friendly.

Make error messages for cross-domain access OOPIF-friendly.

* Make crossDomainAccessErrorMessage and
  sanitizedCrossDomainAccessErrorMessage non-virtual members of
  DOMWindow, and tweak the logic to also work on RemoteDOMWindows
  using replicated origins.

* Change BindingSecurity::shouldAllowAccessToFrame to actually throw
  exceptions when encountering a RemoteFrame.  Previously, we returned
  false without throwing exceptions, which was breaking expectations
  for layout tests when running with --site-per-process.

BUG=478254, 477150
TEST=Run
http/tests/security/cross-frame-access-set-window-properties.html with
--site-per-process, and ensure the calls that are supposed to throw
exceptions actually do so.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194331

[alexmos, 2015-04-15 01:08:13]

alexmos@chromium.org changed reviewers:
+ dcheng@chromium.org

[alexmos, 2015-04-15 17:15:17]

Daniel, what do you think about this?  I discovered we weren't throwing
exceptions when trying to access cross-origin properties on RemoteFrames (we
were just silently denying access), so I tried to fix that, cleaning up error
message printing along the way. This isn't quite ready, but I have some
questions below.

https://codereview.chromium.org/1085973003/diff/1/Source/bindings/core/v8/Bin...
File Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1085973003/diff/1/Source/bindings/core/v8/Bin...
Source/bindings/core/v8/BindingSecurity.cpp:103: return
canAccessDocument(isolate, toLocalFrame(target)->document(), exceptionState);
It seems like I can also go further and turn canAccessDocument into
canAccessFrame, which would let me do throwSecurityError() in just one location.
 Do you think that's a good idea?

Another question: should RemoteFrames support printErrorMessage()?  For some
reason, canAccessDocument and other calls like canNavigate() print access denied
errors on the target frame, not the calling frame.  Alternatively, we could
reverse these to print the error on the calling frame (which should always be
local): there's a FIXME in LocalFrame::printNavigationErrorMessage to that
effect.

https://codereview.chromium.org/1085973003/diff/1/Source/core/frame/DOMWindow...
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/1085973003/diff/1/Source/core/frame/DOMWindow...
Source/core/frame/DOMWindow.cpp:119:
callingWindow.printErrorMessage(crossDomainAccessErrorMessage(&callingWindow));
See my earlier question about printErrorMessage.  I thought it may make more
sense to print this on the callingWindow rather than target (this) window (and
this doesn't seem to break any tests), but if we want to keep this on the
targetFrame, I'll change it back to the old behavior (and not print in the
remote case).

https://codereview.chromium.org/1085973003/diff/1/Source/core/frame/DOMWindow.h
File Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/1085973003/diff/1/Source/core/frame/DOMWindow...
Source/core/frame/DOMWindow.h:186: bool isInsecureScriptAccess(LocalDOMWindow&
callingWindow, const String& urlString);
All of the call sites passed in a LocalDOMWindow, and I don't know why the
callingWindow could be a RemoteDOMWindow, so I changed the signature.  If
there's a good reason to keep it as DOMWindow, please let me know.

Actually, I don't quite understand the point of having this function.  It seems
that all call sites (setLocation, LocalDOMWindow::open, createWindow) call
canNavigate() before calling this.  Do you know what this adds over the security
checks done by canNavigate?

[dcheng, 2015-04-15 21:03:48]

dcheng@chromium.org changed reviewers:
+ haraken@chromium.org, japhet@chromium.org

[dcheng, 2015-04-15 21:03:49]

+haraken as the general bindings reviewer, +japhet for more insight on
isInsecureScriptAccess

https://codereview.chromium.org/1085973003/diff/1/Source/bindings/core/v8/Bin...
File Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1085973003/diff/1/Source/bindings/core/v8/Bin...
Source/bindings/core/v8/BindingSecurity.cpp:103: return
canAccessDocument(isolate, toLocalFrame(target)->document(), exceptionState);
On 2015/04/15 at 17:15:17, alexmos wrote:
> It seems like I can also go further and turn canAccessDocument into
canAccessFrame, which would let me do throwSecurityError() in just one location.
 Do you think that's a good idea?
> 

Yes, let's try to simplify this.

> Another question: should RemoteFrames support printErrorMessage()?  For some
reason, canAccessDocument and other calls like canNavigate() print access denied
errors on the target frame, not the calling frame.  Alternatively, we could
reverse these to print the error on the calling frame (which should always be
local): there's a FIXME in LocalFrame::printNavigationErrorMessage to that
effect.

I think it's more useful to log on the calling frame. Maybe haraken@ knows of a
historical reason why we log it on the target window instead?

https://codereview.chromium.org/1085973003/diff/1/Source/core/frame/DOMWindow.h
File Source/core/frame/DOMWindow.h (right):

https://codereview.chromium.org/1085973003/diff/1/Source/core/frame/DOMWindow...
Source/core/frame/DOMWindow.h:186: bool isInsecureScriptAccess(LocalDOMWindow&
callingWindow, const String& urlString);
On 2015/04/15 at 17:15:17, alexmos wrote:
> All of the call sites passed in a LocalDOMWindow, and I don't know why the
callingWindow could be a RemoteDOMWindow, so I changed the signature.  If
there's a good reason to keep it as DOMWindow, please let me know.
> 

LocalDOMWindow makes more sense.

> Actually, I don't quite understand the point of having this function.  It
seems that all call sites (setLocation, LocalDOMWindow::open, createWindow) call
canNavigate() before calling this.  Do you know what this adds over the security
checks done by canNavigate?

Are you sure all locations call canNavigate() first? It seems like window.open()
might not, even though it ends up calling this function.

https://codereview.chromium.org/1085973003/diff/20001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/1085973003/diff/20001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindow.cpp:230: // Sandbox errors: Use the origin of the
frames' location, rather than their actual origin (since we know that at least
one will be "null").
Is this going to be a problem for RemoteFrames?

https://codereview.chromium.org/1085973003/diff/20001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindow.cpp:245: // Protocol errors: Use the URL's protocol
rather than the origin's protocol so that we get a useful message for
non-heirarchal URLs like 'data:'.
Ditto--are there are non-data: URLs that this will cause trouble for?

[haraken, 2015-04-16 03:41:19]

https://codereview.chromium.org/1085973003/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1085973003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/BindingSecurity.cpp:96: if (target->isRemoteFrame() &&
target->domWindow()) {

Shouldn't this be something like:

if (target->isRemoteFrame()) {
  if (target->domWindow()) {
    throwSecurityError(...);
  }
  return false;
}

?

[alexmos, 2015-04-17 16:49:06]

Please take another look.  I've turned canAccessDocument into canAccessFrame to
make things more uniform.  I had to tweak parameters to support that check for a
Document with a null frame() (apparently exercised by
http/tests/inspector/template-content-inspect-crash.html, which passes this to
shouldAllowAccessToNode).

I also changed the cross-domain access error messages to be printed on the
calling window, rather than the target window, which enables them to work with
OOP targets.  haraken@: do you know of any reason for why this used the target
in the first place?  It seems more logical to use the calling window.

To be sure, I've checked what Firefox and IE does.  The error message from
shouldAllowAccessToFrame seems to only be printed when trying to navigate a
cross-origin frame (in a different window) to a javascript: URL.  For that case,
IE prints an error to the calling window, and FF denies access without printing
anything.  For a repro of the error from canNavigate, FF prints it on the
calling window, and IE doesn't print anything.  So the consensus seems to be
printing on the calling window.

> > Actually, I don't quite understand the point of having this function.  It
> seems that all call sites (setLocation, LocalDOMWindow::open, createWindow)
call
> canNavigate() before calling this.  Do you know what this adds over the
security
> checks done by canNavigate?
> 
> Are you sure all locations call canNavigate() first? It seems like
window.open()
> might not, even though it ends up calling this function.

Yes, looks like that's possible when callingWindow->document()->frame() is null
(as I found out in the inspector test above) -- when would that be the case
generally?  DocumentFragments?

https://codereview.chromium.org/1085973003/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1085973003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/BindingSecurity.cpp:96: if (target->isRemoteFrame() &&
target->domWindow()) {
On 2015/04/16 03:41:19, haraken wrote:
> 
> Shouldn't this be something like:
> 
> if (target->isRemoteFrame()) {
>   if (target->domWindow()) {
>     throwSecurityError(...);
>   }
>   return false;
> }
> 
> ?

Yes.  I've already removed this code though, as this will now be handled by
canAccessFrame(), the OOPIF-friendly replacement for canAccessDocument.

https://codereview.chromium.org/1085973003/diff/20001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/1085973003/diff/20001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindow.cpp:230: // Sandbox errors: Use the origin of the
frames' location, rather than their actual origin (since we know that at least
one will be "null").
On 2015/04/15 21:03:49, dcheng wrote:
> Is this going to be a problem for RemoteFrames?

Yes.  There isn't much we can do if the target frame is remote and sandboxed if
we don't replicate full URLs.  I turned my comment for targetURL below into a
TODO with some more explanation.

https://codereview.chromium.org/1085973003/diff/20001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindow.cpp:245: // Protocol errors: Use the URL's protocol
rather than the origin's protocol so that we get a useful message for
non-heirarchal URLs like 'data:'.
On 2015/04/15 21:03:49, dcheng wrote:
> Ditto--are there are non-data: URLs that this will cause trouble for?

This seems mostly relevant for data: URLs (looking at this code's history), and
that shouldn't be a problem here since frames with data URLs should stay in
their parent's process.  Same for about: and javascript: URLs, and I don't know
of other cases where this would matter (looking at shouldTreatAsUniqueOrigin and
shouldInheritSecurityOriginFromOwner), so this check should actually be fine.

[haraken, 2015-04-18 23:41:54]

> I also changed the cross-domain access error messages to be printed on the
> calling window, rather than the target window, which enables them to work with
> OOP targets.  haraken@: do you know of any reason for why this used the target
> in the first place?  It seems more logical to use the calling window.
> 
> To be sure, I've checked what Firefox and IE does.  The error message from
> shouldAllowAccessToFrame seems to only be printed when trying to navigate a
> cross-origin frame (in a different window) to a javascript: URL.  For that
case,
> IE prints an error to the calling window, and FF denies access without
printing
> anything.  For a repro of the error from canNavigate, FF prints it on the
> calling window, and IE doesn't print anything.  So the consensus seems to be
> printing on the calling window.

Yeah, using a calling window makes a lot of more sense. Can we make the change
separately before landing this CL?

[alexmos, 2015-04-20 23:39:55]

> Yeah, using a calling window makes a lot of more sense. Can we make the change
> separately before landing this CL?

Sure, I've put up a separate CL for you to review:
https://codereview.chromium.org/1096173003/
Once we're happy with it, I'll rebase this CL.

[alexmos, 2015-04-22 19:55:50]

On 2015/04/20 23:39:55, alexmos wrote:
> > Yeah, using a calling window makes a lot of more sense. Can we make the
change
> > separately before landing this CL?
> 
> Sure, I've put up a separate CL for you to review:
> https://codereview.chromium.org/1096173003/
> Once we're happy with it, I'll rebase this CL.

The calling window CL has now landed, and I've rebased this CL.  Please take
another look.

[haraken, 2015-04-23 04:35:56]

On 2015/04/22 19:55:50, alexmos wrote:
> On 2015/04/20 23:39:55, alexmos wrote:
> > > Yeah, using a calling window makes a lot of more sense. Can we make the
> change
> > > separately before landing this CL?
> > 
> > Sure, I've put up a separate CL for you to review:
> > https://codereview.chromium.org/1096173003/
> > Once we're happy with it, I'll rebase this CL.
> 
> The calling window CL has now landed, and I've rebased this CL.  Please take
> another look.

bindings/ looks good. Can we add a test?

[alexmos, 2015-04-23 06:49:41]

> bindings/ looks good. Can we add a test?

Is there something in particular for which you would like me to add a test? 
Without --site-per-process, these paths already have good coverage in various
http/tests/security/* layout tests (e.g., the
cross-frame-access-set-window-properties.html test I mentioned in the
description for exception throwing in canAccessFrame;
xss-DENIED-targeted-link-navigation.html for printing unsafe navigation error
messages), so trybots would've complained if this refactor broke something. 
After this CL, the same tests should also work with --site-per-process -- I
verified that manually as we're not yet running layout tests with
--site-per-process on the bots (there are other problems such as
https://crbug.com/476738 that we need to fix before that can happen).  I also
have a Chromium-side test for the printNavigationErrorMessage path in
https://codereview.chromium.org/1098763003/.

[haraken, 2015-04-23 07:38:10]

On 2015/04/23 06:49:41, alexmos wrote:
> > bindings/ looks good. Can we add a test?
> 
> Is there something in particular for which you would like me to add a test? 
> Without --site-per-process, these paths already have good coverage in various
> http/tests/security/* layout tests (e.g., the
> cross-frame-access-set-window-properties.html test I mentioned in the
> description for exception throwing in canAccessFrame;
> xss-DENIED-targeted-link-navigation.html for printing unsafe navigation error
> messages), so trybots would've complained if this refactor broke something. 
> After this CL, the same tests should also work with --site-per-process -- I
> verified that manually as we're not yet running layout tests with
> --site-per-process on the bots (there are other problems such as
> https://crbug.com/476738 that we need to fix before that can happen).  I also
> have a Chromium-side test for the printNavigationErrorMessage path in
> https://codereview.chromium.org/1098763003/.

ah, thanks, that would be enough. LGTM for bindings/.

[alexmos, 2015-04-23 17:04:26]

Thanks!  dcheng@: could you please take a look at the Source/core stuff?

[dcheng, 2015-04-23 17:39:58]

https://codereview.chromium.org/1085973003/diff/140001/Source/bindings/core/v...
File Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1085973003/diff/140001/Source/bindings/core/v...
Source/bindings/core/v8/BindingSecurity.cpp:87: return target &&
canAccessFrame(isolate, target->document().securityOrigin(),
target->document().domWindow(), exceptionState);
Would it make sense to have canAccessFrame() take a Frame*, and then we just
pass the target frame to it? That way, we don't have to pass the
SecurityOrigin/DOMWindow together like this.

[alexmos, 2015-04-23 17:45:28]

https://codereview.chromium.org/1085973003/diff/140001/Source/bindings/core/v...
File Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1085973003/diff/140001/Source/bindings/core/v...
Source/bindings/core/v8/BindingSecurity.cpp:87: return target &&
canAccessFrame(isolate, target->document().securityOrigin(),
target->document().domWindow(), exceptionState);
On 2015/04/23 17:39:57, dcheng wrote:
> Would it make sense to have canAccessFrame() take a Frame*, and then we just
> pass the target frame to it? That way, we don't have to pass the
> SecurityOrigin/DOMWindow together like this.

That was the first thing I tried, and unfortunately that didn't work, as
shouldAllowAccessToNode can be called with a target that has a Document with a
null frame() (This was exercised by
http/tests/inspector/template-content-inspect-crash.html).

[dcheng, 2015-04-23 18:01:25]

https://codereview.chromium.org/1085973003/diff/140001/Source/bindings/core/v...
File Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1085973003/diff/140001/Source/bindings/core/v...
Source/bindings/core/v8/BindingSecurity.cpp:87: return target &&
canAccessFrame(isolate, target->document().securityOrigin(),
target->document().domWindow(), exceptionState);
On 2015/04/23 17:45:28, alexmos wrote:
> On 2015/04/23 17:39:57, dcheng wrote:
> > Would it make sense to have canAccessFrame() take a Frame*, and then we just
> > pass the target frame to it? That way, we don't have to pass the
> > SecurityOrigin/DOMWindow together like this.
> 
> That was the first thing I tried, and unfortunately that didn't work, as
> shouldAllowAccessToNode can be called with a target that has a Document with a
> null frame() (This was exercised by
> http/tests/inspector/template-content-inspect-crash.html).

Do we actually return a true value in that case? Or would it return false
similar to lines 73-74 and 80-81?

[alexmos, 2015-04-23 21:25:23]

https://codereview.chromium.org/1085973003/diff/140001/Source/bindings/core/v...
File Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1085973003/diff/140001/Source/bindings/core/v...
Source/bindings/core/v8/BindingSecurity.cpp:87: return target &&
canAccessFrame(isolate, target->document().securityOrigin(),
target->document().domWindow(), exceptionState);
On 2015/04/23 18:01:24, dcheng wrote:
> On 2015/04/23 17:45:28, alexmos wrote:
> > On 2015/04/23 17:39:57, dcheng wrote:
> > > Would it make sense to have canAccessFrame() take a Frame*, and then we
just
> > > pass the target frame to it? That way, we don't have to pass the
> > > SecurityOrigin/DOMWindow together like this.
> > 
> > That was the first thing I tried, and unfortunately that didn't work, as
> > shouldAllowAccessToNode can be called with a target that has a Document with
a
> > null frame() (This was exercised by
> > http/tests/inspector/template-content-inspect-crash.html).
> 
> Do we actually return a true value in that case? Or would it return false
> similar to lines 73-74 and 80-81?

That particular test returns false - I think it has a DocumentFragment with a
unique origin and compares it to the test page's 127.0.0.1 origin.  But I'm not
sure that will always be false -- it seems DocumentFragments would pass their
creator Document's origin, which could be anything.  

That said, I'm also not sure under which circumstances shouldAllowAccessToNode
could get called for DocumentFragments.  Here's the call stack from the
inspector test, in case it's useful: 

#1  blink::BindingSecurity::shouldAllowAccessToNode (isolate=, target=,
exceptionState=...)
    at ../../third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:87
#2  blink::InjectedScriptHost::nodeAsScriptValue (scriptState=, node=)
    at
../../third_party/WebKit/Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:79
#3  blink::InspectableNode::get (this=, state=)
    at ../../third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:2160
#4  blink::V8InjectedScriptHost::inspectedObjectMethodCustom (info=...)
    at
../../third_party/WebKit/Source/bindings/core/v8/custom/V8InjectedScriptHostCustom.cpp:115
#5  blink::InjectedScriptHostV8Internal::inspectedObjectMethodCallback
(info=...)
    at gen/blink/bindings/core/v8/V8InjectedScriptHost.cpp:56
#6  v8::internal::FunctionCallbackArguments::Call (this=, 
   
f=<blink::InjectedScriptHostV8Internal::inspectedObjectMethodCallback(v8::FunctionCallbackInfo<v8::Value>
const&)>) at ../../v8/src/arguments.cc:33

[dcheng, 2015-04-23 21:41:21]

lgtm

I think it'd be nice to figure out if we can converge the behavior of
shouldAllowAccessToFrame and shouldAllowAccessToNode: the former denies access
if there is no frame, while the latter might permit it. But I would consider
that a bonus simplification, and not a main priority.

[alexmos, 2015-04-23 21:55:02]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-04-23 21:55:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1085973003/140001

[commit-bot: I haz the power, 2015-04-23 23:29:47]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194331