New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

e10s: user scripts fail to load due to the content process sandbox #2485

Closed
RealDolos opened this Issue Mar 4, 2017 · 8 comments

Comments

Projects
None yet
4 participants
@RealDolos
Contributor

RealDolos commented Mar 4, 2017

Firefox gradually introduces/introduced a sandbox for content processes[1]
Said sandbox will all block file accesses[2] when the level is configured to be at least 2, which is the case for Nightly at the moment. The sandbox works at least on macOS, meaning user scripts fail to load when e10s is enabled.

STR

  • Get Nightly and install GM
  • Create a new user script with whatever content (console.log("hello, world");)
  • Load a page where this user script should run

Expected:
User script runs

Actual:
User script fails to run with the subscript loader bailing with

Error opening input stream (invalid filename?): file:///Users/dolos/gmtest.p/gm_scripts/test/test.user.js

Console.app where sandbox violations are logged in macOS

SandboxViolation: plugin-container(35814) deny(1) file-read-metadata /Users/dolos/gmtest.p/gm_scripts/test/test.user.js

Workarounds

  • Disable e10s
  • Set security.sandbox.content.level to 1 or 0 lowering the sandbox protections

Remedies

Sync-message the main process and have it load the script source, then evalInSandbox the contents.

I prodded the source a bit till it seemed to work. Also @resource is not addressed at all but would work similarly I guess. Due to these caveats, no pull request.

diff --git a/components/greasemonkey.js b/components/greasemonkey.js
index 752a16c9..dccc9bc6 100644
--- a/components/greasemonkey.js
+++ b/components/greasemonkey.js
@@ -77,6 +77,10 @@ function startup(aService) {
       'greasemonkey:script-install', aService.scriptInstall.bind(aService));
   parentMessageManager.addMessageListener(
       'greasemonkey:url-is-temp-file', aService.urlIsTempFile.bind(aService));
+  parentMessageManager.addMessageListener(
+      'greasemonkey:getScript', m => {
+        return GM_util.fileXhr(m.data, "application/javascript,charset=utf-8");
+      });
 
   // Yes, we have to load the frame script once here in the parent scope. Why!?
   globalMessageManager.loadFrameScript(
diff --git a/modules/sandbox.js b/modules/sandbox.js
index 7c91f2c1..9284f2ee 100644
--- a/modules/sandbox.js
+++ b/modules/sandbox.js
@@ -182,12 +182,39 @@ function injectGMInfo(aScript, sandbox, aContentWin) {
   });
 }
 
