78577 - SEC_ASN1DecodeInteger crashes if SECItem has NULL data

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Javier Delgadillo]

When writing the Cert Viewer for PSM 2, I passed &cert->version to
SEC_ASN1DecodeInteger.  version is an optional field in the cert, so the data
field may have a NULL pointer.  When we ran across a v1 cert, there was a crash
in the depths of NSS (probably not too far down the ASN1 engine path).

Perhaps we should make SEC_ASN1Decode* functions more bullet proof to prevent
such cases from happening again.

Comment 1

[Javier Delgadillo]

Once this is fixed/resolved, we should re-visit ProcessVersion in
nsNSSCertificate.cpp in PSM 2 code.

Comment 2

[Wan-Teh Chang]

Javi, do you have a workaround for this crash?

Comment 3

[Javier Delgadillo]

yes.

Comment 4

[Wan-Teh Chang]

Assigned the bug to Kirk.

Javi, note that I set target milestone to NSS 3.3 because
you have a workaround.  Let me know if this needs to be
fixed sooner.

Comment 5

[Kirk Erickson]

Added Null check:

RCS file: /cvsroot/mozilla/security/nss/lib/util/secasn1d.c,v
retrieving revision 1.5
diff -r1.5 secasn1d.c
2232a2233,2235
>     if (src == NULL)
> 	return SECFailure;
> 

Tested before (segv), after (nosegv).
Javi doesn't have time to test.

Comment 6

[Kirk Erickson]

Attachment: test program (sec.c)

Comment 7

[Kirk Erickson]

Attachment: script to compile, link, and run under RedHat Linux 6.2

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Kirk,

Is it an error for this pointer to be null?  
  (I'm not sure if it is or not)

If so, shouldn't these error returns call PORT_SetError to 
set the error code before returning?

If not, then perhaps this should be returning SECSuccess instead
of SECFailure?

Comment 9

[Wan-Teh Chang]

The prototype of the function in question is:

    SECStatus
    SEC_ASN1DecodeInteger(SECItem *src, unsigned long *value)

If SEC_ASN1DecodeInteger returns SECSuccess, it must have stored
a valid integer in *value.

Since a null SECItem (with 'len' == 0 and 'data' == NULL) can't
be decoded into any integer, SEC_ASN1DecodeInteger has no choice
but to return SECFailure due to its current function prototype.
This is not ideal.  I can see two possible solutions.

1. Have NSS clients check for a null SECItem themselves and only
   call SEC_ASN1DecodeInteger on a non-null SECItem; or
2. Have SEC_ASN1DecodeInteger set the error code to an appropriate
   value when a null SECItem is being decoded.  If we do this, we
   must set the error code to something else like PR_INVALID_ARGUMENT_ERROR
   for the real failures.

I like solution 2 better.  What do you think?

Comment 10

[Kirk Erickson]

We need to do (2) to avoid seg faulting.
We'll set the error to SEC_ERROR_INVALID_ARGS,
and return SECFailure in both cases then.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

PSM started doing #1 a week ago.  See 
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&ro
ot=/cvsroot&subdir=mozilla/security/manager/ssl/src&command=DIFF&root=/cvsroot&f
ile=nsNSSCertificate.cpp&rev1=1.19&rev2=1.20

In any case, SEC_ASN1DecodeInteger should set the error code any time
it returns SECFailure.

Comment 12

[Kirk Erickson]

Avoiding seg fault, and returning errors on failures.