From 9a7c591b1f40c3bfaa3e1288b7582c6cfcf76a63 Mon Sep 17 00:00:00 2001 From: Honza Bambas Date: Wed, 29 Jul 2009 22:23:27 +0200 Subject: [PATCH] Bug 483440 - PSM doesn't detect invalid OID encodings in Cert Viewer Details tab, r=nelson+johnath --- build/pgo/certs/bug483440-attack2b.ca | 14 +++ build/pgo/certs/bug483440-attack7.ca | 16 ++++ build/pgo/certs/bug483440-pk10oflo.ca | 14 +++ security/manager/ssl/src/nsNSSCertHelper.cpp | 92 ++++++++++++++----- .../ssl/tests/mochitest/bugs/Makefile.in | 1 + .../tests/mochitest/bugs/test_bug483440.html | 65 +++++++++++++ 6 files changed, 179 insertions(+), 23 deletions(-) create mode 100644 build/pgo/certs/bug483440-attack2b.ca create mode 100644 build/pgo/certs/bug483440-attack7.ca create mode 100644 build/pgo/certs/bug483440-pk10oflo.ca create mode 100644 security/manager/ssl/tests/mochitest/bugs/test_bug483440.html diff --git a/build/pgo/certs/bug483440-attack2b.ca b/build/pgo/certs/bug483440-attack2b.ca new file mode 100644 index 000000000000..8adcf2ac1075 --- /dev/null +++ b/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----- diff --git a/build/pgo/certs/bug483440-attack7.ca b/build/pgo/certs/bug483440-attack7.ca new file mode 100644 index 000000000000..778e7acedd69 --- /dev/null +++ b/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----- diff --git a/build/pgo/certs/bug483440-pk10oflo.ca b/build/pgo/certs/bug483440-pk10oflo.ca new file mode 100644 index 000000000000..7daef524b23b --- /dev/null +++ b/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----- diff --git a/security/manager/ssl/src/nsNSSCertHelper.cpp b/security/manager/ssl/src/nsNSSCertHelper.cpp index 8d8d0c36f69e..d67c477577cf 100755 --- a/security/manager/ssl/src/nsNSSCertHelper.cpp +++ b/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_ 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, ¤t->name.OthName.name, value); } } else { - rv = GetDefaultOIDFormat(¤t->name.OthName.oid, key, ' '); + rv = GetDefaultOIDFormat(¤t->name.OthName.oid, nssComponent, key, ' '); if (NS_FAILED(rv)) goto finish; ProcessRawBytes(nssComponent, ¤t->name.OthName.name, value); @@ -1126,7 +1172,7 @@ ProcessGeneralName(PRArenaPool *arena, } case certRegisterID: nssComponent->GetPIPNSSBundleString("CertDumpRegisterID", key); - rv = GetDefaultOIDFormat(¤t->name.other, value, '.'); + rv = GetDefaultOIDFormat(¤t->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; } diff --git a/security/manager/ssl/tests/mochitest/bugs/Makefile.in b/security/manager/ssl/tests/mochitest/bugs/Makefile.in index 1d6b2305f76e..3188726b06aa 100644 --- a/security/manager/ssl/tests/mochitest/bugs/Makefile.in +++ b/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) diff --git a/security/manager/ssl/tests/mochitest/bugs/test_bug483440.html b/security/manager/ssl/tests/mochitest/bugs/test_bug483440.html new file mode 100644 index 000000000000..96035ce58a69 --- /dev/null +++ b/security/manager/ssl/tests/mochitest/bugs/test_bug483440.html @@ -0,0 +1,65 @@ + + + Test bug 483437 and bug 480509 + + + + + + + + + +