Closed Bug 1393367 Opened 7 years ago Closed 6 years ago

Out-of-bound read due to unchecked sizes used for std::vector::resize()

Categories

(Core :: Graphics, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 60+ fixed
firefox59 --- wontfix
firefox60 + fixed
firefox61 + fixed

People

(Reporter: tedd, Assigned: mikokm)

References

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: [gfx-noted][post-critsmash-triage][adv-main60+][adv-esr52.8+])

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1393362 +++

As part of the sandboxing effort, printing was remoted to the parent in order to allow enabling the content process sandbox.

For remote printing, the content process writes Moz2D commands to a file which is used in the chrome process to redraw the content that shall be printed.

When deserializing the commands, a compromised content process can cause an out-of-bound read when supplying a value used for std::vector::resize()

This is a more practical example of potential issues mentioned in Bug 1393363 and Bug 1393362.

The out-of-bound read happens in [1] where |aFloat| is expected to have a size of 4, note the MOZ_ASSERT in line 2799 is for DEBUG only and not a MOZ_RELEASE_ASSERT, so this check is not present on release builds.

SetAttribute is called here [2] passing in &mPayload.front() and mPayload.size()
|mPayload| is the result of a std::vector::resize() when deserializing values coming from the content process here [3]. Supplying a value of 0 for |size| leads to a resize(0) (a valid call, no exception thrown) and when reaching SetAttribute(), although the vector size is passed as an argument, still a size of 4 is expected, thus leading to the out-of-bound read.

Note, given the architecture complexity and the content <-> parent communication, I haven't debugged this manually. This bug is filed as a result of reading the code.

[1] http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/gfx/2d/RecordedEventImpl.h#3035-3038
[2] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/gfx/2d/RecordedEventImpl.h#3035-3038
[3] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/gfx/2d/RecordedEventImpl.h#3063-3066
Priority: -- → P3
Whiteboard: [gfx-noted]
(In reply to Julian Hector [:tedd] [:jhector] from comment #0)
> The out-of-bound read happens in [1] where |aFloat| is expected to have a
> size of 4, note the MOZ_ASSERT in line 2799 is for DEBUG only and not a
> MOZ_RELEASE_ASSERT, so this check is not present on release builds.

What's your suggested fix? Convert the asserts into release asserts?
If so, is that something you could do? ;)
Flags: needinfo?(julian.r.hector)
I think that would fix that issue, however I am not familiar with the code and don't know if it relies some place else on a size of 4.

Also just realized, that the [1] reference is wrong in the initial report, it should point to:

http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/gfx/2d/FilterNodeSoftware.cpp#2794
Flags: needinfo?(julian.r.hector)
Hi Milan:

I have assigned these security bugs to you to reassign them to appropriate developers in your team to investigate and fix them.

Thanks!

Wennie
Assignee: nobody → milan
@mstange, would you agree that it made sense to turn this MOZ_ASSERT at <https://searchfox.org/mozilla-central/source/gfx/2d/FilterNodeSoftware.cpp#2592> into a release assert?

Seems like a simple one-line fix.
Flags: needinfo?(mstange)
That definitely makes sense, yes.
Flags: needinfo?(mstange)
Markus, thank you for the reply.
I'm also not sure if that should apply to both of the asserts at the beginning of the function.

It would be great if you could take it from here.
Given this is a sec-high, you shouldn't push to try and must request sec-approval before landing.
Flags: needinfo?(mstange)
Attached patch 1393367.patchSplinter Review
Assignee: milan → mikokm
Status: NEW → ASSIGNED
Attachment #8967323 - Flags: review?(fbraun)
Comment on attachment 8967323 [details] [diff] [review]
1393367.patch

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

Thanks!
I'm not a module peer though, so you'll need to request another review from someone else (e.g., mstange).
Afterwards you'll need to request sec-approval on the patch and then wait on further instructions before landing.
Attachment #8967323 - Flags: review?(fbraun) → review+
Comment on attachment 8967323 [details] [diff] [review]
1393367.patch

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

Thanks for taking this!
Attachment #8967323 - Flags: review+
Comment on attachment 8967323 [details] [diff] [review]
1393367.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
- Not easily, see comment 1.

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

Which older supported branches are affected by this flaw?

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
- These would be very similar, changing MOZ_ASSERT to MOZ_RELEASE_ASSERT.

How likely is this patch to cause regressions; how much testing does it need?
- If the existing code has bugs that trigger this assertion, they would cause crash in release build. This should not be the case.
Attachment #8967323 - Flags: sec-approval?
(In reply to Miko Mynttinen [:miko] from comment #10)

> - Not easily, see comment 1.
I meant see description here.
(In reply to Miko Mynttinen [:miko] from comment #10)

> Which older supported branches are affected by this flaw?
> 
> If not all supported branches, which bug introduced the flaw?
> 

No answers here and the status flags aren't set. How far back does this go?
Flags: needinfo?(mikokm)
> Which older supported branches are affected by this flaw?

Goes back to ESR52. Are these all supported branches?

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

See above :)
Flags: needinfo?(mstange)
Flags: needinfo?(mikokm)
57 is marked unaffected in the Status flags, which is part of the confusion. I'll fix that.

sec-approval+ for trunk. We'll want Beta and ESR52 patches made and nominated.
Attachment #8967323 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1908cd8ed88dd4f77a99dff39c193d7d3f435195

Please request Beta and ESR52 approval on this. It applies cleanly to both branches as-landed.
Flags: needinfo?(mikokm)
https://hg.mozilla.org/mozilla-central/rev/1908cd8ed88d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8967323 [details] [diff] [review]
1393367.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is sec-high bug, a possible sandbox escape.

User impact if declined: Possible compromise of parent process.

Fix Landed on Version: 61 so far. 60 in nomination.

Risk to taking this patch (and alternatives if risky): Minimal/none. This changes a debug assertion to release assertion.

String or UUID changes made by this patch:  None.
Flags: needinfo?(mikokm)
Attachment #8967323 - Flags: approval-mozilla-esr52?
Comment on attachment 8967323 [details] [diff] [review]
1393367.patch

Approval Request Comment
[Feature/Bug causing the regression]: A possible sandbox escape.
[User impact if declined]: A possible compromise of the parent process.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No, there is no PoC due to complexity.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The patch changes assertion to release assertion.
[String changes made/needed]: None.
Attachment #8967323 - Flags: approval-mozilla-beta?
Comment on attachment 8967323 [details] [diff] [review]
1393367.patch

Approved for 60.0b13 and ESR 52.8.0.
Attachment #8967323 - Flags: approval-mozilla-esr52?
Attachment #8967323 - Flags: approval-mozilla-esr52+
Attachment #8967323 - Flags: approval-mozilla-beta?
Attachment #8967323 - Flags: approval-mozilla-beta+
Group: gfx-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [gfx-noted] → [gfx-noted][post-critsmash-triage]
Whiteboard: [gfx-noted][post-critsmash-triage] → [gfx-noted][post-critsmash-triage][adv-main60+][adv-esr52.8+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.