commit e47b36fb220d31308765559a6ae09045ffebbf8f Author: David Keeler dkeeler@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]