Bug 1567623 - Update AssertEvalNotUsingSystemPrincipal and re-enable it r=ckerschb

We now correctly handle the following cases:
 - Thunderbird
 - the Browser Toolbox/Console
 - Two safe and common idioms
 - when general.config.filename is set and userChromeJS does shenanigans

We also change the function to only crash in Debug mode, and for Release channels
we report diagnostic information in a way that does not reveal user data.

Differential Revision: https://phabricator.services.mozilla.com/D39557

--HG--
extra : moz-landing-system : lando

browser/app/profile/firefox.js

@@ -500,6 +500,9 @@ pref("browser.tabs.remote.enforceRemoteTypeRestrictions", true);
 #endif
 
 #ifdef NIGHTLY_BUILD
+// allow_eval_with_system_principal is enabled on Firefox Desktop only at this
+// point in time
+pref("security.allow_eval_with_system_principal", false);
 pref("browser.tabs.remote.useHTTPResponseProcessSelection", true);
 #else
 // Disabled outside of nightly due to bug 1554217

caps/nsScriptSecurityManager.cpp

@@ -401,12 +401,6 @@ bool nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(
     JSContext* cx, JS::HandleValue aValue) {
   MOZ_ASSERT(cx == nsContentUtils::GetCurrentJSContext());
 
-#if !defined(ANDROID) && (defined(NIGHTLY_BUILD) || defined(DEBUG))
-  nsCOMPtr<nsIPrincipal> subjectPrincipal = nsContentUtils::SubjectPrincipal();
-  nsContentSecurityManager::AssertEvalNotUsingSystemPrincipal(subjectPrincipal,
-                                                              cx);
-#endif
-
   // Get the window, if any, corresponding to the current global
   nsCOMPtr<nsIContentSecurityPolicy> csp;
   if (nsGlobalWindowInner* win = xpc::CurrentWindowOrNull(cx)) {
@@ -429,24 +423,35 @@ bool nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(
   bool reportViolation = false;
   nsresult rv = csp->GetAllowsEval(&reportViolation, &evalOK);
 
-  if (NS_FAILED(rv)) {
-    NS_WARNING("CSP: failed to get allowsEval");
-    return true;  // fail open to not break sites.
-  }
-
-  if (reportViolation) {
+  // A little convoluted. We want the scriptSample for a) reporting a violation
+  // or b) passing it to AssertEvalNotUsingSystemPrincipal. So do the work to
+  // get it if either of those cases is true.
+  nsAutoJSString scriptSample;
+  nsCOMPtr<nsIPrincipal> subjectPrincipal = nsContentUtils::SubjectPrincipal();
+  if (reportViolation || subjectPrincipal->IsSystemPrincipal()) {
     JS::Rooted<JSString*> jsString(cx, JS::ToString(cx, aValue));
     if (NS_WARN_IF(!jsString)) {
       JS_ClearPendingException(cx);
       return false;
     }
 
-    nsAutoJSString scriptSample;
     if (NS_WARN_IF(!scriptSample.init(cx, jsString))) {
       JS_ClearPendingException(cx);
       return false;
     }
+  }
 
+#if !defined(ANDROID) && (defined(NIGHTLY_BUILD) || defined(DEBUG))
+  nsContentSecurityManager::AssertEvalNotUsingSystemPrincipal(
+      cx, subjectPrincipal, scriptSample);
+#endif
+
+  if (NS_FAILED(rv)) {
+    NS_WARNING("CSP: failed to get allowsEval");
+    return true;  // fail open to not break sites.
+  }
+
+  if (reportViolation) {
     JS::AutoFilename scriptFilename;
     nsAutoString fileName;
     unsigned lineNum = 0;

dom/security/CSPEvalChecker.cpp

@@ -33,8 +33,8 @@ nsresult CheckInternal(nsIContentSecurityPolicy* aCSP,
 
 #if !defined(ANDROID) && (defined(NIGHTLY_BUILD) || defined(DEBUG))
   JSContext* cx = nsContentUtils::GetCurrentJSContext();
-  nsContentSecurityManager::AssertEvalNotUsingSystemPrincipal(aSubjectPrincipal,
-                                                              cx);
+  nsContentSecurityManager::AssertEvalNotUsingSystemPrincipal(
+      cx, aSubjectPrincipal, aExpression);
 #endif
 
   // The value is set at any "return", but better to have a default value here.

dom/security/nsContentSecurityManager.cpp

@@ -44,8 +44,24 @@ static nsLiteralCString evalAllowlist[] = {
     NS_LITERAL_CSTRING("resource://testing-common/ajv-4.1.1.js"),
     // Test-only utility
     NS_LITERAL_CSTRING("resource://testing-common/content-task.js"),
+
+    // The Browser Toolbox/Console
+    NS_LITERAL_CSTRING("debugger"),
+
+    // The following files are NOT supposed to stay on this whitelist.
+    // Bug numbers indicate planned removal of each file.
+
+    // Bug 1498560
+    NS_LITERAL_CSTRING("chrome://global/content/bindings/autocomplete.xml"),
 };
 
+// We also permit two specific idioms in eval()-like contexts. We'd like to
+// elminate these too; but there are in-the-wild Mozilla privileged extensions
+// that use them.
+static NS_NAMED_LITERAL_STRING(sAllowedEval1, "this");
+static NS_NAMED_LITERAL_STRING(sAllowedEval2,
+                               "function anonymous(\n) {\nreturn this\n}");
+
 /* static */
 bool nsContentSecurityManager::AllowTopLevelNavigationToDataURI(
     nsIChannel* aChannel) {
@@ -172,13 +188,38 @@ bool nsContentSecurityManager::AllowInsecureRedirectToDataURI(
 
 /* static */
 void nsContentSecurityManager::AssertEvalNotUsingSystemPrincipal(
-    nsIPrincipal* subjectPrincipal, JSContext* cx) {
-  if (!subjectPrincipal->IsSystemPrincipal()) {
+    JSContext* cx, nsIPrincipal* aSubjectPrincipal, const nsAString& aScript) {
+  if (!aSubjectPrincipal->IsSystemPrincipal()) {
     return;
   }
 
   // Use static pref for performance reasons.
   if (StaticPrefs::security_allow_eval_with_system_principal()) {
+    MOZ_LOG(sCSMLog, LogLevel::Debug,
+            ("Allowing eval() with SystemPrincipal because allowing pref is "
+             "enabled"));
+    return;
+  }
+
+  // This preferences is a file used for autoconfiguration of Firefox
+  // by administrators. It has also been (ab)used by the userChromeJS
+  // project to run legacy-style 'extensions', some of which use eval,
+  // all of which run in the System Principal context.
+  nsAutoString configPref;
+  Preferences::GetString("general.config.filename", configPref);
+  if (!configPref.IsEmpty()) {
+    MOZ_LOG(sCSMLog, LogLevel::Debug,
+            ("Allowing eval() with SystemPrincipal because of "
+             "general.config.filename"));
+    return;
+  }
+
+  // We permit these two common idioms to get access to the global JS object
+  if (!aScript.IsEmpty() &&
+      (aScript == sAllowedEval1 || aScript == sAllowedEval2)) {
+    MOZ_LOG(sCSMLog, LogLevel::Debug,
+            ("Allowing eval() with SystemPrincipal because a key string is "
+             "provided"));
     return;
   }
 
@@ -204,8 +245,20 @@ void nsContentSecurityManager::AssertEvalNotUsingSystemPrincipal(
     fileName = fileName_;
   }
 
-  MOZ_CRASH_UNSAFE_PRINTF("do not use eval with system privileges: %s",
-                          fileName.get());
+#ifdef DEBUG
+  MOZ_CRASH_UNSAFE_PRINTF(
+      "Blocking eval() with SystemPrincipal from file %s and script provided "
+      "%s",
+      fileName.get(), NS_ConvertUTF16toUTF8(aScript).get());
+#else
+  MOZ_LOG(sCSMLog, LogLevel::Debug,
+          ("Blocking eval() with SystemPrincipal from file %s and script "
+           "provided %s",
+           fileName.get(), NS_ConvertUTF16toUTF8(aScript).get()));
+#endif
+
+  // In the future, we will change this function to return false and abort JS
+  // execution without crashing the process. For now, just collect data.
 }
 
 /* static */

dom/security/nsContentSecurityManager.h

@@ -38,8 +38,9 @@ class nsContentSecurityManager : public nsIContentSecurityManager,
   static bool AllowTopLevelNavigationToDataURI(nsIChannel* aChannel);
   static bool AllowInsecureRedirectToDataURI(nsIChannel* aNewChannel);
 
-  static void AssertEvalNotUsingSystemPrincipal(nsIPrincipal* subjectPrincipal,
-                                                JSContext* cx);
+  static void AssertEvalNotUsingSystemPrincipal(JSContext* cx,
+                                                nsIPrincipal* aSubjectPrincipal,
+                                                const nsAString& aScript);
 
  private:
   static nsresult CheckChannel(nsIChannel* aChannel);

modules/libpref/init/StaticPrefList.yaml

@@ -5802,6 +5802,8 @@
   value: 40
   mirror: always
 
+# Disabled by default so it doesn't affect Thunderbird/SeaMonkey, but
+# enabled in firefox.js
 - name: security.allow_eval_with_system_principal
   type: bool
   value: true