Closed Bug 1532708 Opened 1 year ago Closed 1 year ago

Guard against bad layers ids in the RecvSetConfirmedTargetAPZC notifications

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 66+ fixed
firefox65 --- wontfix
firefox66 + fixed
firefox67 + fixed

People

(Reporter: kats, Assigned: kats)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main66-][adv-esr60.6-])

Attachments

(1 file)

PWebRenderBridge and PLayerTransaction have a SetConfirmedTargetAPZC message where the parent side gets ScrollableLayerGuids but doesn't check the layers id to ensure it is the right one, and then later uses it. The equivalent message that goes over PAPZCTreeManager does this checking at https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/gfx/layers/ipc/APZCTreeManagerParent.cpp#78

This opens up a small possibility that a malicious content process can produce incorrect behaviour by "confirming" the target APZC for an input event as one from a different layers subtree. Probably not easily exploitable, but we should guard against for consistency anyway.

Group: core-security → layout-core-security
Assignee: nobody → kats

Comment on attachment 9049599 [details]
Bug 1532708. r?botond

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Not easily
  • 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?: None
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: The patch should apply cleanly everywhere. Although the line numbers on ESR60 have probably changed a lot for WebRenderBridgeParent.
  • How likely is this patch to cause regressions; how much testing does it need?: Not likely; I did a try push that should have uncovered any problems. Normal usage should be sufficient testing.

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: 1154130, maybe
  • User impact if declined: Malicious/hijacked content processes might be able to sneak some incorrect data to the compositor. But it's not clear if that could actually be used to do something nefarious.
  • Is this code covered by automated tests?: Yes
  • 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): Just adding some extra validity checks (copied from other codepaths that do similar things)
  • String changes made/needed: none
Attachment #9049599 - Flags: sec-approval?
Attachment #9049599 - Flags: approval-mozilla-release?
Attachment #9049599 - Flags: approval-mozilla-beta?

Comment on attachment 9049599 [details]
Bug 1532708. r?botond

Since this is rated sec-moderate it doesn't need sec-approval.
Let's take this for the RC build.

Attachment #9049599 - Flags: sec-approval?
Attachment #9049599 - Flags: approval-mozilla-beta?
Attachment #9049599 - Flags: approval-mozilla-beta+

https://hg.mozilla.org/releases/mozilla-beta/rev/51d306795184276ba5b837b644a8802b5aa29eb2

Triggered landing via lando but tree is closed. It'll get through when it gets through I guess.

Group: layout-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Comment on attachment 9049599 [details]
Bug 1532708. r?botond

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: could be used to fool the compositor into doing something dumb, if combined with other vulnerabilities.
  • User impact if declined: Malicious/hijacked content processes might be able to sneak some incorrect data to the compositor. But it's not clear if that could actually be used to do something nefarious.
  • Fix Landed on Version: 67
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Pretty low risk of something going wrong. The codepath hasn't changed much in the intervening versions, and the patch hasn't caused any problems on trunk.
  • String or UUID changes made by this patch: none
Attachment #9049599 - Flags: approval-mozilla-esr60?

Comment on attachment 9049599 [details]
Bug 1532708. r?botond

And I'll drop the request for m-r since there's no point unless we're doing a chemspill in the next week or so.

Attachment #9049599 - Flags: approval-mozilla-release?
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

Comment on attachment 9049599 [details]
Bug 1532708. r?botond

Talked about it with Dan and decided to take this. Approved for 60.6esr.

Attachment #9049599 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main66-][adv-esr60.6-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.