bug 857627 - 1/4: improve nsIX509Cert.windowTitle (and rename to displayName) r?Cykesiopka,jcj draft
authorDavid Keeler <dkeeler@mozilla.com>
Fri, 18 Nov 2016 11:18:23 -0800
changeset 446745 00dbeb049b3c4b1fb9e915f3304954a8df46b4f2
parent 446527 3853c539a1b7c803f1075d2c3ecefbdd314af1d8
child 446746 57fe50c26b19b9becaf01dab941ebc3f94432aca
push id37861
push userdkeeler@mozilla.com
push dateThu, 01 Dec 2016 19:20:01 +0000
reviewersCykesiopka, jcj
bugs857627
milestone53.0a1
bug 857627 - 1/4: improve nsIX509Cert.windowTitle (and rename to displayName) r?Cykesiopka,jcj This patch makes the following changes: 1. renames nsIX509Cert.windowTitle to displayName 2. removes (most of*) displayName's use of the NSS certificate nickname API 3. adds additional fields that displayName will attempt to use to find a good name to return (namely, organizationalUnitName and organizationName) *except for built-in roots, where we have a good hard-coded value for the name MozReview-Commit-ID: Bt864GnOu7D
security/manager/pki/resources/content/certViewer.js
security/manager/pki/resources/content/pippki.js
security/manager/ssl/nsIX509Cert.idl
security/manager/ssl/nsNSSCertHelper.cpp
security/manager/ssl/nsNSSCertificate.cpp
security/manager/ssl/nsNSSCertificateFakeTransport.cpp
security/manager/ssl/tests/unit/test_nsIX509Cert_utf8.js
--- a/security/manager/pki/resources/content/certViewer.js
+++ b/security/manager/pki/resources/content/certViewer.js
@@ -39,28 +39,22 @@ function doPrompt(msg)
  *
  * @param {tree} node
  *        Parent tree node to append to.
  * @param {nsIArray<nsIX509Cert>} chain
  *        Chain where cert element n is issued by cert element n + 1.
  */
 function AddCertChain(node, chain)
 {
-  var child = document.getElementById(node);
-  var currCert;
-  var displayVal;
+  let child = document.getElementById(node);
   for (let i = chain.length - 1; i >= 0; i--) {
-    currCert = chain.queryElementAt(i, nsIX509Cert);
-    if (currCert.commonName) {
-      displayVal = currCert.commonName;
-    } else {
-      displayVal = currCert.windowTitle;
-    }
+    let currCert = chain.queryElementAt(i, nsIX509Cert);
+    let displayValue = currCert.displayName;
     let addTwistie = i != 0;
-    child = addChildrenToTree(child, displayVal, currCert.dbKey, addTwistie);
+    child = addChildrenToTree(child, displayValue, currCert.dbKey, addTwistie);
   }
 }
 
 /**
  * Adds a "verified usage" of a cert to the "General" tab of the cert viewer.
  *
  * @param {String} usage
  *        Verified usage to add.
@@ -77,17 +71,17 @@ function AddUsage(usage)
 }
 
 function setWindowName()
 {
   bundle = document.getElementById("pippki_bundle");
 
   let cert = window.arguments[0].QueryInterface(Ci.nsIX509Cert);
   document.title = bundle.getFormattedString("certViewerTitle",
-                                             [cert.windowTitle]);
+                                             [cert.displayName]);
 
   //
   //  Set the cert attributes for viewing
   //
 
   //  The chain of trust
   AddCertChain("treesetDump", cert.getChain());
   DisplayGeneralDataFromCert(cert);
--- a/security/manager/pki/resources/content/pippki.js
+++ b/security/manager/pki/resources/content/pippki.js
@@ -80,20 +80,17 @@ const DEFAULT_CERT_EXTENSION = "crt";
  * attribute on an nsIFilePicker.
  *
  * @param {nsIX509Cert} cert
  *        The cert to generate a filename for.
  * @returns {String}
  *          Generated filename.
  */
 function certToFilename(cert) {
-  let filename = cert.commonName;
-  if (!filename) {
-    filename = cert.windowTitle;
-  }
+  let filename = cert.displayName;
 
   // Remove unneeded and/or unsafe characters.
   filename = filename.replace(/\s/g, "")
                      .replace(/\./g, "")
                      .replace(/\\/g, "")
                      .replace(/\//g, "");
 
   // nsIFilePicker.defaultExtension is more of a suggestion to some
--- a/security/manager/ssl/nsIX509Cert.idl
+++ b/security/manager/ssl/nsIX509Cert.idl
@@ -140,17 +140,17 @@ interface nsIX509Cert : nsISupports {
   /**
    *  A unique identifier of this certificate within the local storage.
    */
   readonly attribute ACString dbKey;
 
   /**
    *  A human readable identifier to label this certificate.
    */
-  readonly attribute AString windowTitle;
+  readonly attribute AString displayName;
 
   /**
    *  Constants to classify the type of a certificate.
    */
   const unsigned long UNKNOWN_CERT =      0;
   const unsigned long CA_CERT      = 1 << 0;
   const unsigned long USER_CERT    = 1 << 1;
   const unsigned long EMAIL_CERT   = 1 << 2;
--- a/security/manager/ssl/nsNSSCertHelper.cpp
+++ b/security/manager/ssl/nsNSSCertHelper.cpp
@@ -1962,23 +1962,26 @@ nsNSSCertificate::CreateASN1Struct(nsIAS
   if (isAlreadyShutDown())
     return NS_ERROR_NOT_AVAILABLE;
 
   nsCOMPtr<nsIASN1Sequence> sequence = new nsNSSASN1Sequence();
 
   nsCOMPtr<nsIMutableArray> asn1Objects;
   sequence->GetASN1Objects(getter_AddRefs(asn1Objects));
 
-  nsAutoString title;
-  nsresult rv = GetWindowTitle(title);
+  nsAutoString displayName;
+  nsresult rv = GetDisplayName(displayName);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  sequence->SetDisplayName(title);
+  rv = sequence->SetDisplayName(displayName);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
   sequence.forget(aRetVal);
 
   // This sequence will be contain the tbsCertificate, signatureAlgorithm,
   // and signatureValue.
   nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
   if (NS_FAILED(rv))
     return rv;
 
--- a/security/manager/ssl/nsNSSCertificate.cpp
+++ b/security/manager/ssl/nsNSSCertificate.cpp
@@ -4,16 +4,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsNSSCertificate.h"
 
 #include "CertVerifier.h"
 #include "ExtendedValidation.h"
 #include "NSSCertDBTrustDomain.h"
 #include "certdb.h"
+#include "mozilla/Assertions.h"
 #include "mozilla/Base64.h"
 #include "mozilla/Casting.h"
 #include "mozilla/NotNull.h"
 #include "mozilla/Unused.h"
 #include "nsArray.h"
 #include "nsCOMPtr.h"
 #include "nsCRT.h"
 #include "nsICertificateDialogs.h"
@@ -361,44 +362,77 @@ nsNSSCertificate::GetDbKey(const UniqueC
              cert->serialNumber.len);
   buf.Append(BitwiseCast<char*, unsigned char*>(cert->derIssuer.data),
              cert->derIssuer.len);
 
   return Base64Encode(buf, aDbKey);
 }
 
 NS_IMETHODIMP
-nsNSSCertificate::GetWindowTitle(nsAString& aWindowTitle)
+nsNSSCertificate::GetDisplayName(nsAString& aDisplayName)
 {
   nsNSSShutDownPreventionLock locker;
   if (isAlreadyShutDown()) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
-  aWindowTitle.Truncate();
+  aDisplayName.Truncate();
 
+  MOZ_ASSERT(mCert, "mCert should not be null in GetDisplayName");
   if (!mCert) {
-    NS_ERROR("Somehow got nullptr for mCert in nsNSSCertificate.");
     return NS_ERROR_FAILURE;
   }
 
   UniquePORTString commonName(CERT_GetCommonName(&mCert->subject));
+  UniquePORTString organizationalUnitName(CERT_GetOrgUnitName(&mCert->subject));
+  UniquePORTString organizationName(CERT_GetOrgName(&mCert->subject));
 
-  const char* titleOptions[] = {
-    mCert->nickname,
+  bool isBuiltInRoot;
+  nsresult rv = GetIsBuiltInRoot(&isBuiltInRoot);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  // Only use the nickname for built-in roots where we already have a hard-coded
+  // reasonable display name (unfortunately we have to strip off the leading
+  // slot identifier followed by a ':'). Otherwise, attempt to use the following
+  // in order:
+  //  - the common name, if present
+  //  - an organizational unit name, if present
+  //  - an organization name, if present
+  //  - the entire subject distinguished name, if non-empty
+  //  - an email address, if one can be found
+  // In the unlikely event that none of these fields are present and non-empty
+  // (the subject really shouldn't be empty), an empty string is returned.
+  nsAutoCString builtInRootNickname;
+  if (isBuiltInRoot) {
+    nsAutoCString fullNickname(mCert->nickname);
+    int32_t index = fullNickname.Find(":");
+    if (index != kNotFound) {
+      // Substring will gracefully handle the case where index is the last
+      // character in the string (that is, if the nickname is just
+      // "Builtin Object Token:"). In that case, we'll get an empty string.
+      builtInRootNickname = Substring(fullNickname,
+                                      AssertedCast<uint32_t>(index + 1));
+    }
+  }
+  const char* nameOptions[] = {
+    builtInRootNickname.get(),
     commonName.get(),
+    organizationalUnitName.get(),
+    organizationName.get(),
     mCert->subjectName,
     mCert->emailAddr
   };
 
-  nsAutoCString titleOption;
-  for (size_t i = 0; i < ArrayLength(titleOptions); i++) {
-    titleOption = titleOptions[i];
-    if (titleOption.Length() > 0 && IsUTF8(titleOption)) {
-      CopyUTF8toUTF16(titleOption, aWindowTitle);
+  nsAutoCString nameOption;
+  for (auto nameOptionPtr : nameOptions) {
+    nameOption.Assign(nameOptionPtr);
+    if (nameOption.Length() > 0 && IsUTF8(nameOption)) {
+      CopyUTF8toUTF16(nameOption, aDisplayName);
       return NS_OK;
     }
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
--- a/security/manager/ssl/nsNSSCertificateFakeTransport.cpp
+++ b/security/manager/ssl/nsNSSCertificateFakeTransport.cpp
@@ -31,17 +31,17 @@ nsNSSCertificateFakeTransport::~nsNSSCer
 NS_IMETHODIMP
 nsNSSCertificateFakeTransport::GetDbKey(nsACString&)
 {
   NS_NOTREACHED("Unimplemented on content process");
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
-nsNSSCertificateFakeTransport::GetWindowTitle(nsAString&)
+nsNSSCertificateFakeTransport::GetDisplayName(nsAString&)
 {
   NS_NOTREACHED("Unimplemented on content process");
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 NS_IMETHODIMP
 nsNSSCertificateFakeTransport::GetNickname(nsAString&)
 {
--- a/security/manager/ssl/tests/unit/test_nsIX509Cert_utf8.js
+++ b/security/manager/ssl/tests/unit/test_nsIX509Cert_utf8.js
@@ -52,18 +52,18 @@ function run_test() {
   equal(cert.subjectName, "serialNumber=ICA - 10003769,SN=Rašek,name=Luděk Rašek,initials=LR,givenName=Luděk,E=ludek.rasek@centrum.cz,L=\"Pacov, Nádražní 769\",ST=Vysočina,CN=Luděk Rašek,C=CZ",
         "Actual and expected subjectName should match");
   equal(cert.commonName, "Luděk Rašek",
         "Actual and expected commonName should match");
   equal(cert.organization, "",
         "Actual and expected organization should match");
   equal(cert.organizationalUnit, "",
         "Actual and expected organizationalUnit should match");
-  equal(cert.windowTitle, "Luděk Rašek",
-        "Actual and expected windowTitle should match");
+  equal(cert.displayName, "Luděk Rašek",
+        "Actual and expected displayName should match");
   equal(cert.issuerName, "OU=Akreditovaný poskytovatel certifikačních služeb,O=První certifikační autorita a.s.,L=\"Podvinný mlýn 2178/6, 190 00 Praha 9\",C=CZ,CN=I.CA - Qualified root certificate (kvalifikovaný certifikát poskytovatele) - PSEUDONYM",
         "Actual and expected issuerName should match");
   equal(cert.issuerCommonName, "I.CA - Qualified root certificate (kvalifikovaný certifikát poskytovatele) - PSEUDONYM",
         "Actual and expected issuerCommonName should match");
   equal(cert.issuerOrganization, "První certifikační autorita a.s.",
         "Actual and expected issuerOrganization should match");
   equal(cert.issuerOrganizationUnit, "Akreditovaný poskytovatel certifikačních služeb",
         "Actual and expected issuerOrganizationUnit should match");