+const remote = (function() {
+  if (!("@mozilla.org/xre/app-info;1" in Cc)) {
+    return false;
+  }
+  let runtime = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime);
+  return runtime.processType !== Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;
+})();
+
+const cpmm = (function() {
+  try {
+    if (remote) {
+      return Cc["@mozilla.org/childprocessmessagemanager;1"].
+             getService(Ci.nsISyncMessageSender);
+    }
+  } catch (e) {
+    // fall through
+  }
+  return null;
+})();
+
+function loadCode(url) {
+  if (remote) {
+      return cpmm.sendSyncMessage("greasemonkey:getScript", url);
+  }
+  return GM_util.fileXhr(url, "application/javascript,charset=utf-8");
+}
 
 function runScriptInSandbox(script, sandbox) {
   // Eval the code, with anonymous wrappers when/if appropriate.
   function evalWithWrapper(url) {
+    const code = loadCode(url);
     try {
-      subLoader.loadSubScript(url, sandbox, "UTF-8");
+      Cu.evalInSandbox(code, sandbox, "latest", url);
     } catch (e) {
       if ("return not in function" == e.message) {
         // See #1592; we never anon wrap anymore, unless forced to by a return
@@ -198,14 +225,11 @@ function runScriptInSandbox(script, sandbox) {
             e.fileName,
             e.lineNumber
             );
-
-        var code = GM_util.fileXhr(url, "application/javascript");
-        Components.utils.evalInSandbox(
-            '(function(){ '+code+'\n})()', sandbox, gMaxJSVersion, url, 1);
-      } else {
-        // Otherwise raise.
-        throw e;
+        Cu.evalInSandbox(
+            '(function(){ ' + code + '\n})()', sandbox, gMaxJSVersion, url, 1);
+        return;
       }
+      throw e;
     }
   }

[1] https://wiki.mozilla.org/Security/Sandbox
[2] https://wiki.mozilla.org/Sandbox/OS_X_Rule_Set#How_security.sandbox.content.level_Affects_File_Access

@arantius arantius added this to the 3.11 milestone Mar 4, 2017

@arantius

This comment has been minimized.

Show comment
Hide comment
@arantius

arantius Mar 4, 2017

Collaborator

Thank you immensely for your careful and detailed report!

With WebExtensions looming, most of my available time has been looking there, so that Greasemonkey could survive the year. If you finish the above work into a pull request, it's likely to be merged. If not, I can't promise when/if I can do that myself.

Collaborator

arantius commented Mar 4, 2017

Thank you immensely for your careful and detailed report!

With WebExtensions looming, most of my available time has been looking there, so that Greasemonkey could survive the year. If you finish the above work into a pull request, it's likely to be merged. If not, I can't promise when/if I can do that myself.

@janekptacijarabaci

This comment has been minimized.

Show comment
Hide comment
@janekptacijarabaci

janekptacijarabaci Mar 5, 2017

Contributor

@RealDolos

Also @resource is not addressed at all but would work similarly I guess.

AFAIK: It's not so easy... See #2376

Contributor

janekptacijarabaci commented Mar 5, 2017

@RealDolos

Also @resource is not addressed at all but would work similarly I guess.

AFAIK: It's not so easy... See #2376

@RealDolos

This comment has been minimized.

Show comment
Hide comment
@RealDolos

RealDolos Mar 5, 2017

Contributor

@arantius Would it be OK to fix only the script stuff for now and leave @resource stuff for later (if ever). That way at least scripts not using that feature would start to work again...

Contributor

RealDolos commented Mar 5, 2017

@arantius Would it be OK to fix only the script stuff for now and leave @resource stuff for later (if ever). That way at least scripts not using that feature would start to work again...

@arantius

This comment has been minimized.

Show comment
Hide comment
@arantius

arantius Mar 5, 2017

Collaborator

Absolutely. Partially working is better than not working at all.

Collaborator

arantius commented Mar 5, 2017

Absolutely. Partially working is better than not working at all.

@the8472

This comment has been minimized.

Show comment
Hide comment
@the8472

the8472 Mar 12, 2017

Contributor

this seems to be a dup of #2296

Contributor

the8472 commented Mar 12, 2017

this seems to be a dup of #2296

@arantius

This comment has been minimized.

Show comment
Hide comment
@arantius

arantius Apr 25, 2017

Collaborator

WFM as is on Windows, so this error must be Mac only (as hinted at in the original description). Fix verification will be tricky for me as Mac is not a primary (and definitely not a development) platform.

Collaborator

arantius commented Apr 25, 2017

WFM as is on Windows, so this error must be Mac only (as hinted at in the original description). Fix verification will be tricky for me as Mac is not a primary (and definitely not a development) platform.

@arantius arantius closed this in #2486 Apr 25, 2017

@arantius

This comment has been minimized.

Show comment
Hide comment
@arantius

arantius Apr 25, 2017

Collaborator

I've just pushed version 3.11beta1: https://addons.mozilla.org/en-US/firefox/addon/greasemonkey/versions/beta?page=1#version-3.11beta1

It would be very useful if you could install it and confirm that it fixes this issue.

Collaborator

arantius commented Apr 25, 2017

I've just pushed version 3.11beta1: https://addons.mozilla.org/en-US/firefox/addon/greasemonkey/versions/beta?page=1#version-3.11beta1

It would be very useful if you could install it and confirm that it fixes this issue.

@janekptacijarabaci

This comment has been minimized.

Show comment
Hide comment
@janekptacijarabaci

janekptacijarabaci Jun 25, 2017

Contributor

This problem has returned:
#1230 (comment)
(just for info)

Contributor

janekptacijarabaci commented Jun 25, 2017

This problem has returned:
#1230 (comment)
(just for info)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment