Closed Bug 925100 (CVE-2013-1741) Opened 7 years ago Closed 7 years ago
Integer truncation leads to runaway memset in NSS certificate parsing
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+
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.
Please use CVE-2013-1741 for this vulnerability.
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
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.
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)?
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
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
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.
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.
Status: ASSIGNED → RESOLVED
Closed: 7 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.
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).
Matt, can you please verify this is fixed in the latest Aurora and Beta builds?
Confirmed FF27/FF28, 2014-01-18.
You need to log in before you can comment on or make changes to this bug.