Bug 1475775 - Clean up old NSS DB file after upgrade if necessary. r=franziskus, r=mattn, a=RyanVM
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 17 Jul 2018 13:51:00 -0700
changeset 450117 300efdbc9fe1f9165428c7934861033935b5abfa
parent 450116 80a4a7ef281374dbb2afda8edac54665b14b9ef8
child 450118 6546ee839d3002b595765008da323eb2422f0317
push id179
push userryanvm@gmail.com
push dateWed, 19 Sep 2018 01:11:39 +0000
treeherdermozilla-esr60@6546ee839d30 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfranziskus, mattn, RyanVM
bugs1475775
milestone60.2.1
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
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
@@ -1935,16 +1935,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
@@ -1966,16 +2011,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
@@ -27,16 +27,17 @@ support-files =
   test_missing_intermediate/**
   test_name_constraints/**
   test_ocsp_fetch_method/**
   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_signed_dir/**
   test_startcom_wosign/**
   test_symantec_apple_google/**
   test_validity/**
   tlsserver/**
 
 [test_add_preexisting_cert.js]
@@ -148,16 +149,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_signed_dir.js]
 tags = addons psm
 [test_ssl_status.js]
 [test_sss_enumerate.js]
 [test_sss_eviction.js]