Bug 1353074 - Make unload event safe for introspection from content; r=maja_zf draft
authorAndreas Tolfsen <ato@mozilla.com>
Mon, 03 Apr 2017 18:36:43 +0100
changeset 567662 c5f4ab3ff0c4d54e187a38099faa4bd59e9dfc0a
parent 567612 a30dc237c3a600a5231f2974fc2b85dfb5513414
child 567663 3a3b3757f2149e4a2e182be65c9049b0793cc702
push id55659
push userbmo:ato@mozilla.com
push dateTue, 25 Apr 2017 11:34:29 +0000
reviewersmaja_zf
bugs1353074
milestone55.0a1
Bug 1353074 - Make unload event safe for introspection from content; r=maja_zf Marionette does not protect the unloadHandler in testing/marionette/evaluate.js from content introspection or modification, which can happen when web frameworks override window.addEventListener/window.removeEventListener. The script evaluation module used in Marionette relies on sandbox.window.addEventListener/removeEventListener to throw an error when script execution is aborted due to the document unloading itself. If the window.addEventListener/removeEventListener functions have been overridden to introspect the objects that are passed, they may inadvertently touch objects originating from chrome space, such as the unloadHandler. Because the Gecko sandboxing system put in place strict security measures to prevent accidental chrome-space modification from content, inspecting the unloadHandler will throw a permission denied error once the script has finished executing. We have found examples in the wild of this in particular with the Angular web framework. This patch makes the unloadHandler safe for introspection from web content. Fixes: https://github.com/mozilla/geckodriver/issues/515 MozReview-Commit-ID: E2LgPhLLuDT
testing/marionette/evaluate.js
testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py
--- a/testing/marionette/evaluate.js
+++ b/testing/marionette/evaluate.js
@@ -100,18 +100,19 @@ this.evaluate = {};
  */
 evaluate.sandbox = function (sb, script, args = [], opts = {}) {
   let scriptTimeoutID, timeoutHandler, unloadHandler;
 
   let promise = new Promise((resolve, reject) => {
     let src = "";
     sb[COMPLETE] = resolve;
     timeoutHandler = () => reject(new ScriptTimeoutError("Timed out"));
-    unloadHandler = () => reject(
-        new JavaScriptError("Document was unloaded during execution"));
+    unloadHandler = sandbox.cloneInto(
+        () => reject(new JavaScriptError("Document was unloaded during execution")),
+        sb);
 
     // wrap in function
     if (!opts.directInject) {
       if (opts.async) {
         sb[CALLBACK] = sb[COMPLETE];
       }
       sb[ARGUMENTS] = sandbox.cloneInto(args, sb);
 
@@ -140,17 +141,17 @@ evaluate.sandbox = function (sb, script,
       sb.window.onerror = (msg, url, line) => {
         let err = new JavaScriptError(`${msg} at ${url}:${line}`);
         reject(err);
       };
     }
 
     // timeout and unload handlers
     scriptTimeoutID = setTimeout(timeoutHandler, opts.timeout || DEFAULT_TIMEOUT);
-    sb.window.onunload = sandbox.cloneInto(unloadHandler, sb);
+    sb.window.onunload = unloadHandler;
 
     let res;
     try {
       res = Cu.evalInSandbox(src, sb, "1.8", opts.filename || "dummy file", 0);
     } catch (e) {
       let err = new JavaScriptError(
           e,
           "execute_script",
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py
@@ -258,28 +258,50 @@ class TestExecuteContent(MarionetteTestC
             "return window.n", sandbox=None),
             "setTimeout already fired")
 
         # if event was cancelled, this will time out
         Wait(self.marionette, timeout=8).until(
             content_timeout_triggered,
             message="Scheduled setTimeout event was cancelled by call to execute_script")
 
-    def test_privileged_code_inspection(self):
-        # test permission denied on toString of unload event handler
+    def test_access_chrome_objects_in_event_listeners(self):
+        # sandbox.window.addEventListener/removeEventListener
+        # is used by Marionette for installing the unloadHandler which
+        # is used to return an error when a document is unloaded during
+        # script execution.
+        #
+        # Certain web frameworks, notably Angular, override
+        # window.addEventListener/removeEventListener and introspects
+        # objects passed to them.  If these objects originates from chrome
+        # without having been cloned, a permission denied error is thrown
+        # as part of the security precautions put in place by the sandbox.
+
+        # addEventListener is called when script is injected
         self.marionette.navigate(inline("""
             <script>
-            window.addEventListener = (type, handler) => handler.toString();
-            </script>"""))
+            window.addEventListener = (event, listener) => listener.toString();
+            </script>
+            """))
         self.marionette.execute_script("", sandbox=None)
 
+        # removeEventListener is called when sandbox is unloaded
+        self.marionette.navigate(inline("""
+            <script>
+            window.removeEventListener = (event, listener) => listener.toString();
+            </script>
+            """))
+        self.marionette.execute_script("", sandbox=None)
+
+    def test_access_global_objects_from_chrome(self):
         # test inspection of arguments
         self.marionette.execute_script("__webDriverArguments.toString()")
 
 
+
 class TestExecuteChrome(WindowManagerMixin, TestExecuteContent):
 
     def setUp(self):
         super(TestExecuteChrome, self).setUp()
 
         self.marionette.set_context("chrome")
 
     def tearDown(self):
@@ -328,16 +350,19 @@ class TestExecuteChrome(WindowManagerMix
         pass
 
     def test_window_set_timeout_is_not_cancelled(self):
         pass
 
     def test_privileged_code_inspection(self):
         pass
 
+    def test_access_chrome_objects_in_event_listeners(self):
+        pass
+
 
 class TestElementCollections(MarionetteTestCase):
 
     def assertSequenceIsInstance(self, seq, typ):
         for item in seq:
             self.assertIsInstance(item, typ)
 
     def test_array(self):