1170794 - (CVE-2015-4522) Overflow in nsUnicodeToUTF8::GetMaxLength can create memory-safety bugs in callers

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[q1]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150305021524

Steps to reproduce:

nsUnicodeToUTF8::GetMaxLength (38.0.1\intl\uconv\nsUnicodeToUTF8.cpp) computes the destination string length *aDestLength without checking for overflow. This means that it can return a destination string length _smaller_ than the source string length, which can then cause memory-safety bugs in its caller:

15: NS_IMETHODIMP nsUnicodeToUTF8::GetMaxLength(const char16_t * aSrc, 
16:                                              int32_t aSrcLength,
17:                                              int32_t * aDestLength)
18: {
19:   // aSrc is interpreted as UTF16, 3 is normally enough.
20:   // But when previous buffer only contains part of the surrogate pair, we 
21:   // need to complete it here. If the first word in following buffer is not
22:   // in valid surrogate range, we need to convert the remaining of last buffer
23:   // to 3 bytes.
24:   *aDestLength = 3*aSrcLength + 3;
25:   return NS_OK;
26: }

If aSrcLength >= 0x2aaaaaaa, line 24 computes a negative value and returns it in *aDestLength. If the caller then uses this value without checking it, it could, for example, fail to allocate enough space for a string, causing overwriting of unowned memory. For example, EncodeString (38.0.1\netwerk\base\nsStandardURL.cpp) uses the returned value to allocate a buffer:

