commit 45ffbc50f1f2
Author: David Keeler <dkeeler@mozilla.com>
Date:   Fri Oct 13 11:27:30 2017 -0700

    Bug 1406396 - Work around NSS utils potentially loading spurious root cert modules. r=mgoodwin, a=ritu, l10n=flod
    
    NSS command-line utilities may add a built-in root certificate module with the
    name "Root Certs" if run on a profile that has a copy of the module file (which
    is an unexpected configuration in general for Firefox). This can cause breakage.
    To work around this, PSM now simply deletes any module named "Root Certs" at
    startup. In an effort to prevent PSM from deleting unrelated modules
    coincidentally named "Root Certs", we also prevent the user from using the
    Firefox UI to name modules "Root Certs".
    
    MozReview-Commit-ID: ABja3wpShO9
    
    --HG--
    extra : histedit_source : 3f8a20980a1f7fbc838b610763369af3ddf95e16
---
 security/certverifier/NSSCertDBTrustDomain.cpp     |  6 +++++
 .../locales/en-US/chrome/pippki/pippki.properties  |  5 ++++
 .../manager/pki/resources/content/load_device.js   | 17 ++++++++++++
 .../manager/pki/resources/content/load_device.xul  |  4 ++-
 security/manager/ssl/PKCS11ModuleDB.cpp            | 11 ++++++++
 .../browser/browser_loadPKCS11Module_ui.js         | 30 ++++++++++++++++++++++
 .../manager/ssl/tests/unit/test_pkcs11_moduleDB.js | 28 ++++++++++++++++++++
 .../manager/ssl/tests/unit/xpcshell-smartcards.ini |  1 +
 8 files changed, 101 insertions(+), 1 deletion(-)

diff --git security/certverifier/NSSCertDBTrustDomain.cpp security/certverifier/NSSCertDBTrustDomain.cpp
index a352d928a3c9..229a0b5df28c 100644
--- security/certverifier/NSSCertDBTrustDomain.cpp
+++ security/certverifier/NSSCertDBTrustDomain.cpp
@@ -1216,6 +1216,12 @@ LoadLoadableRoots(const nsCString& dir, const nsCString& modNameUTF8)
   // the return value would be detrimental in that case.
   int unusedModType;
   Unused << SECMOD_DeleteModule(modNameUTF8.get(), &unusedModType);
