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)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla61
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)
727 bytes,
patch
|
freddy
:
review+
mstange
:
review+
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr52+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
+++ 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
Updated•7 years ago
|
Keywords: csectype-bounds,
sec-high
Updated•7 years ago
|
Comment 1•7 years ago
|
||
(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)
Reporter | ||
Comment 2•7 years ago
|
||
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
Comment 4•6 years ago
|
||
@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)
Comment 6•6 years ago
|
||
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)
Assignee | ||
Comment 7•6 years ago
|
||
Comment 8•6 years ago
|
||
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 9•6 years ago
|
||
Comment on attachment 8967323 [details] [diff] [review] 1393367.patch Review of attachment 8967323 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for taking this!
Attachment #8967323 -
Flags: review+
Assignee | ||
Comment 10•6 years ago
|
||
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?
Assignee | ||
Comment 11•6 years ago
|
||
(In reply to Miko Mynttinen [:miko] from comment #10) > - Not easily, see comment 1. I meant see description here.
Comment 12•6 years ago
|
||
(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)
Updated•6 years ago
|
status-firefox61:
--- → affected
tracking-firefox61:
--- → +
Comment 13•6 years ago
|
||
> 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)
Comment 14•6 years ago
|
||
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.
status-firefox57:
unaffected → ---
status-firefox59:
--- → wontfix
status-firefox60:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox60:
--- → +
tracking-firefox-esr52:
--- → 60+
Updated•6 years ago
|
Attachment #8967323 -
Flags: sec-approval? → sec-approval+
Comment 15•6 years ago
|
||
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)
Comment 16•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1908cd8ed88d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 17•6 years ago
|
||
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?
Assignee | ||
Comment 18•6 years ago
|
||
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 19•6 years ago
|
||
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+
Comment 20•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/9718d92fab4d https://hg.mozilla.org/releases/mozilla-esr52/rev/1ab40761a856
Updated•6 years ago
|
Group: gfx-core-security → core-security-release
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [gfx-noted] → [gfx-noted][post-critsmash-triage]
Updated•6 years ago
|
Whiteboard: [gfx-noted][post-critsmash-triage] → [gfx-noted][post-critsmash-triage][adv-main60+][adv-esr52.8+]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•