1295747 - Latent overflow in AppendUTF16toUTF8() could cause buffer overrun

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[q1]

AppendUTF16toUTF8() (xpcom\string\nsReadableUtils.cpp) can experience an integer overflow. If it does, it allocates a buffer too short for the data that it writes into it.

This bug appears to be latent because there appear to be no callers that append to a destination string that is long enough to trigger the bug. (Most callers, e.g., NS_ConvertUTF16toUTF8(), append to a zero-length destination string).

The bug is in line 192:

177: bool
178: AppendUTF16toUTF8(const nsAString& aSource, nsACString& aDest,
179:                   const mozilla::fallible_t& aFallible)
180: {
181:   nsAString::const_iterator source_start, source_end;
182:   CalculateUTF8Size calculator;
183:   copy_string(aSource.BeginReading(source_start),
184:               aSource.EndReading(source_end), calculator);
185: 
186:   uint32_t count = calculator.Size();
187: 
188:   if (count) {
189:     uint32_t old_dest_length = aDest.Length();
190: 
191:     // Grow the buffer if we need to.
192:     if (!aDest.SetLength(old_dest_length + count, aFallible)) {
193:       return false;
194:     }
195: 
196:     // All ready? Time to convert
197: 
198:     ConvertUTF16toUTF8 converter(aDest.BeginWriting() + old_dest_length);
199:     copy_string(aSource.BeginReading(source_start),
200:                 aSource.EndReading(source_end), converter);
201: ...

The problem arises because a single UTF-16 character can be represented by up to 3 UTF-8 characters.

Imagine that |aSource| is as long as a UTF-16 nsAString can be (0x3FFFFFF9 characters [1]), and that it contains only characters that are represented by 3 UTF-8 characters (e.g., 0xe000). In this case, the resulting string is 3*0x3FFFFFF9 = 0xBFFFFFEB UTF-8 characters long. Since CalculateURF8Size::write() [2] -- which calculates this quantity -- does not actually attempt to write anything, its result is not limited by the maximum size of a UTF-8 string as enforced by [1] (which would be 0x7FFFFFF5 characters).

This means that, if |aDest| is also at least 0x100000000-0xBFFFFFEB = 0x40000015 characters long -- well within the limit for a UTF-8 strong -- the addition on line 192 overflows, causing allocation of a buffer that is too small to contain the resulting string.

This bug is still present in trunk: http://hg.mozilla.org/mozilla-central/file/tip/xpcom/string/nsReadableUtils.cpp .


[1] This limit is enforced by nsTSubstring_CharT::MutatePrep().

[2] In xpcom\string\nsUTF8Utils.h.

Comment 1

[Daniel Veditz [:dveditz]]

Eric: please take a look when you get a chance.

Comment 2

[Eric Rahm [:erahm]]

(In reply to Daniel Veditz [:dveditz] from comment #1)
> Eric: please take a look when you get a chance.

I'll take a look today, leaving ni until I have more info.

Comment 3

[Eric Rahm [:erahm]]

q1, thank you for the detailed analysis, this does appear to be an issue.

I think we should:
  1) use the proper size types for each call
     * |size_t| for calculator
     * |nsACString::size_type| for dest_length (size_type happens to be uint32_t but we should be explicit)
  2) use CheckedInt to guarantee that we detect if the addition overflows

Patch is forthcoming.

Comment 4

[Eric Rahm [:erahm]]

Attachment: Use CheckedInt in size calculation

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8786074 [details] [diff] [review]
Use CheckedInt in size calculation

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

::: xpcom/string/nsReadableUtils.cpp
@@ +184,5 @@
>    CalculateUTF8Size calculator;
>    copy_string(aSource.BeginReading(source_start),
>                aSource.EndReading(source_end), calculator);
>  
> +  size_t count = calculator.Size();

Avoid truncation of count.

@@ +192,3 @@
>  
>      // Grow the buffer if we need to.
> +    CheckedInt<nsACString::size_type> new_length(count);

If count is too large to fit in size_type the CheckedInt will be invalid.

@@ +192,4 @@
>  
>      // Grow the buffer if we need to.
> +    CheckedInt<nsACString::size_type> new_length(count);
> +    new_length += old_dest_length;

Overflow will be detected here.

@@ +194,5 @@
> +    CheckedInt<nsACString::size_type> new_length(count);
> +    new_length += old_dest_length;
> +
> +    if (!new_length.isValid() ||
> +        !aDest.SetLength(new_length.value(), aFallible)) {

And |SetLength| will enforce any internal size limits (as noted in comment 0).

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8786074 [details] [diff] [review]
Use CheckedInt in size calculation

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

WFM.  Thanks.

Comment 7

[Eric Rahm [:erahm]]

Attachment: Use CheckedInt in size calculation

Add missing mozilla namespace

Comment 8

[Eric Rahm [:erahm]]

Comment on attachment 8786088 [details] [diff] [review]
Use CheckedInt in size calculation

(Carrying forward r+)

Doesn't have a sec rating. This is a potential buffer overflow, but as-is we don't think it's currently exploitable.

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

We don't think this is currently an issue.

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

We're adding CheckedInt, so it's pretty clear we're checking for an integer overflow.

> Which older supported branches are affected by this flaw?

All.

> If not all supported branches, which bug introduced the flaw?

N/A

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

No, it would be pretty easy but we think only new usage of |AppendUTF16toUTF8| would trigger an issue.

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

Low, we're just adding an integer overflow check.

Comment 9

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

Comment on attachment 8786088 [details] [diff] [review]
Use CheckedInt in size calculation

We've made this sec-moderate so this doesn't need sec-approval to land on trunk.

Comment 10

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6203381730cf2ac9f1ad56ce94b1fb0fd9573f8c
Bug 1295747 - Use CheckedInt in size calculation. r=froydnj

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6203381730cf