925100 - (CVE-2013-1741) Integer truncation leads to runaway memset in NSS certificate parsing

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Attachment: Patch by Adam Langley

Tavis Ormandy of Google reported this bug. The Chromium issue is
https://code.google.com/p/chromium/issues/detail?id=302790.

Adam wrote in the Chromium issue:

  Tavis found that it's possible to trigger a runaway memset in
  NSS when parsing a certificate. sec_asn1d_zalloc allocates
  length & 0xffffffff but then memsets with length as a size_t.

  This issue also affects Firefox.

  It's unclear what the severity of this issue is. Tavis doesn't
  think it's good, but it appears that it would always involve
  memsetting at least 4GB of random memory, so it may be just a
  crash.

I attached Adam's patch.

Comment 1

[Wan-Teh Chang]

Attachment: Patch v2

I inspected all calls to NSPR functions and made sure
we check the size argument before passing it to NSPR.

The only check I am not sure about is in PORT_ArenaGrow.
I rely on the assertion that newsize >= oldsize and only
ensure newsize <= MAX_SIZE. If we look at the arguments
passed to PL_ARENA_GROW, we will need to ensure
oldsize <= MAX_SIZE and ( newsize - oldsize ) <= MAX_SIZE.
Both of these follow if newsize >= oldsize and
newsize <= MAX_SIZE.

Comment 2

[Daniel Veditz [:dveditz]]

Please use CVE-2013-1741 for this vulnerability.

Comment 3

[Robert Relyea]

wtc, the patch isn't for anything in the sec ans1 parser, it's to the generic allocate functions. There I think we should be checking against max uint. On a 64-bit platform, it's permissible to allocate more than 4 G (though I don't think NSS does it, it's possible someone may had us a large encrypted buffer, for instance).

Comment 4

[Wan-Teh Chang]

Bob: the NSPR memory allocation functions (PR_Malloc, PR_Realloc, and PR_Calloc)
use the PRUint32 type for the size parameter. So we can increase MAX_SIZE from
0x7fffffffUL to 0xffffffffUL, but not further.

An alternative is to change the NSS memory allocation functions (PORT_Alloc,
PORT_Realloc, and PORT_ZAlloc) to call the libc memory allocation functions
directly. Then we can safely pass the size_t arguments to malloc, realloc,
and calloc.

Comment 5

[Robert Relyea]

Ah, OK, so we can't allocation more than 4G today anyway. I think my only issue then is we probably should get MAX_SIZE from an NSPR define, but that's a minor issue. We probably should include a comment on where it came from.

bob

Comment 6

[Wan-Teh Chang]

Attachment: Patch v3

Bob: I define MAX_SIZE as PR_UINT32_MAX and added a comment.

Let me know if you want me to PR_INT32_MAX instead, which is
the original value (0x7fffffffUL). Thanks.

Comment 7

[Robert Relyea]

It should match whatever NSPR's parameter is. Thanks.

bob

Comment 8

[Robert Relyea]

Comment on attachment 815482 [details] [diff] [review]
Patch v3

r+ rrelyea

Comment 9

[Wan-Teh Chang]

I looked into why the current value of MAX_SIZE is 0x7fffffffUL.
Nelson added it. Nelson, do you remember why you cap allocation
size below 2GB rather than 4GB?

Here is Nelson's change to secport.c:

2003-11-19 18:06  Dont attempt to allocate 2GB or more from an arenapool.
                  Bugscape bug 53875. r=relyea.

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=secport.c&branch=1.31&root=/cvsroot&subdir=mozilla/security/nss/lib/util&command=DIFF_FRAMESET&rev1=1.15&rev2=1.16

and a related change to secasn1d.c:

2003-11-19 18:08  Don't accept ASN.1 items whose length is 2GB or more.
                  Bugscape bug 53875. r=wchang0222 and r=relyea.

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=secasn1d.c&branch=1.40&root=/cvsroot&subdir=mozilla/security/nss/lib/util&command=DIFF_FRAMESET&rev1=1.31&rev2=1.32

The change to secasn1d.c is especially interesting because the old
maximum ASN.1 item length was 4GB-1.

Perhaps the 2GB-1 maximum length is to allow the PLArena code to add some
small values to the size without overflowing PRUint32? I searched for "+"
in PL_ArenaAllocate and found these:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/lib/ds/plarena.c&rev=3.22&mark=143,156,174,198,199#143

a->base, a->avail, and a->limit are PRUwords, which are large enough to hold
pointers. So the problematic one is probably this:

198         PRUint32 sz = PR_MAX(pool->arenasize, nb);
199         sz += sizeof *a + pool->mask;  /* header and alignment slop */

So perhaps we should still define MAX_SIZE as PR_INT32_MAX or (PR_UINT32_MAX >> 1)?

Comment 10

[Wan-Teh Chang]

Attachment: Patch v4

To be safe, I changed MAX_SIZE back to the original value (0x7fffffff).
The rationale I used in the comment is that it will catch a negative
size.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/612d7d1eb9e7

Comment 11

[Wan-Teh Chang]

Attachment: Ensure an unsigned sum in PL_ArenaAllocate does not wrap around

I'm using the Precondition Test from
https://www.securecoding.cert.org/confluence/display/seccode/INT30-C.+Ensure+that+unsigned+integer+operations+do+not+wrap

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Can someone suggest a security rating for this?

Comment 13

[Wan-Teh Chang]

Al: the security rating should be a crash caused by buffer overrun of zero bytes
of an arbitrary length, that can be caused by a malicious https:// server using
a broken certificate.

Does this help?

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Yes. I'm going to just go with sec-critical here.

Comment 15

[Wan-Teh Chang]

Comment on attachment 816889 [details] [diff] [review]
Ensure an unsigned sum in PL_ArenaAllocate does not wrap around

I moved (a new version of) this patch to NSPR bug 927687.

Comment 16

[Daniel Veditz [:dveditz]]

writes limited to 0 bytes aren't the kind of immanent threat sec-critical usually represents. It _is_ possible to exploit a strategically placed 0 write in some cases; not sure this is one of those cases, but not sure it isn't so we'll call it sec-high.

Comment 17

[Daniel Veditz [:dveditz]]

I'm less worried about 4Gb of null, though.

Comment 18

[Kai Engert (:KaiE:)]

Does this fix **REQUIRE** the NSPR patches in the separate two bugs?

Comment 19

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #18)
> Does this fix **REQUIRE** the NSPR patches in the separate two bugs?

Answer is: NO

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

This should have been fixed by the landing of bug 935959 and the release of 25.0.1/17.0.11esr/24.1.1esr - Matt can you confirm before we go to build on our last beta next Monday (Dec 2)?

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

I can't really comment on this, as I don't have a deep understanding of the relationship between NSS/NSPR, and how we track issues like these across multiple bugs.

I have to punt this question directly to someone like Brian. Did our changes to and subsequent update to NSS 3.15.3 for bug 935959 already address this? If so, I will call it verified.

Comment 22

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

Yes, this is in Firefox 26 beta. See https://hg.mozilla.org/releases/mozilla-beta/annotate/43cd7d6a3970/security/nss/lib/util/secport.c#l72

I verified this by looking at the patch above, going to https://mxr.mozilla.org/mozilla-beta/, and then searching for the text in the comment that was added by the patch. I switched to the blame view and saw that the blame for this belongs to bug 935959, and that the change looks like the patch (didn't verify bit-for-bit).

Comment 23

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified FF24.2.0esr.

Comment 24

[u279076]

Matt, can you please verify this is fixed in the latest Aurora and Beta builds?

Comment 25

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed FF27/FF28, 2014-01-18.