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

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Simon Hausmann (hausmann@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2006, 2008, 2009 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Library General Public License for more details.
  *
  * You should have received a copy of the GNU Library General Public License
  * along with this library; see the file COPYING.LIB.  If not, write to
  * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
  * Boston, MA 02110-1301, USA.
  */
 
 #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"
 #include "core/dom/Attribute.h"
 #include "core/dom/Document.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/RemoteFrame.h"
 #include "core/frame/RemoteFrameView.h"
 #include "core/html/parser/HTMLParserIdioms.h"
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "core/page/FocusController.h"
 #include "core/page/Page.h"
 
 namespace blink {
 
 using namespace HTMLNames;
 
 HTMLFrameElementBase::HTMLFrameElementBase(const QualifiedName& tagName,
                                            Document& document)
     : HTMLFrameOwnerElement(tagName, document),
       m_scrollingMode(ScrollbarAuto),
       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();
   if (parentFrame)
     return parentFrame->isURLAllowed(completeURL);
 
   return true;
 }
 
 void HTMLFrameElementBase::openURL(bool replaceCurrentItem) {
@@ skipping 173 matching lines @@
   m_marginWidth = marginWidth;
   frameOwnerPropertiesChanged();
 }
 
 void HTMLFrameElementBase::setMarginHeight(int marginHeight) {
   m_marginHeight = marginHeight;
   frameOwnerPropertiesChanged();
 }
 
 }  // namespace blink