Bug 1250256 - Partially clean up nsSDR.cpp. r=keeler draft
authorCykesiopka <cykesiopka.bmo@gmail.com>
Tue, 01 Mar 2016 20:07:53 -0800
changeset 336027 0385275406e6fbfd102cf6d992f74960b0c17672
parent 335996 80549d9e752805da90e2c294d6bb68abcccafe38
child 515279 b7fa8d377b750baa16aaaa10f7a22165bbc5a43d
push id11947
push usercykesiopka.bmo@gmail.com
push dateWed, 02 Mar 2016 04:09:52 +0000
reviewerskeeler
bugs1250256
milestone47.0a1
Bug 1250256 - Partially clean up nsSDR.cpp. r=keeler MozReview-Commit-ID: FoS4oTjnd7F
security/manager/ssl/ScopedNSSTypes.h
security/manager/ssl/nsSDR.cpp
security/manager/ssl/tests/unit/head_psm.js
security/manager/ssl/tests/unit/test_cert_blocklist.js
security/manager/ssl/tests/unit/test_pkcs11_safe_mode.js
security/manager/ssl/tests/unit/test_sdr.js
security/manager/ssl/tests/unit/xpcshell.ini
--- a/security/manager/ssl/ScopedNSSTypes.h
+++ b/security/manager/ssl/ScopedNSSTypes.h
@@ -336,16 +336,19 @@ MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(Un
 
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueNSSCMSMessage,
                                       NSSCMSMessage,
                                       NSS_CMSMessage_Destroy)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniqueNSSCMSSignedData,
                                       NSSCMSSignedData,
                                       NSS_CMSSignedData_Destroy)
 
+MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePK11SlotInfo,
+                                      PK11SlotInfo,
+                                      PK11_FreeSlot)
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePK11SlotList,
                                       PK11SlotList,
                                       PK11_FreeSlotList)
 
 MOZ_TYPE_SPECIFIC_UNIQUE_PTR_TEMPLATE(UniquePLArenaPool,
                                       PLArenaPool,
                                       internal::PORT_FreeArena_false)
 
--- a/security/manager/ssl/nsSDR.cpp
+++ b/security/manager/ssl/nsSDR.cpp
@@ -57,96 +57,98 @@ NS_IMETHODIMP
 nsSecretDecoderRing::Encrypt(unsigned char* data, int32_t dataLen,
                              unsigned char** result, int32_t* _retval)
 {
   nsNSSShutDownPreventionLock locker;
   if (isAlreadyShutDown()) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
-  nsresult rv = NS_OK;
-  ScopedPK11SlotInfo slot;
-  SECItem keyid;
-  SECItem request;
-  SECItem reply;
-  SECStatus s;
   nsCOMPtr<nsIInterfaceRequestor> ctx = new PipUIContext();
 
-  slot = PK11_GetInternalKeySlot();
-  if (!slot) { rv = NS_ERROR_NOT_AVAILABLE; goto loser; }
+  UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
+  if (!slot) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
 
   /* Make sure token is initialized. */
-  rv = setPassword(slot, ctx, locker);
-  if (NS_FAILED(rv))
-    goto loser;
+  nsresult rv = setPassword(slot.get(), ctx, locker);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
 
   /* Force authentication */
-  s = PK11_Authenticate(slot, true, ctx);
-  if (s != SECSuccess) { rv = NS_ERROR_FAILURE; goto loser; }
+  if (PK11_Authenticate(slot.get(), true, ctx) != SECSuccess) {
+    return NS_ERROR_FAILURE;
+  }
 
   /* Use default key id */
-  keyid.data = 0;
+  SECItem keyid;
+  keyid.data = nullptr;
   keyid.len = 0;
+  SECItem request;
   request.data = data;
   request.len = dataLen;
-  reply.data = 0;
+  SECItem reply;
+  reply.data = nullptr;
   reply.len = 0;
-  s= PK11SDR_Encrypt(&keyid, &request, &reply, ctx);
-  if (s != SECSuccess) { rv = NS_ERROR_FAILURE; goto loser; }
+  if (PK11SDR_Encrypt(&keyid, &request, &reply, ctx) != SECSuccess) {
+    return NS_ERROR_FAILURE;
+  }
 
   *result = reply.data;
   *_retval = reply.len;
 
-loser:
-  return rv;
+  return NS_OK;
 }
 
