[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

[tor-commits] [tor-browser/tor-browser-60.2.0esr-8.0-1] Bug 1475775 - Clean up old NSS DB file after upgrade if necessary. r=franziskus, r=mattn, a=RyanVM



commit ae13b0bbbf37edc464def7231992529eaf4ab731
Author: David Keeler <dkeeler@xxxxxxxxxxx>
Date:   Tue Jul 17 13:51:00 2018 -0700

    Bug 1475775 - Clean up old NSS DB file after upgrade if necessary. r=franziskus, r=mattn, a=RyanVM
    
    Reviewers: franziskus, mattn
    
    Bug #: 1475775
    
    Differential Revision: https://phabricator.services.mozilla.com/D2202
    
    --HG--
    rename : security/manager/ssl/tests/unit/test_sdr_preexisting_with_password.js => security/manager/ssl/tests/unit/test_sdr_upgraded_with_password.js
    rename : security/manager/ssl/tests/unit/test_sdr_preexisting_with_password/key3.db => security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/key3.db
    extra : source : 26ac31d53e50217dff8829e6d9bae18c7e36b812
    extra : intermediate-source : 1e05e8939a189df7b3d8309ad7936079ec012057
---
 security/manager/ssl/nsNSSComponent.cpp            |  48 +++++++++
 .../tests/unit/test_sdr_upgraded_with_password.js  | 115 +++++++++++++++++++++
 .../unit/test_sdr_upgraded_with_password/cert8.db  | Bin 0 -> 65536 bytes
 .../unit/test_sdr_upgraded_with_password/cert9.db  | Bin 0 -> 229376 bytes
 .../unit/test_sdr_upgraded_with_password/key3.db   | Bin 0 -> 16384 bytes
 .../unit/test_sdr_upgraded_with_password/key4.db   | Bin 0 -> 294912 bytes
 security/manager/ssl/tests/unit/xpcshell.ini       |   4 +
 7 files changed, 167 insertions(+)

diff --git a/security/manager/ssl/nsNSSComponent.cpp b/security/manager/ssl/nsNSSComponent.cpp
index da5dd60222ad..5d887f99dde0 100644
--- a/security/manager/ssl/nsNSSComponent.cpp
+++ b/security/manager/ssl/nsNSSComponent.cpp
@@ -1940,6 +1940,51 @@ AttemptToRenameBothPKCS11ModuleDBVersions(const nsACString& profilePath)
   }
   return AttemptToRenamePKCS11ModuleDB(profilePath, sqlModuleDBFilename);
 }
+
+// When we changed from the old dbm database format to the newer sqlite
+// implementation, the upgrade process left behind the existing files. Suppose a
+// user had not set a password for the old key3.db (which is about 99% of
+// users). After upgrading, both the old database and the new database are
+// unprotected. If the user then sets a password for the new database, the old
+// one will not be protected. In this scenario, we should probably just remove
+// the old database (it would only be relevant if the user downgraded to a
+// version of Firefox before 58, but we have to trade this off against the
+// user's old private keys being unexpectedly unprotected after setting a
+// password).
+// This was never an issue on Android because we always used the new
+// implementation.
+static void
+MaybeCleanUpOldNSSFiles(const nsACString& profilePath)
+{
+  UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
+  if (!slot) {
+    return;
+  }
+  // Unfortunately we can't now tell the difference between "there already was a
+  // password when the upgrade happened" and "there was not a password but then
+  // the user added one after upgrading".
+  bool hasPassword = PK11_NeedLogin(slot.get()) &&
+                     !PK11_NeedUserInit(slot.get());
+  if (!hasPassword) {
+    return;
+  }
+  nsCOMPtr<nsIFile> dbFile = do_CreateInstance("@mozilla.org/file/local;1");
+  if (!dbFile) {
+    return;
+  }
+  nsresult rv = dbFile->InitWithNativePath(profilePath);
+  if (NS_FAILED(rv)) {
+    return;
+  }
+  NS_NAMED_LITERAL_CSTRING(keyDBFilename, "key3.db");
+  rv = dbFile->AppendNative(keyDBFilename);
+  if (NS_FAILED(rv)) {
+    return;
+  }
+  // Since this isn't a directory, the `recursive` argument to `Remove` is
+  // irrelevant.
+  Unused << dbFile->Remove(false);
+}
 #endif // ifndef ANDROID
 
 // Given a profile directory, attempt to initialize NSS. If nocertdb is true,
