bug 1475775 - clean up old NSS DB file after upgrade if necessary r=franziskus,mattn
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 17 Jul 2018 13:51:00 -0700
changeset 428103 26ac31d53e50217dff8829e6d9bae18c7e36b812
parent 428102 b3c8d1cdee5009b46bb769f5386a9b58993dd787
child 428104 4cba8b6eb37a007b60485167576b9b435d3cb2ad
push id34327
push userarchaeopteryx@coole-files.de
push dateWed, 25 Jul 2018 14:18:02 +0000
treeherdermozilla-central@fa78cd1a6880 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfranziskus, mattn
bugs1475775
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1475775 - clean up old NSS DB file after upgrade if necessary r=franziskus,mattn Reviewers: franziskus, mattn Bug #: 1475775 Differential Revision: https://phabricator.services.mozilla.com/D2202
security/manager/ssl/nsNSSComponent.cpp
security/manager/ssl/tests/unit/test_sdr_upgraded_with_password.js
security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/cert8.db
security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/cert9.db
security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/key3.db
security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/key4.db
security/manager/ssl/tests/unit/xpcshell.ini
--- a/security/manager/ssl/nsNSSComponent.cpp
+++ b/security/manager/ssl/nsNSSComponent.cpp
@@ -1649,16 +1649,61 @@ AttemptToRenameBothPKCS11ModuleDBVersion
   NS_NAMED_LITERAL_CSTRING(sqlModuleDBFilename, "pkcs11.txt");
   nsresult rv = AttemptToRenamePKCS11ModuleDB(profilePath,
                                               legacyModuleDBFilename);
   if (NS_FAILED(rv)) {
     return rv;
   }
   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,
 // (or if we don't have a profile directory) simply initialize NSS in no DB mode
 // and return. Otherwise, first attempt to initialize in read/write mode, and
 // then read-only mode if that fails. If both attempts fail, we may be failing
 // to initialize an NSS DB collection that has FIPS mode enabled. Attempt to
 // ascertain if this is the case, and if so, rename the offending PKCS#11 module
