Bug 483440 - PSM doesn't detect invalid OID encodings in Cert Viewer Details tab, r=nelson+johnath

build/pgo/certs/bug483440-attack2b.ca

@@ -0,0 +1,14 @@
+-----BEGIN CERTIFICATE-----
+MIICKDCCAZGgAwIBAgIFAIyjFPowDQYJKoZIhvcNAQEFBQAwKDEXMBUGA1UEAwwO
+KgB3d3cubXlDQS5vcmcxDTALBgNVBAMTBG15Q0EwHhcNMDkwMzE0MTg0NzU2WhcN
+MTkwMzE0MTg0NzU2WjBhMRMwEQYDVQQKEwpCYWRndXkgSW5jMRcwFQYDVQQDEw53
+d3cuYmFkZ3V5LmNvbTEZMBcGA1UECxMQSGFja2luZyBEaXZpc2lvbjEWMBQGBFUE
+gAMTDHd3dy5iYW5rLmNvbTCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEA2YvL
+GgmF0OTLBKz0nYTvR+DlnZai7b2MqAIM9IUEpMfqzJPNYCsXziYXgHtr/do9ppJP
+BhDjeyIGEOSpgBqdkWItxlLopUHnf8VKwnDPPj4KkNyXuTLm60X/ph+/zrjTw+kU
+m+/kVYstgGMuTIoTuu7loxCqqeVlAgc5lzTpUhkCAwEAAaMlMCMwDAYDVR0TAQH/
+BAIwADATBgNVHSUEDDAKBggrBgEFBQcDATANBgkqhkiG9w0BAQUFAAOBgQAKHl1G
+vaXftj5QPK3eIT6Q3fWuGKR39grlg5GL/WocPanYycOlm9zvT1Hx95cY6msIXSKp
+xycndJ02ODX35DDgolV6VHUsM9yoagk+eqs5kCqW2qiv3moIshL0yWVhuCobMA+E
+D3wHFCPqVU+igRdCrEQDxZHoFOR4J/DKHfGANg==
+-----END CERTIFICATE-----

build/pgo/certs/bug483440-attack7.ca

@@ -0,0 +1,16 @@
+-----BEGIN CERTIFICATE-----
+MIICljCCAf+gAwIBAgIBATANBgkqhkiG9w0BAQUFADCBkDERMA8GBPMlBAMTB2F0
+dGFjazcxEDAOBgOABAMTB2F0dGFjazYxEzARBgZVBP///38TB2F0dGFjazUxEjAQ
+BgVVBAOBgRMHYXR0YWNrNDEUMBIGB1UEgICAgIATB2F0dGFjazMxFDASBgdVBJCA
+gIABEwdhdHRhY2syMRQwEgYHVQSIgICAARMHYXR0YWNrMTAeFw0wOTA0MTMxNDAw
+MzdaFw0yOTA0MTMxNDAwMzdaMIGQMREwDwYE8yUEAxMHYXR0YWNrNzEQMA4GA4AE
+AxMHYXR0YWNrNjETMBEGBlUE////fxMHYXR0YWNrNTESMBAGBVUEA4GBEwdhdHRh
+Y2s0MRQwEgYHVQSAgICAgBMHYXR0YWNrMzEUMBIGB1UEkICAgAETB2F0dGFjazIx
+FDASBgdVBIiAgIABEwdhdHRhY2sxMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKB
+gQC77fQ1wrywBnVmr8XO0/78/qFOz+sjnMlpBvLx5UImittgMNSgEqNulRDbO0qG
+K4tlFF2sNsS7aOun6Cq7yl2+a8mIljmjzs+iwCLOEAkQTOM4RsdCosJVy/fjwmH1
+xI0uXt5cPkA0FM7B/IZSzWSC+2gY1+u1AhRJ35bXDhu92wIDAQABMA0GCSqGSIb3
+DQEBBQUAA4GBAFZitQjsQJ1+XsxKchBefilaHsi4oncc05P29IXcRbHI8wK2vNk8
+kkG2c6M4a4Rx1R4C3n99NwXH4vyNUbA9FuMSAdjaS3TW3zm8lKNCuIWGuI2Vvefy
++wNcCfb8B4AuP8pZOqqKsspgiBAE1EPPErnb7nMVLCnf+ts9ARXLBZTi
+-----END CERTIFICATE-----

