bug 1264761 - improve handling of x509 versions in certificate manager r?Cykesiopka draft
authorDavid Keeler <dkeeler@mozilla.com>
Mon, 18 Apr 2016 11:07:24 -0700
changeset 353860 1d63e17e44f864d7996456ec395fc9f26c56bb7a
parent 353712 ae7413abfa4d3954a6a4ce7c1613a7100f367f9a
child 518897 ae918e285463ea695a080191cfdabbcf57715fcb
push id15963
push userdkeeler@mozilla.com
push dateWed, 20 Apr 2016 00:30:40 +0000
reviewersCykesiopka
bugs1264761
milestone48.0a1
bug 1264761 - improve handling of x509 versions in certificate manager r?Cykesiopka MozReview-Commit-ID: B7EPx63ttlt
security/manager/locales/en-US/chrome/pipnss/pipnss.properties
security/manager/ssl/nsNSSCertHelper.cpp
security/manager/ssl/tests/unit/test_cert_version.js
--- a/security/manager/locales/en-US/chrome/pipnss/pipnss.properties
+++ b/security/manager/locales/en-US/chrome/pipnss/pipnss.properties
@@ -48,19 +48,18 @@ VerifyCAVerifier=CA Verifier
 VerifyStatusResponder=Status Responder Certificate
 HighGrade=High Grade
 MediumGrade=Medium Grade
 # LOCALIZATION NOTE (nick_template): $1s is the common name from a cert (e.g. "Mozilla"), $2s is the CA name (e.g. VeriSign)
 nick_template=%1$s's %2$s ID
 #These are the strings set for the ASN1 objects in a certificate.
 CertDumpCertificate=Certificate
 CertDumpVersion=Version
-CertDumpVersion1=Version 1
-CertDumpVersion2=Version 2
-CertDumpVersion3=Version 3
+# LOCALIZATION NOTE (CertDumpVersionValue): %S is a version number (e.g. "3" in "Version 3")
+CertDumpVersionValue=Version %S
 CertDumpSerialNo=Serial Number
 CertDumpMD2WithRSA=PKCS #1 MD2 With RSA Encryption
 CertDumpMD5WithRSA=PKCS #1 MD5 With RSA Encryption
 CertDumpSHA1WithRSA=PKCS #1 SHA-1 With RSA Encryption
 CertDumpSHA256WithRSA=PKCS #1 SHA-256 With RSA Encryption
 CertDumpSHA384WithRSA=PKCS #1 SHA-384 With RSA Encryption
 CertDumpSHA512WithRSA=PKCS #1 SHA-512 With RSA Encryption
 CertDumpDefOID=Object Identifier (%S)
--- a/security/manager/ssl/nsNSSCertHelper.cpp
+++ b/security/manager/ssl/nsNSSCertHelper.cpp
@@ -1,31 +1,30 @@
 /* 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/. */
 
 #include "nsNSSCertHelper.h"
 
 #include <algorithm>
 
+#include "ScopedNSSTypes.h"
 #include "mozilla/Snprintf.h"
 #include "mozilla/UniquePtr.h"
+#include "nsCOMPtr.h"
 #include "nsComponentManagerUtils.h"
-#include "nsCOMPtr.h"
 #include "nsDateTimeFormatCID.h"
 #include "nsIDateTimeFormat.h"
 #include "nsNSSASN1Object.h"
-#include "nsNSSCertificate.h"
 #include "nsNSSCertTrust.h"
 #include "nsNSSCertValidity.h"
+#include "nsNSSCertificate.h"
 #include "nsNSSComponent.h"
-#include "nsIDateTimeFormat.h"
 #include "nsServiceManagerUtils.h"
 #include "prerror.h"
-#include "ScopedNSSTypes.h"
 #include "secder.h"
 
 using namespace mozilla;
  
 /* Object Identifier constants */
 #define CONST_OID static const unsigned char
 #define MICROSOFT_OID 0x2b, 0x6, 0x1, 0x4, 0x1, 0x82, 0x37
 #define PKIX_OID 0x2b, 0x6, 0x01, 0x05, 0x05, 0x07
@@ -79,63 +78,57 @@ GetIntValue(SECItem *versionItem,
   if (srv != SECSuccess) {
     NS_ERROR("Could not decode version of cert");
     return NS_ERROR_FAILURE;
   }
   return NS_OK;
 }
 
 static nsresult
-ProcessVersion(SECItem         *versionItem,
-               nsINSSComponent *nssComponent,
-               nsIASN1PrintableItem **retItem)
+ProcessVersion(SECItem* versionItem, nsINSSComponent* nssComponent,
+               nsIASN1PrintableItem** retItem)
 {
-  nsresult rv;
   nsAutoString text;
+  nssComponent->GetPIPNSSBundleString("CertDumpVersion", text);
   nsCOMPtr<nsIASN1PrintableItem> printableItem = new nsNSSASN1PrintableItem();
- 
-  nssComponent->GetPIPNSSBundleString("CertDumpVersion", text);
-  rv = printableItem->SetDisplayName(text);
-  if (NS_FAILED(rv))
+  nsresult rv = printableItem->SetDisplayName(text);
+  if (NS_FAILED(rv)) {
     return rv;
+  }
 
   // Now to figure out what version this certificate is.
-  unsigned long version;
-
+  unsigned int version;
   if (versionItem->data) {
-    rv = GetIntValue(versionItem, &version);
-    if (NS_FAILED(rv))
-      return rv;
+    // Filter out totally bogus version values/encodings.
+    if (versionItem->len != 1) {
+      return NS_ERROR_FAILURE;
+    }
+    version = *reinterpret_cast<const uint8_t*>(versionItem->data);
   } else {
-    // If there is no version present in the cert, then rfc2459
-    // says we default to v1 (0)
+    // If there is no version present in the cert, then RFC 5280 says we
+    // default to v1 (0).
     version = 0;
   }
 
-  switch (version){
-  case 0:
-    rv = nssComponent->GetPIPNSSBundleString("CertDumpVersion1", text);
-    break;
-  case 1:
-    rv = nssComponent->GetPIPNSSBundleString("CertDumpVersion2", text);
-    break;
-  case 2:
-    rv = nssComponent->GetPIPNSSBundleString("CertDumpVersion3", text);
-    break;
-  default:
-    NS_ERROR("Bad value for cert version");
-    rv = NS_ERROR_FAILURE;
+  // A value of n actually corresponds to version n + 1
+  nsAutoString versionString;
+  versionString.AppendInt(version + 1);
+  const char16_t* params[1] = { versionString.get() };
+  rv = nssComponent->PIPBundleFormatStringFromName("CertDumpVersionValue",
+                                                   params,
+                                                   MOZ_ARRAY_LENGTH(params),
+                                                   text);
+  if (NS_FAILED(rv)) {
+    return rv;
   }
-    
-  if (NS_FAILED(rv))
-    return rv;
 
   rv = printableItem->SetDisplayValue(text);
-  if (NS_FAILED(rv))
+  if (NS_FAILED(rv)) {
     return rv;
+  }
 
   printableItem.forget(retItem);
   return NS_OK;
 }
 
 static nsresult 
 ProcessSerialNumberDER(SECItem         *serialItem, 
                        nsINSSComponent *nssComponent,
--- a/security/manager/ssl/tests/unit/test_cert_version.js
+++ b/security/manager/ssl/tests/unit/test_cert_version.js
@@ -44,16 +44,26 @@ function loadCertWithTrust(certName, tru
 function checkEndEntity(cert, expectedResult) {
   checkCertErrorGeneric(certdb, cert, expectedResult, certificateUsageSSLServer);
 }
 
 function checkIntermediate(cert, expectedResult) {
   checkCertErrorGeneric(certdb, cert, expectedResult, certificateUsageSSLCA);
 }
 
+// Test that the code that decodes certificates to display them in the
+// certificate manager correctly handles the version field.
+function checkCertVersion(cert, expectedVersionString) {
+  let asn1 = cert.ASN1Structure.QueryInterface(Ci.nsIASN1Sequence);
+  let tbsCertificate = asn1.ASN1Objects.queryElementAt(0, Ci.nsIASN1Sequence);
+  let version = tbsCertificate.ASN1Objects.queryElementAt(0, Ci.nsIASN1Object);
+  equal(version.displayValue, expectedVersionString,
+        "Actual and expected version strings should match");
+}
+
 function run_test() {
   loadCertWithTrust("ca", "CTu,,");
 
   // Section for CAs lacking the basicConstraints extension entirely:
   loadCertWithTrust("int-v1-noBC_ca", ",,");
   checkIntermediate(certFromFile("int-v1-noBC_ca"), MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA);
   checkEndEntity(certFromFile("ee_int-v1-noBC"), MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA);
   // A v1 certificate with no basicConstraints extension may issue certificates
@@ -167,9 +177,14 @@ function run_test() {
   checkEndEntity(certFromFile("ss-v2-BC-not-cA"), SEC_ERROR_UNKNOWN_ISSUER);
   checkEndEntity(certFromFile("ss-v3-BC-not-cA"), SEC_ERROR_UNKNOWN_ISSUER);
   checkEndEntity(certFromFile("ss-v4-BC-not-cA"), SEC_ERROR_UNKNOWN_ISSUER);
 
   checkEndEntity(certFromFile("ss-v1-BC-cA"), SEC_ERROR_UNKNOWN_ISSUER);
   checkEndEntity(certFromFile("ss-v2-BC-cA"), SEC_ERROR_UNKNOWN_ISSUER);
   checkEndEntity(certFromFile("ss-v3-BC-cA"), SEC_ERROR_UNKNOWN_ISSUER);
   checkEndEntity(certFromFile("ss-v4-BC-cA"), SEC_ERROR_UNKNOWN_ISSUER);
+
+  checkCertVersion(certFromFile("ss-v1-noBC"), "Version 1");
+  checkCertVersion(certFromFile("int-v2-BC-cA_ca"), "Version 2");
+  checkCertVersion(certFromFile("ee-v3-BC-not-cA_ca"), "Version 3");
+  checkCertVersion(certFromFile("int-v4-BC-not-cA_ca"), "Version 4");
 }