1440926 - (CVE-2018-5144) Overflow in nsUnicodeToBIG5::GetMaxLength can create memory-safety bugs in callers

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[A.]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171128222554

Steps to reproduce:

nsUnicodeToBIG5::GetMaxLength (/ff-esr52/intl/uconv/ucvtw/nsUnicodeToBIG5.cpp) performs an unchecked integer computation based on the int32 value provided by the caller, and returns the result in the int32 out parameter provided by the caller:

NS_IMETHODIMP
nsUnicodeToBIG5::GetMaxLength(const char16_t* aSrc,
                              int32_t aSrcLength,
                              int32_t* aDestLength)
{
  *aDestLength = (aSrcLength * 2) + // <<<<<<<<<<<<<< (0)
                 (mPendingTrail ? 1 : 0) +
                 // If the lead ends up being paired, the bytes produced
                 // are already included above.
                 // If not, it produces a single '?'.
                 (mUtf16Lead ? 1 : 0);
  return NS_OK;
}

In case that aSrcLength is 0x7fffffff, or even 0x7ffffffd if the two flags mPendingTrail and mUtf16Lead are set to true, aDestLength will overflow at (0), and a near-zero negative value will be returned to the caller.

nsUnicodeToBIG5 is a member of nsIUnicodeEncoder family of classes, which provides an abstracted Unicode encoding facility to a variety of callers around the code base, including DOM and Javascript engines, which operate almost entirely on user-controlled data. The various nsUnicodeTo* classes may be invoked via a dispatcher instance of nsIUnicodeEncoder, as follows, for instance:

nsCOMPtr<nsIUnicodeEncoder> mEncoder;
mEncoder = EncodingUtils::EncoderForEncoding(charset); // charset value may be obtained from user-controlled data
mEncoder->GetMaxLength(src, src.Length(), ...);

In case that the charset is set to "big5", nsUnicodeToBIG5::GetMaxLength will be called. 

GetMaxLength is declared as follows in nsIUnicodeEncoder.h: 

  /**
   * Returns a quick estimation of the size of the buffer needed to hold the
   * converted data. Remember: this estimation is >= with the actual size of 
   * the buffer needed. It will be computed for the "worst case"
   *
   * @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_OK_UENC_EXACTLENGTH if an exact length was computed
   *                    NS_ERROR_OUT_OF_MEMORY if OOM
   *                    NS_OK if all we have is an approximation
   */
  MOZ_MUST_USE NS_IMETHOD GetMaxLength(const char16_t* aSrc,
                                       int32_t aSrcLength,
                                       int32_t* aDestLength) = 0;
...

Note that because GetMaxLength is prototyped as fallible, the callers expect it to signal of any internal failure, and therefore are unlikely to double-check the validity of the returned value.

The implications of a buffer size estimation procedure returning a negative value are almost certainly security relevant, however the actual impact depends on how exactly the caller uses the returned value. Consider for example, nsScriptableUnicodeConverter::ConvertFromUnicodeWithLength (/ff-esr52/intl/uconv/nsScriptableUConv.cpp):

nsresult
nsScriptableUnicodeConverter::ConvertFromUnicodeWithLength(const nsAString& aSrc,
                                                           int32_t* aOutLen,
                                                           char **_retval)
{
  if (!mEncoder)
    return NS_ERROR_FAILURE;

  nsresult rv = NS_OK;
  int32_t inLength = aSrc.Length();
  const nsAFlatString& flatSrc = PromiseFlatString(aSrc);
  rv = mEncoder->GetMaxLength(flatSrc.get(), inLength, aOutLen); // <<< (1)
  if (NS_SUCCEEDED(rv)) {
    *_retval = (char*)malloc(*aOutLen+1); // <<<<<<<<<< (2)
    if (!*_retval)
      return NS_ERROR_OUT_OF_MEMORY;

    rv = mEncoder->Convert(flatSrc.get(), &inLength, *_retval, aOutLen); // <<<<<<<<<< (3)
    if (NS_SUCCEEDED(rv))
    {
      (*_retval)[*aOutLen] = '\0'; // <<<<<<<<<<< (4)
      return NS_OK;
    }
    free(*_retval);
  }
  *_retval = nullptr;
  return NS_ERROR_FAILURE;
}

At (1), GetMaxLength() can return -1 (0xffffffff), which then would be used by malloc at (2) to allocate a buffer of size 0, leading to a heap corruption (i.e. a buffer overflow, or nulling of the trailing byte of the preceding heap chunk) at either (3) or/and (4). 

There are also other places in the code base, where GetMaxLength() is used in a similar fashion.

The issue was verified with the latest release code of Firefox ESR.


Actual results:

n/a


Expected results:

n/a

Comment 1

[:Gijs (he/him)]

This code was removed from Firefox 56 and later in bug 1261841. Henri/:emk, can you take a look and see if this needs fixing in ESR?

Comment 2

[:Gijs (he/him)]

+ni for comment #1.

Comment 3

