Last Comment Bug 629816 - (CVE-2013-0791) CERT_DecodeCertPackage reads bytes outside the input buffer
(CVE-2013-0791)
: CERT_DecodeCertPackage reads bytes outside the input buffer
Status: RESOLVED FIXED
[adv-main20+][adv-esr1705+]
: sec-moderate
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: 3.14.2
Assigned To: Robert Relyea
:
Mentors:
Depends on: 839141
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-28 16:17 PST by Ambroz Bizjak
Modified: 2014-11-19 19:37 PST (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
wontfix
+
wontfix
+
wontfix
+
fixed
+
fixed
wontfix
20+
fixed
20+
fixed


Attachments
Option1: multiple checks. (2.01 KB, patch)
2012-10-16 11:59 PDT, Robert Relyea
no flags Details | Diff | Splinter Review
Option 2: single check (2.06 KB, patch)
2012-10-16 12:02 PDT, Robert Relyea
no flags Details | Diff | Splinter Review
Option 2: single check, v2 (2.90 KB, patch)
2013-01-22 12:30 PST, Wan-Teh Chang
rrelyea: superreview+
wtc: checked‑in+
Details | Diff | Splinter Review
Add a comment to explain the max oid length of 9 (1.30 KB, patch)
2013-01-23 14:34 PST, Wan-Teh Chang
rrelyea: review+
wtc: checked‑in+
Details | Diff | Splinter Review

Description Ambroz Bizjak 2011-01-28 16:17:26 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.13 (KHTML, like Gecko) Chrome/9.0.597.83 Safari/534.13
Build Identifier: 

http://mxr.mozilla.org/security/source/security/nss/lib/pkcs7/certread.c#187

CERT_DecodeCertPackage may read bytes outside of the input buffer.

First, it reads certbuf[0] and certbuf[1] without checking certlen.

Though, it may be assuming the input is zero-terminated. But even in that case it's incorrect: for example, at line 218, it reads certbuf[5] ( cp[4] ), but it only knows that bytes certbuf[0] and certbuf[1] are nonzero; it never makes sure that certbuf[2], 3, and 4 are nonzero.

This may result in the program crashing if the procedure (and CERT_DecodeCertFromPackage, which calls it) is supplied with a very small input.

Reproducible: Always
Comment 1 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-15 17:07:56 PDT
Out of the blue, I was pointed to this bug as one that should have my attention by IRC user mjrosenb (Marty Rosenberg). It seems to be an out-of-bounds array read. I asked him why he pointed out this bug specifically and he just said that generally out-of-bounds array access is bad. I'm not sure if he's being coy and trying to hint at a exploitable/exploited vulnerability, or if he's just generally concerned about unfixed array access bugs. So, out of an abundance of caution, I recommend that we look at this bug soon and try to develop a fix. I am also marking it sec-critical and security-group.
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-15 17:11:58 PDT
I was mistaken. I had thought that Marty was a student at CMU, but he's actually an esteemed colleague of mine at Mozilla :). So, I trust Marty to clarify the concern I have in comment 1 himself.
Comment 3 Robert Relyea 2012-10-16 09:46:56 PDT
There certainly needs to be checks for overrunning certlen, but the reporter's assumption that the input is a NULL terminated string is incorrect. The input is either DER data (which has embedded NULL's) or ascii data. The purpose of the function is to determine which it is.

The function should be checking that certlen is long enough for the expected references.
Comment 4 Robert Relyea 2012-10-16 11:12:52 PDT
I see 2 ways of fixing this problem:

1) we can add the needed length checks. The three places the length checks are missing are:
    a) Before our initial decode. This check can be handled by simply requiring certlen to be at least 6 bytes. If the length isn't 6 bytes, there is no way that there is a valid encoded cert.
    b) Checking the string type
    c) Checking the oid

2) We can create templates and pass the data to our existing DER decoder for the 3 cases we are interested in, and detect the INVALID_DER error.
Comment 5 Robert Relyea 2012-10-16 11:59:57 PDT
Created attachment 671958 [details] [diff] [review]
Option1: multiple checks.

Both of these patches implement my first suggestion. This one has multiple length checks over each of the relevant cases.
Comment 6 Robert Relyea 2012-10-16 12:02:28 PDT
Created attachment 671960 [details] [diff] [review]
Option 2: single check

