1438425 - PDocumentRenderer __delete__() passes a size and a buffer, without length checks

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Randell Jesup [:jesup] (needinfo me)]

In PDocumentRenderer (part of Canvas), on __delete() it passes a buffer (as an nsCString; sigh) and a size (nsIntSize).  It then sets up an DataSourceSurface using the raw ptr to the buffer and the size -- but doesn't check that the "string's" size is large enough.  

With a crafted __delete__ from an owned content process, this could cause the render of the canvas to grab data from memory following the buffer used by the IPC reception.  This would then be rendered as part of the page, and could be captured via several mechanisms.

Doing something useful with this might be tough, but seems possible, if combined with other issues.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Error-checking

We need a general 'failed validation; kill sender and log' method we can call from lots of places, but not part of this bug.  On validation fail, don't kill the Master process.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Remove DocumentRenderer

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8951148 [details] [diff] [review]
Error-checking

I'd prefer my approach

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8951372 [details] [diff] [review]
Remove DocumentRenderer

Review of attachment 8951372 [details] [diff] [review]:
-----------------------------------------------------------------

Remove it?  Sure, if it's not needed; I presumed it was!

We should consider uplifting this if we believe there's no use of DocumentRenderer, though the risk of it riding the trains isn't huge I think

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0db116b21ac09675a37a4904fa75777b6bc19b3d
https://hg.mozilla.org/mozilla-central/rev/0db116b21ac0

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Uplifting the removal patch to ESR52 makes me nervous, but Beta sounds reasonable. Given that this is sec-high, should we consider taking Randell's patch for ESR52 at least?

Comment 8

[Jeff Muizelaar [:jrmuizel]]

It should be fine to take the removal patch. This code hasn't been used for a long time. If for some reason it's deemed to risky  we don't need Randell's patch we can just unconditionally early return.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

I think taking the removal on beta should be safe, in that it's on inbound with no fallout and it's a removal (and straightforward)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please request Beta & ESR52 approval on the removal patch. Sounds like it's pretty safe to land everywhere.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Jeff actually wrote the patch

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8951372 [details] [diff] [review]
Remove DocumentRenderer

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: N/A
User impact if declined: A rogue content process could potentially use this code to escalate. 
Fix Landed on Version: 60
Risk to taking this patch (and alternatives if risky): Very low risk
String or UUID changes made by this patch: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8951372 [details] [diff] [review]
Remove DocumentRenderer

According to bug 1282177, this has been unused for a really long time, so removing the dangerous code in question seems like the safest option for backport. Taking for Beta and ESR52.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/56c70e546295

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Jeff, this is going to need a rebased patch for ESR52.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Attachment: Remove DocumentRenderer

Here's an ESR patch

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/6531ca6690e5

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out from ESR52 for bustage.
https://hg.mozilla.org/releases/mozilla-esr52/rev/7a0654f127ca

https://treeherder.mozilla.org/logviewer.html#?job_id=165632970&repo=mozilla-esr52

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Here's a try attempt at something that builds on ESR:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b0d93856005ce4bc03008f2475b0a17f9e462d4

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Attachment: Remove DocumentRenderer ESR

Jeff can you look at the WebGL parts of this (specifically the weird cycle collector change)?

Comment 21

[Jeff Muizelaar [:jrmuizel]]

This should be ready to go.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8956511 [details] [diff] [review]
Remove DocumentRenderer ESR

Thanks, Jeff!

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/8a16f439117c