Bug 1262645 - Address misc issues with nsGetUserCertChoice(). r=keeler draft
authorCykesiopka <cykesiopka.bmo@gmail.com>
Fri, 15 Apr 2016 16:51:41 -0700
changeset 352234 214be2832fa5abdb33f4fb2c55449eb66c1c3a22
parent 352233 89e7caeaad61edb3ff63eaafc7fd2a1cfa1fffcb
child 518617 46ec6dba94a32dec05073d557b356ce5592b4478
push id15655
push usercykesiopka.bmo@gmail.com
push dateSat, 16 Apr 2016 00:07:25 +0000
reviewerskeeler
bugs1262645
milestone48.0a1
Bug 1262645 - Address misc issues with nsGetUserCertChoice(). r=keeler The follow issues are fixed: - Returning a failure result when failing to get a pref value instead of more gracefully falling back to a default. - Using an enum instead of a more strongly typed enum class. - Using a pref branch instead of the preferred Preferences.h API. - Manual memory management. - Unnecessary use of pointers. MozReview-Commit-ID: FKw5kBhnwxL
security/manager/ssl/nsNSSIOLayer.cpp
--- a/security/manager/ssl/nsNSSIOLayer.cpp
+++ b/security/manager/ssl/nsNSSIOLayer.cpp
@@ -60,19 +60,16 @@ void
 getSiteKey(const nsACString& hostName, uint16_t port,
            /*out*/ nsCSubstring& key)
 {
   key = hostName;
   key.AppendASCII(":");
   key.AppendInt(port);
 }
 
-// SSM_UserCertChoice: enum for cert choice info
-typedef enum {ASK, AUTO} SSM_UserCertChoice;
-
 // Historically, we have required that the server negotiate ALPN or NPN in
 // order to false start, as a compatibility hack to work around
 // implementations that just stop responding during false start. However, now
 // false start is resricted to modern crypto (TLS 1.2 and AEAD cipher suites)
 // so it is less likely that requring NPN or ALPN is still necessary.
 static const bool FALSE_START_REQUIRE_NPN_DEFAULT = false;
 
 } // unnamed namespace
@@ -1932,53 +1929,42 @@ nsConvertCANamesToStrings(const UniquePL
     return SECSuccess;
 loser:
     if (newitem.data) {
         PR_Free(newitem.data);
     }
     return SECFailure;
 }
 
-// Sets certChoice by reading the preference
-//
-// If done properly, this function will read the identifier strings for ASK and
-// AUTO modes read the selected strings from the preference, compare the
-// strings, and determine in which mode it is in. We currently use ASK mode for
-// UI apps and AUTO mode for UI-less apps without really asking for
-// preferences.
-nsresult
-nsGetUserCertChoice(SSM_UserCertChoice* certChoice)
+// Possible behaviors for choosing a cert for client auth.
+enum class UserCertChoice {
+  // Ask the user to choose a cert.
+  Ask = 0,
+  // Automatically choose a cert.
+  Auto = 1,
+};
+
+// Returns the most appropriate user cert choice based on the value of the
+// security.default_personal_cert preference.
+UserCertChoice
+nsGetUserCertChoice()
 {
-  char* mode = nullptr;
-  nsresult ret;
-
-  NS_ENSURE_ARG_POINTER(certChoice);
-
-  nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID);
-
-  ret = pref->GetCharPref("security.default_personal_cert", &mode);
-  if (NS_FAILED(ret)) {
-    goto loser;
+  nsAutoCString value;
+  nsresult rv = Preferences::GetCString("security.default_personal_cert", &value);
+  if (NS_FAILED(rv)) {
+    return UserCertChoice::Ask;
   }
 
-  if (PL_strcmp(mode, "Select Automatically") == 0) {
-    *certChoice = AUTO;
-  } else if (PL_strcmp(mode, "Ask Every Time") == 0) {
-    *certChoice = ASK;
-  } else {
-    // Most likely we see a nickname from a migrated cert.
-    // We do not currently support that, ask the user which cert to use.
-    *certChoice = ASK;
-  }
-
-loser:
-  if (mode) {
-    free(mode);
-  }
-  return ret;
+  // There are three cases for what the preference could be set to:
+  //   1. "Select Automatically" -> Auto.
+  //   2. "Ask Every Time" -> Ask.
+  //   3. Something else -> Ask. This might be a nickname from a migrated cert,
+  //      but we no longer support this case.
+  return value.EqualsLiteral("Select Automatically") ? UserCertChoice::Auto
+                                                     : UserCertChoice::Ask;
 }
 
 static bool
 hasExplicitKeyUsageNonRepudiation(CERTCertificate* cert)
 {
   // There is no extension, v1 or v2 certificate
   if (!cert->extensions)
     return false;
@@ -2089,25 +2075,28 @@ nsNSS_SSLGetClientAuthData(void* arg, PR
   }
 
   return runnable->mRV;
 }
 
 void
 ClientAuthDataRunnable::RunOnTargetThread()
 {
+  // We check the value of a pref in this runnable, so this runnable should only
+  // be run on the main thread.
+  MOZ_ASSERT(NS_IsMainThread());
+
   UniquePLArenaPool arena;
   char** caNameStrings;
   ScopedCERTCertificate cert;
   UniqueSECKEYPrivateKey privKey;
   ScopedCERTCertList certList;
   CERTCertListNode* node;
   UniqueCERTCertNicknames nicknames;
   int keyError = 0; // used for private key retrieval error
-  SSM_UserCertChoice certChoice;
   int32_t NumberOfCerts = 0;
   void* wincx = mSocketInfo;
   nsresult rv;
 
   nsCOMPtr<nsIX509Cert> socketClientCert;
   mSocketInfo->GetClientCert(getter_AddRefs(socketClientCert));
 
   // If a client cert preference was set on the socket info, use that and skip
@@ -2142,23 +2131,18 @@ ClientAuthDataRunnable::RunOnTargetThrea
     goto loser;
   }
 
   mRV = nsConvertCANamesToStrings(arena, caNameStrings, mCANames);
   if (mRV != SECSuccess) {
     goto loser;
   }
 
-  // get the preference
-  if (NS_FAILED(nsGetUserCertChoice(&certChoice))) {
-    goto loser;
-  }
-
   // find valid user cert and key pair
-  if (certChoice == AUTO) {
+  if (nsGetUserCertChoice() == UserCertChoice::Auto) {
     // automatically find the right cert
 
     // find all user certs that are valid and for SSL
     certList = CERT_FindUserCertsByUsage(CERT_GetDefaultCertDB(),
                                          certUsageSSLClient, false,
                                          true, wincx);
     if (!certList) {
       goto noCert;