+  // Some NSS command-line utilities will load a roots module under the name
+  // "Root Certs" if there happens to be a `DLL_PREFIX "nssckbi" DLL_SUFFIX`
+  // file in the directory being operated on. In some cases this can cause us to
+  // fail to load our roots module. In these cases, deleting the "Root Certs"
+  // module allows us to load the correct one. See bug 1406396.
+  Unused << SECMOD_DeleteModule("Root Certs", &unusedModType);
 
   nsAutoCString fullLibraryPath;
   if (!dir.IsEmpty()) {
diff --git security/manager/locales/en-US/chrome/pippki/pippki.properties security/manager/locales/en-US/chrome/pippki/pippki.properties
index 4bf67da7bab2..163384651067 100644
--- security/manager/locales/en-US/chrome/pippki/pippki.properties
+++ security/manager/locales/en-US/chrome/pippki/pippki.properties
@@ -185,3 +185,8 @@ addExceptionCheckingShort=Checking Information
 addExceptionCheckingLong2=Attempting to identify this site…
 addExceptionNoCertShort=No Information Available
 addExceptionNoCertLong2=Unable to obtain identification status for this site.
+
+# Load Module Dialog
+loadModuleHelp_emptyModuleName=The module name cannot be empty.
+# LOCALIZATION NOTE(loadModuleHelp_rootCertsModuleName): Do not translate 'Root Certs'
+loadModuleHelp_rootCertsModuleName=‘Root Certs‘ is reserved and cannot be used as the module name.
diff --git security/manager/pki/resources/content/load_device.js security/manager/pki/resources/content/load_device.js
index b3b020236b93..08831f16e27e 100644
--- security/manager/pki/resources/content/load_device.js
+++ security/manager/pki/resources/content/load_device.js
@@ -51,3 +51,20 @@ function onDialogAccept() {
 
   return true;
 }
+
+function validateModuleName() {
+  let bundle = document.getElementById("pippki_bundle");
+  let name = document.getElementById("device_name").value;
+  let helpText = document.getElementById("helpText");
+  helpText.value = "";
+  let dialogNode = document.querySelector("dialog");
+  dialogNode.removeAttribute("buttondisabledaccept");
+  if (name == "") {
+    helpText.value = bundle.getString("loadModuleHelp_emptyModuleName");
+    dialogNode.setAttribute("buttondisabledaccept", true);
+  }
+  if (name == "Root Certs") {
+    helpText.value = bundle.getString("loadModuleHelp_rootCertsModuleName");
+    dialogNode.setAttribute("buttondisabledaccept", true);
+  }
+}
diff --git security/manager/pki/resources/content/load_device.xul security/manager/pki/resources/content/load_device.xul
index b3a82df7ac57..f7cfca3cacf7 100644
--- security/manager/pki/resources/content/load_device.xul
+++ security/manager/pki/resources/content/load_device.xul
@@ -29,7 +29,8 @@
   <hbox align="center">
     <label value="&loaddevice.modname2;" accesskey="&loaddevice.modname2.accesskey;"
            control="device_name"/>
-    <textbox id="device_name" flex="1" value="&loaddevice.modname.default;"/>
+    <textbox id="device_name" flex="1" value="&loaddevice.modname.default;"
+             onchange="validateModuleName();"/>
   </hbox>
   <hbox align="center">
     <label value="&loaddevice.filename2;" accesskey="&loaddevice.filename2.accesskey;"
@@ -39,5 +40,6 @@
             accesskey="&loaddevice.browse.accesskey;"
             oncommand="onBrowseBtnPress();"/>
   </hbox>
+  <label id="helpText" value=""/>
 
 </dialog>
diff --git security/manager/ssl/PKCS11ModuleDB.cpp security/manager/ssl/PKCS11ModuleDB.cpp
index f5fc4c8ce4ba..3e7f2586ff62 100644
--- security/manager/ssl/PKCS11.cpp
+++ security/manager/ssl/PKCS11.cpp
@@ -118,6 +118,17 @@ PKCS11::AddModule(const nsAString& aModuleName,
     return NS_ERROR_INVALID_ARG;
   }
 
+  // "Root Certs" is the name some NSS command-line utilities will give the
+  // roots module if they decide to load it when there happens to be a
+  // `DLL_PREFIX "nssckbi" DLL_SUFFIX` file in the directory being operated on.
+  // This causes failures, so as a workaround, the PSM initialization code will
+  // unconditionally remove any module named "Root Certs". We should prevent the
+  // user from adding an unrelated module named "Root Certs" in the first place
+  // so PSM doesn't delete it. See bug 1406396.
+  if (aModuleName.EqualsLiteral("Root Certs")) {
+    return NS_ERROR_ILLEGAL_VALUE;
+  }
+
   NS_ConvertUTF16toUTF8 moduleName(aModuleName);
   nsCString fullPath;
   // NSS doesn't support Unicode path.  Use native charset
diff --git security/manager/ssl/tests/mochitest/browser/browser_loadPKCS11Module_ui.js security/manager/ssl/tests/mochitest/browser/browser_loadPKCS11Module_ui.js
index c458181a0067..65649d453a6b 100644
--- security/manager/ssl/tests/mochitest/browser/browser_loadPKCS11Module_ui.js
+++ security/manager/ssl/tests/mochitest/browser/browser_loadPKCS11Module_ui.js
@@ -231,3 +231,33 @@ add_task(async function testCancel() {
 
   await BrowserTestUtils.windowClosed(win);
 });
+
+async function testModuleNameHelper(moduleName, acceptButtonShouldBeDisabled) {
+  let win = await openLoadModuleDialog();
+  resetCallCounts();
+
+  info(`Setting Module Name to '${moduleName}'`);
+  let moduleNameBox = win.document.getElementById("device_name");
+  moduleNameBox.value = moduleName;
+  // this makes this not a great test, but it's the easiest way to simulate this
+  moduleNameBox.onchange();
+
+  let dialogNode = win.document.querySelector("dialog");
+  Assert.equal(dialogNode.getAttribute("buttondisabledaccept"),
+               acceptButtonShouldBeDisabled ? "true" : "", // it's a string
+               `dialog accept button should ${acceptButtonShouldBeDisabled ? "" : "not "}be disabled`);
+
+  return BrowserTestUtils.closeWindow(win);
+}
+
+add_task(async function testEmptyModuleName() {
+  await testModuleNameHelper("", true);
+});
+
+add_task(async function testReservedModuleName() {
+  await testModuleNameHelper("Root Certs", true);
+});
+
+add_task(async function testAcceptableModuleName() {
+  await testModuleNameHelper("Some Module Name", false);
+});
diff --git security/manager/ssl/tests/unit/test_pkcs11_moduleDB.js security/manager/ssl/tests/unit/test_pkcs11_moduleDB.js
new file mode 100644
index 000000000000..110f5a8461e5
--- /dev/null
+++ security/manager/ssl/tests/unit/test_pkcs11_moduleDB.js
@@ -0,0 +1,28 @@
+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/publicdomain/zero/1.0/
+"use strict";
+
+// Tests that adding modules with invalid names are prevented.
+
+// Ensure that the appropriate initialization has happened.
+do_get_profile();
+
+const gModuleDB = Cc["@mozilla.org/security/pkcs11moduledb;1"]
+                    .getService(Ci.nsIPKCS11ModuleDB);
+
+function run_test() {
+  let libraryFile = Services.dirsvc.get("CurWorkD", Ci.nsIFile);
+  libraryFile.append("pkcs11testmodule");
+  libraryFile.append(ctypes.libraryName("pkcs11testmodule"));
+  ok(libraryFile.exists(), "The pkcs11testmodule file should exist");
+
+  let pkcs11ModuleDB = Cc["@mozilla.org/security/pkcs11moduledb;1"]
+                         .getService(Ci.nsIPKCS11ModuleDB);
+  throws(() => pkcs11ModuleDB.addModule("Root Certs", libraryFile.path, 0, 0),
+         /NS_ERROR_ILLEGAL_VALUE/,
+         "Adding a module named 'Root Certs' should fail");
+  throws(() => pkcs11ModuleDB.addModule("", libraryFile.path, 0, 0),
+         /NS_ERROR_ILLEGAL_VALUE/,
+         "Adding a module with an empty name should fail.");
+}
diff --git security/manager/ssl/tests/unit/xpcshell-smartcards.ini security/manager/ssl/tests/unit/xpcshell-smartcards.ini
index 66f4d842642f..eab7ee368e7c 100644
--- security/manager/ssl/tests/unit/xpcshell-smartcards.ini
+++ security/manager/ssl/tests/unit/xpcshell-smartcards.ini
@@ -7,6 +7,7 @@ support-files =
 
 [test_pkcs11_insert_remove.js]
 [test_pkcs11_module.js]
+[test_pkcs11_moduleDB.js]
 [test_pkcs11_no_events_after_removal.js]
 [test_pkcs11_safe_mode.js]
 skip-if = coverage # bug 1336728