[tbb-commits] [tor-browser/tor-browser-60.2.0esr-8.5-1] Bug 1475775 - Clean up old NSS DB file after upgrade if necessary. r=franziskus, r=mattn, a=RyanVM
gk at torproject.org
gk at torproject.org
Thu Sep 20 08:52:53 UTC 2018
commit e47b36fb220d31308765559a6ae09045ffebbf8f
Author: David Keeler <dkeeler at mozilla.com>
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]
More information about the tbb-commits
mailing list