r2 Reporter
	Description • 6 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20100101

Steps to reproduce:

The issue is in ScriptLoader::ConvertToUTF16 (https://searchfox.org/mozilla-central/source/dom/script/ScriptLoader.cpp) :

  CheckedInt<size_t> unicodeLength =
    unicodeDecoder->MaxUTF16BufferLength(aLength);
  if (!unicodeLength.isValid()) {
    return NS_ERROR_OUT_OF_MEMORY;
  }

  aBufOut =
    static_cast<char16_t*>(js_malloc(unicodeLength.value() * sizeof(char16_t)));
  if (!aBufOut) {
    aLengthOut = 0;
    return NS_ERROR_OUT_OF_MEMORY;
  }

At line 2826, unicodeLength is calculated and sanitized against an integer overflow via the CheckedInt class. However just a few lines below, the raw value of unicodeLength (unicodeLength.value()) is multiplied by 2 (=sizeof(char16_t)) and thus can cause an overflow! In such case js_malloc will allocate a smaller buffer than required, will all the usual arbitrary out-of-bounds write consequences.

It's not clear whether any checks in the (multiple) callers sanitize away the overflow window, however the fact that this code uses the CheckedInt class strongly suggests that either it has overflowed in the past, or it has a potential for the overflow.

I checked the crash stats and they do contain quite a few bugs around this code, but maybe not all of them due to this specific issue.

	Ryan VanderMeulen [:RyanVM]
	Comment 1 • 6 years ago

Henri, it appears you wrote the code in question. Can you please take a look?

	Henri Sivonen (:hsivonen) Assignee
	Comment 2 • 6 years ago

(In reply to r from comment #0)
> It's not clear whether any checks in the (multiple) callers sanitize away
> the overflow window, however the fact that this code uses

Indeed we shouldn't trust the callers not to pass values that can't overflow. I'll fix this.

At least we know there won't be overflow in 64-bit builds.

> the CheckedInt
> class strongly suggests that either it has overflowed in the past, or it has
> a potential for the overflow.

Note that MaxUTF16BufferLength() returns CheckedInt<size_t>, so it's not an indication of past overflow at this particular place in the code. (Indeed, MaxUTF16BufferLength() returning CheckedInt<size_t> also explains why a mass refactoring resulted in seemingly silly code.)

	Henri Sivonen (:hsivonen) Assignee
	Comment 3 • 6 years ago

MozReview-Commit-ID: APHZ45HWNt

	Henri Sivonen (:hsivonen) Assignee
	Comment 4 • 6 years ago

Comment on attachment 9013261 [details]
Bug 1495011.

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Depends on details in the various callers. I haven't analyzed the callers well enough to say.

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

If not all supported branches, which bug introduced the flaw?: Bug 987556

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: Either the same patch applies or the backport is trivial.

How likely is this patch to cause regressions; how much testing does it need?: Extremely unlikely to cause regressions, since it affects cases where an unsigned 32-bit integer would overflow. (In 32-bit builds only; 64-bit builds are unaffected.)

	Ryan VanderMeulen [:RyanVM]
	Comment 5 • 6 years ago

No worries from RelMan on taking this for the last 63 Betas this coming week.

	Al Billings [:abillings - ex-MoCo]
	Comment 6 • 6 years ago

Comment on attachment 9013261 [details]
Bug 1495011.

sec-approval+ for trunk.
Please nominate beta and ESR60 patches as well.

	Henri Sivonen (:hsivonen) Assignee
	Comment 7 • 6 years ago

Comment on attachment 9013261 [details]
Bug 1495011.

Turns out the same patch applies to branches.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 987556

User impact if declined: There might exist (not verified either way) a way to cause out-of-bounds write in 32-bit builds. The impact would be the general consequences of out-of-bounds write.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The change is not risky, because it merely adds an integer overflow check and shouldn't affect cases that don't overflow.

String changes made/needed: None

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: There might exist (not verified either way) a way to cause out-of-bounds write in 32-bit builds.

User impact if declined: The impact would be the general consequences of out-of-bounds write.

Fix Landed on Version: 64

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The change is not risky, because it merely adds an integer overflow check and shouldn't affect cases that don't overflow.

String or UUID changes made by this patch: None

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 6 years ago

This was pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f6d07b35d1

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 9 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/a5f6d07b35d1

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 10 • 6 years ago

Comment on attachment 9013261 [details]
Bug 1495011.

Uplift approved for 63 beta 14, thanks.

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/a3adfbef6bfa

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 6 years ago

Comment on attachment 9013261 [details]
Bug 1495011.

Fixes possible integer overflows on 32-bit builds. Approved for ESR 60.3.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/c4fb48bb5d28