175163 - SEC_ASN1DecodeItem(NULL, ...) leaks by design . It should always be called with an arena

Status: NEW

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

This was discovered as a result of investigating bug # 95311 .

This decoder doesn't require an arena to be passed in.

When an arena is not passed in, memory is allocated on the regular heap.
If there is an error in the middle of decoding, it makes no attempt to free the
memory allocated. This would require tracking all the pointers. The function
will therefore return SECFailure with a partly-filled structure possibly
containing allocated data. This is almost a guarantee for a memory leak.

If the decoding succeeds, it then becomes the responsibility of the caller to
track all those pointers allocated on the heap. It is not realistic to expect
applications to be able to do that except for the simplest ASN.1 structures.

Therefore, I think we should make it a rule that SEC_ASN1DecodeItem always be
called with an arena. Every place where no arena is passed is a potential memory
leak.

At the minimum, all NSS code under lib should be reviewed and modified to use
arenas in every place. This is far from the case today.

As part of this effort, if possible, the decoder should also be changed to
SEC_QuickDERDecodeItem, if the input data is DER and streaming isn't required.
See bug # 160805 .

Comment 1

[Terry Hayes]

When does the NSS library pass NULL for the arena to SEC_ASN1DecodeItem?  How 
does it free the memory allocated in those cases?  Does it only do this 
for "simple" structures?  Is it possible to create a "destructor" for those 
data types so that we can clean up, even when the decoding fails?

Comment 2

[Julien Pierre]

Terry,

The NSS library calls this function without an arena in the following 14 cases :

C:\nss\37\mozilla\security\nss\lib\certhigh\crlv2.c(92):    rv =
SEC_ASN1DecodeItem (NULL, value, SEC_IntegerTemplate,
C:\nss\37\mozilla\security\nss\lib\certhigh\crlv2.c(118):    rv =
SEC_ASN1DecodeItem (NULL, &decodedExtenValue,
C:\nss\37\mozilla\security\nss\lib\crmf\challcli.c(159):    rv =
SEC_ASN1DecodeItem(NULL, &randStr, CMMFRandTemplate,
C:\nss\37\mozilla\security\nss\lib\crmf\crmfcont.c(619):    rv =
SEC_ASN1DecodeItem(NULL, params, 
C:\nss\37\mozilla\security\nss\lib\cryptohi\dsautil.c(200):    status =
SEC_ASN1DecodeItem(NULL, &sig, DSA_SignatureTemplate, item);
C:\nss\37\mozilla\security\nss\lib\pk11wrap\pk11slot.c(3563):        rv =
SEC_ASN1DecodeItem(NULL, &rc2 ,sec_rc2ecb_parameter_template,
C:\nss\37\mozilla\security\nss\lib\pk11wrap\pk11slot.c(3582):        rv =
SEC_ASN1DecodeItem(NULL, &rc2 ,sec_rc2cbc_parameter_template,
C:\nss\37\mozilla\security\nss\lib\pk11wrap\pk11slot.c(3603):        rv =
SEC_ASN1DecodeItem(NULL, &rc5 ,sec_rc5ecb_parameter_template,
C:\nss\37\mozilla\security\nss\lib\pk11wrap\pk11slot.c(3626):        rv =
SEC_ASN1DecodeItem(NULL, &rc5 ,sec_rc5cbc_parameter_template,
C:\nss\37\mozilla\security\nss\lib\pk11wrap\pk11slot.c(3681):    rv =
SEC_ASN1DecodeItem(NULL, &iv, SEC_OctetStringTemplate, 
C:\nss\37\mozilla\security\nss\lib\pkcs7\p7decode.c(513):	      err =
SEC_ASN1DecodeItem(NULL,
C:\nss\37\mozilla\security\nss\lib\pkcs7\p7decode.c(588):		      p7dcx->error =
SEC_ASN1DecodeItem(NULL, &bulkLength,
C:\nss\37\mozilla\security\nss\lib\smime\cmspubkey.c(303):    err =
SEC_ASN1DecodeItem(NULL, &keaParams, NSS_SMIMEKEAParamTemplateAllParams,
C:\nss\37\mozilla\security\nss\lib\smime\cmspubkey.c(351):	    err =
SEC_ASN1DecodeItem(NULL, &bulkLength,

Each of the cases need to be investigated separately.

However, by taking a quick glance, I didn't see a single place where things
inside the target structure were being checked and freed when the decoder
function returned a failure. So basically, all of these 14 places have a memory
leak when invalid input data is being passed.

Investigating the freeing in the success case is much more complicated because
sometimes it is done in a different function than the one that did the decode. I
would say that it is unlikely that they are all correctly done, though.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

This seems like a real issue. Almost a vulnerability.  Proper implementations
of peers will result in few decoding failures, but an attacker who knew that
he could cause leaks by sending improperly encoded data might cause a DOS 
attack, although it might take a long time if the leaks are really small.