build/pgo/certs/bug483440-pk10oflo.ca

@@ -0,0 +1,14 @@
+-----BEGIN CERTIFICATE-----
+MIICMTCCAZqgAwIBAgIFAIyjFTAwDQYJKoZIhvcNAQEFBQAwKDEXMBUGA1UEAwwO
+KgB3d3cubXlDQS5vcmcxDTALBgNVBAMTBG15Q0EwHhcNMDkwMzE0MTg0ODI0WhcN
+MTkwMzE0MTg0ODI0WjBqMRMwEQYDVQQKEwpCYWRndXkgSW5jMRcwFQYDVQQDEw53
+d3cuYmFkZ3V5LmNvbTEZMBcGA1UECxMQSGFja2luZyBEaXZpc2lvbjEfMB0GDVUE
+goCAgICAgICAgAMTDHd3dy5iYW5rLmNvbTCBnzANBgkqhkiG9w0BAQEFAAOBjQAw
+gYkCgYEA2YvLGgmF0OTLBKz0nYTvR+DlnZai7b2MqAIM9IUEpMfqzJPNYCsXziYX
+gHtr/do9ppJPBhDjeyIGEOSpgBqdkWItxlLopUHnf8VKwnDPPj4KkNyXuTLm60X/
+ph+/zrjTw+kUm+/kVYstgGMuTIoTuu7loxCqqeVlAgc5lzTpUhkCAwEAAaMlMCMw
+DAYDVR0TAQH/BAIwADATBgNVHSUEDDAKBggrBgEFBQcDATANBgkqhkiG9w0BAQUF
+AAOBgQBr+ekYoADBm6kbHBR1oc/6O9ZciRsTbxIAl3xjA3kNEeiUXXSoe+1dlt3Z
+7D6EaQztqR8usCW728J3vi8p/XxociK3r4aq0Sxu29gp21N1V/Um8y3ssI+Yt9Im
+oHlo5ikUXra5PtGAwi4FymrU5dWlHxYk1PlNP5nsvxdElPZnZA==
+-----END CERTIFICATE-----

security/manager/ssl/src/nsNSSCertHelper.cpp

