Closed Bug 1451297 Opened 2 years ago Closed 2 years ago

IPC: crash with PImageBridge::Msg_PTextureConstructor [@I422ToARGBRow_Any_AVX2]

Categories

(Core :: Graphics, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 61+ fixed
firefox-esr60 61+ fixed
firefox60 --- wontfix
firefox61 + fixed
firefox62 + fixed

People

(Reporter: posidron, Assigned: aosmond)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-wildptr, sec-high, Whiteboard: [fuzzblocker][adv-main61+][adv-esr52.9+][adv-esr60.1+][post-critsmash-triage])

Attachments

(4 files)

Attached file messages.zip
The following message was identified to be responsible for this crash and got blacklisted from fuzzing until fixed.

Message: PImageBridge::Msg_PTextureConstructor


$ hexdiff message.25400.48265.{o,m}


NOTE: I am not seeing a OnMessageReceived() in the callstack.
Attached file faulty.txt
Group: core-security → gfx-core-security
Whiteboard: [fuzzblocker]
Milan, could you assign somebody to investigate this bug?
Thx!
Flags: needinfo?(milan)
Perhaps related to bug 1451302?
Flags: needinfo?(milan)
Bug 1388020 added a check to make sure the buffer size and buffer descriptor are compatible for shmems. This is just speculation at this point, but I wonder if we are failing to meet this assert condition somehow:

https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/gfx/layers/basic/BasicCompositor.cpp#129

Idea would be it gets accepted as RGB when we create the texture (this was the impact of the fuzz), but when we use the texture, something expected it to be YCbCr.
Looks like I didn't take into account how the YCbCr offsets can interact with the buffer data pointer in the texture validation. Will add checks for what fails here and look at how the rest of the fields are used as well...
Assignee: nobody → aosmond
Attachment #8971435 - Flags: review?(nical.bugzilla)
Attachment #8971435 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8971435 [details] [diff] [review]
[central/beta/release] 0001-Bug-1451297.patch, v1

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

It will be very clear we are verifying the buffer size. If one has control over the content process, one would easily conclude they could access arbitrary memory in the parent process using this.

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

No comments or additional tests.

> Which older supported branches are affected by this flaw?

All.

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

N/A

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Yes. It applies cleanly to the release/beta trees, and I slightly modified the patch for esr52. They are not very different and not very risky.

> How likely is this patch to cause regressions; how much testing does it need?

Unlikely, given we just add a new bounds check. It is covered by automated tests, and can be manually verified by playing videos (e.g. YouTube is fine).
Attachment #8971435 - Flags: sec-approval?
Attachment #8971435 - Attachment description: 0001-Bug-1451297.patch, v1 → [central/beta/release] 0001-Bug-1451297.patch, v1
We're making release candidates for Firefox 60 now so this is too late for the current release.

I'm giving sec-approval+ for checkin *NO EARLIER* than May 22, two weeks into the new cycle. Please check it in then and, following that, nominate the patch for ESR52 and ESR60.
Whiteboard: [fuzzblocker] → [fuzzblocker][checkin on 5/22]
Attachment #8971435 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/a4c1afd30b13
Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Please request Beta/ESR60/ESR52 approval on the relevant patches whenever you're comfortable doing so.
Flags: needinfo?(aosmond)
Whiteboard: [fuzzblocker][checkin on 5/22] → [fuzzblocker]
Comment on attachment 8971435 [details] [diff] [review]
[central/beta/release] 0001-Bug-1451297.patch, v1

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: May access arbitrary memory to copy into a GL texture. Could crash or leak information from GPU or main process.
[Is this code covered by automated tests?]: Yes, the bounds check to hit during normal testing. The fuzzer hits the out-of-bounds cases.
[Has the fix been verified in Nightly?]: Yes, landed a few days ago, no issues reported since.
[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?]: It just improves the bounds check, and ensures any failures are passed up to the caller. If we are outside the bounds, we likely would crash anyways.
[String changes made/needed]: None.
Flags: needinfo?(aosmond)
Attachment #8971435 - Flags: approval-mozilla-beta?
Comment on attachment 8971958 [details] [diff] [review]
[esr52] 0001-Bug-1451297.esr52.patch, v1

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: It is sec-high.
Fix Landed on Version: 62
Risk to taking this patch (and alternatives if risky): See comment 12.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8971958 - Flags: approval-mozilla-esr52?
Comment on attachment 8971435 [details] [diff] [review]
[central/beta/release] 0001-Bug-1451297.patch, v1

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: It is sec-high.
Fix Landed on Version: 62
Risk to taking this patch (and alternatives if risky): See comment 12.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8971435 - Flags: approval-mozilla-esr60?
Comment on attachment 8971435 [details] [diff] [review]
[central/beta/release] 0001-Bug-1451297.patch, v1

Fixes a sec-high. Approved for 61.0b9, ESR 60.1, and ESR 52.9.
Attachment #8971435 - Flags: approval-mozilla-esr60?
Attachment #8971435 - Flags: approval-mozilla-esr60+
Attachment #8971435 - Flags: approval-mozilla-beta?
Attachment #8971435 - Flags: approval-mozilla-beta+
Attachment #8971958 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [fuzzblocker] → [fuzzblocker][adv-main61+][adv-esr52.9+][adv-esr60.1+]
Flags: qe-verify-
Whiteboard: [fuzzblocker][adv-main61+][adv-esr52.9+][adv-esr60.1+] → [fuzzblocker][adv-main61+][adv-esr52.9+][adv-esr60.1+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.