Don't skip security checks for javascript: URLs when the JS stack is empty.

third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp

Index: third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp b/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp
index 5168a162275426a27313b095d744f7cc1b87cfc3..b0f8a39877a55f0f1b0a625959820cfbdedc51e2 100644
--- a/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp
@@ -23,6 +23,7 @@
 
 #include "core/html/HTMLFrameElementBase.h"
 
+#include "bindings/core/v8/BindingSecurity.h"
 #include "bindings/core/v8/ScriptController.h"
 #include "bindings/core/v8/ScriptEventListener.h"
 #include "core/HTMLNames.h"
@@ -49,17 +50,24 @@ HTMLFrameElementBase::HTMLFrameElementBase(const QualifiedName& tagName,
       m_marginWidth(-1),
       m_marginHeight(-1) {}
 
-bool HTMLFrameElementBase::isURLAllowed() const {
+bool HTMLFrameElementBase::isURLAllowed(bool fromLayout) const {

[esprehn, 2016/11/15 08:27:12]

This check is named wrong, you set that bool inside the layout object creation
check which is inside style recalc. All returning false there does is make the
iframe display none, it doesn't control if the frame is loaded or not, and it
isn't from layout.

I guess what you're trying to do is keep the tests passing that display that
frame but it's not at all clear to me why we make iframes for urls that are not
allowed display none. Why shouldn't the iframe display a border at least? What
do other browsers do here?

If we want to keep this behavior we should split this function and have some
shared checks and one method for if the layout object is needed and another for
is we should load the URL.

   if (m_URL.isEmpty())
     return true;
 
   const KURL& completeURL = document().completeURL(m_URL);
 
-  if (protocolIsJavaScript(completeURL)) {
-    if (contentFrame() &&
-        !ScriptController::canAccessFromCurrentOrigin(toIsolate(&document()),
-                                                      contentFrame()))
-      return false;
+  if (contentFrame() && protocolIsJavaScript(completeURL)) {
+    v8::Isolate* isolate = toIsolate(&document());
+    if (isolate->InContext()) {

[haraken, 2016/11/15 08:44:24]

In the first place, would you help me understand why we need to check a
different thing depending on whether we have JavaScript on stack or not?

[dcheng, 2016/11/15 08:55:07]

Here is my understanding:

If there is no current context, the javascript: URL must come from the parser.
If it is coming from the parser, it must have been specified like this:

  <iframe src="javascript:..."></iframe>

Since the iframe must be same-origin, it is always safe to execute the
javascript: URL, so we can skip the security checks.

If there is a current context, then the javascript: URL could have been set by
script like this:

  window[0].location = 'javascript:...';

In this case, we must make sure that the current context is same-origin with the
target frame; otherwise, the javascript: URL will be executing in a cross-origin
context.

However, the problem is with the "if there is no context, always allow" branch.
In theory, this should be true. However, if there is a Blink bug that causes a
frame that already has a cross-origin document to be re-inserted from the
parser, the security check gets skipped since there is no current context.

(I don't quite understand the last part: the calls that move nodes around in the
DOM are coming from JS, so I'm not sure why there's no current context. See
https://crbug.com/456518, which hints this to be the case)

+      if (!BindingSecurity::shouldAllowAccessToFrame(
+              currentDOMWindow(isolate), contentFrame(),
+              BindingSecurity::ErrorReportOption::Report))
+        return false;
+    } else {
+      if (!fromLayout && !document().getSecurityOrigin()->canAccess(

[dcheng, 2016/11/15 08:14:21]

How come we need to skip this block if we're in layout?

If we didn't have this, we could (in theory) combine this into:

LocalDOMWindow* accessingWindow = isolate->InContext() ?
currentDOMWindow(isolate) : document()->window();
if (!BindingSecurity::shouldAllowAccessToFrame(...))

(I would be OK investigating this in a followup though, there is some risk here:
if there's a contentFrame(), the containing document() should be active, but
there's no asserts that document that, and who knows what strange corner cases
are out there)

+              contentFrame()->securityContext()->getSecurityOrigin()))
+        return false;
+    }
   }
 
   LocalFrame* parentFrame = document().frame();