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
--- 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;