1174015 - (CVE-2016-1951) Overflow in prprf/GrowStuff can cause memory-safety bug

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[q1]

GrowStuff (38.0.1\nsprpub\pr\src\io\prprf.c) can cause an overflow if the size of the existing buffer in units of bytes (maxlen) + the size of the string to be appended (len) > 0xffffffff. In this case, the function either allocates a tiny buffer or doesn't even attempt to allocate one large enough, then writes the new string beyond the buffer's end.

1081: static int GrowStuff(SprintfState *ss, const char *sp, PRUint32 len)
1082: {
1083:     ptrdiff_t off;
1084:     char *newbase;
1085:     PRUint32 newlen;
1086: 
1087:     off = ss->cur - ss->base;
1088:     if (off + len >= ss->maxlen) {
1089:        /* Grow the buffer */
1090:        newlen = ss->maxlen + ((len > 32) ? len : 32);
1091:        if (ss->base) {
1092:            newbase = (char*) PR_REALLOC(ss->base, newlen);
1093:        } else {
1094:            newbase = (char*) PR_MALLOC(newlen);
1095:        }
1096:        if (!newbase) {
1097:            /* Ran out of memory */
1098:            return -1;
1099:        }
1100:        ss->base = newbase;
1101:        ss->maxlen = newlen;
1102:        ss->cur = ss->base + off;
1103:     }
1104: 
1105:     /* Copy data */
1106:     while (len) {
1107:        --len;
1108:        *ss->cur++ = *sp++;
...


On 32-bit platforms, line 1088 can overflow. In that case, the code doesn't allocate a new buffer, but instead writes the new data beginning just beyond the end of the existing one. Presumably this can occur only if a caller attempts to write a single very long string, because otherwise the system would run out of memory.

On 64-bit platforms, line 1088 doesn't overflow (because ptrdiff_t isa signed 64-bit int), but line 1090 does. In that case, the code allocates a buffer that is much too small, then writes the new data far beyond its end.

The functions that use GrowStuff (PR_vsmprintf and PR_vsprintf_append in prptr.c) are used in several places, but I don't know whether any of the callers ultimately derive potentially very long strings from untrusted sources.

[BTW, I notice that the new Bugzilla doesn't handle pasting of long lines as gracefully as the old one].

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

q1: thank you very much for the bug report.

Kai and q1: please review this patch. I check for potential
overflows before the two additions.

Note that PR_Malloc and PR_Realloc functions are defined to
take a PRUint32 allocation size argument. This is why the
sums must not exceed PR_UINT32_MAX.

Comment 2

[q1]

Comment on attachment 8624977 [details] [diff] [review]
Proposed patch

>diff --git a/pr/src/io/prprf.c b/pr/src/io/prprf.c
>--- a/pr/src/io/prprf.c
>+++ b/pr/src/io/prprf.c
>@@ -1100,19 +1100,28 @@ PR_IMPLEMENT(PRUint32) PR_vsxprintf(PRSt
> */
> static int GrowStuff(SprintfState *ss, const char *sp, PRUint32 len)
> {
>     ptrdiff_t off;
>     char *newbase;
>     PRUint32 newlen;
> 
>     off = ss->cur - ss->base;

Hmm, I think that the above expression is not technically safe. ptrdiff_t is a *signed* type per C++11 s.5.7(5). Under 32 bit Windows, it's typedefed to int, so differences > 0x7fffffff underflow to negative values. This seems to invoke the clause of s.5.7(5) that says:

"As with any other arithmetic overflow, if the result does not fit in the space provided, the behavior is undefined."

It depends what that section means by "fit". The standards team is discussing changes to this section that would ratify my conclusion (http://open-std.org/JTC1/SC22/WG21/docs/papers/2015/n4430.html ):

"If the expressions P and Q point to, respectively, elements i and j of the same array object, the expression P - Q has the value i - j; otherwise, the behavior is undefined. [ Note: If the value i - j is not in the range of representable values of type std::ptrdiff_t, the behavior is undefined. — end note ]"

I *suppose* that, on real platforms, this expression produces the same bit pattern it would produce if ptrdiff_t were unsigned, so this is realistically safe?

Perhaps we should disallow strings long enough to invoke this problem on any supported platform (> 0x7fffffff). If you choose to do that, I recommend using mfbt\CheckedInt.h for all calculations involved in determining buffer sizes.

>+    if (PR_UINT32_MAX - len < off) {

With the above provisos, I think that this is OK, though the reason is rather subtle: s.4.7(2) by way of the "usual arithmetic conversions" s.5(10):

"If the destination type is unsigned, the resulting value is the least unsigned integer congruent to the source
integer (modulo 2 n where n is the number of bits used to represent the unsigned type). [Note: In a two’s
complement representation, this conversion is conceptual and there is no change in the bit pattern (if there
is no truncation). —end note ]"

>+	/* off + len would be too big. */
>+	return -1;
>+    }
>     if (off + len >= ss->maxlen) {
> 	/* Grow the buffer */
>-	newlen = ss->maxlen + ((len > 32) ? len : 32);
>+	PRUint32 increment = (len > 32) ? len : 32;
>+	if (PR_UINT32_MAX - ss->maxlen < increment) {
>+	    /* ss->maxlen + increment would overflow. */
>+	    return -1;
>+	}
>+	newlen = ss->maxlen + increment;

Same comments as above.

Comment 3

[q1]

How do I turn off the review-needed flag?

Comment 4

[Wan-Teh Chang]

(In reply to q1 from comment #2)
> Comment on attachment 8624977 [details] [diff] [review]
> Proposed patch
> 
> >diff --git a/pr/src/io/prprf.c b/pr/src/io/prprf.c
> >--- a/pr/src/io/prprf.c
> >+++ b/pr/src/io/prprf.c
> >@@ -1100,19 +1100,28 @@ PR_IMPLEMENT(PRUint32) PR_vsxprintf(PRSt
> > */
> > static int GrowStuff(SprintfState *ss, const char *sp, PRUint32 len)
> > {
> >     ptrdiff_t off;
> >     char *newbase;
> >     PRUint32 newlen;
> > 
> >     off = ss->cur - ss->base;
> 
> Hmm, I think that the above expression is not technically safe.

ptrdiff_t is defined as the type of the difference between two pointers
of the same type. So I think a C implementation needs to do whatever it
takes to prevent differences from underflowing to negative values, such
as making malloc() fail if the requested size is > 0x7fffffff under 32-bit
Windows.

I will attach a new version of the patch that doesn't allow ss->maxlen
to be > 0x7fffffff (PR_INT32_MAX). That will ensure ss->cur - ss->base
<= 0x7fffffff (PR_INT32_MAX).

Note: NSPR cannot use mfbt/CheckedInt.h. NSPR can only use system
libraries.

Comment 5

[Wan-Teh Chang]

Attachment: Proposed patch v2

Please review the new patch. Thanks.

Comment 6

[Wan-Teh Chang]

Attachment: Proposed patch v3

In this patch I updated the assertion and check in PR_vsnprintf
more accurately, preserving the comparisons of |outlen| with 0.

Comment 7

[Kai Engert [:KaiE:]]

+    if (PR_UINT32_MAX - ss->maxlen < len || ss->maxlen + len > PR_INT32_MAX) {

I agree this code is correct.

But this code took me too long to understand. I would prefer to simplify it, to make it easier to understand for less intelligent people like me. ;-)

Given that the sum must be smaller than the signed max, we can require that each must be smaller than the signed max. If both are smaller than the signed max, the sum will be smaller than the unsigned max.

Therefore I'd prefer to rewrite the failure condition above as:

  /* If both len and maxlen are smaller than PR_INT32_MAX, the sum cannot overflow. */
  if (len > PR_INT32_MAX || ss->maxlen > PR_INT32_MAX || ss->maxlen + len > PR_INT32_MAX) {
    return -1;
  }

If you don't like my code, then please add a comment to your original code like this:

  /* We require "ss->maxlen + len < PR_UINT32_MAX".
   * It ensures our calculation of the sum will not overflow PRUint32.
   * Rewriting the test as "PR_UINT32_MAX - ss->maxlen < len" ensures we don't need
   * a larger type for the test.
   */

If you think that a rewrite or a comment is unnecessary, I'd argue, this overflow risk wasn't obvious to the original programmer, and it might not be obvious for future readers.

Comment 8

[Kai Engert [:KaiE:]]

+	PRUint32 increment = (len > 32) ? len : 32;
+	if (PR_UINT32_MAX - ss->maxlen < increment) {
+	    /* ss->maxlen + increment would overflow. */
+	    return -1;
+	}

What if "ss->maxlen + 32 > PR_INT32_MAX",
but "ss->maxlen + len <= PR_INT32_MAX" ?

Do we want to detect this scenario, and only grow by len, even if "len<32" ?

Comment 9

[Kai Engert [:KaiE:]]

Comment on attachment 8625397 [details] [diff] [review]
Proposed patch v3

Your code looks correct to me. I believe it might be acceptable that we might not use the full size up to PR_INT32_MAX, but might potentially limit it to PR_INT32_MAX-31.

I should have your full patch first, because you added a comment to the later calculations, which made the intention clear. So, please consider to add some kind of comment to the first condition

Because I don't want to delay this fix further, r=kaie, with or without further improvements.

Comment 10

[Wan-Teh Chang]

Attachment: Proposed patch v4

Kai: thanks for the review! I added a comment to the first check
as you requested.

https://hg.mozilla.org/projects/nspr/rev/96381e3aaae2

Comment 11

[Kai Engert [:KaiE:]]

Should this get a CVE?

Who wants to contribute a wording for release notes?

Should this get backported to Firefox branches?

Comment 12

[Kai Engert [:KaiE:]]

How about:

* Fixed a memory allocation bug in the PR_vsmprintf and 
  PR_vsprintf_append functions.

Comment 13

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie) from comment #12)
> How about:
> 
> * Fixed a memory allocation bug in the PR_vsmprintf and 
>   PR_vsprintf_append functions.

Since nobody has responded, and since the function PR_vsxprintf might also be affected, and because I don't have time to fully analyze in order to write the release notes, I will use a simplified text like the following:

* fixed a memory allocation bug related to the PR_*printf functions

Comment 14

[Daniel Veditz [:dveditz]]

You can use CVE-2016-1951 for the CVE.

It would probably be nice to uplift this to Aurora and Beta so it's on the next ESR branch

Comment 15

[Kai Engert [:KaiE:]]

(In reply to Daniel Veditz [:dveditz] from comment #14)
> You can use CVE-2016-1951 for the CVE.

thanks


> It would probably be nice to uplift this to Aurora and Beta so it's on the
> next ESR branch

Aurora 46 has already been uplifted to use NSPR 4.12


It would be a good idea to uplift Beta 45 to NSPR 4.12, too.

Comment 16

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:
This bug is already fixed in Firefox 46, which means we'll need it in the ESR 45.1 release to match. In terms of testing it would be better to uplift NSS 4.12 to 45 directly (bug 1244062) now rather than do it after 45 for 45.1.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

If we want to do that, it should happen today...

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Too late for 45 as today is the release day.

Comment 19

[Kai Engert [:KaiE:]]

But there's ESR 45.

Comment 20

[Huzaifa Sidhpurwala]

Is it ok to mention this CVE publicly and link to the commit which fixes it?
(In a public Red Hat bug that is), i am asking because there is no mention on the CVE id in the release notes or anywhere else and it makes little sense to keep this private now.

Comment 21

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

(In reply to Huzaifa Sidhpurwala from comment #20)
> Is it ok to mention this CVE publicly and link to the commit which fixes it?
> (In a public Red Hat bug that is), i am asking because there is no mention
> on the CVE id in the release notes or anywhere else and it makes little
> sense to keep this private now.

Because we haven't shipped a release with this fixed yet. This is fixed in Firefox 46. We just shipped 45.

So, when 46 comes out, this will be in an advisory then.

As a sec-moderate, it is unlikely to be taken into an ESR release as well.

Comment 22

[Kai Engert [:KaiE:]]

(In reply to Al Billings [:abillings] from comment #21)
> (In reply to Huzaifa Sidhpurwala from comment #20)
> > Is it ok to mention this CVE publicly and link to the commit which fixes it?
> > (In a public Red Hat bug that is), i am asking because there is no mention
> > on the CVE id in the release notes or anywhere else and it makes little
> > sense to keep this private now.
> 
> Because we haven't shipped a release with this fixed yet. This is fixed in
> Firefox 46. We just shipped 45.
> 
> So, when 46 comes out, this will be in an advisory then.
> 
> As a sec-moderate, it is unlikely to be taken into an ESR release as well.

Al, I think the above is wrong.

This bug is fixed in NSPR 4.12.
NSPR 4.12 is already used by Firefox ESR 45.
The upgrade was done in bug 1244062.

See this page for an overview of NSPR/NSS versions used in the Firefox branches:
http://kuix.de/mozilla/versions/

Comment 23

[:Gijs (out for now; he/him)]

(In reply to Kai Engert (:kaie) from comment #22)
> This bug is fixed in NSPR 4.12.
> NSPR 4.12 is already used by Firefox ESR 45.
> The upgrade was done in bug 1244062.

In that case, should the flags on this bug be updated? If so, please do that.

Comment 24

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

(In reply to Kai Engert (:kaie) from comment #22)

> Al, I think the above is wrong.

 Too bad no one noticed a month ago.

> This bug is fixed in NSPR 4.12.
> NSPR 4.12 is already used by Firefox ESR 45.

Comment 15 from you stated that NSPR 4.12 needed uplift to Beta. No one ever said differently in this bug so, without going digging on my own, I had no idea this was fixed in 45...

> The upgrade was done in bug 1244062.

Someone should have mentioned that here a month ago.
 
> See this page for an overview of NSPR/NSS versions used in the Firefox
> branches:
> http://kuix.de/mozilla/versions/

Yes, I'm aware of this but when I have comments in a bug saying "We should maybe take this on beta", that implies it isn't on beta (which is 45) and then no one said otherwise.

Comment 25

[Huzaifa Sidhpurwala]

Folks, should this be public now?