User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.2) Gecko/20040820 Debian/1.7.2-4 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.2) Gecko/20040820 Debian/1.7.2-4 This patch adds support for various extensions occurring in certificates, which are currently only displayed in their DER form. In particular, support is added for - various PKIX extensions (basic constraints, alternative names, CRL distribution points, AIA, extended key usage, key IDs, and certificate policies) - Netscape extensions (all extensions that have an IA5String as their value) - Microsoft extensions (certificate type, CA version, Principal name, Server GUID) - Verisign extensions (Verisign user notices) To support these changes, a number of changes have been made to NSS, and the existing dumping code: - OIDs can now be printed in the dotted form as well - a number of OID symbolic numbers have been added - a number of additional functions are now exported from NSS, for decoding well-known datatypes Reproducible: Always Steps to Reproduce: 1. Open certificate manager 2. navigate to any of the builtin CAs with extensions in their certificates 3. try reading the extension value Actual Results: It displays DER Expected Results: It should display legible text whereever possible. I hope I can attach the patch somehow.

I believe this patch fixes bug 230655, bug 127052, bug 161140.

marking NEW (has patch)

A bug with a patch. Hope some PSM reviewer can review it soon.

Making bugs mentioned by Martin dependent on this one.

Martin, thanks a lot for your patch. You have coded a lot of details! I do not have the time to play with your patch and test it, but if you are sure it works, I think we should get it into Mozilla. However, I would like you to do the following: - please test it with various certificates, but given the complexity of your patch, I'm sure you have already done so - please ask an NSS expert, either Nelson Bolyard or Bob Relyea, to briefly review your calls to NSS functions - please make sure you don't introduce NSS resource leaks with your new code. The best way to test is, after having executed your new code, to use the "Tools/Switch Profile" menu command, which requires a re-init of NSS, which only works if there are no NSS leaks. I'm giving you r=kaie for changes made in directory mozilla/manager which means you have "module owner approval" to check it in, once you have done the above. However, your patch also includes changes to NSS. I assume those changes are not yet contained in NSS. If your Mozilla UI patch depends on the NSS library changes, please file a separate bug on NSS, and attach a patch containing only your NSS changes, and make your bug dependent on the other one. We will need to wait for the NSS snapshot marked with NSS_CLIENT_TAG to contain your changed, before you can check in this one. And you will still need a usual Mozilla superreview. I'm assigning the bug to you, in the hope you are able and willing to drive the various actions required before checking in is possible. If you're not, please let us know. Thanks!

Comment on attachment 158682 [details] [diff] [review] Proposed patch r=kaie - provided that you do what I suggested in comment 6

Any NSS changes need to be in a separate patch. checkin access to NSS is administered separately, and only an NSS developer can checkin to NSS, presently. There are several issues with the proposed NSS changes in the patch. Let me explain several. 1. This patch appears to me to be based on NSS 3.9.x which is now over a year old. No more NSS 3.9.x releases are planned. NSS 3.10 is the next planned release. Some of the work shown in this patch is already done in NSS 3.10. 2. In NSS 3.10, there is a way for an application to dynamically extend the OID table at run time. This allows applications to add to the known OIDs without every application burdening NSS with its OIDs. So, the proper thing for mozilla to do is to use that API to add OIDs at run time, rather than adding them to the NSS sources. You can find an example of code that does this very thing at http://lxr.mozilla.org/security/source/security/nss/cmd/lib/moreoids.c 3. Obviously if the PSM changes depend on the proposed NSS changes, then the PSM patch will have to wait until NSS 3.10 is released (or until moz/FF/TB/switch to the NSS 3.10 sources) before it can be checked in.

I'm willing to drive the changes necessary. AFAICT, it looks like this: the patch needs changes to NSS 3.9, which are unlikely to be accepted anymore. So I need to update my patch for NSS 3.10, which then means that integration of the rest of the patch needs to be delayed until some? all? PSM applications have switched to NSS 3.10. I'm uncertain about the OID changes: are you suggesting that I should avoid adding more OID constants to NSS, or that I should add them to moreoids.c, or that adding them to the constant list is still fine? I likely need to change NSS anyway, to support decoding more extension types.

Martin, Thanks for your willingnews to contribute! moreoids.c is code from a command line application that is part of the NSS test/QA suite, but is not part of the NSS shared libraries. I'm suggesting that you use it as an example of what mozilla PSM code should do. I am not suggesting that you add oids to NSS source code. I am suggesting that you add oids in PSM source code, using the method demonstrated in moreoids.c I doubt that you need to add any code to NSS 3.10. NSS does not need to contain templates and functions to parse every known cert extension. It only needs code to parse the extensions that are relevant to cert validity checking or other cert processing done by NSS itself. If applications want to process additional extensions, NSS provides the functions by which they may do so themselves. The ASN.1 DER and BER decoders are publicly callable, and work based on "templates" that describe the ASN.1 syntax, and the related structure. Application are free to create their own templates. The new templates need not be in NSS. If you're adding templates to parse *standard* extensions, I think that would be welcome in NSS shared libs. But the kinds of (mostly proprietary) extensions you see in moreoids.c are example of extensions that belong in applciations, not in NSS, IMO. The NSS development team maintains a CVS tag named "NSS_CLIENT_TAG" that the mozilla applications use to pull NSS for their trunk development. Presently it is on the NSS 3.9 branch. But at some point, hopefully before too long, it will move to the NSS 3.10 sources. At that point, the mozilla applications that pull NSS_CLIENT_TAG will all switch to 3.10.