-NS_IMETHODIMP nsSecretDecoderRing::
-Decrypt(unsigned char * data, int32_t dataLen, unsigned char * *result, int32_t *_retval)
+NS_IMETHODIMP
+nsSecretDecoderRing::Decrypt(unsigned char* data, int32_t dataLen,
+                             unsigned char** result, int32_t* _retval)
 {
   nsNSSShutDownPreventionLock locker;
-  nsresult rv = NS_OK;
-  ScopedPK11SlotInfo slot;
-  SECStatus s;
-  SECItem request;
-  SECItem reply;
+  if (isAlreadyShutDown()) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
+
   nsCOMPtr<nsIInterfaceRequestor> ctx = new PipUIContext();
 
-  *result = 0;
+  *result = nullptr;
   *_retval = 0;
 
   /* Find token with SDR key */
-  slot = PK11_GetInternalKeySlot();
-  if (!slot) { rv = NS_ERROR_NOT_AVAILABLE; goto loser; }
+  UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
+  if (!slot) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
 
   /* Force authentication */
-  if (PK11_Authenticate(slot, true, ctx) != SECSuccess)
-  {
-    rv = NS_ERROR_NOT_AVAILABLE;
-    goto loser;
+  if (PK11_Authenticate(slot.get(), true, ctx) != SECSuccess) {
+    return NS_ERROR_NOT_AVAILABLE;
   }
 
+  SECItem request;
   request.data = data;
   request.len = dataLen;
-  reply.data = 0;
+  SECItem reply;
+  reply.data = nullptr;
   reply.len = 0;
-  s = PK11SDR_Decrypt(&request, &reply, ctx);
-  if (s != SECSuccess) { rv = NS_ERROR_FAILURE; goto loser; }
+  if (PK11SDR_Decrypt(&request, &reply, ctx) != SECSuccess) {
+    return NS_ERROR_FAILURE;
+  }
 
   *result = reply.data;
   *_retval = reply.len;
 
-loser:
-  return rv;
+  return NS_OK;
 }
 
-NS_IMETHODIMP nsSecretDecoderRing::
-EncryptString(const char *text, char **_retval)
+NS_IMETHODIMP
+nsSecretDecoderRing::EncryptString(const char* text, char** _retval)
 {
-  nsNSSShutDownPreventionLock locker;
   nsresult rv = NS_OK;
   unsigned char *encrypted = 0;
   int32_t eLen;
 
   if (!text || !_retval) {
     rv = NS_ERROR_INVALID_POINTER;
     goto loser;
   }
@@ -157,20 +159,19 @@ EncryptString(const char *text, char **_
   rv = encode(encrypted, eLen, _retval);
 
 loser:
   if (encrypted) PORT_Free(encrypted);
 
   return rv;
 }
 
-NS_IMETHODIMP nsSecretDecoderRing::
-DecryptString(const char *crypt, char **_retval)
+NS_IMETHODIMP
+nsSecretDecoderRing::DecryptString(const char* crypt, char** _retval)
 {
-  nsNSSShutDownPreventionLock locker;
   nsresult rv = NS_OK;
   char *r = 0;
   unsigned char *decoded = 0;
   int32_t decodedLen;
   unsigned char *decrypted = 0;
   int32_t decryptedLen;
 
   if (!crypt || !_retval) {
@@ -204,123 +205,133 @@ loser:
 NS_IMETHODIMP
 nsSecretDecoderRing::ChangePassword()
 {
   nsNSSShutDownPreventionLock locker;
   if (isAlreadyShutDown()) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
-  nsresult rv;
-  ScopedPK11SlotInfo slot(PK11_GetInternalKeySlot());
-  if (!slot) return NS_ERROR_NOT_AVAILABLE;
+  UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
+  if (!slot) {
+    return NS_ERROR_NOT_AVAILABLE;
+  }
 
-  /* Convert UTF8 token name to UCS2 */
-  NS_ConvertUTF8toUTF16 tokenName(PK11_GetTokenName(slot));
+  NS_ConvertUTF8toUTF16 tokenName(PK11_GetTokenName(slot.get()));
 
-  /* Get the set password dialog handler imlementation */
   nsCOMPtr<nsITokenPasswordDialogs> dialogs;
-
-  rv = getNSSDialogs(getter_AddRefs(dialogs),
-                     NS_GET_IID(nsITokenPasswordDialogs),
-                     NS_TOKENPASSWORDSDIALOG_CONTRACTID);
-  if (NS_FAILED(rv)) return rv;
+  nsresult rv = getNSSDialogs(getter_AddRefs(dialogs),
+                              NS_GET_IID(nsITokenPasswordDialogs),
+                              NS_TOKENPASSWORDSDIALOG_CONTRACTID);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
 
   nsCOMPtr<nsIInterfaceRequestor> ctx = new PipUIContext();
-  bool canceled;
-  rv = dialogs->SetPassword(ctx, tokenName.get(), &canceled);
-  /* canceled is ignored */
-
-  return rv;
+  bool canceled; // Ignored
+  return dialogs->SetPassword(ctx, tokenName.get(), &canceled);
 }
 
-NS_IMETHODIMP nsSecretDecoderRing::
-Logout()
+NS_IMETHODIMP
+nsSecretDecoderRing::Logout()
 {
   static NS_DEFINE_CID(kNSSComponentCID, NS_NSSCOMPONENT_CID);
 
   nsresult rv;
   nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
   if (NS_FAILED(rv))
     return rv;
 
   {
     nsNSSShutDownPreventionLock locker;
+    if (isAlreadyShutDown()) {
+      return NS_ERROR_NOT_AVAILABLE;
+    }
+
     PK11_LogoutAll();
     SSL_ClearSessionCache();
   }
 
   return NS_OK;
 }
 
-NS_IMETHODIMP nsSecretDecoderRing::
-LogoutAndTeardown()
+NS_IMETHODIMP
+nsSecretDecoderRing::LogoutAndTeardown()
 {
   static NS_DEFINE_CID(kNSSComponentCID, NS_NSSCOMPONENT_CID);
 
   nsresult rv;
   nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
   if (NS_FAILED(rv))
     return rv;
 
   {
     nsNSSShutDownPreventionLock locker;
+    if (isAlreadyShutDown()) {
+      return NS_ERROR_NOT_AVAILABLE;
+    }
+
     PK11_LogoutAll();
     SSL_ClearSessionCache();
   }
 
   rv = nssComponent->LogoutAuthenticatedPK11();
 
   // After we just logged out, we need to prune dead connections to make
   // sure that all connections that should be stopped, are stopped. See
   // bug 517584.
   nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
   if (os)
     os->NotifyObservers(nullptr, "net:prune-dead-connections", nullptr);
 
   return rv;
 }
 
-NS_IMETHODIMP nsSecretDecoderRing::
-SetWindow(nsISupports *w)
+NS_IMETHODIMP
+nsSecretDecoderRing::SetWindow(nsISupports*)
 {
-  return NS_OK;
+  return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 // Support routines
 
-nsresult nsSecretDecoderRing::
-encode(const unsigned char *data, int32_t dataLen, char **_retval)
+nsresult
+nsSecretDecoderRing::encode(const unsigned char* data, int32_t dataLen,
+                            char** _retval)
 {
-  nsresult rv = NS_OK;
-
-  char *result = PL_Base64Encode((const char *)data, dataLen, nullptr);
-  if (!result) { rv = NS_ERROR_OUT_OF_MEMORY; goto loser; }
+  char* result = PL_Base64Encode((const char*)data, dataLen, nullptr);
+  if (!result) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
 
   *_retval = NS_strdup(result);
   PR_DELETE(result);
-  if (!*_retval) { rv = NS_ERROR_OUT_OF_MEMORY; goto loser; }
+  if (!*_retval) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
 
-loser:
-  return rv;
+  return NS_OK;
 }
 
-nsresult nsSecretDecoderRing::
-decode(const char *data, unsigned char **result, int32_t * _retval)
+nsresult
+nsSecretDecoderRing::decode(const char* data, unsigned char** result,
+                            int32_t* _retval)
 {
-  nsresult rv = NS_OK;
   uint32_t len = strlen(data);
   int adjust = 0;
 
   /* Compute length adjustment */
   if (data[len-1] == '=') {
     adjust++;
-    if (data[len-2] == '=') adjust++;
+    if (data[len - 2] == '=') {
+      adjust++;
+    }
   }
 
   *result = (unsigned char *)PL_Base64Decode(data, len, nullptr);
-  if (!*result) { rv = NS_ERROR_ILLEGAL_VALUE; goto loser; }
+  if (!*result) {
+    return NS_ERROR_ILLEGAL_VALUE;
+  }
 
   *_retval = (len*3)/4 - adjust;
 
-loser:
-  return rv;
+  return NS_OK;
 }
--- a/security/manager/ssl/tests/unit/head_psm.js
+++ b/security/manager/ssl/tests/unit/head_psm.js
@@ -1,22 +1,25 @@
 /* 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";
 
-var { 'classes': Cc, 'interfaces': Ci, 'utils': Cu, 'results': Cr } = Components;
+const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
 
-var { NetUtil } = Cu.import("resource://gre/modules/NetUtil.jsm", {});
-var { FileUtils } = Cu.import("resource://gre/modules/FileUtils.jsm", {});
-var { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
-var { Promise } = Cu.import("resource://gre/modules/Promise.jsm", {});
-var { HttpServer } = Cu.import("resource://testing-common/httpd.js", {});
-var { ctypes } = Cu.import("resource://gre/modules/ctypes.jsm");
+const { ctypes } = Cu.import("resource://gre/modules/ctypes.jsm", {});
+const { FileUtils } = Cu.import("resource://gre/modules/FileUtils.jsm", {});
+const { HttpServer } = Cu.import("resource://testing-common/httpd.js", {});
+const { MockRegistrar } =
+  Cu.import("resource://testing-common/MockRegistrar.jsm", {});
+const { NetUtil } = Cu.import("resource://gre/modules/NetUtil.jsm", {});
+const { Promise } = Cu.import("resource://gre/modules/Promise.jsm", {});
+const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
+const { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
 
 const isDebugBuild = Cc["@mozilla.org/xpcom/debug;1"]
                        .getService(Ci.nsIDebug2).isDebugBuild;
 
 // The test EV roots are only enabled in debug builds as a security measure.
 //
 // Bug 1008316: B2G doesn't have EV enabled, so EV is not expected even in debug
 // builds.
--- a/security/manager/ssl/tests/unit/test_cert_blocklist.js
+++ b/security/manager/ssl/tests/unit/test_cert_blocklist.js
@@ -6,18 +6,16 @@
 // This test checks a number of things:
 // * it ensures that data loaded from revocations.txt on startup is present
 // * it ensures that certItems in blocklist.xml are persisted correctly
 // * it ensures that items in the CertBlocklist are seen as revoked by the
 //   cert verifier
 // * it does a sanity check to ensure other cert verifier behavior is
 //   unmodified
 
-var { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
-
 // First, we need to setup appInfo for the blocklist service to work
 var id = "xpcshell@tests.mozilla.org";
 var appName = "XPCShell";
 var version = "1";
 var platformVersion = "1.9.2";
 var appInfo = {
   // nsIXULAppInfo
   vendor: "Mozilla",
--- a/security/manager/ssl/tests/unit/test_pkcs11_safe_mode.js
+++ b/security/manager/ssl/tests/unit/test_pkcs11_safe_mode.js
@@ -2,18 +2,16 @@
 /* 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";
 
 // In safe mode, PKCS#11 modules should not be loaded. This test tests this by
 // simulating starting in safe mode and then attempting to load a module.
 
-var { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
-
 function run_test() {
   do_get_profile();
 
   // Simulate starting in safe mode.
   let xulRuntime = {
     inSafeMode: true,
     logConsoleErrors: true,
     OS: "XPCShell",
new file mode 100644
--- /dev/null
+++ b/security/manager/ssl/tests/unit/test_sdr.js
@@ -0,0 +1,84 @@
+// -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/publicdomain/zero/1.0/
+"use strict";
+
+// Tests various aspects of the nsISecretDecoderRing implementation.
+
+const { AppConstants } =
+  Cu.import("resource://gre/modules/AppConstants.jsm", {});
+
+do_get_profile();
+
+let gSetPasswordShownCount = 0;
+
+// Mock implementation of nsITokenPasswordDialogs.
+const gTokenPasswordDialogs = {
+  setPassword: (ctx, tokenName, canceled) => {
+    gSetPasswordShownCount++;
+    do_print(`setPassword() called; shown ${gSetPasswordShownCount} times`);
+    do_print(`tokenName: ${tokenName}`);
+    canceled.value = false;
+  },
+
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsITokenPasswordDialogs])
+};
+
+function run_test() {
+  // We have to set a password and login before we attempt to encrypt anything.
+  // In particular, failing to do so will cause the Encrypt() implementation to
+  // pop up a dialog asking for a password to be set. This won't work in the
+  // xpcshell environment and will lead to an assertion.
+  let tokenDB = Cc["@mozilla.org/security/pk11tokendb;1"]
+                  .getService(Ci.nsIPK11TokenDB);
+  let token = tokenDB.getInternalKeyToken();
+  token.initPassword("");
+  token.login(/*force*/ false);
+
+  let sdr = Cc["@mozilla.org/security/sdr;1"]
+              .getService(Ci.nsISecretDecoderRing);
+
+  // Test valid inputs for encryptString() and decryptString().
+  let inputs = [
+    "",
+    "foo",
+    "1234567890`~!#$%^&*()-_=+{[}]|\\:;'\",<.>/?",
+  ];
+  for (let input of inputs) {
+    let encrypted = sdr.encryptString(input);
+
+    notEqual(input, encrypted,
+             "Encypted input should not just be the input itself");
+
+    try {
+      atob(encrypted);
+    } catch (e) {
+      ok(false, `encryptString() should have returned Base64: ${e}`);
+    }
+
+    equal(input, sdr.decryptString(encrypted),
+          "decryptString(encryptString(input)) should return input");
+  }
+
+  // Test invalid inputs for decryptString().
+  throws(() => sdr.decryptString("*"), /NS_ERROR_ILLEGAL_VALUE/,
+         "decryptString() should throw if given non-Base64 input");
+
+  // Test calling changePassword() pops up the appropriate dialog.
+  // Note: On Android, nsITokenPasswordDialogs is apparently not implemented,
+  //       which also seems to prevent us from mocking out the interface.
+  if (AppConstants.platform != "android") {
+    let tokenPasswordDialogsCID =
+      MockRegistrar.register("@mozilla.org/nsTokenPasswordDialogs;1",
+                             gTokenPasswordDialogs);
+    do_register_cleanup(() => {
+      MockRegistrar.unregister(tokenPasswordDialogsCID);
+    });
+
+    equal(gSetPasswordShownCount, 0,
+          "changePassword() dialog should have been shown zero times");
+    sdr.changePassword();
+    equal(gSetPasswordShownCount, 1,
+          "changePassword() dialog should have been shown exactly once");
+  }
+}
--- a/security/manager/ssl/tests/unit/xpcshell.ini
+++ b/security/manager/ssl/tests/unit/xpcshell.ini
@@ -102,16 +102,17 @@ skip-if = toolkit == 'gonk'
 run-sequentially = hardcoded ports
 [test_pinning.js]
 run-sequentially = hardcoded ports
 # This test can take longer than 300 seconds on B2G emulator debug builds, so
 # give it enough time to finish. See bug 1081128.
 requesttimeoutfactor = 2
 [test_pinning_dynamic.js]
 [test_pinning_header_parsing.js]
+[test_sdr.js]
 [test_signed_apps.js]
 [test_signed_apps-marketplace.js]
 [test_signed_dir.js]
 tags = addons psm
 [test_sss_eviction.js]
 [test_sss_readstate.js]
 [test_sss_readstate_child.js]
 support-files = sss_readstate_child_worker.js