66: static nsresult
67: EncodeString(nsIUnicodeEncoder *encoder, const nsAFlatString &str, nsACString &result)
68: {
69:    nsresult rv;
70:    int32_t len = str.Length();
71:    int32_t maxlen;
72:
73:    rv = encoder->GetMaxLength(str.get(), len, &maxlen);
74:    if (NS_FAILED(rv))
75:        return rv;
76:
77:    char buf[256], *p = buf;
78:    if (uint32_t(maxlen) > sizeof(buf) - 1) {
79:        p = (char *) malloc(maxlen + 1);
80:        if (!p)
81:            return NS_ERROR_OUT_OF_MEMORY;
82:    }
83:
84:    rv = encoder->Convert(str.get(), &len, p, &maxlen);
85:    if (NS_FAILED(rv))
86:        goto end;
87:    ...
92:    p[maxlen] = 0;
93:    result.Assign(p);
94:
95:    len = sizeof(buf) - 1;
96:    rv = encoder->Finish(buf, &len);
97:    if (NS_FAILED(rv))
98:        goto end;
99:    buf[len] = 0;    ...

If str.Length() == 0x55555554 and encoder isa nsUnicodeToUTF8, then nsUnicodeToUTF8::GetMaxLength will compute maxlen == 0xffffffff (== -1), and line 79 will compute and allocate a buffer of size -1+1 == 0. Line 84 won't cause a problem because of how nsUnicodeToUTF8::Convert is implemented, but line 92 will zero the byte at p-1.

This write will cause unpredictable effects, possibly including disclosure of sensitive information.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: crash3.patch

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8614653 [details] [diff] [review]
crash3.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I think quite hard.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes.

Which older supported branches are affected by this flaw?

All of the branches.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Backporting is easy.

How likely is this patch to cause regressions; how much testing does it need?

No regressions.

Comment 3

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

This needs a security rating before it can get sec-approval+. 

Is this simply information disclosure? What would happen if it is exploited?

Comment 4

[q1]

That's a good question. Zeroing a random byte of memory could do almost anything, depending upon what the byte is. If it's a prohibited operations mask, it could give an attacker permission to read and/or write something to which she shouldn't have access. If it's part of a pointer, it could cause data to be read or written from an improper location, with unknown effects. If it's part of a string, it could cause truncation, with unknown effects. If it's an object's virtual function pointer, it could cause execution of incorrect functions, again with unknown effects.

Comment 5

[Simon Montagu :smontagu]

Has anybody audited other encodings for a similar attack? The base implementation of GetMaxLength, http://mxr.mozilla.org/mozilla-central/source/intl/uconv/util/nsUCSupport.cpp#550, does

*aDestLength = aSrcLength * mMaxLengthFactor;

where mMaxLengthFactor can be 2 or 4 in multi-byte character sets.

Comment 6

[(no longer active)]

(In reply to Simon Montagu :smontagu from comment #5)
> Has anybody audited other encodings for a similar attack? The base
> implementation of GetMaxLength,
> http://mxr.mozilla.org/mozilla-central/source/intl/uconv/util/nsUCSupport.
> cpp#550, does
> 
> *aDestLength = aSrcLength * mMaxLengthFactor;
> 
> where mMaxLengthFactor can be 2 or 4 in multi-byte character sets.

That looks to be similarly affected based on a quick look.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 8614653 [details] [diff] [review]
crash3.patch

r- dveditz

Checking this in highlights the problem while not fixing the identical problem in several easily-found equivalent spots (that is, all the other decoders that implement the same method).

Comment 8

[Daniel Veditz [:dveditz]]

Without more evidence this can be reached/triggered we need to go with sec-moderate

Comment 9

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

Comment on attachment 8614653 [details] [diff] [review]
crash3.patch

clearing sec-approval? since sec-moderate bugs don't need sec-approval+

Comment 10

[Mats Palmgren (inactive)]

It's not just encoders, it's decoders as well, e.g.:
http://mxr.mozilla.org/mozilla-central/source/intl/uconv/util/nsUCSupport.cpp#183
http://mxr.mozilla.org/mozilla-central/source/intl/uconv/nsUTF8ToUnicode.cpp#51
...

After we make GetMaxLength fail on overflow, we need to fix some consumers -
several don't check the return value:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsUnicharStreamLoader.cpp#214
http://mxr.mozilla.org/mozilla-central/source/parser/htmlparser/nsScanner.cpp#244
http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsClipboard.cpp#719
...

Here's one encoder consumer that does a potential buffer overflow on failure:
http://mxr.mozilla.org/mozilla-central/source/intl/uconv/nsIUnicodeEncoder.h#49
fortunately I can't find any uses of ENCODER_BUFFER_ALLOC_IF_NEEDED so we
should just remove it.
http://mxr.mozilla.org/mozilla-central/search?string=ENCODER_BUFFER_ALLOC_IF_NEEDED

We should also update the @return documentation in
http://mxr.mozilla.org/mozilla-central/source/intl/uconv/nsIUnicodeDecoder.h#105
http://mxr.mozilla.org/mozilla-central/source/intl/uconv/nsIUnicodeEncoder.h#158

We should add MOZ_WARN_UNUSED_RESULT to the GetMaxLength signatures, if possible.

We should audit most of these:
http://mxr.mozilla.org/comm-central/search?string=GetMaxLength

Comment 11

[Andrea Marchesini [:baku]]

Attachment: crash3.patch

Comment 12

[Andrea Marchesini [:baku]]

Attachment: crash3.patch

Comment 13

[Andrea Marchesini [:baku]]

Any news about this patch?

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 8617799 [details] [diff] [review]
crash3.patch

Review of attachment 8617799 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with some nits to fix.
r=dveditz

::: intl/uconv/nsIUnicodeDecoder.h
@@ +102,5 @@
>     * @param aSrc        [IN] the source data buffer
>     * @param aSrcLength  [IN] the length of source data buffer
>     * @param aDestLength [OUT] the needed size of the destination buffer
>     * @return            NS_EXACT_LENGTH if an exact length was computed
> +   *                    NS_ERROR_OUT_OF_MEMORY if OOM

Here you add NS_ERROR_OUT_OF_MEMORY to the comment, but in the implementations you return NS_ERROR_FAILURE (and it's not exactly OOM). I don't really care which you use but they should match.

@@ +107,5 @@
>     *                    NS_OK is all we have is an approximation
>     */
> +   MOZ_WARN_UNUSED_RESULT NS_IMETHOD GetMaxLength(const char * aSrc,
> +                                                  int32_t aSrcLength,
> +                                                  int32_t * aDestLength) = 0;

Personally I don't really care, but in all my recent patches I've been nagged to rewrite these to put the * on the left any time I've touched old code that had them written with the * (or &) on the right or in the middle as here. If you get the chance please edit this before checking in.

Ditto all the similar places below.

The key bit of this edit, MOZ_WARN_UNUSUED_RESULT, is of course great.

::: intl/uconv/ucvlatin/nsUnicodeToUTF16.cpp
@@ +57,5 @@
>  {
> +  mozilla::CheckedInt32 length = 2;
> +
> +  if(0 != mBOM) {
> +    length *= (aSrcLength+1);

Space after "if" please. In theory aSecLength could be max int32 and the +1 could overflow to -max int32. Here the "length *=" check should catch it, but it might be better to rewrite this so all the math is checked:

  mozilla::CheckedInt32 length = aSrcLength;

  if (0 != mBOM) {
    length += 1;
  }
  length *=2;

Comment 15

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d87836297505

Comment 16

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d772d098a392

I forgot to run |hg qref| with the comments.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d87836297505
https://hg.mozilla.org/mozilla-central/rev/d772d098a392

Comment 18

[Daniel Holbert [:dholbert] (PTO until 2/24)]

This bug's fix inadvertently caused clang 3.6 & newer to spam some "-Winconsistent-missing-override" build warnings. (by adding 'override' annotations to classes that were previously 100% "override"-annotation-free -- so now they're "inconsistent", hence the warning)

I pushed a followup to add that keyword where it was previously missing, to fix this warning. Using blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2.
https://hg.mozilla.org/integration/mozilla-inbound/rev/46adc427071d

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/46adc427071d