222568 - Many common bugs in NSS cert/crl printing code

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

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[0], then increments the pointer byte, then looks at byte[1].  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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v1

This patch also contains the portions of the fix for bug 211540 that are 
relevant to this source file.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Marking for 3.9

Comment 3

[Julien Pierre]

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[0] 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[1] 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 ...

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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 6

[Nelson Bolyard (seldom reads bugmail)]

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 7

[Julien Pierre]

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.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 9

[Jamie Nicolson]

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.

Comment 10

[Julien Pierre]

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.

Comment 11

[Wan-Teh Chang]

Attachment: Fixed a bug in patch v3

This patch should be applied on top of patch v3.
I've checked it in on.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 135869 [details] [diff] [review]
Fixed a bug in patch v3

Good catch!  Thanks!

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

I have created bug 228816 to track the issue (from comment 7 above) about 
function SECU_StripTagAndLength, and am marking this bug fixed.