@@ -1680,16 +1725,19 @@ InitializeNSSWithFallbacks(const nsACStr
 
   // Try read/write mode. If we're in safeMode, we won't load PKCS#11 modules.
 #ifndef ANDROID
   PRErrorCode savedPRErrorCode1;
 #endif // ifndef ANDROID
   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
   savedPRErrorCode1 = PR_GetError();
   PRErrorCode savedPRErrorCode2;
 #endif // ifndef ANDROID
   // That failed. Try read-only mode.
   srv = ::mozilla::psm::InitializeNSS(profilePath, true, !safeMode);
copy from security/manager/ssl/tests/unit/test_sdr_preexisting_with_password.js
copy to security/manager/ssl/tests/unit/test_sdr_upgraded_with_password.js
--- a/security/manager/ssl/tests/unit/test_sdr_preexisting_with_password.js
+++ b/security/manager/ssl/tests/unit/test_sdr_upgraded_with_password.js
@@ -1,21 +1,21 @@
 // -*- 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) is in the old
-// dbm format.
-// To create such a database, run a version Firefox (or xpcshell) where the
-// default database format is the old dbm format (i.e. pre-bug 783994), set a
-// master password, and then encrypt something using nsISecretDecoderRing.
+// 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
@@ -49,18 +49,30 @@ function run_test() {
   let windowWatcherCID =
     MockRegistrar.register("@mozilla.org/embedcomp/window-watcher;1",
                            gWindowWatcher);
   registerCleanupFunction(() => {
     MockRegistrar.unregister(windowWatcherCID);
   });
 
   let profile = do_get_profile();
-  let keyDBFile = do_get_file("test_sdr_preexisting_with_password/key3.db");
-  keyDBFile.copyTo(profile, "key3.db");
+  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" },
@@ -87,9 +99,17 @@ function run_test() {
 
   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");
 }
new file mode 100644
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..ac40a3325724b598ba93a314d250bd03eb7f479f
GIT binary patch
literal 65536
zc%1Fru?fOp5C+gMzapvZoF`6UZ((5*L2JiwF;}p&7OW%&lMy6o-h+=jxP#l^vfAvE
zlw^5Nwtbs*Qe{Z$u_<F+{OWHsEZy4cBZD$#a&<MwRR910000000000000000_@9z@
z^J<=bIsgCw000000000000000U_y10-Qj#Wp4z$J4*&oF0000000000000000KL!L
X&j<hj0000000000000000D$fbJGXse
new file mode 100644
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..163d07a6f325564ec436832b1abb6775b579edc9
GIT binary patch
literal 229376
zc%1FjL2nyX5CCAiO<ai7bA&i#4;+YukiBbfoC~Vk8Yv<qZAc_~K$V@iQXsUVNhCy(
z>Kixy75pYn2*26XuPq0T=J0*ej%N40nH{g~{pRuh`PHm=_TuG_ldGa?&0Fnu>t0c`
zTCMitxYNqk7suLL=k3;iYqviCWPbPcU#-qBf46@9@bw?B{(Lomb#HM30000000000
z00000000000000000000000000Kosyy-sIybF2O2{Q24J=hGi1=V#9^FCSe`FV0Wz
zJUAMUj>pB`!Nc*lMXVKv2gP!|_;k|imfK&fM~9Q>a50(AE|xnw?{tW@_d8VE57#=I
zA8)n4i~7s!>G!kKtL2XMcRJjx{(Fkv_ULwJ^P@MP<NWgSdiHX;W$m3N&Eoq#hQ00I
zcH4J0x3=29kFF-ui`jB@ncTYRb3D4gKYqJ<@n9Yu?R`BudQyBfeo~wq>^(jir#n12
zJos{d@4;~~x_@|d{8=&SE{@*fC>KYS^0<($^wOnaIvu9dVVtHd!+r{b6zUXqQW&PN
zo5Cc8=_2e#t8NpcW4Dah+@~LL5V4N96LA=EH{vAXG~#It){NK87kSy_(YcJyWppm1
za~Yk>=v+qUDypies-mijsw%4LSgm8Vj@3F=>t;22*3ok(b`F~uPcS}d*=_PvTgJ5g
z?m`(O-=y)ZV|Nw1tJvM|HoKdR&6O%*KjI)_x_3YB+K;>T<9iuI?I3Cg-R4TFJ)I4z
zel_Ujw9M&S8~QmN<h0J|PELo*w9aL9E~|4{oy+PsWpy5>&g0a1oH~zF=W*&hPMyc8
z^EiuiH>Z=FPIG#i)3cn;a{6qU#t)%Mr+J)d8s}^<>7GsFi4VIWKi@ijx8?43?^b7X
zywz?sFOi=v&aY-qC)Zam;{56IullsNOm8n<ejlVa;N2AFt9%Co000000000000000
z00000000000000000000006-1ZKN<?@elw2000000000000000000000000000000
z000000I*^kDa=<q1ONa400000000000000000000000000000000000tk^~h^A!&P
z000000000000000000000000000000000000000hwvobo#X|r9000000000000000
z000000000000000000000KkfEq%dFc5C8xG000000000000000000000000000000
L00000uwwrJi;-GR
copy from security/manager/ssl/tests/unit/test_sdr_preexisting_with_password/key3.db
copy to security/manager/ssl/tests/unit/test_sdr_upgraded_with_password/key3.db
new file mode 100644
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..8c853543cc3e9d727862c6550865a7537eb05ff6
GIT binary patch
literal 294912
zc%1FjUu;!(9RToi?!DapfwBoTg)Ahu#s`YcyuJ7K&?X=&U14MuSYdJCLv!2SVYR?U
z%OK#mG#ZvHQ4{|qdvS|Od~ox{#5c#1CB7MaF!4{KF(Faozq!N+CS>#6-=#a0snhtN
zfzPM+e9!m%{(R53=d}0GW0Uhsjnb1RPaUr>m8#LjD2}6tN~I`@^6AkPWs2!hxa~TQ
z-djF;@Pik7nm3}(b5qf$3(eQhUpar~{P4Lq&rO|s?cdlB00000000000000000000
z0000000000000000B{#r=;-L%vL#;r&{BP7p>e#iRDZa>R9`uFj!#XD9iAv19@{-R
zQCb--ZJnPh9h}<t(Xpu`rAH@@lpf!|@3F@xN(c6r#t-b@Gr4d4aA|7d;N;l&#9(QD
zsd0R$eX0z6xU<+dz9o+4Ps}yWE<U}Gj!xH4FP#k6(<}9-hgPz#-Nj;GUw{1ZuweF6
z_4&CIi;D+O%|BCLYINLcZr}cgC!Q#U$yN*5g3{LdP`NVj(3()=W1+@EeWtOHEh(<5
z5hmYNqdKssQ0#l4Kb~&4eR}4TjoGDa$(mI)ZcYD#PKE}?^Toc+E5FVB;^OJXsccPQ
zRhjneyQ+)~4Lr9a?&$09k3T;iURgGsWw}+a?e+ri{o=-UADB8kSgM!PBYki6N_tdV
zc^GJo47G+vT9+fO%aL%|S~5~=h2d66T48%DjI_dNE7V(ICJm#ZRJk2P#d0OY_I+w0
z4u_b8xIM&?5Jy9-hd2}BY-p_AUc0}LSK4`~TnUvcp>icuu7t{!P`MH+SHrGq*i{X?
zs$o|(>`KCH5@wSyn}pe<JsWB!q2~6ma-<!@6AZs;rQFV2dn=*aS~*n-E#FGRvkt4P
zVRbdEu9e%X+Y8$x)evhT4u{yfcP-qt7VcUL?`1gb9S(bk%k7cY-r2@*wN@P-%F;@f
z-nO8YrNdd8Wa;)S9eF2BvTaGWEy=be*|ub5TavYtWbGtbJ4x0~lC_g$?Ic+{N!Cu9
zj%H~+OJ}llHcRKSw2`GxzLSPOgmyZUwKLOdXKuJ&o|_3ze54#?&o>Ewx0TU>YOdI~
zxBs7g1!n)N(?eOFPrs-;Ti@0_>5+T(t?0trX#fBK0000000000000000000000000
z0000000000xMwY2$ek_pu3x_)THd;PrZ*Z~A4KWTh7EV@>%BGeU+C`5T}w}eIKCVm
zEl-xW7T0dO^vI>_J@L9ValX8{(0gC?cc1wAt4Bv?Ck{PR@7(y+7mp?Vxi~JA_C2_L
zVaH#aYrpc+ec!u2dv)+y|7X7Vo5J94cYU+@^3}fQewCJe|Bcmmi&vyKOF!wzrZ`SV
zmoLQss|T>Defk#<;Mkt&!sLlRzB>Brw~svkoxLw@+wqGd;Q_qx!#{oZ_wU=(wez*c
zm1iHgvgemOqJ!Um`gqS1`%fP_R{i>o|KI_1r&+G~M$~+xdB+X_000000000000000
z00000000000000000000006jab><6kcW>@mt|MRQ+LR7-r$?@NJ!;-e0{{R300000
z000000000000000000000000000000+*=CyD9+{cQCD|*k!xO$nm5w`0000000000
z00000000000000000000000000002@mO?&?bNPJK)tz4Cnr}wUzoh{H0000000000
z0000000000000000000000000008b49r+FM<>+XxnBUNqp1aebT=OSU^ABkN00000
z000000000000000000000000000000004lyOK*NKzOi~J&TY&OcHKGH(+bTOqx1s+
z0000000000000000000000000000000000000955rsFkRYRjEZ)fX4foIEx6tuGbM
z?A&|*2X_AS8`TrvKKR+)Yk&Ns@{aP>;@WMO9=UYACtkNE&X+eAdPk35d~4nEo<E<v
z`GwED{)4Z5<mTZoPvzpcaR06sHva6DvxhFvJwH~y`sl9n-RVu@=3k=p0{{R300000
r000000000000000000000000000000_ms8yEnQF57Z=Z*JT>=sYk4Gg
--- a/security/manager/ssl/tests/unit/xpcshell.ini
+++ b/security/manager/ssl/tests/unit/xpcshell.ini
@@ -26,16 +26,17 @@ support-files =
   test_keysize_ev/**
   test_missing_intermediate/**
   test_name_constraints/**
   test_ocsp_url/**
   test_onecrl/**
   test_pinning_dynamic/**
   test_sdr_preexisting/**
   test_sdr_preexisting_with_password/**
+  test_sdr_upgraded_with_password/**
   test_signed_apps/**
   test_startcom_wosign/**
   test_symantec_apple_google/**
   test_validity/**
   tlsserver/**
 
 [test_add_preexisting_cert.js]
 [test_baseline_requirements_subject_common_name.js]
@@ -147,16 +148,19 @@ run-sequentially = hardcoded ports
 requesttimeoutfactor = 2
 [test_pinning_dynamic.js]
 [test_pinning_header_parsing.js]
 [test_sdr.js]
 [test_sdr_preexisting.js]
 [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]
 [test_ssl_status.js]
 run-sequentially = hardcoded ports
 [test_sss_enumerate.js]
 [test_sss_eviction.js]
 [test_sss_originAttributes.js]