@@ -1971,6 +2016,9 @@ InitializeNSSWithFallbacks(const nsACString& profilePath, bool nocertdb,
   SECStatus srv = ::mozilla::psm::InitializeNSS(profilePath, false, !safeMode);
   if (srv == SECSuccess) {
     MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("initialized NSS in r/w mode"));
+#ifndef ANDROID
+    MaybeCleanUpOldNSSFiles(profilePath);
+#endif // ifndef ANDROID
     return NS_OK;
   }
 #ifndef ANDROID
diff --git a/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password.js b/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password.js
new file mode 100644
index 000000000000..4949c2cf7956
--- /dev/null
+++ b/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password.js
@@ -0,0 +1,115 @@
+// -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+"use strict";
+
+// Tests that the SDR implementation is able to decrypt strings encrypted using
+// a preexisting NSS key database that a) has a password and b) has already been
+// upgraded from the old dbm format in a previous run of Firefox.
+// To create such a database, run the xpcshell test
+// `test_sdr_preexisting_with_password.js` and locate the file `key4.db` created
+// in the xpcshell test profile directory.
+// This does not apply to Android as the dbm implementation was never enabled on
+// that platform.
+
+var gMockPrompter = {
+  passwordToTry: "password",
+  numPrompts: 0,
+
+  // This intentionally does not use arrow function syntax to avoid an issue
+  // where in the context of the arrow function, |this != gMockPrompter| due to
+  // how objects get wrapped when going across xpcom boundaries.
+  promptPassword(dialogTitle, text, password, checkMsg, checkValue) {
+    this.numPrompts++;
+    if (this.numPrompts > 1) { // don't keep retrying a bad password
+      return false;
+    }
+    equal(text,
+          "Please enter your master password.",
+          "password prompt text should be as expected");
+    equal(checkMsg, null, "checkMsg should be null");
+    ok(this.passwordToTry, "passwordToTry should be non-null");
+    password.value = this.passwordToTry;
+    return true;
+  },
+
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIPrompt]),
+};
+
+// Mock nsIWindowWatcher. PSM calls getNewPrompter on this to get an nsIPrompt
+// to call promptPassword. We return the mock one, above.
+var gWindowWatcher = {
+  getNewPrompter: () => gMockPrompter,
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWindowWatcher]),
+};
+
+function run_test() {
+  let windowWatcherCID =
+    MockRegistrar.register("@mozilla.org/embedcomp/window-watcher;1",
+                           gWindowWatcher);
+  registerCleanupFunction(() => {
+    MockRegistrar.unregister(windowWatcherCID);
+  });
+
+  let profile = do_get_profile();
+  let key3DBFile = do_get_file("test_sdr_upgraded_with_password/key3.db");
+  key3DBFile.copyTo(profile, "key3.db");
+  let key4DBFile = do_get_file("test_sdr_upgraded_with_password/key4.db");
+  key4DBFile.copyTo(profile, "key4.db");
+  // Unfortunately we have to also copy the certificate databases as well.
+  // Otherwise, NSS will think it has to create them, which will cause NSS to
+  // think it has to also do a migration, which will open key3.db and not close
+  // it until shutdown, which means that on Windows removing the file just after
+  // startup fails. Luckily users profiles will have both key and certificate
+  // databases anyway, so this is an accurate reflection of normal use.
+  let cert8DBFile = do_get_file("test_sdr_upgraded_with_password/cert8.db");
+  cert8DBFile.copyTo(profile, "cert8.db");
+  let cert9DBFile = do_get_file("test_sdr_upgraded_with_password/cert9.db");
+  cert9DBFile.copyTo(profile, "cert9.db");
+
+  let sdr = Cc["@mozilla.org/security/sdr;1"]
+              .getService(Ci.nsISecretDecoderRing);
+
+  let testcases = [
+    // a full padding block
+    { ciphertext: "MDoEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECGeDHwVfyFqzBBAYvqMq/kDMsrARVNdC1C8d",
+      plaintext: "password" },
+    // 7 bytes of padding
+    { ciphertext: "MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECCAzLDVmYG2/BAh3IoIsMmT8dQ==",
+      plaintext: "a" },
+    // 6 bytes of padding
+    { ciphertext: "MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECPN8zlZzn8FdBAiu2acpT8UHsg==",
+      plaintext: "bb" },
+    // 1 byte of padding
+    { ciphertext: "MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECD5px1eMKkJQBAgUPp35GlrDvQ==",
+      plaintext: "!seven!" },
+    // 2 bytes of padding
+    { ciphertext: "MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECMh0hLtKDyUdBAixw9UZsMt+vA==",
+      plaintext: "sixsix" },
+    // long plaintext requiring more than two blocks
+    { ciphertext: "MFoEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECDRX1qi+/FX1BDATFIcIneQjvBuq3wdFxzllJt2VtUD69ACdOKAXH3eA87oHDvuHqOeCDwRy4UzoG5s=",
+      plaintext: "thisismuchlongerandsotakesupmultipleblocks" },
+    // this differs from the previous ciphertext by one bit and demonstrates
+    // that this implementation does not enforce message integrity
+    { ciphertext: "MFoEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECDRX1qi+/FX1BDAbFIcIneQjvBuq3wdFxzllJt2VtUD69ACdOKAXH3eA87oHDvuHqOeCDwRy4UzoG5s=",
+      plaintext: "nnLbuwLRkhlongerandsotakesupmultipleblocks" },
+  ];
+
+  for (let testcase of testcases) {
+    let decrypted = sdr.decryptString(testcase.ciphertext);
+    equal(decrypted, testcase.plaintext,
+          "decrypted ciphertext should match expected plaintext");
+  }
+  equal(gMockPrompter.numPrompts, 1,
+        "Should have been prompted for a password once");
+
+  // NSS does not close the old database when performing an upgrade. Thus, on
+  // Windows, we can't delete the old database file on the run that we perform
+  // an upgrade. However, we can delete it on subsequent runs.
+  let key3DBInProfile = do_get_profile();
+  key3DBInProfile.append("key3.db");
+  ok(!key3DBInProfile.exists(),
+     "key3.db should not exist after running with key4.db with a password");
+}
diff --git a/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/cert8.db b/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/cert8.db
new file mode 100644
index 000000000000..ac40a3325724
Binary files /dev/null and b/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/cert8.db differ
diff --git a/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/cert9.db b/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/cert9.db
new file mode 100644
index 000000000000..163d07a6f325
Binary files /dev/null and b/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/cert9.db differ
diff --git a/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/key3.db b/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/key3.db
new file mode 100644
index 000000000000..cac0808ac32c
Binary files /dev/null and b/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/key3.db differ
diff --git a/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/key4.db b/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/key4.db
new file mode 100644
index 000000000000..8c853543cc3e
Binary files /dev/null and b/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/key4.db differ
diff --git a/security/manager/ssl/tests/unit/xpcshell.ini b/security/manager/ssl/tests/unit/xpcshell.ini
index 5e5a1190f170..db865f6757bb 100644
--- a/security/manager/ssl/tests/unit/xpcshell.ini
+++ b/security/manager/ssl/tests/unit/xpcshell.ini
@@ -32,6 +32,7 @@ support-files =
   test_pinning_dynamic/**
   test_sdr_preexisting/**
   test_sdr_preexisting_with_password/**
+  test_sdr_upgraded_with_password/**
   test_signed_apps/**
   test_signed_dir/**
   test_startcom_wosign/**
@@ -153,6 +154,9 @@ requesttimeoutfactor = 2
 [test_sdr_preexisting_with_password.js]
 # Not relevant to Android. See the comment in the test.
 skip-if = toolkit == 'android'
+[test_sdr_upgraded_with_password.js]
+# Not relevant to Android. See the comment in the test.
+skip-if = toolkit == 'android'
 [test_session_resumption.js]
 run-sequentially = hardcoded ports
 [test_signed_apps.js]



_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits