Open Bug 175163 Opened 18 years ago Updated 10 years ago
_ASN1Decode Item(NULL, ...) leaks by design . It should always be called with an arena
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 .
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?
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.
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.
Priority: -- → P2
QA Contact: bishakhabanerjee → jason.m.reid
Assignee: wtchang → nobody
QA Contact: jason.m.reid → libraries
You need to log in before you can comment on or make changes to this bug.