I have checked NSS 3.10, and I think these changes would be needed: - add CERT_DestroyUserNotice to nss.def - add SEC_OID_ACCESS_DESCR_CA_ISSUERS as a OID constant, (this is a standard OID from PKIX 3) I'm going to submit this as a patch to NSS, and update the PSM patch to use NSS 3.10.

(In reply to comment #11) > I have checked NSS 3.10, and I think these changes would be needed: > - add CERT_DestroyUserNotice to nss.def > - add SEC_OID_ACCESS_DESCR_CA_ISSUERS as a OID constant, (this is a standard > OID from PKIX 3) Sounds reasonable to me. Please file a separate NSS bug about those, and mark this bug as deepending on that one. Please cc me on that bug. Thanks.

I've updated the patch to rely on the two NSS features for which I've submitted bug reports. I have tested the patch with various real-world certificates that I could find. To allows others to reproduce the tests, I could try to come up with a synthetic certificate that demonstrates all the extensions (although one may need a set of certificates to cover all CHOICEs). If it is desired that I create such a thing, please also indicate whether I should try to do so through NSS or openssh. Notice that neither package naturally supports all extensions in newly-created certificates, so I would have to escape to plain DER bytes, in which case there is no guarantee that the resulting test certificate would really comply with the standards.

In reply to comment 13, create the test certs with any tool you like, as long as the results are properly encoded standards-comforming certs. The DER-encoded (and perhaps Base64 encoded) certs themselves, not the scripts/tools that create them, should be attached to the bug and eventually become part of the QA test suite.

Martin, both NSS features that you had requested were implemented in NSS 3.10 and are now part of the mozilla nightly builds. So, please feel free to continue the work on this bug.

I have now updated the patch to NSS 3.10. Working on the demo certificate will take more time.

Note on the attachment label: none of the changes made are to NSS, so it's a patch to PSM, not NSS (NSS is the code that lives under mozilla/security/nss and is shared between clients and servers). This note should not be construed as some sort of rejection of the patch (I have not reviewed it). The patch is made to exactly the code that it should be made to (PSM, or the code under mozilla/security/manager).

Sorry for the misunderstanding. I meant to say it is a patch *based on* NSS 3.10, not a patch *to* NSS 3.10. I.e. the patch requires features only present in NSS 3.10 - it is a patch *to* the PSM.

Update to the patch, tested with test certificate. This adds a few more certificate usages (as present in the test certificate), and deals more gracefully with a failure to decode the user notice.

This is a test certificate demonstrating the patch. It features all extensions that OpenSSL SNAP-20050405 is capable of generating. The patch can display all of them, except for the UserNotice in the policy. There is an obvious bug in NSS 3.10's DecodeUserNotice (which assumes that the organization is IA5String, but it does not need to be). However, in the test certificate, this bug is worked-around, and it still fails to decode the certificate for some reason; I was not able to figure out where the problem is. I believe the patch deals with this case gracefully, by adding some alternative text and cleaning up what can be cleaned.

Martin, if you haven't already, please file an NSS bug on the DecodeUserNotice issues. Thanks.

I hadn't done this before, but it now is bug #289823.

Comment on attachment 180313 [details] [diff] [review] Patch v4 r+ relyea.

Comment on attachment 180313 [details] [diff] [review] Patch v4 For this amount of code, you should put your name in the Contributors section in the license header in the files you modified. Bob Relyea is not a Mozilla super reviewer, so you need to get a super review from someone else. They are listed on http://www.mozilla.org/hacking/reviewers.html. If you don't know any of them, I suggest dveditz@cruzio.com or bzbarsky@mit.edu.

Comment on attachment 180313 [details] [diff] [review] Patch v4 that should be r+ relyea, not sr+ relyea ;)

patch v4 containa a comment that says, in part: + /* XXX: currently, polcyxtn.c assumes that organization + is an IA5String, whereas it really ought to be a + choice of VisibleString, BMPString, and UTF8String. RFC 3280 says The DirectoryString type is defined as a choice of PrintableString, TeletexString, BMPString, UTF8String, and UniversalString. The UTF8String encoding [RFC 2279] is the preferred encoding, and all certificates issued after December 31, 2003 MUST use the UTF8String encoding of DirectoryString (except as noted below).

One more point regarding the above comment. NSS functions for parsing cert components convert all of the directory string types to UTF8 internally, IINM. The values returned by the various CERT_ function that return cert components all return strings that have been converted to UTF8, I believe.

It doesn't seem really optimal to have in ProcessExtKeyUsage a separate case for each possible KeyUsage in the code. Wouldn't it be better to generate the bundlekeys based on the OID ? That would shorten the code, and only require to change the ressource to add new OIDs. bundlekeys would be less explicit, but does it matter ? AppendBMPtoUTF16 : /* XXX instead of converting to and from UTF-8, it would be sufficient to just swap bytes, or do nothing */ This part introduces a call to PORT_UCS2_UTF8Conversion from security/manager. It never happens elsewhere. This is a hint of something wrong, I think. I believe everywhere else Mozilla interfaces with NSS at a level where it receives already decoded UTF-8 string. In this patch, it goes much lower. The only problem with that is that it incorporates inside Mozilla code that could be useful for other NSS users. Maybe part of that code could be part of NSS in a manner similar to the CRMF code, that is not included in NSS3 or another shared library, but in a separate static library that anyone who really needs that part can incorporate.