This patch depends on the fact that any legitimate certificate must larger than the space we need to reference all the values in the array, so we can implement a single check. The code is simpler, but may be harder to verify as correct than patch 1.
Comment 7 David Bolter [:davidb] 2012-10-18 13:05:12 PDT
Thanks Robert, Brian can you help this patch along?
Comment 8 Daniel Veditz [:dveditz] 2012-10-25 13:16:34 PDT
Not sure it's sec-critical if it's only reading from beyond the buffer (writes would be another story). On the other if that means it might accept an invalid cert that could be used in conjunction with our install system or your bank data (though if it's beyond the buffer it might be hard to control the data that read from it).
Comment 9 Lukas Blakk [:lsblakk] use ?needinfo 2012-11-07 17:47:40 PST
Given the sec rating and lack of actionable fix here, we must wontfix for 17 at this point.
Comment 10 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-11-27 15:23:00 PST
Bob, are these patches ready for review? I think the strategy you suggested seems reasonable.
Comment 11 Alex Keybl [:akeybl] 2012-12-19 13:30:20 PST
We're pretty close to untracking this for all upcoming releases, given the inaction here.
Comment 12 Robert Relyea 2013-01-16 15:07:36 PST
re comment 10, sorry, yes, that's why it has a review? on it;).

bob
Comment 13 Wan-Teh Chang 2013-01-21 16:40:41 PST
Comment on attachment 671960 [details] [diff] [review]
Option 2: single check

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

Bob: I have some questions about the patch.

::: certread.c
@@ +169,5 @@
> +    /* 
> +     * Make sure certlen is long enough to handle the longest possible 
> +     * reference in the code below:
> +     * 0x30 0x84 l1 l2 l3 l4  +
> +     *                       tag 11 c e r t i f i c a t e

Can you review the PORT_Strcmp((char *)&cp[2], CERTIFICATE_TYPE_STRING)
call in this file?

PORT_Strcmp requires the string at &cp[2] is null-terminated. This
means we need a '\0' character after c e r t i f i c a t e. That seems
wrong. It seems that we should call PORT_Memcmp instead:
  PORT_Memcmp(&cp[2], CERTIFICATE_TYPE_STRING)

This bug probably means the netscape wrapped DER cert code is never
exercised. Maybe we should delete it.

@@ +174,5 @@
> +     *                      or
> +     *                       tag 9 o1 o2 o3 o4 o5 o6 o7 o8 o9
> +     *   6 + 13 = 19. 19 bytes is clearly too small to code any kind of 
> +     *  certificate (a 128 bit ECC certificate contains at least an 8 bit 
> +     * key and a 16 bit signature, plus coding overhead). Typically a cert

Typo: 8 bit key => 8 byte key
      16 bit signature => 16 byte signature

@@ +216,3 @@
>  		/* indefinite length */
>  		seqLen = 0;
> +	      default:

BUG?: Should case 0 have a break statement?

If this is an intentional fall through, please add a
/* Fall through. */ comment.

@@ +257,4 @@
>  	    SECItem oiditem;
>  	    /* XXX - assume DER encoding of OID len!! */
>  	    oiditem.len = cp[1];
> +	    /* if we add an oid below that is longer than 

Nit: the Splinter Review tool shows a space at the end of the line.

@@ +260,5 @@
> +	    /* if we add an oid below that is longer than 
> +	     * CERTIICATE_TYPE_LEN, then we need to change the certlen
> +	     * check at the top of the function to prevent a buffer overflow
> +	     */
> +	    if (oiditem.len > CERTIFICATE_TYPE_LEN ) {

The comment at the top of the function describes this case as:
      tag 9 o1 o2 o3 o4 o5 o6 o7 o8 o9

So I would expect to check if (oiditem.len > 9) here.

By the way, is 9 the max. of the lengths of the two OIDs
that we support (SEC_OID_PKCS7_SIGNED_DATA and
SEC_OID_NS_TYPE_CERT_SEQUENCE)?
Comment 14 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-01-21 18:43:32 PST
(In reply to Wan-Teh Chang from comment #13)
> Can you review the PORT_Strcmp((char *)&cp[2], CERTIFICATE_TYPE_STRING)
> call in this file?
> 
> PORT_Strcmp requires the string at &cp[2] is null-terminated. This
> means we need a '\0' character after c e r t i f i c a t e. That seems
> wrong. It seems that we should call PORT_Memcmp instead:
>   PORT_Memcmp(&cp[2], CERTIFICATE_TYPE_STRING)
> 
> This bug probably means the netscape wrapped DER cert code is never
> exercised. Maybe we should delete it.

I agree. Plus, AFAICT, the sense of the PORT_Strcmp call is backwards (PORT_Strcmp vs !PORT_Strcmp). I support deleting the netscape-wrapped-DER-cert support.
Comment 15 Wan-Teh Chang 2013-01-22 12:08:01 PST
Comment on attachment 671958 [details] [diff] [review]
Option1: multiple checks.

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

Bob: you didn't ask me to review this patch, but I looked at it
to improve my understanding of the bug. I have some comments.

::: certread.c
@@ +206,3 @@
>  		/* indefinite length */
>  		seqLen = 0;
> +	      default:

BUG?: I believe it is wrong for case 0 to fall through to the
default case. We should add a break statement before this line.

@@ +229,4 @@
>  	
>  	/* check the type string */
>  	/* netscape wrapped DER cert */
> +	if ( ( certlen > seqLenLen + 4 + CERTIFICATE_TYPE_LEN ) &&

I suggest adding a comment to explain the constant 4.
I believe it is the tag and the first length octet, plus
the cp[0] and cp[1] that we are about to read.

An alternative approach is to use cp in the check. For
example, this check could be rewritten as:

  if ( ( cp + 2 + CERTIFICATE_TYPE_LEN < certbuf + certlen ) &&

certbuf + certlen is the end of the buffer. cp + 2 + CERTIFICATE_TYPE_LEN
suggests we are going to read 2 + CERTIFICATE_TYPE_LEN bytes
starting at cp. I am not sure if you find this form more readable
though. (We can even write &cp[2 + CERTIFICATE_TYPE_LEN]
instead of cp + 2 + CERTIFICATE_TYPE_LEN.)

@@ +243,4 @@
>  	    rv = (* f)(arg, &pcertitem, 1);
>  	    
>  	    return(rv);
> +	} else if ( ( certlen > seqLenLen + 3 ) &&

I believe 3 is the tag and the first length octet, plus
the cp[0] that we are about to read.

Note: the alternative form of this check would be
    } else if ( ( cp + 1 < certbuf + certlen ) &&

But we are reading cp[1] without protection. So it seems
that we should use 4 instead of 3 in this check.
Comment 16 Wan-Teh Chang 2013-01-22 12:30:20 PST
Created attachment 705057 [details] [diff] [review]
Option 2: single check, v2

Bob,

Please review this version of your Option 2 patch.

1. I removed the support for "netscape wrapped DER cert". The
length 19 is adjusted to 17 accordingly.

2. I fixed the "bit vs. byte" typos.

3. I added a break statement to case 0 in the switch statement.

4. In the oid length check, I use the constant 9. It would be nice
to add a comment to explain where 9 comes from.

Feel free to mark this patch obsolete if I misunderstood anything
in your patch. Thanks.
Comment 17 Robert Relyea 2013-01-22 13:34:53 PST
Comment on attachment 705057 [details] [diff] [review]
Option 2: single check, v2

r+

The use of CERTIFICATE_TYPE_LENGTH before was because I new the length had to be at least that long anyway. Now that you've removed the certificate bundle, your '9' is fine.
Comment 18 Wan-Teh Chang 2013-01-22 16:54:58 PST
Comment on attachment 705057 [details] [diff] [review]
Option 2: single check, v2

I will make any suggested change in a new patch.

Bob, could you explain where the magic oid length 9 comes from?
I'll add a comment. Thanks.

Patch checked in on the NSS trunk (NSS 3.14.2).

Checking in certread.c;
/cvsroot/mozilla/security/nss/lib/pkcs7/certread.c,v  <--  certread.c
new revision: 1.19; previous revision: 1.18
done
Comment 19 Robert Relyea 2013-01-22 17:00:15 PST
That's the longest length of one the expected oids we are testing here. If we ever need to add longer oids to test, then we would have to bump the test up to the length thest (as well as the total length test above).

bob
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2013-01-23 13:27:20 PST
Please nominate for aurora/beta approval as soon as this is verified fixed on trunk so we can assess the risk for landing and decide if we can take this in 19.
Comment 21 Wan-Teh Chang 2013-01-23 14:34:36 PST
Created attachment 705588 [details] [diff] [review]
Add a comment to explain the max oid length of 9

I got the oid bytes from lib/util/secoid.c.
Comment 22 Wan-Teh Chang 2013-02-04 11:21:44 PST
This bug is fixed in NSS 3.14.2. The only remaining work is a
comment patch (attachment 705588 [details] [diff] [review]).
Comment 23 Robert Relyea 2013-02-26 10:06:44 PST
Comment on attachment 705588 [details] [diff] [review]
Add a comment to explain the max oid length of 9

r+ rrelyea
Comment 24 Kai Engert (:kaie) 2013-02-26 10:13:11 PST
We must check in, but delay it until after the NSS code migration is over.
Comment 25 Alex Keybl [:akeybl] 2013-03-04 15:40:40 PST
(In reply to Kai Engert (:kaie) from comment #24)
> We must check in, but delay it until after the NSS code migration is over.

We're tracking bug 839141 for FF21, but what are we looking to do for FF20?
Comment 26 Kai Engert (:kaie) 2013-03-06 14:57:03 PST
Alex, I realize this bug already got fixed in NSS 3.14.2, according to the target milestone field and comment 18

Mozilla 20 contains NSS 3.14.2 already.

The remaining patch is a clarification comment, only, not a functional change.
Comment 27 Kai Engert (:kaie) 2013-03-06 14:58:21 PST
setting trust flags to firefox20-fixed and firefox21-fixed, because they contain an NSS with this fix.
Comment 28 Alex Keybl [:akeybl] 2013-03-07 17:17:58 PST
Thanks for the clarification. For ESR17/B2G18, we'll look into resolving bug 839141.
Comment 29 Ryan VanderMeulen [:RyanVM] 2013-03-09 19:43:55 PST
(In reply to Kai Engert (:kaie) from comment #24)
> We must check in, but delay it until after the NSS code migration is over.

Now that NSS is on hg.mozilla.org, I believe we're good to go here? Mind if I do the honors? :)
Comment 30 Kai Engert (:kaie) 2013-03-10 05:41:01 PDT
> Now that NSS is on hg.mozilla.org, I believe we're good to go here? Mind if
> I do the honors? :)

There are ACLs in place that should only allow the NSPR/NSS developers to commit.
Comment 31 Ryan VanderMeulen [:RyanVM] 2013-03-10 06:21:23 PDT
(In reply to Kai Engert (:kaie) from comment #30)
> > Now that NSS is on hg.mozilla.org, I believe we're good to go here? Mind if
> > I do the honors? :)
> 
> There are ACLs in place that should only allow the NSPR/NSS developers to
> commit.

That takes all the fun out of it :P. In that case, can you please do so so this doesn't show up in my bug queries anymore? Thanks!
Comment 32 Wan-Teh Chang 2013-03-14 12:43:31 PDT
Comment on attachment 705588 [details] [diff] [review]
Add a comment to explain the max oid length of 9

Patch pushed to the NSS hg repository (NSS 3.15).
http://hg.mozilla.org/projects/nss/rev/7d875a0678df
Comment 33 Huzaifa Sidhpurwala 2013-03-24 23:18:13 PDT
Unless i am mis-understanding the issue here, this seems to be a OOB read, which can only cause crash. (perhaps not even that, depending on the arch. used and how memory is aligned/padded).

I dont see a reason why this should be sec-high?
Comment 34 Huzaifa Sidhpurwala 2013-03-24 23:34:51 PDT
Doing a needinfo on bsmith, since he proposed sec-critical in the first place :)
Comment 35 Daniel Veditz [:dveditz] 2013-03-28 22:38:37 PDT
(In reply to Huzaifa Sidhpurwala from comment #33)
> Unless i am mis-understanding the issue here, this seems to be a OOB read,
> which can only cause crash.

Not quite: an OOB read might NOT crash and that's worse. The program will think the bogus data is what it's looking for. The bogus data might be random, or it might be in a location that could be filled by an attacker. What happens next depends on what the code does with that value. It might allow an attacker to read the contents of memory and gain information that way. The data might be interpreted as an object or pointer or offset and lead to running arbitary code.

Running code doesn't seem to be in the cards here, but the data being read might influence the way the certificate is parsed. I don't know enough to know whether it could be used to make an invalid cert look valid, or change an OID and give the cert properties it ought not to have or something along those lines.

Seems remote, but clever hacks usually do until you learn the trick. Was hoping one of the crypto guys who knows what this routine is doing could take a stab at whether there's any interesting possibilities.
Comment 36 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-03-29 12:36:13 PDT
To be honest, I do not research in detail these things because I just assume that such bugs are probably very, very bad. Often it takes more research to determine how bad a bug is than it does to just fix it and I think this is one such case. That said, I think Dan's rating adjustment is reasonable.
Comment 37 bhavana bajaj [:bajaj] 2013-12-04 16:07:02 PST
b2g18: was fixed by : https://bugzilla.mozilla.org/show_bug.cgi?id=839141
Comment 38 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2014-07-11 17:23:44 PDT
Comment on attachment 705057 [details] [diff] [review]
Option 2: single check, v2

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

I seems this review request isn't needed any more since the patch was checked in.

Note You need to log in before you can comment on or make changes to this bug.