@@ -204,23 +204,21 @@ ProcessSerialNumberDER(SECItem         *serialItem,
 
 static nsresult
 GetDefaultOIDFormat(SECItem *oid,
+                    nsINSSComponent *nssComponent,
                     nsAString &outString,
-		    char separator)
+                    char separator)
 {
   char buf[300];
-  unsigned int len;
-  int written;
+  unsigned int len = 0;
+  int written, invalidCount = 0;
     
-  unsigned long val  = oid->data[0];
-  unsigned int  i    = val % 40;
-  val /= 40;
-  written = PR_snprintf(buf, 300, "%lu%c%u", val, separator, i);
-  if (written < 0)
-    return NS_ERROR_FAILURE;	
-  len = written;
+  unsigned int i;
+  unsigned long val  = 0;
+  PRBool invalid = PR_FALSE;
+  PRBool first = PR_TRUE;
 
   val = 0;
-  for (i = 1; i < oid->len; ++i) {
+  for (i = 0; i < oid->len; ++i) {
     // In this loop, we have to parse a DER formatted 
     // If the first bit is a 1, then the integer is 
     // represented by more than one byte.  If the 
@@ -231,16 +229,64 @@ GetDefaultOIDFormat(SECItem *oid,
 
     j = oid->data[i];
     val = (val << 7) | (j & 0x7f);
-    if (j & 0x80)
-      continue;
-    written = PR_snprintf(&buf[len], sizeof(buf)-len, "%c%lu", 
-			  separator, val);
+    if (j & 0x80) {
+      // - If val is 0 in this block, the OID number particle starts with 0x80
+      // what is specified as an invalid formating.
+      // - If val is larger then 2^32-7, on next left shift by 7 we will loose
+      // the most significant bits, this OID number particle cannot be read
+      // by our implementation.
+      // - If the first bit is set while this is the last component of the OID
+      // we are also in an invalid state.
+      if (val == 0 || (val >= (1 << (32-7))) || (i == oid->len-1)) {
+        invalid = PR_TRUE;
+      }
+
+      if (i < oid->len-1)
+        continue;
+    }
+
+    if (!invalid) {
+      if (first) {
+        unsigned long one = PR_MIN(val/40, 2); // never > 2
+        unsigned long two = val - (one * 40);
+
+        written = PR_snprintf(&buf[len], sizeof(buf)-len, "%lu%c%lu", 
+			      one, separator, two);
+      }
+      else {
+        written = PR_snprintf(&buf[len], sizeof(buf)-len, "%c%lu", 
+			      separator, val);
+      }
+    }
+    else {
+      nsAutoString unknownText;
+      nssComponent->GetPIPNSSBundleString("CertUnknown", 
+                                          unknownText);
+      if (first) {
+        written = PR_snprintf(&buf[len], sizeof(buf)-len, "%s",
+                              NS_ConvertUTF16toUTF8(unknownText).get());
+      }
+      else {
+        written = PR_snprintf(&buf[len], sizeof(buf)-len, "%c%s",
+                              separator, 
+                              NS_ConvertUTF16toUTF8(unknownText).get());
+      }
+
+      if (++invalidCount > 3) {
+        // Allow only 3 occurences of Unknown in OID display string to
+        // prevent bloat.
+        break;
+      }
+    }
+
     if (written < 0)
       return NS_ERROR_FAILURE;
 
     len += written;
     NS_ASSERTION(len < sizeof(buf), "OID data to big to display in 300 chars.");
     val = 0;      
+    invalid = PR_FALSE;
+    first = PR_FALSE;
   }
 
   CopyASCIItoUTF16(buf, outString);
@@ -600,7 +646,7 @@ GetOIDText(SECItem *oid, nsINSSComponent *nssComponent, nsAString &text)
     rv = nssComponent->GetPIPNSSBundleString(bundlekey, text);
   } else {
     nsAutoString text2;
-    rv = GetDefaultOIDFormat(oid, text2, ' ');
+    rv = GetDefaultOIDFormat(oid, nssComponent, text2, ' ');
     if (NS_FAILED(rv))
       return rv;
 
@@ -834,14 +880,14 @@ ProcessExtKeyUsage(SECItem  *extData,
     // of the form CertDumpEKU_<underlined-OID>
     nsAutoString oidname;
     oid = *oids;
-    rv = GetDefaultOIDFormat(oid, oidname, '_');
+    rv = GetDefaultOIDFormat(oid, nssComponent, oidname, '_');
     if (NS_FAILED(rv))
       return rv;
     nsAutoString bundlekey = NS_LITERAL_STRING("CertDumpEKU_")+ oidname;
     NS_ConvertUTF16toUTF8 bk_ascii(bundlekey);
     
     rv = nssComponent->GetPIPNSSBundleString(bk_ascii.get(), local);
-    nsresult rv2 = GetDefaultOIDFormat(oid, oidname, '.');
+    nsresult rv2 = GetDefaultOIDFormat(oid, nssComponent, oidname, '.');
     if (NS_FAILED(rv2))
       return rv2;
     if (NS_SUCCEEDED(rv)) {
@@ -1066,7 +1112,7 @@ ProcessGeneralName(PRArenaPool *arena,
 	  ProcessRawBytes(nssComponent, &current->name.OthName.name, value);
 	}
     } else {
-      rv = GetDefaultOIDFormat(&current->name.OthName.oid, key, ' ');
+      rv = GetDefaultOIDFormat(&current->name.OthName.oid, nssComponent, key, ' ');
       if (NS_FAILED(rv))
 	goto finish;
       ProcessRawBytes(nssComponent, &current->name.OthName.name, value);
@@ -1126,7 +1172,7 @@ ProcessGeneralName(PRArenaPool *arena,
     }
   case certRegisterID:
     nssComponent->GetPIPNSSBundleString("CertDumpRegisterID", key);
-    rv = GetDefaultOIDFormat(&current->name.other, value, '.');
+    rv = GetDefaultOIDFormat(&current->name.other, nssComponent, value, '.');
     if (NS_FAILED(rv))
       goto finish;
     break;
@@ -1356,7 +1402,7 @@ ProcessCertificatePolicies(SECItem  *extData,
       text.Append(local);
       break;
     default:
-      GetDefaultOIDFormat(&policyInfo->policyID, local, '.');
+      GetDefaultOIDFormat(&policyInfo->policyID, nssComponent, local, '.');
       text.Append(local);
     }
 
@@ -1407,7 +1453,7 @@ ProcessCertificatePolicies(SECItem  *extData,
 				 text, nssComponent);
 	  break;
 	default:
-	  GetDefaultOIDFormat(&policyQualifier->qualifierID, local, '.');
+	  GetDefaultOIDFormat(&policyQualifier->qualifierID, nssComponent, local, '.');
 	  text.Append(local);
 	  text.Append(NS_LITERAL_STRING(": "));
 	  ProcessRawBytes(nssComponent, &policyQualifier->qualifierValue, text);
@@ -1545,7 +1591,7 @@ ProcessAuthInfoAccess(SECItem  *extData,
       nssComponent->GetPIPNSSBundleString("CertDumpCAIssuers", local);
       break;
     default:
-      rv = GetDefaultOIDFormat(&desc->method, local, '.');
+      rv = GetDefaultOIDFormat(&desc->method, nssComponent, local, '.');
       if (NS_FAILED(rv))
 	goto finish;
     }

security/manager/ssl/tests/mochitest/bugs/Makefile.in

@@ -47,6 +47,7 @@ include $(topsrcdir)/config/rules.mk
 
 _TEST_FILES = \
         test_bug480509.html \
+        test_bug483440.html \
         test_bug484111.html \
         $(NULL)
 

security/manager/ssl/tests/mochitest/bugs/test_bug483440.html

@@ -0,0 +1,65 @@
+<html>
+<head>
+  <title>Test bug 483437 and bug 480509</title>
+  <script type="text/javascript" src="chrome://mochikit/content/MochiKit/packed.js"></script>
+  <script type="text/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>        
+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css" />
+</head>
+<body>
+
+<script class="testbody" type="text/javascript">
+
+netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
+
+var certdb = Components.classes["@mozilla.org/security/x509certdb;1"]
+  .getService(Components.interfaces.nsIX509CertDB);
+
+function test(certNick, expected)
+{
+  var cert1 = certdb.findCertByNickname(null, certNick);
+  var certDumpTree1 = Components.classes["@mozilla.org/security/nsASN1Tree;1"]
+                     .createInstance(Components.interfaces.nsIASN1Tree);
+  certDumpTree1.loadASN1Structure(cert1.ASN1Structure);
+  var value1 = certDumpTree1.getDisplayData(9);
+  
+  is(value1, expected, "Incorrect OID recognized");
+}
+
+test("bug483440-attack2b", 
+     "Object Identifier (2 5 4 Unknown) = www.bank.com\n"+
+     "OU = Hacking Division\n"+
+     "CN = www.badguy.com\nO = Badguy Inc\n");
+
+test("bug483440-pk10oflo",
+     "Object Identifier (2 5 4 Unknown) = www.bank.com\n"+
+     "OU = Hacking Division\n"+
+     "CN = www.badguy.com\nO = Badguy Inc\n");
+
+test("bug483440-attack7",
+
+     // Check 88 80 80 80 01, not leading, have to pass
+     "Object Identifier (2 5 4 2147483649) = attack1\n"+
+     
+     // Check 90 80 80 80 01, not leading, have to fail
+     "Object Identifier (2 5 4 Unknown) = attack2\n"+
+     
+     // Check 80 80 80 80 80, not leading, have to fail
+     "Object Identifier (2 5 4 Unknown) = attack3\n"+
+     
+     // Check 81 81, trailing, have to fail
+     "Object Identifier (2 5 4 3 Unknown) = attack4\n"+
+     
+     // Check FF FF FF 7F, not leading, have to pass
+     "Object Identifier (2 5 4 268435455) = attack5\n"+
+     
+     // Check 80 leading, have to fail
+     "Object Identifier (Unknown 3) = attack6\n"+
+     
+     // Check 14757 = 2*40 + 14677 leading single byle encoded as F325, 
+     // have to pass
+     "Object Identifier (2 14677 4 3) = attack7\n");
+
+</script>
+
+</body>
+</html>