Bug 1250256 - Partially clean up nsSDR.cpp. r=keeler

MozReview-Commit-ID: FoS4oTjnd7F

security/manager/ssl/ScopedNSSTypes.h

@@ -341,6 +341,9 @@ 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)

security/manager/ssl/nsSDR.cpp

@@ -62,86 +62,88 @@ nsSecretDecoderRing::Encrypt(unsigned char* data, int32_t dataLen,
     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; }
-
-  /* Force authentication */
-  if (PK11_Authenticate(slot, true, ctx) != SECSuccess)
-  {
-    rv = NS_ERROR_NOT_AVAILABLE;
-    goto loser;
+  UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
+  if (!slot) {
+    return NS_ERROR_NOT_AVAILABLE;
   }
 
+  /* Force authentication */
+  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;
@@ -162,10 +164,9 @@ loser:
   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;
@@ -209,31 +210,28 @@ nsSecretDecoderRing::ChangePassword()
     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);
 
@@ -244,6 +242,10 @@ Logout()
 
   {
     nsNSSShutDownPreventionLock locker;
+    if (isAlreadyShutDown()) {
+      return NS_ERROR_NOT_AVAILABLE;
+    }
+
     PK11_LogoutAll();
     SSL_ClearSessionCache();
   }
@@ -251,8 +253,8 @@ Logout()
   return NS_OK;
 }
 
-NS_IMETHODIMP nsSecretDecoderRing::
-LogoutAndTeardown()
+NS_IMETHODIMP
+nsSecretDecoderRing::LogoutAndTeardown()
 {
   static NS_DEFINE_CID(kNSSComponentCID, NS_NSSCOMPONENT_CID);
 
@@ -263,6 +265,10 @@ LogoutAndTeardown()
 
   {
     nsNSSShutDownPreventionLock locker;
+    if (isAlreadyShutDown()) {
+      return NS_ERROR_NOT_AVAILABLE;
+    }
+
     PK11_LogoutAll();
     SSL_ClearSessionCache();
   }
@@ -279,48 +285,53 @@ LogoutAndTeardown()
   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;
 }

security/manager/ssl/tests/unit/head_psm.js

@@ -4,14 +4,17 @@
  */
 "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;

security/manager/ssl/tests/unit/test_cert_blocklist.js

@@ -11,8 +11,6 @@
 // * 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";

security/manager/ssl/tests/unit/test_pkcs11_safe_mode.js

@@ -7,8 +7,6 @@
 // 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();
 

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");
+  }
+}

security/manager/ssl/tests/unit/xpcshell.ini

@@ -107,6 +107,7 @@ run-sequentially = hardcoded ports
 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]