I went looking at secutil.c today to see why it was misprinting certain elements in CRLs. I found something like 17 bugs in that source file, and fixed them. Certain bugs were repated in numerous places, including 1. Lots of off-by-1 errors, including a) code that must check the first two bytes in an array of char looks at byte, then increments the pointer byte, then looks at byte. Consequently, it looks at the first and third bytes, not the first and second. b) code that miscomputes the length of the length bytes, forgetting to count the byte that is the length of the variable-length length field. 2, failing to strip off the DER encoding (tag, length) before passing the data to a function that expects only the value octets. These errors explained why common OIDs were unrecognized, why nested sets and sequences were incorrectly parsed. Other errors include: - multiplying by 255 (not 256) to effect an 8-bit shift. - memory leaks. - masking with 1F when the correct value is 7F I coded fixes for many problems. As a result, many cert and CRL extensions are now correctly parsed that were not before. Patch forthcoming.
Created attachment 133481 [details] [diff] [review] Patch v1 This patch also contains the portions of the fix for bug 211540 that are relevant to this source file.
Marking for 3.9
Comment on attachment 133481 [details] [diff] [review] Patch v1 This code has many problems, even with this patch. I think you should change the following : - for SECU_PrintEncodedInteger, it would be desirable to remove the ASN.1 logic and use SEC_QuickDERDecodeItem . You can use the default integer template SECIntegerTemplate . This reduces the possibility for bugs and the need to maintain this code - Same remark for SECU_PrintString . To strip the type and length, you can use a template of type SEC_ASN1_ANY | SEC_ASN1_INNER - for secu_PrintBoolean, your patched version still suffers from a bug. The code expects the encoded boolean. This means you need to strip the tag and length first. You can get a confirmation of this by looking at secu_PrintUniversal which has a switch statement on the tag in i->data then passes the unmodified i (SECItem*) to the various print functions . The solution in this case again is to use the QuickDER decoder. Use the default template SEC_BooleanTemplate . Then the rest of your function will work as expected. - for SECU_PrintSet, I would also use the decoder rather than having all the logic in there. You can use SEC_SetOfAnyTemplate . - in secu_PrintContextSpecific, i->data is not checked for NULL, even though it is dereferenced immediately at the beginning of the function. You also address i->data at the end of the function without checking the length. As a result, tmp.len could end up negative . - in secu_PrintOctetString, again it would be preferable to use the decoder, with SEC_OctetStringTemplate . - for secu_PrintBitString, use the decoder with SEC_BitStringTemplate - SECU_PrintEncodedObjectID should be implemented with the decoder and SEC_ObjectIDTemplate - secu_PrintNSCertType should also be implemented with the decoder . Use SEC_BitStringTemplate - in SECU_PrintFingerPrints, you check for rv from PK11_HashBuf too late . It should be checked right after the first call. Otherwise, the value of PORT_GetError() may have been overwritten in between (eg. by PORT_Free). You don't check the rv immediately after the second call, also . - in SECU_PrintCRLInfo, you shouldn't use SECU_PrintUTCTime for lastUpdate and nextUpdate . Use SECU_PrintTimeChoice instead . I could have sworn I made that change before, but somehow it's not in the tree ...
Created attachment 133993 [details] [diff] [review] patch v2 - with some of Julien's comments This patch addresses some, but not all of Julien's comments above. It: - does NOT convert all places that strip off tag and length to use the quick DER decoder. That is way beyond the scope of this bug. - DOES create a new function SECU_PrintEncodedBoolean to be called when the secitem points at an encoded boolean (with type and length). - DOES ensure that len doesn't go negative in SECU_PrintContextDependent - Does NOT change the logic in SECU_PrintFingerprints, which works as intended. - Does NOT convert any SECU_PrintUTCTime calls to SECU_PrintTimeChoice. That conversion is the subject of another bug report.
Created attachment 134103 [details] [diff] [review] patch v3 - incorporating Julien's latest comments This patch is MUCH larger than the previous version. It does what the previous patch did, PLUS: a) it replaces all the old code to strip der tag and length with a call to a single new function to do that. b) It changes all but one of the SECU_PrintUTCTime calls in secutil.c to call SECU_PrintTimeChoice instead. c) it exports two functions from libNSS, - one returns a string representation of an OID - one destroys an object returned by a previously-exported function. d) The printing code now prints unknown OIDs in decimal representation. e) It plugs the memory leaks in secutil.c and in pp.c. pp now runs cleanly under purify. f) Long strings no longer exceed 80 columns of output, but now are broken up and wrapped in a pretty fashion. Unprintable characters in strings are now printed as a dot (".") instead of garbage. g) it no longer attempts to print UCS2 and UCS4 strings as if they were ASCII.
Comment on attachment 134103 [details] [diff] [review] patch v3 - incorporating Julien's latest comments Julien, please especially review my changes of PrintUTCTime to PrintTimeChoice to ensure that I didn't change any that should not have been changed.
Comment on attachment 134103 [details] [diff] [review] patch v3 - incorporating Julien's latest comments The patch looks good for the most part. However, there is still one thing that bothers me : In the new function SECU_StripTagAndLength, you just skip over the length bytes, without calculating them to see if they match (or at least are not greater than) the amount in i->len . The code does perform the calculation of the length, but only in SECU_PrintSet. I think that's not the right place to do it. It should be done and checked in SECU_StripTagAndLength. For the callers that want to have the value of the length bytes returned to them (primarily the printset function), you could return the value in an optional PRIn32* argument to SECU_StripTagAndLength (set to NULL if the caller doesn't care about the length bytes). As far as the SECU_PrintTimeChoice, the only thing to remember is that the SECItem must have come from the ASN.1 or QuickDER decoder to set the "type" field. Otherwise, printing will fail. I believe that's the case in every instance that you changed, so your changes are OK.
Julien, As you observed, the code in SECU_StripTagAndLength does not check the parsed length field. However, the code that it is replacing did not, either. Per our earlier discussion, the objective of creating SECU_StripTagAndLength was to centralize the logic that removes the length, and I think patch v3 accomplishes that. With it centralized, we can later decide what, if any, additional tests we'd like to impose. So, if your ONLY objection to patch v3 is that SECU_StripTagAndLength doesn't do as much checking as you'd like, then I propose to check that patch in as is, and to embellish SECU_StripTagAndLength at a later time. OK?
In Julien's absence, I agree with Nelson that it is ok to check in the patch as is. Verifying and/or returning the length field is a useful feature that can be added separately.
Nelson, Your patch is OK without the additional check, but I thought the requested change was not much to add and should go in if not at the same time as the other changes then soon after, preferably before this bug is closed.
Created attachment 135869 [details] [diff] [review] Fixed a bug in patch v3 This patch should be applied on top of patch v3. I've checked it in on.
Comment on attachment 135869 [details] [diff] [review] Fixed a bug in patch v3 Good catch! Thanks!
I have created bug 228816 to track the issue (from comment 7 above) about function SECU_StripTagAndLength, and am marking this bug fixed.