Closed Bug 1570358 Opened 11 months ago Closed 11 months ago

Moz2D recording should check constrained values in RecordedEvents before playing them.

Categories

(Core :: Graphics, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- fixed
firefox68 --- wontfix
firefox69 + fixed
firefox70 + fixed

People

(Reporter: bobowen, Assigned: bobowen)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main69+][adv-esr68.1+][post-critsmash-triage])

Attachments

(3 files)

Moz2D APIs might assume that enum arguments are valid.
Moz2D recording should check any constrained values like enums read out of the stream before passing them to other Moz2D APIs.

The reading functions in Moz2D don't return any errors, so the simplest way of achieving this is to set the stream as bad, because this should always be checked before the RecordedEvent is played.

Are these messages passed across processes? I see some code in webrender, and some in "gfx/2d". Is the latter the content-process canvas stuff? What would malicious/fake enums cause us to do?

Group: core-security → gfx-core-security
Flags: needinfo?(bobowencode)

(In reply to Daniel Veditz [:dveditz] from comment #1)

Are these messages passed across processes? I see some code in webrender, and some in "gfx/2d". Is the latter the content-process canvas stuff? What would malicious/fake enums cause us to do?

Yes they are passed across process, as part of (AIUI) some things that don't go via webrender, but only when webrender is enabled, also the remote Canvas 2D (which isn't enabled by default yet) and the remote printing.
I think in general that most of the Moz2D code will default to a sensible or at least safe code path with an invalid enum, but I'm not sure it is guaratanteed. jrmuizel probably has a better idea as to whether there is much risk of mishandling.

Flags: needinfo?(bobowencode) → needinfo?(jmuizelaar)

(In reply to Bob Owen (:bobowen) from comment #2)

(In reply to Daniel Veditz [:dveditz] from comment #1)

Are these messages passed across processes? I see some code in webrender, and some in "gfx/2d". Is the latter the content-process canvas stuff? What would malicious/fake enums cause us to do?

Yes they are passed across process, as part of (AIUI) some things that don't go via webrender, but only when webrender is enabled, also the remote Canvas 2D (which isn't enabled by default yet) and the remote printing.
I think in general that most of the Moz2D code will default to a sensible or at least safe code path with an invalid enum, but I'm not sure it is guaratanteed. jrmuizel probably has a better idea as to whether there is much risk of mishandling.

There's definitely some risk, I don't have anything concrete in mind but it's a fairly broad attack surface. It's worth trying to do better.

Flags: needinfo?(jmuizelaar)

Comment on attachment 9082601 [details]
Bug 1570358: Check validity of enums before playing back Moz2D RecordedEvents. r=jrmuizel!

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: There is no known exploit currently, but it is relatively clear from the patch what the issue is and where to start looking for problems.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
  • Which older supported branches are affected by this flaw?: all (if you include printing)
  • 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?: (I don't have a patch for ESR60 (I do for ESR68), all hunks fail to apply there, so the rebase will take a while. Only printing is affected on ESR60, so an attacker would have to exploit the process and convince the user to authorise a print via the dialog first.)
  • How likely is this patch to cause regressions; how much testing does it need?: There are a lot of sites to be updated where the enums are read, so there is some chance of regressions.
    I have pushed to try without linking to this bug to try to mitigate those risks:
    https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ae43260a3e3fdae56b00a8d40e2943db21c6414
Attachment #9082601 - Flags: sec-approval?
Keywords: sec-moderate

Comment on attachment 9082601 [details]
Bug 1570358: Check validity of enums before playing back Moz2D RecordedEvents. r=jrmuizel!

sec-approval+ but Dan made it a sec-moderate so it doesn't need it now.
I think we should probably backport it to beta just to be safe.

Attachment #9082601 - Flags: sec-approval? → sec-approval+
Group: gfx-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Comment on attachment 9083563 [details] [diff] [review]
Beta Patch - Check validity of enums before playing back Moz2D RecordedEvents.

Beta/Release Uplift Approval Request

  • User impact if declined: Risk of potential for sandbox escapes via invalid Moz2D recorded gfx enums will remain.
  • Is this code covered by automated tests?: Unknown
  • 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: Medium
  • Why is the change risky/not risky? (and alternatives if risky): I'm only saying medium, because of the number of changes and the potential for a mistake in the constraints given.
    Each individual change is very simple.
  • String changes made/needed: None
Attachment #9083563 - Flags: approval-mozilla-beta?

In case we want to take this on ESR68 at some point.

Comment on attachment 9083563 [details] [diff] [review]
Beta Patch - Check validity of enums before playing back Moz2D RecordedEvents.

Approved for 69.0b13. Leaving esr68 set to affected for now so it stays on the radar - I do think we'll probably want to take this there as well before we ship 68.1esr. We can let it bake a bit more first, though.
Attachment #9083563 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Please nominate this for ESR68 approval when you get a chance.

Flags: needinfo?(bobowencode)

Comment on attachment 9083923 [details] [diff] [review]
ESR68 Patch - Check validity of enums before playing back Moz2D RecordedEvents.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Uplift requested.
  • User impact if declined: Risk of potential for sandbox escapes via invalid Moz2D recorded gfx enums will remain.
  • Fix Landed on Version: 70
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): Medium, because of the number of changes and the potential for a mistake in the constraints given.
    Probably less risky now, as a very similar patch has landed on Nightly and Beta with no reported problems.
  • String or UUID changes made by this patch: None
Flags: needinfo?(bobowencode)
Attachment #9083923 - Flags: approval-mozilla-esr68?
Comment on attachment 9083923 [details] [diff] [review]
ESR68 Patch - Check validity of enums before playing back Moz2D RecordedEvents.

sec fix, approved for 68.1esr
Attachment #9083923 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Whiteboard: [adv-main69+][adv-esr68.1+]
Flags: qe-verify-
Whiteboard: [adv-main69+][adv-esr68.1+] → [adv-main69+][adv-esr68.1+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.