Bug 925100 (CVE-2013-1741)

Integer truncation leads to runaway memset in NSS certificate parsing

VERIFIED FIXED in Firefox 25

Status

defect
P1
major
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: wtc, Assigned: wtc)

Tracking

({sec-moderate})

Dependency tree / graph

Firefox Tracking Flags

(firefox25+ fixed, firefox26+ verified, firefox27+ verified, firefox28+ verified, firefox-esr17 fixed, firefox-esr24 verified, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed, b2g-v1.3 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Posted patch Patch by Adam Langley (obsolete) — Splinter Review
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.
Attachment #815080 - Flags: review+
Posted patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #815080 - Attachment is obsolete: true
Attachment #815103 - Flags: review?(agl)
Please use CVE-2013-1741 for this vulnerability.
Alias: CVE-2013-1741
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).
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.
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
Posted patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #815103 - Attachment is obsolete: true
Attachment #815103 - Flags: review?(agl)
Attachment #815482 - Flags: review?(rrelyea)
It should match whatever NSPR's parameter is. Thanks.

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

r+ rrelyea
Attachment #815482 - Flags: review?(rrelyea) → review+
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)?
Posted patch Patch v4Splinter Review
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
Attachment #815482 - Attachment is obsolete: true
Attachment #816884 - Flags: checked-in+
Can someone suggest a security rating for this?
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?
Yes. I'm going to just go with sec-critical here.
Keywords: sec-critical
Depends on: CVE-2013-5607
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.
Attachment #816889 - Attachment is obsolete: true
Attachment #816889 - Flags: review?(rrelyea)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Keywords: sec-criticalsec-high
Blocks: 935959
I'm less worried about 4Gb of null, though.
Keywords: sec-highsec-moderate
Does this fix **REQUIRE** the NSPR patches in the separate two bugs?
(In reply to Kai Engert (:kaie) from comment #18)
> Does this fix **REQUIRE** the NSPR patches in the separate two bugs?

Answer is: NO
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)?
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.
Flags: needinfo?(mwobensmith) → needinfo?(brian)
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).
Flags: needinfo?(brian)
Group: crypto-core-security → core-security
Matt, can you please verify this is fixed in the latest Aurora and Beta builds?
Flags: needinfo?(mwobensmith)
Confirmed FF27/FF28, 2014-01-18.
Flags: needinfo?(mwobensmith)
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.