CSP: Check inline event handlers on each run, not only the first
authordbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 Mar 2016 19:08:43 +0000 (19:08 +0000)
committerdbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 Mar 2016 19:08:43 +0000 (19:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=115700
<rdar://problem/24211159>

Reviewed by Andy Estes.

Source/WebCore:

Fixes an issue where an inline event handler would always be allowed to execute if it
executed at least once.

Currently we query whether the Content Security Policy (CSP) of the page permits inline event
handlers each time we register a new handler for an event. And a handler is registered exactly
once the first time the event associated with it is dispatched. Once a handler is registered
as a listener for an event E then we will always invoke the handler when event E is dispatched
regardless of whether the CSP of the page changes (say, as a result of programmatically inserting
a <meta http-equiv="Content-Security-Policy">). Instead we should always check the
CSP of the page whenever we are going to invoke an event handler.

* bindings/js/JSEventListener.cpp:
(WebCore::JSEventListener::handleEvent): Check the CSP of the page and bail out if the
policy does not permit execution of an inline event handler.
* bindings/js/JSEventListener.h:
(WebCore::JSEventListener::sourceURL): Added. Default implementation that returns an empty string.
(WebCore::JSEventListener::sourcePosition): Added. Default implementation that returns a default position.
* bindings/js/JSLazyEventListener.cpp:
(WebCore::JSLazyEventListener::JSLazyEventListener): Update code following instance variable
renaming in JSLazyEventListener.h.
(WebCore::JSLazyEventListener::initializeJSFunction): Ditto.
* bindings/js/JSLazyEventListener.h: Override JSEventListener::sourceURL() and JSEventListener::sourcePosition().
Changed all mutable instance variables to immutable ones as we do not modify these variables
in any const member functions. Also renamed instance variable m_position to m_sourcePosition
to better describe that it represents the source code position where the event handler was defined.

LayoutTests:

Update expected result for test http/tests/security/contentSecurityPolicy/inline-event-handler-blocked-after-injecting-meta.html
and remove its entry from file LayoutTests/TestExpectations now that it passes.

* TestExpectations:
* http/tests/security/contentSecurityPolicy/inline-event-handler-blocked-after-injecting-meta-expected.txt:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@198541 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/http/tests/security/contentSecurityPolicy/inline-event-handler-blocked-after-injecting-meta-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSEventListener.cpp
Source/WebCore/bindings/js/JSEventListener.h
Source/WebCore/bindings/js/JSLazyEventListener.cpp
Source/WebCore/bindings/js/JSLazyEventListener.h

index 32bb70a..63d7743 100644 (file)
@@ -1,3 +1,17 @@
+2016-03-22  Daniel Bates  <dabates@apple.com>
+
+        CSP: Check inline event handlers on each run, not only the first
+        https://bugs.webkit.org/show_bug.cgi?id=115700
+        <rdar://problem/24211159>
+
+        Reviewed by Andy Estes.
+
+        Update expected result for test http/tests/security/contentSecurityPolicy/inline-event-handler-blocked-after-injecting-meta.html
+        and remove its entry from file LayoutTests/TestExpectations now that it passes.
+
+        * TestExpectations:
+        * http/tests/security/contentSecurityPolicy/inline-event-handler-blocked-after-injecting-meta-expected.txt:
+
 2016-03-22  Ryan Haddad  <ryanhaddad@apple.com>
 
         Skipping media/media-document-audio-repaint.html on El Capitan Debug WK2
index 4ad6b84..65cafdd 100644 (file)
@@ -861,7 +861,6 @@ http/tests/security/contentSecurityPolicy/1.1/plugintypes-url-01.html [ Pass ]
 http/tests/security/contentSecurityPolicy/1.1/plugintypes-url-02.html [ Pass ]
 webkit.org/b/154203 http/tests/security/contentSecurityPolicy/1.1/frame-ancestors/frame-ancestors-overrides-xfo.html
 webkit.org/b/111869 http/tests/security/contentSecurityPolicy/eval-blocked-and-sends-report.html
-webkit.org/b/115700 http/tests/security/contentSecurityPolicy/inline-event-handler-blocked-after-injecting-meta.html [ Failure ]
 webkit.org/b/153148 http/tests/security/contentSecurityPolicy/eval-allowed-in-report-only-mode-and-sends-report.html
 webkit.org/b/153150 http/tests/security/contentSecurityPolicy/frame-src-cross-origin-load.html
 webkit.org/b/153150 http/tests/security/contentSecurityPolicy/1.1/child-src/frame-fires-load-event-when-blocked.html
index f485bdc..cf14095 100644 (file)
@@ -2,6 +2,6 @@ CONSOLE MESSAGE: line 8: Clicking a link, pre-policy:
 CONSOLE MESSAGE: line 21: PASS: Event handler triggered pre-policy.
 CONSOLE MESSAGE: line 14: Injecting Content-Security-Policy.
 CONSOLE MESSAGE: line 19: Clicking a link, post-policy:
-CONSOLE MESSAGE: line 21: Refused to execute inline event handler because it violates the following Content Security Policy directive: "default-src 'self'". Either the 'unsafe-inline' keyword, a hash ('sha256-...'), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'script-src' was not explicitly set, so 'default-src' is used as a fallback.
+CONSOLE MESSAGE: line 21: Refused to execute inline event handler because it violates the following Content Security Policy directive: "default-src 'self'". Note that 'script-src' was not explicitly set, so 'default-src' is used as a fallback.
 
 This test checks that CSP is evaluated on each call to an inline event handler, even if it's been executed pre-policy. It passes if one 'PASS' and no 'FAIL' messages appear.
index 80e1025..678a19b 100644 (file)
@@ -1,3 +1,37 @@
+2016-03-22  Daniel Bates  <dabates@apple.com>
+
+        CSP: Check inline event handlers on each run, not only the first
+        https://bugs.webkit.org/show_bug.cgi?id=115700
+        <rdar://problem/24211159>
+
+        Reviewed by Andy Estes.
+
+        Fixes an issue where an inline event handler would always be allowed to execute if it
+        executed at least once.
+
+        Currently we query whether the Content Security Policy (CSP) of the page permits inline event
+        handlers each time we register a new handler for an event. And a handler is registered exactly
+        once the first time the event associated with it is dispatched. Once a handler is registered
+        as a listener for an event E then we will always invoke the handler when event E is dispatched
+        regardless of whether the CSP of the page changes (say, as a result of programmatically inserting
+        a <meta http-equiv="Content-Security-Policy">). Instead we should always check the
+        CSP of the page whenever we are going to invoke an event handler.
+
+        * bindings/js/JSEventListener.cpp:
+        (WebCore::JSEventListener::handleEvent): Check the CSP of the page and bail out if the
+        policy does not permit execution of an inline event handler.
+        * bindings/js/JSEventListener.h:
+        (WebCore::JSEventListener::sourceURL): Added. Default implementation that returns an empty string.
+        (WebCore::JSEventListener::sourcePosition): Added. Default implementation that returns a default position.
+        * bindings/js/JSLazyEventListener.cpp:
+        (WebCore::JSLazyEventListener::JSLazyEventListener): Update code following instance variable
+        renaming in JSLazyEventListener.h.
+        (WebCore::JSLazyEventListener::initializeJSFunction): Ditto. 
+        * bindings/js/JSLazyEventListener.h: Override JSEventListener::sourceURL() and JSEventListener::sourcePosition().
+        Changed all mutable instance variables to immutable ones as we do not modify these variables
+        in any const member functions. Also renamed instance variable m_position to m_sourcePosition
+        to better describe that it represents the source code position where the event handler was defined.
+
 2016-03-22  Jer Noble  <jer.noble@apple.com>
 
         Media elements allowed to play without a user gesture, but requiring fullscreen playback, should not be allowed to autoplay.
index ee1ed93..dd28204 100644 (file)
@@ -21,6 +21,7 @@
 #include "JSEventListener.h"
 
 #include "BeforeUnloadEvent.h"
+#include "ContentSecurityPolicy.h"
 #include "Event.h"
 #include "Frame.h"
 #include "HTMLElement.h"
@@ -93,6 +94,8 @@ void JSEventListener::handleEvent(ScriptExecutionContext* scriptExecutionContext
         JSDOMWindow* window = jsCast<JSDOMWindow*>(globalObject);
         if (!window->wrapped().isCurrentlyDisplayedInFrame())
             return;
+        if (wasCreatedFromMarkup() && !scriptExecutionContext->contentSecurityPolicy()->allowInlineEventHandlers(sourceURL(), sourcePosition().m_line))
+            return;
         // FIXME: Is this check needed for other contexts?
         ScriptController& script = window->wrapped().frame()->script();
         if (!script.canExecuteScripts(AboutToExecuteScript) || script.isPaused())
index aa71cba..a039553 100644 (file)
@@ -26,6 +26,8 @@
 #include <heap/Weak.h>
 #include <heap/WeakInlines.h>
 #include <wtf/Ref.h>
+#include <wtf/text/TextPosition.h>
+#include <wtf/text/WTFString.h>
 
 namespace WebCore {
 
@@ -61,6 +63,9 @@ public:
     JSC::JSObject* wrapper() const { return m_wrapper.get(); }
     void setWrapper(JSC::VM&, JSC::JSObject* wrapper) const { m_wrapper = JSC::Weak<JSC::JSObject>(wrapper); }
 
+    virtual String sourceURL() const { return String(); }
+    virtual TextPosition sourcePosition() const { return TextPosition::minimumPosition(); }
+
 private:
     virtual JSC::JSObject* initializeJSFunction(ScriptExecutionContext*) const;
     void visitJSFunction(JSC::SlotVisitor&) override;
index 48479d0..26af024 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  Copyright (C) 2001 Peter Kelly (pmk@post.com)
- *  Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2013 Apple Inc. All Rights Reserved.
+ *  Copyright (C) 2003-2016 Apple Inc. All Rights Reserved.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Lesser General Public
@@ -37,13 +37,13 @@ namespace WebCore {
 
 DEFINE_DEBUG_ONLY_GLOBAL(WTF::RefCountedLeakCounter, eventListenerCounter, ("JSLazyEventListener"));
 
-JSLazyEventListener::JSLazyEventListener(const String& functionName, const String& eventParameterName, const String& code, ContainerNode* node, const String& sourceURL, const TextPosition& position, JSObject* wrapper, DOMWrapperWorld& isolatedWorld)
+JSLazyEventListener::JSLazyEventListener(const String& functionName, const String& eventParameterName, const String& code, ContainerNode* node, const String& sourceURL, const TextPosition& sourcePosition, JSObject* wrapper, DOMWrapperWorld& isolatedWorld)
     : JSEventListener(0, wrapper, true, isolatedWorld)
     , m_functionName(functionName)
     , m_eventParameterName(eventParameterName)
     , m_code(code)
     , m_sourceURL(sourceURL)
-    , m_position(position)
+    , m_sourcePosition(sourcePosition)
     , m_originalNode(node)
 {
     // We don't retain the original node because we assume it
@@ -54,8 +54,8 @@ JSLazyEventListener::JSLazyEventListener(const String& functionName, const Strin
 
     // A JSLazyEventListener can be created with a line number of zero when it is created with
     // a setAttribute call from JavaScript, so make the line number 1 in that case.
-    if (m_position == TextPosition::belowRangePosition())
-        m_position = TextPosition::minimumPosition();
+    if (m_sourcePosition == TextPosition::belowRangePosition())
+        m_sourcePosition = TextPosition::minimumPosition();
 
     ASSERT(m_eventParameterName == "evt" || m_eventParameterName == "event");
 
@@ -87,7 +87,7 @@ JSObject* JSLazyEventListener::initializeJSFunction(ScriptExecutionContext* exec
     if (!document.frame())
         return nullptr;
 
-    if (!document.contentSecurityPolicy()->allowInlineEventHandlers(m_sourceURL, m_position.m_line))
+    if (!document.contentSecurityPolicy()->allowInlineEventHandlers(m_sourceURL, m_sourcePosition.m_line))
         return nullptr;
 
     ScriptController& script = document.frame()->script();
@@ -106,11 +106,11 @@ JSObject* JSLazyEventListener::initializeJSFunction(ScriptExecutionContext* exec
 
     // We want all errors to refer back to the line on which our attribute was
     // declared, regardless of any newlines in our JavaScript source text.
-    int overrideLineNumber = m_position.m_line.oneBasedInt();
+    int overrideLineNumber = m_sourcePosition.m_line.oneBasedInt();
 
     JSObject* jsFunction = constructFunctionSkippingEvalEnabledCheck(
         exec, exec->lexicalGlobalObject(), args, Identifier::fromString(exec, m_functionName),
-        m_sourceURL, m_position, overrideLineNumber);
+        m_sourceURL, m_sourcePosition, overrideLineNumber);
 
     if (exec->hadException()) {
         reportCurrentException(exec);
index 6148aeb..3836579 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  Copyright (C) 2001 Peter Kelly (pmk@post.com)
- *  Copyright (C) 2003, 2008, 2009, 2013 Apple Inc. All rights reserved.
+ *  Copyright (C) 2003-2016 Apple Inc. All rights reserved.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Lesser General Public
@@ -21,8 +21,6 @@
 #define JSLazyEventListener_h
 
 #include "JSEventListener.h"
-#include <wtf/text/TextPosition.h>
-#include <wtf/text/WTFString.h>
 
 namespace WebCore {
 
@@ -40,6 +38,9 @@ public:
 
     virtual ~JSLazyEventListener();
 
+    String sourceURL() const final { return m_sourceURL; }
+    TextPosition sourcePosition() const final { return m_sourcePosition; }
+
 private:
     struct CreationArguments;
     static RefPtr<JSLazyEventListener> create(const CreationArguments&);
@@ -49,11 +50,11 @@ private:
     JSC::JSObject* initializeJSFunction(ScriptExecutionContext*) const override;
     bool wasCreatedFromMarkup() const override { return true; }
 
-    mutable String m_functionName;
-    mutable String m_eventParameterName;
-    mutable String m_code;
-    mutable String m_sourceURL;
-    TextPosition m_position;
+    String m_functionName;
+    String m_eventParameterName;
+    String m_code;
+    String m_sourceURL;
+    TextPosition m_sourcePosition;
     ContainerNode* m_originalNode;
 };