Bug 629816 (CVE-2013-0791)

CERT_DecodeCertPackage reads bytes outside the input buffer

RESOLVED FIXED in Firefox 20

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: Ambroz Bizjak, Assigned: Robert Relyea)

Tracking

({sec-moderate})

unspecified
3.14.2
sec-moderate

Firefox Tracking Flags

(firefox17+ wontfix, firefox18+ wontfix, firefox19+ wontfix, firefox20+ fixed, firefox21+ fixed, firefox-esr10 wontfix, firefox-esr1720+ fixed, b2g1820+ fixed)

Details

(Whiteboard: [adv-main20+][adv-esr1705+])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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
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.
Group: core-security
Keywords: sec-critical
OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All
Target Milestone: --- → 3.14.1
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.
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
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.
Thanks Robert, Brian can you help this patch along?
Assignee: nobody → rrelyea
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).
Status: UNCONFIRMED → NEW
status-firefox-esr10: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox17: --- → -
tracking-firefox18: --- → +
tracking-firefox19: --- → +
Ever confirmed: true
Keywords: sec-critical → sec-high
tracking-firefox17: - → ?
tracking-firefox-esr10: ? → ---
tracking-firefox17: ? → +
Given the sec rating and lack of actionable fix here, we must wontfix for 17 at this point.
status-firefox17: affected → wontfix
Bob, are these patches ready for review? I think the strategy you suggested seems reasonable.

Updated

5 years ago
Target Milestone: 3.14.1 → 3.14.2
We're pretty close to untracking this for all upcoming releases, given the inaction here.
status-firefox18: affected → wontfix
(Assignee)

Updated

5 years ago
Attachment #671960 - Flags: review?(bsmith)
(Assignee)

Updated

5 years ago
Attachment #671960 - Flags: review?(wtc)
(Assignee)

Comment 12

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

bob

Comment 13

5 years ago
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)?
(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

5 years ago
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

5 years ago
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.
Attachment #705057 - Flags: superreview?(rrelyea)
Attachment #705057 - Flags: review?(bsmith)
(Assignee)

Comment 17

5 years ago
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.
Attachment #705057 - Flags: superreview?(rrelyea) → superreview+

Comment 18

5 years ago
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
Attachment #705057 - Flags: checked-in+

Updated

5 years ago
Attachment #671960 - Attachment is obsolete: true
Attachment #671960 - Flags: review?(wtc)
Attachment #671960 - Flags: review?(bsmith)
(Assignee)

Comment 19

5 years ago
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
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.
status-firefox-esr10: affected → wontfix
status-firefox20: --- → affected
status-firefox-esr17: --- → affected
tracking-firefox20: --- → +

Comment 21

5 years ago
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.
Attachment #705588 - Flags: review?(rrelyea)

Comment 22

4 years ago
This bug is fixed in NSS 3.14.2. The only remaining work is a
comment patch (attachment 705588 [details] [diff] [review]).
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
status-b2g18: --- → affected
status-firefox19: affected → wontfix
status-firefox21: --- → affected
tracking-b2g18: --- → 20+
tracking-firefox21: --- → +
tracking-firefox-esr17: --- → 20+
(Assignee)

Comment 23

4 years ago
Comment on attachment 705588 [details] [diff] [review]
Add a comment to explain the max oid length of 9

r+ rrelyea
Attachment #705588 - Flags: review?(rrelyea) → review+

Comment 24

4 years ago
We must check in, but delay it until after the NSS code migration is over.
Keywords: checkin-needed
(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

4 years ago
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

4 years ago
setting trust flags to firefox20-fixed and firefox21-fixed, because they contain an NSS with this fix.
status-firefox20: affected → fixed
status-firefox21: affected → fixed
Thanks for the clarification. For ESR17/B2G18, we'll look into resolving bug 839141.
Depends on: 839141
(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

4 years ago
> 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.
(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

4 years ago
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
Attachment #705588 - Flags: checked-in+

Updated

4 years ago
Keywords: checkin-needed

Updated

4 years ago
status-firefox-esr17: affected → fixed
Whiteboard: [adv-main20+][adv-esr1705+]

Comment 33

4 years ago
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

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

Updated

4 years ago
Flags: needinfo?(bsmith)
Alias: CVE-2013-0791
(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.
Keywords: sec-high → sec-moderate
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.
Flags: needinfo?(bsmith)
b2g18: was fixed by : https://bugzilla.mozilla.org/show_bug.cgi?id=839141
status-b2g18: affected → fixed
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.
Attachment #705057 - Flags: review?(brian)
Group: core-security
You need to log in before you can comment on or make changes to this bug.