629816 - (CVE-2013-0791) CERT_DecodeCertPackage reads bytes outside the input buffer

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Ambroz Bizjak]

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?)]

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?)]

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]

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]

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]

Attachment: 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]

Attachment: 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] (NeedInfo me for attention)]

Thanks Robert, Brian can you help this patch along?

Comment 8

[Daniel Veditz [:dveditz]]

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]

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?)]

Bob, are these patches ready for review? I think the strategy you suggested seems reasonable.

Comment 11

[Alex Keybl [:akeybl]]

We're pretty close to untracking this for all upcoming releases, given the inaction here.

Comment 12

[Robert Relyea]

re comment 10, sorry, yes, that's why it has a review? on it;).

bob

Comment 13

[Wan-Teh Chang]

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?)]

(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]

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]

Attachment: 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]

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]

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]

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]

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]

Attachment: 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]

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]

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:)]

We must check in, but delay it until after the NSS code migration is over.

Comment 25

[Alex Keybl [:akeybl]]

(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:)]

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:)]

setting trust flags to firefox20-fixed and firefox21-fixed, because they contain an NSS with this fix.

Comment 28

[Alex Keybl [:akeybl]]

Thanks for the clarification. For ESR17/B2G18, we'll look into resolving bug 839141.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

(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:)]

> 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]]

(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]

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]

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]

Doing a needinfo on bsmith, since he proposed sec-critical in the first place :)

Comment 35

[Daniel Veditz [:dveditz]]

(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?)]

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]]

b2g18: was fixed by : https://bugzilla.mozilla.org/show_bug.cgi?id=839141

Comment 38

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.