Bug 1406396 - Work around NSS utils potentially loading spurious root cert modules. r=mgoodwin, a=ritu, l10n=flod DEVEDITION_57_0b11_RELEASE FENNEC_57_0b11_BUILD1 FENNEC_57_0b11_RELEASE FIREFOX_57_0b11_BUILD1 FIREFOX_57_0b11_RELEASE
authorDavid Keeler <dkeeler@mozilla.com>
Fri, 13 Oct 2017 11:27:30 -0700
changeset 685190 86534d5daeef8066928eef910d6d5c60442b24b0
parent 685189 3d0ba43d0afd86b10a25213390180c8019a103c3
child 685191 fef8de33c4b682d2ffb9379cedda45f84ef7dd02
child 685199 8e260532c69e1ef871cb97f49445b1bf57f24277
child 685465 de721aab4866c6c18c0f85cd11d53e93b781acf2
child 685518 d67d67a8f985c027c434880cc388556a580d255b
child 687591 0bf335bbd491ad87a1dfeb7d58b9b782e1d3e040
push id85831
push userbmo:mstriemer@mozilla.com
push dateTue, 24 Oct 2017 03:46:09 +0000
reviewersmgoodwin, ritu
bugs1406396
milestone57.0
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
security/certverifier/NSSCertDBTrustDomain.cpp
security/manager/locales/en-US/chrome/pippki/pippki.properties
security/manager/pki/resources/content/load_device.js
security/manager/pki/resources/content/load_device.xul
security/manager/ssl/PKCS11ModuleDB.cpp
security/manager/ssl/tests/mochitest/browser/browser_loadPKCS11Module_ui.js
security/manager/ssl/tests/unit/test_pkcs11_moduleDB.js
security/manager/ssl/tests/unit/xpcshell-smartcards.ini
--- a/security/certverifier/NSSCertDBTrustDomain.cpp
+++ b/security/certverifier/NSSCertDBTrustDomain.cpp
@@ -1211,16 +1211,22 @@ DisableMD5()
 bool
 LoadLoadableRoots(const nsCString& dir, const nsCString& modNameUTF8)
 {
   // If a module exists with the same name, make a best effort attempt to delete
   // it. Note that it isn't possible to delete the internal module, so checking
   // 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()) {
     fullLibraryPath.Assign(dir);
     fullLibraryPath.AppendLiteral(FILE_PATH_SEPARATOR);
   }
   fullLibraryPath.Append(DLL_PREFIX "nssckbi" DLL_SUFFIX);
   // Escape the \ and " characters.
--- a/security/manager/locales/en-US/chrome/pippki/pippki.properties
+++ b/security/manager/locales/en-US/chrome/pippki/pippki.properties
@@ -180,8 +180,13 @@ addExceptionExpiredLong2=The certificate
 addExceptionUnverifiedOrBadSignatureShort=Unknown Identity
 addExceptionUnverifiedOrBadSignatureLong2=The certificate is not trusted because it hasn’t been verified as issued by a trusted authority using a secure signature.
 addExceptionValidShort=Valid Certificate
 addExceptionValidLong=This site provides valid, verified identification.  There is no need to add an exception.
 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.
--- a/security/manager/pki/resources/content/load_device.js
+++ b/security/manager/pki/resources/content/load_device.js
@@ -46,8 +46,25 @@ function onDialogAccept() {
     pkcs11ModuleDB.addModule(nameBox.value, pathBox.value, 0, 0);
   } catch (e) {
     alertPromptService(null, bundle.getString("AddModuleFailure"));
     return false;
   }
 
   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);
+  }
+}
--- a/security/manager/pki/resources/content/load_device.xul
+++ b/security/manager/pki/resources/content/load_device.xul
@@ -24,20 +24,22 @@
   <script type="application/javascript" src="chrome://pippki/content/pippki.js"/>
   <script type="application/javascript"
           src="chrome://pippki/content/load_device.js"/>
 
   <description>&loaddevice.info;</description>
   <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;"
            control="device_path"/>
     <textbox id="device_path" flex="1"/>
     <button id="browse" label="&loaddevice.browse;" flex="1"
             accesskey="&loaddevice.browse.accesskey;"
             oncommand="onBrowseBtnPress();"/>
   </hbox>
+  <label id="helpText" value=""/>
 
 </dialog>
--- a/security/manager/ssl/PKCS11ModuleDB.cpp
+++ b/security/manager/ssl/PKCS11ModuleDB.cpp
@@ -109,16 +109,27 @@ PKCS11ModuleDB::AddModule(const nsAStrin
   if (isAlreadyShutDown()) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   if (aModuleName.IsEmpty()) {
     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
   NS_CopyUnicodeToNative(aLibraryFullPath, fullPath);
   uint32_t mechFlags = SECMOD_PubMechFlagstoInternal(aCryptoMechanismFlags);
   uint32_t cipherFlags = SECMOD_PubCipherFlagstoInternal(aCipherFlags);
   SECStatus srv = SECMOD_AddNewModule(moduleName.get(), fullPath.get(),
                                       mechFlags, cipherFlags);
--- a/security/manager/ssl/tests/mochitest/browser/browser_loadPKCS11Module_ui.js
+++ b/security/manager/ssl/tests/mochitest/browser/browser_loadPKCS11Module_ui.js
@@ -226,8 +226,38 @@ add_task(async function testCancel() {
 
   Assert.equal(gMockPKCS11ModuleDB.addModuleCallCount, 0,
                "addModule() should never have been called");
   Assert.equal(gMockPromptService.alertCallCount, 0,
                "alert() should never have been called");
 
   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);
+});
new file mode 100644
--- /dev/null
+++ b/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.");
+}
--- a/security/manager/ssl/tests/unit/xpcshell-smartcards.ini
+++ b/security/manager/ssl/tests/unit/xpcshell-smartcards.ini
@@ -2,14 +2,15 @@
 head = head_psm.js
 tail =
 tags = psm
 skip-if = toolkit == 'android'
 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
 [test_pkcs11_slot.js]
 [test_pkcs11_token.js]
 [test_pkcs11_tokenDB.js]