Bug 1500011 (CVE-2018-18498)

Unsafe use of CheckedInt32 in nsContentUtils::CalculateBufferSizeForImage

RESOLVED FIXED in Firefox -esr60

Status

()

defect
RESOLVED FIXED
10 months ago
12 days ago

People

(Reporter: r2, Assigned: Nika)

Tracking

({csectype-intoverflow, regression, sec-low})

Trunk
mozilla65
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty -
qe-verify -

Firefox Tracking Flags

(firefox-esr6064+ fixed, firefox63 wontfix, firefox64+ fixed, firefox65+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main64+][adv-esr60.4+])

Attachments

(1 attachment)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:61.0) Gecko/20100101 Firefox/61.0

Steps to reproduce:

This is the same issue as bug#1495011. 

nsresult
nsContentUtils::CalculateBufferSizeForImage(const uint32_t& aStride,
                                            const IntSize& aImageSize,
                                            const SurfaceFormat& aFormat,
                                            size_t* aMaxBufferSize,
                                            size_t* aUsedBufferSize)
{
  CheckedInt32 requiredBytes =
    CheckedInt32(aStride) * CheckedInt32(aImageSize.height);
  if (!requiredBytes.isValid()) {
    return NS_ERROR_FAILURE;
  }
  *aMaxBufferSize = requiredBytes.value();
  *aUsedBufferSize = *aMaxBufferSize - aStride + (aImageSize.width * BytesPerPixel(aFormat));
  return NS_OK;
}

https://searchfox.org/mozilla-central/source/dom/base/nsContentUtils.cpp#7863

requiredBytes is checked for an overflow, however a couple of lines later its raw unchecked value is used in arithmetic calculations again. Thus aUsedBufferSize can potentially overflow on x86 systems.
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM
Product: Firefox → Core
Looks like this was introduced in bug 1473161 which was also a sec bug and backported to esr60. Nika, you reviewed, can you take a look or pass this onto someone else who can?
Blocks: 1473161
Flags: needinfo?(nika)
I've changed it to use CheckedInt32 more pervasively - thanks.
Assignee: nobody → nika
Flags: needinfo?(nika)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3fad30f081a0bcd4aeae913942fc360dbdbf30e
https://hg.mozilla.org/mozilla-central/rev/a3fad30f081a
Group: dom-core-security → core-security-release
Status: UNCONFIRMED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
This needs a sec rating (and potentially a retroactive sec approval request depending on severity).
Comment on attachment 9018379 [details]
Bug 1500011 - Use CheckedInt more in CalculateBufferSizeForImage,

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: potential parent process memory error with content process code execution.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

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): Minor obviously-correct changes to small function

String changes made/needed: None

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Low impact

User impact if declined: potential parent process memory error with content process code execution.

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Minor obviously-correct changes to small function

String or UUID changes made by this patch: None
Flags: needinfo?(nika)
Attachment #9018379 - Flags: approval-mozilla-esr60?
Attachment #9018379 - Flags: approval-mozilla-beta?
I think this is sec-low btw.
Comment on attachment 9018379 [details]
Bug 1500011 - Use CheckedInt more in CalculateBufferSizeForImage,

Fixes a minor sec issue, approved for 64.0b4. We'll revisit the ESR request later in the cycle.
Attachment #9018379 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Comment on attachment 9018379 [details]
Bug 1500011 - Use CheckedInt more in CalculateBufferSizeForImage,

Seems pretty low-impact, but keeps our code consistent across releases anyway and very low-risk. Approved for 60.4.0esr.
Attachment #9018379 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: sec-bounty?
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main64+][adv-esr60.4+]
Alias: CVE-2018-18498
Summary: Unsafe use of CheckedInt32 #2 → Unsafe use of CheckedInt32 in nsContentUtils::CalculateBufferSizeForImage
Sadly sec-low bugs do not qualify for the bug bounty program
Flags: sec-bounty? → sec-bounty-
Component: DOM → DOM: Core & HTML
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.