Bug 372980 - XPInstall reports "(Author not verified)" when signing certificate has no organization subject. r=dveditz

toolkit/mozapps/extensions/src/nsExtensionManager.js.in

@@ -2919,10 +2919,12 @@ ExtensionManager.prototype = {
               zipReader.QueryInterface(Ci.nsIJAR);
               var principal = zipReader.getCertificatePrincipal(null);
               if (principal && principal.hasCertificate) {
-                if (principal.hasCertificate && verifyZipSigning(zipReader, principal)) {
-                  // XXX Bug 372980 This string could be empty. This needs
-                  // better UI to present principal.value.certificate's subject.
-                  prettyName = principal.prettyName;
+                if (verifyZipSigning(zipReader, principal)) {
+                  x509 = principal.certificate;
+                  if (x509 instanceof Ci.nsIX509Cert && x509.commonName.length > 0)
+                    prettyName = x509.commonName;
+                  else
+                    prettyName = principal.prettyName;
                 }
                 else {
                   // The xpi isn't correctly signed, don't offer to install.

toolkit/mozapps/extensions/test/unit/test_bug386153.js

@@ -43,7 +43,7 @@ const URI_XPINSTALL_CONFIRM_DIALOG = "chrome://mozapps/content/xpinstall/xpinsta
 
 // Finds the index of the given xpi in the dialogparamblock strings
 function findXPI(dpb, name) {
-  for (var i = 0; i < 3; i++) {
+  for (var i = 0; i < 5; i++) {
     if (dpb.GetString(i * 4 + 1).substr(-(name.length + 1)) == "/" + name)
       return i * 4;
   }
@@ -56,19 +56,25 @@ var WindowWatcher = {
     do_check_eq(url, URI_XPINSTALL_CONFIRM_DIALOG);
     var dpb = arguments.QueryInterface(Ci.nsISupportsInterfacePointer)
                        .data.QueryInterface(Ci.nsIDialogParamBlock);
-    do_check_eq(dpb.GetInt(1), 12);
+    do_check_eq(dpb.GetInt(1), 20);
 
     // Not defined what order they will be in so find them based on the filename
     var unsigned = findXPI(dpb, "unsigned.xpi");
     var signed = findXPI(dpb, "signed.xpi");
     var untrusted = findXPI(dpb, "signed-untrusted.xpi");
+    var no_o = findXPI(dpb, "signed-no-o.xpi");
+    var no_cn = findXPI(dpb, "signed-no-cn.xpi");
 
     // Test the names and certs are correct
     do_check_eq(dpb.GetString(unsigned), "XPI Test");
     do_check_eq(dpb.GetString(unsigned + 3), "");
 
     do_check_eq(dpb.GetString(signed), "Signed XPI Test");
-    do_check_eq(dpb.GetString(signed + 3), "Mozilla Testing");
+    do_check_eq(dpb.GetString(signed + 3), "Object Signer");
+    do_check_eq(dpb.GetString(no_o), "Signed XPI Test (No Org)");
+    do_check_eq(dpb.GetString(no_o + 3), "Object Signer");
+    do_check_eq(dpb.GetString(no_cn), "Signed XPI Test (No Common Name)");
+    do_check_eq(dpb.GetString(no_cn + 3), "Mozilla Testing");
 
     // XPIs signed by an unknown CA just appear to not be signed at all
     do_check_eq(dpb.GetString(untrusted), "Signed XPI Test - Untrusted");
@@ -119,6 +125,8 @@ function run_test()
   do_get_file("data/signed.xpi").copyTo(il, null);
   do_get_file("data/signed-untrusted.xpi").copyTo(il, null);
   do_get_file("data/signed-tampered.xpi").copyTo(il, null);
+  do_get_file("data/signed-no-o.xpi").copyTo(il, null);
+  do_get_file("data/signed-no-cn.xpi").copyTo(il, null);
 
   // Starting the EM will detect and attempt to install the xpis
   startupEM();
@@ -126,6 +134,8 @@ function run_test()
   do_check_neq(gEM.getItemForID("unsigned-xpi@tests.mozilla.org"), null);
   do_check_neq(gEM.getItemForID("untrusted-xpi@tests.mozilla.org"), null);
   do_check_eq(gEM.getItemForID("tampered-xpi@tests.mozilla.org"), null);
+  do_check_neq(gEM.getItemForID("signed-xpi-no-o@tests.mozilla.org"), null);
+  do_check_neq(gEM.getItemForID("signed-xpi-no-cn@tests.mozilla.org"), null);
 
   shutdownEM();
 }

xpinstall/src/nsXPITriggerInfo.cpp

@@ -49,6 +49,7 @@
 #include "nsIJSContextStack.h"
 #include "nsIScriptSecurityManager.h"
 #include "nsICryptoHash.h"
+#include "nsIX509Cert.h"
 
 //
 // nsXPITriggerItem
@@ -153,10 +154,17 @@ nsXPITriggerItem::SetPrincipal(nsIPrincipal* aPrincipal)
     PRBool hasCert;
     aPrincipal->GetHasCertificate(&hasCert);
     if (hasCert) {
+        nsCOMPtr<nsISupports> certificate;
+        aPrincipal->GetCertificate(getter_AddRefs(certificate));
+
+        nsCOMPtr<nsIX509Cert> x509 = do_QueryInterface(certificate);
+        if (x509) {
+            x509->GetCommonName(mCertName);
+            if (mCertName.Length() > 0)
+                return;
+        }
+
         nsCAutoString prettyName;
-        // XXXbz should this really be using the prettyName?  Perhaps
-        // it wants to get the subjectName or nsIX509Cert and display
-        // it sanely?
         aPrincipal->GetPrettyName(prettyName);
         CopyUTF8toUTF16(prettyName, mCertName);
     }

xpinstall/tests/Makefile.in

@@ -60,6 +60,7 @@ _BROWSER_FILES = harness.js \
                  browser_signed_untrusted.js \
                  browser_signed_tampered.js \
                  browser_signed_multiple.js \
+                 browser_signed_naming.js \
                  browser_empty.js \
                  browser_corrupt.js \
                  browser_cookies.js \
@@ -85,6 +86,8 @@ _BROWSER_FILES = harness.js \
                  unsigned.xpi \
                  signed.xpi \
                  signed2.xpi \
+                 signed-no-o.xpi \
+                 signed-no-cn.xpi \
                  signed-untrusted.xpi \
                  signed-tampered.xpi \
                  empty.xpi \

xpinstall/tests/browser_signed_multiple.js

@@ -29,11 +29,11 @@ function confirm_install(window) {
   is(items.length, 2, "Should be 2 items listed in the confirmation dialog");
   is(items[0].name, "Signed XPI", "Should have seen the name from the trigger list");
   is(items[0].url, TESTROOT + "signed.xpi", "Should have listed the correct url for the item");
-  is(items[0].cert, "(Mozilla Testing)", "Should have seen the signer");
+  is(items[0].cert, "(Object Signer)", "Should have seen the signer");
   is(items[0].signed, "true", "Should have listed the item as signed");
   is(items[1].name, "Signed XPI 2", "Should have seen the name from the trigger list");
   is(items[1].url, TESTROOT + "signed2.xpi", "Should have listed the correct url for the item");
-  is(items[1].cert, "(Mozilla Testing)", "Should have seen the signer");
+  is(items[1].cert, "(Object Signer)", "Should have seen the signer");
   is(items[1].signed, "true", "Should have listed the item as signed");
   return true;
 }

xpinstall/tests/browser_signed_naming.js

@@ -0,0 +1,65 @@
+// Load in the test harness
+var scriptLoader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
+                             .getService(Components.interfaces.mozIJSSubScriptLoader);
+scriptLoader.loadSubScript("chrome://mochikit/content/browser/xpinstall/tests/harness.js", this);
+
+// ----------------------------------------------------------------------------
+// Tests that the correct signer is presented for combinations of O and CN present.
+// The signed files have (when present) O=Mozilla Testing, CN=Object Signer
+// This verifies bug 372980
+function test() {
+  Harness.installConfirmCallback = confirm_install;
+  Harness.installEndedCallback = check_xpi_install;
+  Harness.installsCompletedCallback = finish_test;
+  Harness.setup();
+
+  var pm = Components.classes["@mozilla.org/permissionmanager;1"]
+                     .getService(Components.interfaces.nsIPermissionManager);
+  pm.add(makeURI("http://example.com/"), "install", pm.ALLOW_ACTION);
+
+  var triggers = encodeURIComponent(JSON.stringify({
+    "Signed XPI (O and CN)": TESTROOT + "signed.xpi",
+    "Signed XPI (CN)": TESTROOT + "signed-no-o.xpi",
+    "Signed XPI (O)": TESTROOT + "signed-no-cn.xpi",
+  }));
+  gBrowser.selectedTab = gBrowser.addTab();
+  gBrowser.loadURI(TESTROOT + "installtrigger.html?" + triggers);
+}
+
+function confirm_install(window) {
+  items = window.document.getElementById("itemList").childNodes;
+  is(items.length, 3, "Should be 3 items listed in the confirmation dialog");
+  is(items[0].name, "Signed XPI (O and CN)", "Should have seen the name from the trigger list");
+  is(items[0].url, TESTROOT + "signed.xpi", "Should have listed the correct url for the item");
+  is(items[0].cert, "(Object Signer)", "Should have seen the signer");
+  is(items[0].signed, "true", "Should have listed the item as signed");
+  is(items[1].name, "Signed XPI (CN)", "Should have seen the name from the trigger list");
+  is(items[1].url, TESTROOT + "signed-no-o.xpi", "Should have listed the correct url for the item");
+  is(items[1].cert, "(Object Signer)", "Should have seen the signer");
+  is(items[1].signed, "true", "Should have listed the item as signed");
+  is(items[2].name, "Signed XPI (O)", "Should have seen the name from the trigger list");
+  is(items[2].url, TESTROOT + "signed-no-cn.xpi", "Should have listed the correct url for the item");
+  is(items[2].cert, "(Mozilla Testing)", "Should have seen the signer");
+  is(items[2].signed, "true", "Should have listed the item as signed");
+  return true;
+}
+
+function check_xpi_install(addon, status) {
+  is(status, 0, "Installs should succeed");
+}
+
+function finish_test() {
+  var em = Components.classes["@mozilla.org/extensions/manager;1"]
+                     .getService(Components.interfaces.nsIExtensionManager);
+  em.cancelInstallItem("signed-xpi@tests.mozilla.org");
+  em.cancelInstallItem("signed-xpi-no-o@tests.mozilla.org");
+  em.cancelInstallItem("signed-xpi-no-cn@tests.mozilla.org");
+
+  var pm = Components.classes["@mozilla.org/permissionmanager;1"]
+                     .getService(Components.interfaces.nsIPermissionManager);
+  pm.remove("example.com", "install");
+
+  gBrowser.removeCurrentTab();
+  Harness.finish();
+}
+// ----------------------------------------------------------------------------

xpinstall/tests/browser_signed_tampered.js

@@ -27,7 +27,7 @@ function confirm_install(window) {
   is(items.length, 1, "Should only be 1 item listed in the confirmation dialog");
   is(items[0].name, "Tampered Signed XPI", "Should have seen the name from the trigger list");
   is(items[0].url, TESTROOT + "signed-tampered.xpi", "Should have listed the correct url for the item");
-  is(items[0].cert, "(Mozilla Testing)", "Should have seen the signer");
+  is(items[0].cert, "(Object Signer)", "Should have seen the signer");
   is(items[0].signed, "true", "Should have listed the item as signed");
   return true;
 }

xpinstall/tests/browser_signed_trigger.js

@@ -28,7 +28,7 @@ function confirm_install(window) {
   is(items.length, 1, "Should only be 1 item listed in the confirmation dialog");
   is(items[0].name, "Signed XPI", "Should have seen the name from the trigger list");
   is(items[0].url, TESTROOT + "signed.xpi", "Should have listed the correct url for the item");
-  is(items[0].cert, "(Mozilla Testing)", "Should have seen the signer");
+  is(items[0].cert, "(Object Signer)", "Should have seen the signer");
   is(items[0].signed, "true", "Should have listed the item as signed");
   return true;
 }

xpinstall/tests/browser_signed_untrusted.js

@@ -28,7 +28,7 @@ function confirm_install(window) {
   is(items.length, 1, "Should only be 1 item listed in the confirmation dialog");
   is(items[0].name, "Untrusted Signed XPI", "Should have seen the name from the trigger list");
   is(items[0].url, TESTROOT + "signed-untrusted.xpi", "Should have listed the correct url for the item");
-  is(items[0].cert, "(Unknown Organisation)", "Should have seen the supposed signer");
+  is(items[0].cert, "(Unknown Signer)", "Should have seen the supposed signer");
   is(items[0].signed, "true", "Should have listed the item as signed");
   return true;
 }

xpinstall/tests/browser_signed_url.js

@@ -20,7 +20,7 @@ function confirm_install(window) {
   is(items.length, 1, "Should only be 1 item listed in the confirmation dialog");
   is(items[0].name, "signed.xpi", "Should have had the filename for the item name");
   is(items[0].url, TESTROOT + "signed.xpi", "Should have listed the correct url for the item");
-  is(items[0].cert, "(Mozilla Testing)", "Should have seen the signer");
+  is(items[0].cert, "(Object Signer)", "Should have seen the signer");
   is(items[0].signed, "true", "Should have listed the item as signed");
   return true;
 }