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 555140 62518c7d62df0844cf6dc6ebbb357dd4bb793545
parent 555105 9aacfa8081b35bb8ae1a59ce3fd9d7aba57cfc7b
child 555141 d8d3ef8f703158df813859eab02042c647ed7a8d
push id52171
push userbmo:ato@mozilla.com
push dateMon, 03 Apr 2017 18:22:15 +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):