[Masatoshi Kimura [:emk]]

This bug was once fixed in bug 1170794 and then re-introduced in bug 912470 :(  Hopefully Rust will prevent from re-introducing this once again...

Comment 4

[Henri Sivonen (:hsivonen)]

This needs a fix analogous to bug 1170794.

> Hopefully Rust will prevent from re-introducing this once again...

The corresponding Rust code introduced in 56 uses overflow-checking math, but Rust in itself doesn't prevent this class of bug, because integer overflow is deemed safe in Rust, because attempts to use integers that have overflown for indexing is safe, because Rust checks bounds at that point.

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Check for overflow

Comment 6

[Masatoshi Kimura [:emk]]

Comment on attachment 8953974 [details] [diff] [review]
Check for overflow

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

Please also fix nsBIG5ToUnicode while we are here.

::: intl/uconv/ucvtw/nsUnicodeToBIG5.cpp
@@ +222,5 @@
> +  if (mUtf16Lead) {
> +    length += 1;
> +  }
> +  if (!length.isValid()) {
> +    return NS_ERROR_FAILURE;

NS_ERROR_OUT_OF_MEMORY for consistency with the interface contract and other GetMaxLength implementations.

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Check for overflow, v2

Comment 8

[Henri Sivonen (:hsivonen)]

Comment on attachment 8953979 [details] [diff] [review]
Check for overflow, v2

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:

Potential memory-unsafety if the attacker manages to provide very large inputs, which seems feasible on 64-bit systems.

> User impact if declined: 

Unfixed potential memory-unsafety.

> Fix Landed on Version:

This fix has not landed on central, because this is ESR-specific. For non-ESR, this was fixed in 56 by the means of replacing this code with Rust code that uses overflow-checking math, so that fix isn't practically backportable.

> Risk to taking this patch (and alternatives if risky): 

Very low risk.

> String or UUID changes made by this patch: 

None.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Do have have an idea of what the sec rating is for this bug?

Comment 10

[Henri Sivonen (:hsivonen)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> Do have have an idea of what the sec rating is for this bug?

While we don't have a demonstrated exploit, the general characteristics of the bug together with reachability via setting the query string on a URL (<a href>) via JS and via form submission look like a presumptive sec-critical to me.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8953979 [details] [diff] [review]
Check for overflow, v2

Thanks. Fixes a sec-crit, approved for 52.7.0.

Comment 12

[Henri Sivonen (:hsivonen)]

Hmm. Bug 1170794 was rated sec-moderate. I'll leave it to the security team to downgrade the rating here if appropriate.

Comment 13

[Daniel Veditz [:dveditz]]

I don't think malicious servers can feed us strings long enough to trigger this bug. Javascript strings are limited (in firefox) to 0x0fffffff characters, or 0x1ffffffe bytes.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/1df9b4404acd

Comment 15

[A.]

A few general notes regarding possible impact of bugs similar to this one:

1. An attack on Firefox was demonstrated a while ago, involving very large inputs from a malicious server (bug #1299686). [0] It used a simple custom web server written in Python to send 4Gb of webpage to clients. [1] Both the scenario and the implementation are entirely practical within a trivial threat model. 

2. Consumers of nsIUnicodeEncoder/Decoder can use reinterpret_cast from a byte array, which may be less restricting in terms of the input size. Consider for instance, TextDecoder (/ff-esr52/dom/encoding/TextDecoder.cpp). TextDecoder::Decode is an overloaded function, which initially accepts an ArrayBuffer from JavaScript as its input, then casts it to a C-string and calls its variant which does the actual work:

void
TextDecoder::Decode(const Optional<ArrayBufferViewOrArrayBuffer>& aBuffer,
                    const TextDecodeOptions& aOptions,
                    nsAString& aOutDecodedString,
                    ErrorResult& aRv)
{
  ...
  if (length > INT32_MAX) {   // <<<<<<<<< (0)
    aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
    return;
  }
  Decode(reinterpret_cast<char*>(data), length, aOptions.mStream,
         aOutDecodedString, aRv);
}

void
TextDecoder::Decode(const char* aInput, const int32_t aLength,
                    const bool aStream, nsAString& aOutDecodedString,
                    ErrorResult& aRv)
{
...
  int32_t outLen;
  nsresult rv = mDecoder->GetMaxLength(aInput, aLength, &outLen); // <<<<<<<< (1)
...
  auto buf = MakeUniqueFallible<char16_t[]>(outLen + 1);
...
}

The input ArrayBuffer is limited to INT32_MAX via (0), which is enough to overflow later in GetMaxLength().
mDecoder at (1) is an instance of nsIUnicodeDecoder class, where the charset (and therefore the specific implementation of GetMaxLength to be called) is user-controlled.


--
[0] https://saelo.github.io/posts/firefox-script-loader-overflow.html
[1] https://github.com/saelo/foxpwn

Comment 16

[Daniel Veditz [:dveditz]]

If you can demonstrate that actually works in practice we can up-rate the bug.