Bug 1535752 - avoid unnecessarily base64-encoding inputs to nsICertStorage when we already have DER r=mgoodwin

mozkeeler · mozkeeler · commit 61bd4db5661b · 2019-04-05T21:19:21.000Z
Differential Revision: https://phabricator.services.mozilla.com/D26034 --HG-- extra : moz-landing-system : lando
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/security/certverifier/NSSCertDBTrustDomain.cpp b/security/certverifier/NSSCertDBTrustDomain.cpp
@@ -201,20 +201,20 @@ Result NSSCertDBTrustDomain::GetCertTrust(EndEntityOrCA endEntityOrCA,
   if (mCertDBTrustType == trustSSL) {
     int16_t revocationState;
 
-    nsAutoCString encIssuer;
-    nsAutoCString encSerial;
-    nsAutoCString encSubject;
-    nsAutoCString encPubKey;
+    nsTArray<uint8_t> issuerBytes;
+    nsTArray<uint8_t> serialBytes;
+    nsTArray<uint8_t> subjectBytes;
+    nsTArray<uint8_t> pubKeyBytes;
 
-    nsresult nsrv = BuildRevocationCheckStrings(
-        candidateCert.get(), encIssuer, encSerial, encSubject, encPubKey);
+    nsresult nsrv = BuildRevocationCheckArrays(
+        candidateCert, issuerBytes, serialBytes, subjectBytes, pubKeyBytes);
 
     if (NS_FAILED(nsrv)) {
       return Result::FATAL_ERROR_LIBRARY_FAILURE;
     }
 
-    nsrv = mCertBlocklist->GetRevocationState(encIssuer, encSerial, encSubject,
-                                              encPubKey, &revocationState);
+    nsrv = mCertBlocklist->GetRevocationState(
+        issuerBytes, serialBytes, subjectBytes, pubKeyBytes, &revocationState);
     if (NS_FAILED(nsrv)) {
       return Result::FATAL_ERROR_LIBRARY_FAILURE;
     }
@@ -1249,41 +1249,35 @@ nsresult DefaultServerNicknameForCert(const CERTCertificate* cert,
   return NS_ERROR_FAILURE;
 }
 
-nsresult BuildRevocationCheckStrings(const CERTCertificate* cert,
-                                     /*out*/ nsCString& encIssuer,
-                                     /*out*/ nsCString& encSerial,
-                                     /*out*/ nsCString& encSubject,
-                                     /*out*/ nsCString& encPubKey) {
-  // Convert issuer, serial, subject and pubKey data to Base64 encoded DER
-  nsDependentCSubstring issuerString(
-      BitwiseCast<char*, uint8_t*>(cert->derIssuer.data), cert->derIssuer.len);
-  nsDependentCSubstring serialString(
-      BitwiseCast<char*, uint8_t*>(cert->serialNumber.data),
-      cert->serialNumber.len);
-  nsDependentCSubstring subjectString(
-      BitwiseCast<char*, uint8_t*>(cert->derSubject.data),
-      cert->derSubject.len);
-  nsDependentCSubstring pubKeyString(
-      BitwiseCast<char*, uint8_t*>(cert->derPublicKey.data),
-      cert->derPublicKey.len);
-
-  nsresult rv = Base64Encode(issuerString, encIssuer);
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-  rv = Base64Encode(serialString, encSerial);
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-  rv = Base64Encode(subjectString, encSubject);
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-  rv = Base64Encode(pubKeyString, encPubKey);
-  if (NS_FAILED(rv)) {
-    return rv;
+nsresult BuildRevocationCheckArrays(const UniqueCERTCertificate& cert,
+                                    /*out*/ nsTArray<uint8_t>& issuerBytes,
+                                    /*out*/ nsTArray<uint8_t>& serialBytes,
+                                    /*out*/ nsTArray<uint8_t>& subjectBytes,
+                                    /*out*/ nsTArray<uint8_t>& pubKeyBytes) {
+  issuerBytes.Clear();
+  if (!issuerBytes.AppendElements(
+          BitwiseCast<char*, uint8_t*>(cert->derIssuer.data),
+          cert->derIssuer.len)) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
+  serialBytes.Clear();
+  if (!serialBytes.AppendElements(
+          BitwiseCast<char*, uint8_t*>(cert->serialNumber.data),
+          cert->serialNumber.len)) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
+  subjectBytes.Clear();
+  if (!subjectBytes.AppendElements(
+          BitwiseCast<char*, uint8_t*>(cert->derSubject.data),
+          cert->derSubject.len)) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
+  pubKeyBytes.Clear();
+  if (!pubKeyBytes.AppendElements(
+          BitwiseCast<char*, uint8_t*>(cert->derPublicKey.data),
+          cert->derPublicKey.len)) {
+    return NS_ERROR_OUT_OF_MEMORY;
   }
-
   return NS_OK;
 }
 
diff --git a/security/certverifier/NSSCertDBTrustDomain.h b/security/certverifier/NSSCertDBTrustDomain.h
@@ -60,28 +60,28 @@ nsresult DefaultServerNicknameForCert(const CERTCertificate* cert,
                                       /*out*/ nsCString& nickname);
 
 /**
- * Build strings of base64 encoded issuer, serial, subject and public key data
- * from the supplied certificate for use in revocation checks.
+ * Build nsTArray<uint8_t>s out of the issuer, serial, subject and public key
+ * data from the supplied certificate for use in revocation checks.
  *
  * @param cert
  *        The CERTCertificate* from which to extract the data.
  * @param out encIssuer
- *        The string to populate with base64 encoded issuer data.
+ *        The array to populate with issuer data.
  * @param out encSerial
- *        The string to populate with base64 encoded serial number data.
+ *        The array to populate with serial number data.
  * @param out encSubject
- *        The string to populate with base64 encoded subject data.
+ *        The array to populate with subject data.
  * @param out encPubKey
- *        The string to populate with base64 encoded public key data.
+ *        The array to populate with public key data.
  * @return
- *        NS_OK, unless there's a Base64 encoding problem, in which case
- *        NS_ERROR_FAILURE.
+ *        NS_OK, unless there's a memory allocation problem, in which case
+ *        NS_ERROR_OUT_OF_MEMORY.
  */
-nsresult BuildRevocationCheckStrings(const CERTCertificate* cert,
-                                     /*out*/ nsCString& encIssuer,
-                                     /*out*/ nsCString& encSerial,
-                                     /*out*/ nsCString& encSubject,
-                                     /*out*/ nsCString& encPubKey);
+nsresult BuildRevocationCheckArrays(const UniqueCERTCertificate& cert,
+                                    /*out*/ nsTArray<uint8_t>& issuerBytes,
+                                    /*out*/ nsTArray<uint8_t>& serialBytes,
+                                    /*out*/ nsTArray<uint8_t>& subjectBytes,
+                                    /*out*/ nsTArray<uint8_t>& pubKeyBytes);
 
 void SaveIntermediateCerts(const UniqueCERTCertList& certList);
 
diff --git a/security/manager/ssl/CSTrustDomain.cpp b/security/manager/ssl/CSTrustDomain.cpp
@@ -44,20 +44,20 @@ Result CSTrustDomain::GetCertTrust(EndEntityOrCA endEntityOrCA,
     return MapPRErrorCodeToResult(PR_GetError());
   }
 
-  nsAutoCString encIssuer;
-  nsAutoCString encSerial;
-  nsAutoCString encSubject;
-  nsAutoCString encPubKey;
+  nsTArray<uint8_t> issuerBytes;
+  nsTArray<uint8_t> serialBytes;
+  nsTArray<uint8_t> subjectBytes;
+  nsTArray<uint8_t> pubKeyBytes;
 
-  nsresult nsrv = BuildRevocationCheckStrings(candidateCert.get(), encIssuer,
-                                              encSerial, encSubject, encPubKey);
+  nsresult nsrv = BuildRevocationCheckArrays(
+      candidateCert, issuerBytes, serialBytes, subjectBytes, pubKeyBytes);
   if (NS_FAILED(nsrv)) {
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
   }
 
   int16_t revocationState;
-  nsrv = mCertBlocklist->GetRevocationState(encIssuer, encSerial, encSubject,
-                                            encPubKey, &revocationState);
+  nsrv = mCertBlocklist->GetRevocationState(
+      issuerBytes, serialBytes, subjectBytes, pubKeyBytes, &revocationState);
   if (NS_FAILED(nsrv)) {
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
   }
diff --git a/security/manager/ssl/cert_storage/Cargo.toml b/security/manager/ssl/cert_storage/Cargo.toml
@@ -13,5 +13,6 @@ nsstring = { path = "../../../../xpcom/rust/nsstring" }
 rkv = "^0.9"
 sha2 = "^0.7"
 style = { path = "../../../../servo/components/style" }
+thin-vec = { version = "0.1.0", features = ["gecko-ffi"] }
 time = "0.1"
 xpcom = { path = "../../../../xpcom/rust/xpcom" }
diff --git a/security/manager/ssl/cert_storage/src/lib.rs b/security/manager/ssl/cert_storage/src/lib.rs
@@ -10,6 +10,7 @@ extern crate nserror;
 extern crate nsstring;
 extern crate rkv;
 extern crate sha2;
+extern crate thin_vec;
 extern crate time;
 #[macro_use]
 extern crate xpcom;
@@ -36,6 +37,7 @@ use std::slice;
 use std::str;
 use std::sync::{Arc, Mutex, RwLock};
 use std::time::{Duration, SystemTime};
+use thin_vec::ThinVec;
 use xpcom::interfaces::{
     nsICertStorage, nsICertStorageCallback, nsIFile, nsIObserver, nsIPrefBranch, nsISupports,
     nsIThread,
@@ -799,24 +801,17 @@ impl CertStorage {
 
     unsafe fn GetRevocationState(
         &self,
-        issuer: *const nsACString,
-        serial: *const nsACString,
-        subject: *const nsACString,
-        pub_key_base64: *const nsACString,
+        issuer: *const ThinVec<u8>,
+        serial: *const ThinVec<u8>,
+        subject: *const ThinVec<u8>,
+        pub_key: *const ThinVec<u8>,
         state: *mut i16,
     ) -> nserror::nsresult {
         // TODO (bug 1541212): We really want to restrict this to non-main-threads only, but we
         // can't do so until bug 1406854 and bug 1534600 are fixed.
-        if issuer.is_null() || serial.is_null() || subject.is_null() || pub_key_base64.is_null() {
+        if issuer.is_null() || serial.is_null() || subject.is_null() || pub_key.is_null() {
             return NS_ERROR_FAILURE;
         }
-        // TODO (bug 1535752): If we're calling this function when we already have binary data (e.g.
-        // in a TrustDomain::GetCertTrust callback), we should be able to pass in the binary data
-        // directly. See also bug 1535486.
-        let issuer_decoded = try_ns!(base64::decode(&*issuer));
-        let serial_decoded = try_ns!(base64::decode(&*serial));
-        let subject_decoded = try_ns!(base64::decode(&*subject));
-        let pub_key_decoded = try_ns!(base64::decode(&*pub_key_base64));
         *state = nsICertStorage::STATE_UNSET as i16;
         // The following is a way to ensure the DB has been opened while minimizing lock
         // acquisitions in the common (read-only) case. First we acquire a read lock and see if we
@@ -838,12 +833,7 @@ impl CertStorage {
                 try_ns!(self.security_state.read())
             }
         };
-        match ss.get_revocation_state(
-            &issuer_decoded,
-            &serial_decoded,
-            &subject_decoded,
-            &pub_key_decoded,
-        ) {
+        match ss.get_revocation_state(&*issuer, &*serial, &*subject, &*pub_key) {
             Ok(st) => {
                 *state = st;
                 NS_OK
diff --git a/security/manager/ssl/nsICertStorage.idl b/security/manager/ssl/nsICertStorage.idl
@@ -88,17 +88,17 @@ interface nsICertStorage : nsISupports {
    * Get the revocation state of a certificate. STATE_UNSET indicates the
    * certificate is not revoked. STATE_ENFORCE indicates the certificate is
    * revoked.
-   * issuer - issuer name, DER, Base64 encoded
-   * serial - serial number, DER, Base64 encoded
-   * subject - subject name, DER, Base64 encoded
-   * pubkey - public key, DER, Base64 encoded
+   * issuer - issuer name, DER encoded
+   * serial - serial number, DER encoded
+   * subject - subject name, DER encoded
+   * pubkey - public key, DER encoded
    * Must not be called from the main thread.
    */
   [must_use]
-  short getRevocationState(in ACString issuer,
-                           in ACString serial,
-                           in ACString subject,
-                           in ACString pubkey);
+  short getRevocationState(in Array<octet> issuer,
+                           in Array<octet> serial,
+                           in Array<octet> subject,
+                           in Array<octet> pubkey);
 
   /**
    * Get the CRLite enrollment status of a certificate.
