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

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Bob Owen (:bobowen)]

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.

Comment 1

[Daniel Veditz [:dveditz]]

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?

Comment 2

[Bob Owen (:bobowen)]

(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.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Bug 1570358: Check validity of enums before playing back Moz2D RecordedEvents. r=jrmuizel!

Comment 5

[Bob Owen (:bobowen)]

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

Comment 6

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 7

[Bob Owen (:bobowen)]

Attachment: Beta Patch - Check validity of enums before playing back Moz2D RecordedEvents.

Comment 8

[Bob Owen (:bobowen)]

https://hg.mozilla.org/integration/autoland/rev/ee1ae2cf5dd9a01391a29084727750d65d1cf554

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ee1ae2cf5dd9

Comment 10

[Bob Owen (:bobowen)]

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

Comment 11

[Bob Owen (:bobowen)]

Attachment: ESR68 Patch - Check validity of enums before playing back Moz2D RecordedEvents.

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b2e1689352c5

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Bob Owen (:bobowen)]

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

Comment 16

[Julien Cristau [:jcristau]]

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

sec fix, approved for 68.1esr

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/26b6bb71abca