(In reply to comment #26) > RFC 3280 says > > The DirectoryString type However, the organization is not a DirectoryString, it is a DisplayText DisplayText ::= CHOICE { ia5String IA5String (SIZE (1..200)), visibleString VisibleString (SIZE (1..200)), bmpString BMPString (SIZE (1..200)), utf8String UTF8String (SIZE (1..200)) } The alternative of IA5String is new in RFC 3280 (was not in RFC 2459); apparently in response to the common misunderstanding what precisely goes into the organization field.

(In reply to comment #28) > AppendBMPtoUTF16 : > /* XXX instead of converting to and from UTF-8, it would > be sufficient to just swap bytes, or do nothing */ > > This part introduces a call to PORT_UCS2_UTF8Conversion from security/manager. > It never happens elsewhere. This is a hint of something wrong, I think. I think it is not surprising that this not used elsewhere: BMPString is an uncommon type. It is only needed in two places in the patch, in one alternative of DisplayText (which typically will contain IA5String, so the code is not executed), and in Microsoft certificate extensions. So I think reuse is unlikely, and the conversion via UTF-8 is used so infrequently that the slight performance hit will not matter much in practice.

(In reply to comment #28) > Wouldn't it be better to generate the bundlekeys based on the OID ? That would > shorten the code, and only require to change the ressource to add new OIDs. > bundlekeys would be less explicit, but does it matter ? I don't know how to do that: what procedure should be used to generate them, and how would that integrate with the rest of the properties file? I have no idea how mozilla localization works in the first place - the workload for translators would not change at all, would it?

Comment #31 : In fact, it's the code : switch (oidTag) { case SEC_OID_EXT_KEY_USAGE_SERVER_AUTH: bundlekey = "CertDumpServerAuth"; break; case XXX : [... repeat for n identifier ] default: if (oidTag == SEC_OID(MS_KEY_USAGE_CODE_IND)) { bundlekey = "CertDumpMSCodeInd"; break;[...] if (oidTag == SEC_OID(XXX)) [... repeat for n identifier ] if (bundlekey) { rv = nssComponent->GetPIPNSSBundleString(bundlekey, local); } which I find really ugly. It's a lot of code to convert OID values to a bundlekey which *only* use is to get the real description from the properties file. And the more value we support, the more it grows. Adding a new value means both modifying that and pipnss.properties. I think it would like it better to replace it all with something like (beware, the string usage might be bad) : nsAutoString bundleref; rv = GetDefaultOIDFormat(oid, bundleref, true); if (NS_FAILED(rv)) return rv; nsAutoString bundlekey = NS_LITERAL_STRING("CertDumpEKU_") + bundleref; rv = nssComponent->GetPIPNSSBundleString(bundlekey, local); if (NS_FAILED(rv)) { rv = GetDefaultOIDFormat(oid, local, false); } Just by renaming the entries adequately, we'd get rid of around 40 lines of code, and we'd be able to support future values without writing code. pipnss.properties would grow, but it's compressed. A similar thing could be done at some other places in the code, maybe with less gain.

(In reply to comment #30) BMPString is rarely actually used, but there are other cases in X509 where it's a possible case in a choice, so there ought to be some more case of use. My main point is that PSM usually interfaces with NSS at a level where it can get already decoded utf-8 string. The reason it doesn't happen here is that we are implementing the knowledge about how to decode extensions inside PSM instead of inside NSS. This is a bad choice because those extensions are general usage and other product than PSM will certainly be interested in them, *but* it was already done this way before your patch. It would look cleaner to me to interface at the UTF-8 level, CERT_DecodeAVAValue could be adequate, except that it accepts several types in addition to BMP. Else change the comment to document that PORT_UCS2_UTF8Conversion takes UCS2LE as input, whereas the usual Mozilla type for UTF16 has a platform dependant byte-order, so can not be used to handle that input directly and we need the intermediate UTF-8 conversion.

(In reply to comment #33) > This is a bad choice because those extensions are general usage > and other product than PSM will certainly be interested in > them, *but* it was already done this way before your patch. It seems there is disagreement wrt. this view. I originally proposed to add more extensions to NSS, but in comment #10, I was specifically advised not to, and do everything in PSM unless NSS has already support for it. > Else change the comment to document that PORT_UCS2_UTF8Conversion takes UCS2LE > as input, whereas the usual Mozilla type for UTF16 has a platform dependant > byte-order, so can not be used to handle that input directly and we need the > intermediate UTF-8 conversion. But that is not the point of the XXX comment. The comment complains about the lack for a UCS2LE conversion function in Mozilla, so that the only way to do the conversion (short of manually swapping bytes) is to go via UTF-8. So if a BMPString-to-nsAString conversion function is ever introduced, the code should be changed to use it. Notice that CERT_DecodeAVAValue also does PORT_UCS2_UTF8Conversion, so by calling this function, I would merely hide the fact that I'm going back and forth to UTF-8, the "problem" would persist.

In reply to comment 33 : > [...] change the comment to document that PORT_UCS2_UTF8Conversion takes > UCS2LE as input, Actually, it takes Big Endian UCS2 as input. Whenever UCS2 appears in a cert, it is always big endian, regardless of the local CPU's preferred byte order.

> Notice that CERT_DecodeAVAValue also does PORT_UCS2_UTF8Conversion, > so by calling this function, I would merely hide the fact that I'm > going back and forth to UTF-8, the "problem" would persist. Maybe it's not necessary to discuss it much further, but the problem for me is not the conversion, we're already doing it everywhere PSM interfaces with NSS. It's the fact the reason for the conversion is not perfectly clear if you don't document that BMPString is UCS2BE. Everywhere else the two conventions never mix in the same product, because only NSS calls PORT_UCS2* with UCS2BE, and mozilla/PSM always uses it's own functions with platform dependent byte order. > It seems there is disagreement wrt. this view. I originally > proposed to add more extensions to NSS, but in comment #10, > I was specifically advised not to, and do everything in PSM > unless NSS has already support for it. The disagreement is not as direct as it seems. I agree with Nelson there's no reason to include this code in the NSS shared libraries. I thought about putting it in a static library, like the CRMF code. This way it wouldn't grow NSS for those who don't use it. But the final decision is Nelson's or Relyea/WTC's.

Comment on attachment 180313 [details] [diff] [review] Patch v4 Dan, please review. Thanks!

This revision (v5) implements the remaining suggestions: I added myself to the contributor's list, and I changed the rendering of extended usages to generate bundle identifiers (of the form CertDumpEKU_1_3_6_1_5_5_7_3_1), so that new key usages can be added without changing the code.

This would be really nice to get into aviary1.5. (though it does and some i18n strings). bob

Missed l10n window for 1.8 branch. Also not a regression since 1.0.x so should deploy on trunk and not on 1.8 branch.

I just compared patch revisions 4 and 5. I am fine with the changes between those versions but have the following comments, I'm just asking you check again the code is what you are intending. In the following code, in the earlier patch reviewed by Bob, you had a true value in the call to GetDefaultOIDFormat, and in all the other places, you changed such arguments to '.'. So I wonder: are you sure you want a ' ' argument here? + "{%.2x%.2x%.2x%.2x-%.2x%.2x-%.2x%.2x-%.2x%.2x-%.2x%.2x%.2x%.2x%.2x%.2x}", + d[3], d[2], d[1], d[0], d[5], d[4], d[7], d[6], + d[8], d[9], d[10], d[11], d[12], d[13], d[14], d[15]); + value.AssignASCII(buf); + } else { + ProcessRawBytes(&current->name.OthName.name, value); + } + } else { + rv = GetDefaultOIDFormat(&current->name.OthName.oid, key, ' '); if (NS_FAILED(rv)) - return rv; + goto finish; + ProcessRawBytes(&current->name.OthName.name, value); + } + break; + } + case certRFC822Name: + nssComponent->GetPIPNSSBundleString("CertDumpRFC822Name", key); + value.AssignASCII((char*)current->name.other.data, current->name.other.len); + break; + case certDNSName: I assume that separator has a value of either space or a dot character. In the following code, are you really intending to introduce a space? (because in your patch reviewed by Bob Relyea, you either added a dot, or no character at all. @@ -186,7 +225,8 @@ val = (val << 7) | (j & 0x7f); if (j & 0x80) continue; - written = PR_snprintf(&buf[len], sizeof(buf)-len, "%lu ", val); + written = PR_snprintf(&buf[len], sizeof(buf)-len, "%c%lu", + separator, val); if (written < 0) return NS_ERROR_FAILURE;

Of course, I'd be glad to see Dan review that code, too, but: - you already have r=kaie on patch v1 - you already have r=rrelyea on patch v4 After you have addressed my previous comment regarding differences between revisions 4 and 5, you have r=kaie If you think somebody should review the new OID details in patch v5, you could ask Bob to diff patch v4 against v5 and confirm these additional changes are ok. After that, all you need is a superreview.

(In reply to comment #41) > In the following code, in the earlier patch reviewed by Bob, you had a true > value in the call to GetDefaultOIDFormat, and in all the other places, you > changed such arguments to '.'. So I wonder: are you sure you want a ' ' > argument here? It's a minor display issue: the "true ASN.1 way" uses spaces; I'm uncertain where the dotted notation originates from. It doesn't matter much either way. In the ASN.1 syntax, the identifiers are also written in curly braces. So it could be changed back to a dot, or the curly braces could be added, or it could be left as-is: the user will be able to read it in any notation. > I assume that separator has a value of either space or a dot character. > In the following code, are you really intending to introduce a space? (because > in your patch reviewed by Bob Relyea, you either added a dot, or no > character at all. In v4, I either added the dot before, or the space after each arc of the OID. This goes back to the original code, which always added a space after each arc. In v5, I consistently add the separator before each arc, which works nicely because the first two arcs are handled separately, anyway. This entire change was necessary to support a third separator, _, which is only used in ProcessExtKeyUsage (to denote resource names there).

The dotted decimal notation for OIDs comes from RFCs 1485, 1779 and 2253. But I think we should not let the choice of space or dot stand in the way of this patch, if that is all that remains to be resolved.

Comment on attachment 192140 [details] [diff] [review] Patch v5 Kai, Simon, could you review this very useful patch that improves our certificate display? Thanks.

Comment on attachment 192140 [details] [diff] [review] Patch v5 Thanks for the explanation, sounds good to me. As I said in comment 42: r=kaie

Comment on attachment 192140 [details] [diff] [review] Patch v5 sr=shaver, though it would nbe good to get bugs on file for the new XXX comments (the different string possibilities, the excessive UTF8 transcoding, etc.)

Comment on attachment 192140 [details] [diff] [review] Patch v5 >Index: ssl/src/nsNSSCertHelper.cpp >=================================================================== >+static SECOidData more_oids[] = { Make this data const (as well as any existing static data tables) if you can.

> >+static SECOidData more_oids[] = { > Make this data const if you can. Unfortunately we can not, because there is a dynamic registration process that will set a struct member to a value determined at runtime.

Patch checked in. Martin, thanks a lot for your contribution.

Reopening, because this patch does not build on Windows. It fails trying to compile IP v6 code. Build Error Summary c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(845) : error C2065: 'INET6_ADDRSTRLEN' : undeclared identifier c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(845) : error C2057: expected constant expression c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(845) : error C2466: cannot allocate an array of constant size 0 c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(845) : error C2133: 'buf' : unknown size c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(848) : error C2065: 'inet_ntop' : undeclared identifier c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(848) : error C2065: 'AF_INET' : undeclared identifier c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(848) : error C2070: illegal sizeof operand c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(850) : error C2065: 'AF_INET6' : undeclared identifier c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(850) : error C2070: illegal sizeof operand

I do not have a Windows development system at the moment. Who can help to adjust the patch to work on Windows?

I've got a Windows build system (MSVC2003). I can test out any future patches if you want.

Comment on attachment 192140 [details] [diff] [review] Patch v5 Kai, Please try the following code: + case certIPAddress: + { + char buf[128]; PRNetAddr addr; + nssComponent->GetPIPNSSBundleString("CertDumpIPAddress", key); + if (current->name.other.len == 4) { addr.inet.family = PR_AF_INET; memcpy(&addr.inet.ip, current->name.other.data, 4); PR_NetAddrToString(&addr, buf, sizeof(buf)); + } else if (current->name.other.len == 16) { addr.ipv6.family = PR_AF_INET6; memcpy(&addr.ipv6.ip, current->name.other.data, 16); PR_NetAddrToString(&addr, buf, sizeof(buf)); + } else + PL_strncpyz(buf, "Invalid IP address", sizeof(buf)); + value.AssignASCII(buf); + break; + } You can replace 128 by the value of INET6_ADDRSTRLEN. Note that inet_ntop doesn't exist on Windows. The "Invalid IP address" is an internationalization bug. You may want to replace it by something like "***" or "???" (if the latter, watch out for "trigraph" problems).

As for the "Invalid IP Address" case: It shouldn't happen in a "correct" certificate; if it does happen, we probably can just display the raw bytes: case certIPAddress: { char buf[128]; PRNetAddr addr; nssComponent->GetPIPNSSBundleString("CertDumpIPAddress", key); if ((current->name.other.len == 4) || (current->name.other.len == 16)) addr.inet.family = (current->name.other.len == 4) ? PR_AF_INET : PR_AF_INET6; memcpy(&addr.inet.ip, current->name.other.data, current->name.other.len); PR_NetAddrToString(&addr, buf, sizeof(buf)); value.AssignASCII(buf); } else /* invalid IP address */ ProcessRawBytes(&current->name.other, value); break; }

Martin: good idea. Displaying the raw bytes is more useful than a (localized) error message in this case. Kai, if you use Martin's code snippet, please change addr.inet.family to addr.raw.family, and use &addr.ipv6.ip for the IPv6 case because inet.ip and ipv6.ip have different offsets in the PRNetAddr union.

This is the combination of Wan-Teh's code with Martin's proposal for the "invalid IP" case, with my added definition of the fallback INET6_ADDSTRLEN definition. Does that look good? +#ifndef INET6_ADDRSTRLEN +#define INET6_ADDRSTRLEN 46 +#endif ... + case certIPAddress: + { + char buf[INET6_ADDRSTRLEN]; + PRNetAddr addr; + nssComponent->GetPIPNSSBundleString("CertDumpIPAddress", key); + if (current->name.other.len == 4) { + addr.inet.family = PR_AF_INET; + memcpy(&addr.inet.ip, current->name.other.data, 4); + PR_NetAddrToString(&addr, buf, sizeof(buf)); + } else if (current->name.other.len == 16) { + addr.ipv6.family = PR_AF_INET6; + memcpy(&addr.ipv6.ip, current->name.other.data, 16); + PR_NetAddrToString(&addr, buf, sizeof(buf)); + value.AssignASCII(buf); + } else { + /* invalid IP address */ + ProcessRawBytes(&current->name.other, value); + } + break; + }

Kai, value.AssignASCII(buf) also needs to be called in the IPv4 (PR_AF_INET) case.

Here is an updated version of patch v5, it has the following differences: added: +#ifndef INET6_ADDRSTRLEN +#define INET6_ADDRSTRLEN 46 +#endif certIPAddress section is now: + case certIPAddress: + { + char buf[INET6_ADDRSTRLEN]; + PRNetAddr addr; + nssComponent->GetPIPNSSBundleString("CertDumpIPAddress", key); + if (current->name.other.len == 4) { + addr.inet.family = PR_AF_INET; + memcpy(&addr.inet.ip, current->name.other.data, 4); + PR_NetAddrToString(&addr, buf, sizeof(buf)); + } else if (current->name.other.len == 16) { + addr.ipv6.family = PR_AF_INET6; + memcpy(&addr.ipv6.ip, current->name.other.data, 16); + PR_NetAddrToString(&addr, buf, sizeof(buf)); + value.AssignASCII(buf); + } else { + /* invalid IP address */ + ProcessRawBytes(&current->name.other, value); + } + break; + }

Ryan, thanks a lot for trying to compile this patch on Windows! Unfortunately, the build still failed. Patch v5 b has been created 10 days ago. It took me until today to look at the reported error in detail: nsBrowserApp.obj : warning LNK4217: locally defined symbol _XRE_main imported in function _main pipnss.lib(nsNSSCertHelper.obj) : error LNK2019: unresolved external symbol _SEC_BMPStringTemplate referenced in function "unsigned int __cdecl ProcessBMPString(struct SECItemStr *,class nsAString_internal &,class nsINSSComponent *)" (?ProcessBMPString@@YAIPAUSECItemStr@@AAVnsAString_internal@@PAVnsINSSComponent@@@Z) pipnss.lib(nsNSSCertHelper.obj) : error LNK2019: unresolved external symbol _SEC_IA5StringTemplate referenced in function "unsigned int __cdecl ProcessIA5String(struct SECItemStr *,class nsAString_internal &,class nsINSSComponent *)" (?ProcessIA5String@@YAIPAUSECItemStr@@AAVnsAString_internal@@PAVnsINSSComponent@@@Z) pipnss.lib(nsNSSCertHelper.obj) : error LNK2019: unresolved external symbol _SEC_OctetStringTemplate referenced in function "unsigned int __cdecl ProcessSubjectKeyId(struct SECItemStr *,class nsAString_internal &,class nsINSSComponent *)" (?ProcessSubjectKeyId@@YAIPAUSECItemStr@@AAVnsAString_internal@@PAVnsINSSComponent@@@Z) firefox.exe : fatal error LNK1120: 3 unresolved externals It seems these data symbols are not exported on Windows, the nss.def file says: ;+# Data objects ;+# Don't export these DATA symbols on Windows because they don't work right. ;;SEC_BMPStringTemplate DATA ; ;;SEC_IA5StringTemplate DATA ; ;;SEC_OctetStringTemplate DATA ;

When exporting symbols from a shared library for use in another shared lib or in a program, Windows works the same as unix for function names, but not for data. On unix, the exported symbol has the value of the address of the named item, but in Windows, the exported symbol has the value of the address of a pointer to the named item. NSS has devised a set of macros to solve this issue for ASN1 templates, so that a single piece of source code can be written that works on both Unix and Windows. When attempting to access an ASN1 template as a data symbol from a (another) shared lib, code should use the NSS macros SEC_ASN1_MKSUB, SEC_ASN1_SUB, SEC_ASN1_GET, and in templates should also use SEC_ASN1_XTRN. That code will then work properly on both Windows and Unix. The macros expand on Unix to the usual symbols, and on Windows they expand to pointer to functions in the remote shared lib that returns the desired address. There are MANY examples of the use of these macros in http://lxr.mozilla.org/security/source/security/nss/lib/smime/cmsasn1.c Search that file for SEC_PointerToOctetStringTemplate to see the proper use of those macros. You will also find use of some of those macros for the 3 templates named in the preceeding comment. Essentially,, the rules are: a) in a template, instead of referring to the remote template directly, use the macro SEC_ASN1_SUB to reference it, and then add SEC_ASN1_XTRN to the "kind" member of the template. See an example of this at http://lxr.mozilla.org/security/source/security/nss/lib/smime/cmsasn1.c#116 Then for each symbol used with SEC_ASN1_SUB, you need a single declaration of an accessor function, using the macro SEC_ASN1_MKSUB, as shown at http://lxr.mozilla.org/security/source/security/nss/lib/smime/cmsasn1.c#60 b) in a function, instead of referring directly to the remote template, code should use the macro SEC_ASN1_GET, as shown at http://lxr.mozilla.org/security/source/security/nss/lib/smime/cmsasn1.c#562 The documentation for those macros is in this page: http://www.mozilla.org/projects/security/pki/nss/release_notes_32.html#What's

Comment on attachment 205942 [details] [diff] [review] Patch v5 b >+ case certIPAddress: >+ { >+ char buf[INET6_ADDRSTRLEN]; >+ PRNetAddr addr; >+ nssComponent->GetPIPNSSBundleString("CertDumpIPAddress", key); >+ if (current->name.other.len == 4) { >+ addr.inet.family = PR_AF_INET; >+ memcpy(&addr.inet.ip, current->name.other.data, 4); >+ PR_NetAddrToString(&addr, buf, sizeof(buf)); >+ } else if (current->name.other.len == 16) { >+ addr.ipv6.family = PR_AF_INET6; >+ memcpy(&addr.ipv6.ip, current->name.other.data, 16); >+ PR_NetAddrToString(&addr, buf, sizeof(buf)); >+ value.AssignASCII(buf); >+ } else { >+ /* invalid IP address */ >+ ProcessRawBytes(&current->name.other, value); >+ } >+ break; >+ } You also need the "value.AssignASCII(buf);" statement in the PR_AF_INET case.

Nelson, thanks a lot for your explanation. This new patch now uses macro SEC_ASN1_GET. > You also need the "value.AssignASCII(buf);" statement in > the PR_AF_INET case. Thanks Wan-Teh, sorry that you had to say that twice.

Comment on attachment 206407 [details] [diff] [review] Patch v6 r=wtc. As a matter of style, please use current->name.other.len instead of 4 and 16 in the following two lines: >+ memcpy(&addr.inet.ip, current->name.other.data, 4); >+ memcpy(&addr.ipv6.ip, current->name.other.data, 16);

This patch has Wan-Teh's requested change. carrying forward r=wtc Mike, could you please review again? Here are the differences between the patch you had already reviewed and the current one: - added: +#ifndef INET6_ADDRSTRLEN +#define INET6_ADDRSTRLEN 46 +#endif - wrapped three variables with an accessor function call: SEC_ASN1_GET - changed one code section to this: + case certIPAddress: + { + char buf[INET6_ADDRSTRLEN]; + PRNetAddr addr; + nssComponent->GetPIPNSSBundleString("CertDumpIPAddress", key); + if (current->name.other.len == 4) { + addr.inet.family = PR_AF_INET; + memcpy(&addr.inet.ip, current->name.other.data, current->name.other.len); + PR_NetAddrToString(&addr, buf, sizeof(buf)); + value.AssignASCII(buf); + } else if (current->name.other.len == 16) { + addr.ipv6.family = PR_AF_INET6; + memcpy(&addr.ipv6.ip, current->name.other.data, current->name.other.len); + PR_NetAddrToString(&addr, buf, sizeof(buf)); + value.AssignASCII(buf); + } else { + /* invalid IP address */ + ProcessRawBytes(&current->name.other, value); + } + break; + }

Comment on attachment 206410 [details] [diff] [review] Patch v7 Bob, can you please review the new snippets in this patch (see previous comment).

Comment on attachment 206410 [details] [diff] [review] Patch v7 r+ on the diff from v6 to v7

checked in to trunk, second attempt

A very useful patch - I've been badly missing this feature in Firefox/Thunderbird for quite some time, so I really appreciate it has now been committed to trunk. Can I propose a patch to the patch (attached to this comment)? It includes the following fixes/improvements: 1) basic constraints extension: if the CA flag is false (i.e. in an end-entity certificate), then there can't be a pathLenConstraint value. However, ProcessBasicConstraints() currently doesn't take this into account and will display bogus data for the pathLenConstraint (value.pathLenConstraint never gets initialized properly - look at the certificate from https://www.verisign.com to see what I mean - it'll say something like "Maximum number of intermediate CAs: 36514320"). Additionally, I've added support for the case where the path length is unlimited. 2) authority information access: support for the OCSP responder OID added; fixed bug that information about the criticality gets overwritten inadvertently; removes unnecessary line breaks (which get added by ProcessGeneralName() already) 3) extended key usage: in addition to text description, also display the OID afterwards (in parentheses) 4) adds support for a bunch of (Microsoft) Extended Key Usage OIDs (mostly from http://support.microsoft.com/default.aspx?scid=kb;en-us;287547) 5) adds basic recognition of the PKIX Logotype extension (RFC 3709) - no, it won't parse the contents of this rather quirky extension... but at least you know what it refers to when you encounter it in certificates from Verisign (take https://www.amazon.com as an example) 6) minor wording changes ("Server Gated Crypto" instead of "Strong Crypto Approved" etc.) Thanks for considering these suggested changes (I hope my code is not too ugly/buggy).

I'm not entirely sure what the procedures are, but, as the original submitter of the patch, I would hope that the status of that patch stays at "FIXED" - it took long enough to get into that state. So I would suggest the right procedure is to submit this patch as a separate, new item (I'm sure Mozilla folks will correct me if I'm wrong here). Conceptually, I see three kinds of changes in the patch, each of which should be discussed separately (and which might get accepted/rejected separately): - bug fixes/corrections - additions - message changes With additions (like the logo extensions), I have no issues: I expected more and more additions to be contributed over time. With the message changes, I think great care is necessary; gratuitous renamings should be avoided. Keep in mind that these are i18n changes: every message you change will have to be translated, so there should be a really good reason to change it. One reason would be that you found the "official" name of some extension somewhere; clearly, things should be named by their official name. As for the Microsoft extensions, I think I took most of the names from some MS KB article, so if you found something more official elsewhere, I would like to know. For example, why is "Microsoft Time Stamping" better than "Microsoft Time Stamp Signing"? As for the BasicConstraints fix: this should be done (regardless of the rest of the patch). I think it still isn't completely right, though, partly due to a problem in NSS: It is *conforming* to have cA set to FALSE, yet pathLenConstraint present. 4.2.1.10 just says that pathLenConstraint won't be meaningful in that case, but does not forbid that (through a MUST NOT specification). Unfortunately, NSS doesn't provide a CERT_PATH_CONSTRAINT_ABSENT value, instead, it will report an error in such a case.

Comment on attachment 208551 [details] [diff] [review] Proposed patch to the patch This is not a full review, but just some comments. 1. I like all the changes you've proposed to make here. 2. I wouldn't worry too much about the L10n implications of the changes to existing strings. It's OK if there's a minor change to the English string and no corresponding changes to the other languages immediately. 3. Adding new strings that must be localized *is* an issue, and may affect whether this patch can go into this next release or must wait until a later one. If the L10n efforts on the prior patch have not yet begun, then I'd say we want to get these latest changes in ASAP. As for whether a new bug is needed, I think that mostly depends on whether the new patch is going to get into the same release as the previous one. If so, then I think one bug can serve both, but if not then the new patch needs a new bug. It wouldn't hurt for the new patch to get a new bug of its own anyway. Kai, please advise us about this. Are you willing to champion this patch into PSM rather immediately? Is it too late for this release? Also, In response to Martin's observation about NSS's parsing of BasicConstraints extensions, I have filed bug 323557 and attached a patch thereto. Martin, I invite your review comments on that patch, and please attach a sample cert to it. Thanks.

One other thought about all the new OIDs now recognized by cert manager. Many of these new OIDs are not actually used by mozilla/FF/TB/SM/NSS/PSM. Having the OID names decoded in cert manager may set the user's expectation that the extensions (or extended usages) will be recognized/honored, when they actually will not. So, I might suggest adding some indicator to the OID name strings for OIDs not actually used by mozilla, indicating that fact. Perhaps an asterisk "*" at the end of each such string.

> As for the Microsoft extensions, I think I took most of the names from some > MS KB article, so if you found something more official elsewhere, I would > like to know. For example, why is "Microsoft Time Stamping" better than > "Microsoft Time Stamp Signing"? I wouldn't say it's "better", but where available I took the wording from an English version of the operating system itself (i.e., the Windows GUI will display "Microsoft Time Stamping"). It's possible that I didn't catch all of them, but I can look into it again if that helps (e.g., 1.3.6.1.4.1.311.10.3.4.1 would just be "File Recovery" then, not "Encrypting Filesystem Recovery Agent"). > As for the BasicConstraints fix: this should be done (regardless of the rest > of the patch). I think it still isn't completely right, though, partly due to > a problem in NSS: It is *conforming* to have cA set to FALSE, yet > pathLenConstraint present. 4.2.1.10 just says that pathLenConstraint won't be > meaningful in that case, but does not forbid that (through a MUST NOT > specification). Unfortunately, NSS doesn't provide a > CERT_PATH_CONSTRAINT_ABSENT value, instead, it will report an error in such a > case. Correct, that's why I added this to a later version of the patch, in order to fail more gracefully when CERT_DecodeBasicConstraintValue fails (the "Details" tab will remain blank otherwise): @@ -558,8 +569,8 @@ rv = CERT_DecodeBasicConstraintValue (&value, extData); if (rv != SECSuccess) { - NS_ASSERTION(0,"Could not decode basic constraints"); - return NS_ERROR_FAILURE; + ProcessRawBytes(extData, text); + return NS_OK; } if (value.isCA) rv2 = nssComponent->GetPIPNSSBundleString("CertDumpIsCA", local); Depending on how bug 323557 is fixed, this might no longer be necessary, though.

(In reply to comment #72) Is it really likely that users will have that expectation? For most of the extensions, there isn't any meaningful action the browser could take (e.g. Netscape certificate comment). Should it display the extension as supported or unsupported? Also, for some extensions (in particular "extended key usage") it might be that the browser supports some values, but not others. Again, for each key usage, it might be that there is no meaningful action the browser could take. For other extensions, whether or not it is used is a configuration setting, e.g. OCSP. Or it could depend on the specific tool (browser vs. mail reader) whether the extension is used. So I would expect such an asterisk is more confusing to the user than helpful.

(In reply to comment #73) > I wouldn't say it's "better", but where available I took the wording from an > English version of the operating system itself (i.e., the Windows GUI will > display "Microsoft Time Stamping"). Ah, that would be better indeed (hoping that Microsoft doesn't reword it that often between operating system releases). I can accept this, atleast for the Microsoft OIDs. I would appreciate if some place in the code pointed out where the strings come from (noting that I failed to do so myself, so collecting this information incrementally would be fine).

I appreciate your work on this. Usually, once some issue is done, and did not introduce regressions, I prefer to keep a bug fixed, and stop working on the bug. This simplifies tracking. Usually all ongoing work should be done in a new, separate bug. My opinion is: If you have reached an agreement already, that the above additional patch is perfect to be checked in already, I'm fine to review it and land it shortly. However, if we expect additional revisions of the patch and some more discussion, I'd prefer to have a new, separate bug. Note that having a new, separate bug does not mean a slowdown. As this is a backend only change, we will be able to check this in quickly, after you have reached agreements. Note we no longer need a superreview for backend only changes to PSM, that do not touch UI, but a module owner/peer review is sufficient.

Comment on attachment 206410 [details] [diff] [review] Patch v7 Requesting approval1.8.1 for Firefox and Thunderbird 2.0. This patch greatly improves the display of certificate extensions. It has been on the Mozilla trunk since 2005-12-25.

(In reply to comment #76) > However, if we expect additional revisions of the patch and some more > discussion, I'd prefer to have a new, separate bug. Ok, I think this is what's going to happen, so I've filed bug 323903 and hope that we can soon reach an agreement (Martin, please have a look at it). On a side note (with regard to comment 71): there's one big misnomer(TM) in the whole Mozilla tree... CA stands for "Certification Authority", not "Certificate Authority" - but no, I didn't want to change that ;-) (PEM [RFC 1421, February 1993] already had it like this, maybe it was a glitch in the early Netscape days...?) I'm marking this version of the patch as obsolete.

Comment on attachment 208551 [details] [diff] [review] Proposed patch to the patch for v2 of this patch, see bug 323903

(In reply to comment #2) > I believe this patch fixes bug 230655, bug 127052, bug 161140. I agree - but the first two are still open as of today. Would it be correct to resolve these as duplicates of this one here, or what's the proper procedure?