Closed Bug 1253101 Opened 8 years ago Closed 7 years ago

SEC_ASN1Decode should not allocate more than a 'sane' amount of memory

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox47 affected)

RESOLVED FIXED
Tracking Status
firefox47 --- affected

People

(Reporter: ryan.sleevi, Assigned: ryan.sleevi)

References

Details

(Keywords: csectype-dos, csectype-oom, sec-low)

Attachments

(1 file)

The QuickDER ASN.1 decoder limits the maximum size it's willing to decode to being the length of the input buffer (minus the ASN.1 tag/length). If an ASN.1 TLV is encountered whose length is greater than the input buffer, it will reject it as invalid.

The 'legacy' ASN.1 decoder, because it supports streaming BER, does not include such length checks. As a result, it's possible to cause extremely large allocations (20 MB, 200 MB, 2GB) simply by having an ASN.1 TLV whose length is in long-form, but whose value is small. This is similar to a zlib bomb - a very small input results in an outrageously large output.

Given that the 'legacy' decoder is reachable within NSS through functions like softoken's key decoding routines, we should limit the maximum allocation that the legacy decoder will perform for a given ASN.1 item to some reasonable bounds.

For consumers using the one-shot form of the legacy decoder, that reasonable bounds, at max, should be the size of the input data.
Attached patch New API & optionSplinter Review
Bob: This is the patch that just sets the upper limit to allocations. I'll handle the zeroization concern in a separate bug/change.
Attachment #8725965 - Flags: review?(rrelyea)
Comment on attachment 8725965 [details] [diff] [review]
New API & option

Review of attachment 8725965 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assumong my the answer to my question in secasn1d.c is 'yes'.

::: lib/util/secasn1d.c
@@ +307,5 @@
>  
>      SEC_ASN1WriteProc filter_proc;	/* pass field bytes to this  */
>      void *filter_arg;			/* argument to that function */
>      PRBool filter_only;			/* do not allocate/store fields */
>  };

Is sec_DecoderContext always zalloc'd? If not max_element_size should be initialized to zero in SEC_ASN1DecoderStart().
Attachment #8725965 - Flags: review?(rrelyea) → review+
Yes, it's always zalloc'd, that's why I left it off from Start.
Thanks, then it's good to go (I thought it may be zalloc's, it's pretty common in NSS -- I just wanted to make sure).
Should this land? Did it land?
Group: crypto-core-security
Flags: needinfo?(ryan.sleevi)
1) Should: Yes
2) Did: No

I'm going to keep the NI so I get pinged next week; too much OOO. However, if someone else wants to pick this up sooner (see thread to nss-dev@ just now to understand why), no objections.
https://hg.mozilla.org/projects/nss/rev/04a60a1de49e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(ryan.sleevi)
Resolution: --- → FIXED
Target Milestone: --- → 3.29
Comment on attachment 8725965 [details] [diff] [review]
New API & option

Review of attachment 8725965 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/util/nssutil.def
@@ +282,5 @@
>  NSSUTIL_ArgParseModuleSpecEx;
>  ;+    local:
>  ;+       *;
>  ;+};
> +;+NSSUTIL_3.23 {         # NSS Utilities 3.21 release

It's too late now, but this was landed as 3.25, it should have been 3.29...
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: