Closed Bug 1495011 (CVE-2018-12393) Opened 11 months ago Closed 11 months ago

Unsafe use of CheckedInt (possible buffer overflow) in ScriptLoader::ConvertToUTF16

Categories

(Core :: Internationalization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 63+ fixed
firefox62 --- wontfix
firefox63 + fixed
firefox64 + fixed

People

(Reporter: r2, Assigned: hsivonen)

Details

(Keywords: csectype-intoverflow, sec-high, Whiteboard: [post-critsmash-triage][adv-main63+][adv-esr60.3+])

Attachments

(1 file)

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.
Henri, it appears you wrote the code in question. Can you please take a look?
Group: firefox-core-security → dom-core-security
Component: Untriaged → Internationalization
Flags: needinfo?(hsivonen)
Product: Firefox → Core
Version: Trunk → unspecified
(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.)
Assignee: nobody → hsivonen
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(hsivonen)
Attached file Bug 1495011.
MozReview-Commit-ID: APHZ45HWNt
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.)
Attachment #9013261 - Flags: sec-approval?
No worries from RelMan on taking this for the last 63 Betas this coming week.
Flags: needinfo?(abillings)
Comment on attachment 9013261 [details]
Bug 1495011.

sec-approval+ for trunk.
Please nominate beta and ESR60 patches as well.
Flags: needinfo?(abillings)
Attachment #9013261 - Flags: sec-approval? → sec-approval+
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
Attachment #9013261 - Flags: approval-mozilla-esr60?
Attachment #9013261 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/a5f6d07b35d1
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment on attachment 9013261 [details]
Bug 1495011.

Uplift approved for 63 beta 14, thanks.
Attachment #9013261 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 9013261 [details]
Bug 1495011.

Fixes possible integer overflows on 32-bit builds. Approved for ESR 60.3.
Attachment #9013261 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main63+][adv-esr60.3+]
Alias: CVE-2018-12393
Summary: Unsafe use of CheckedInt (possible buffer overflow) → Unsafe use of CheckedInt (possible buffer overflow) in ScriptLoader::ConvertToUTF16
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.