Bug 1353074 - Make unload event safe for introspection from content; r=maja_zf
☠☠ backed out by 7c7029cf53f5 ☠ ☠
authorAndreas Tolfsen <ato@mozilla.com>
Mon, 03 Apr 2017 18:36:43 +0100
changeset 558287 2670eec1ed8ab10cb44c7010d996fa28af565442
parent 558286 4b2f57ec5a584f4c52638cf3024b8a3ddbf88cfe
child 558288 8112153e07938722d989d6079d0e98ca3cedbac4
push id52860
push userbmo:walkingice0204@gmail.com
push dateFri, 07 Apr 2017 13:29:26 +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.addEventListener("unload", 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):