Closed Bug 1438425 Opened 3 years ago Closed 3 years ago

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


(Core :: Graphics, defect, P3)

58 Branch



Tracking Status
firefox-esr52 59+ fixed
firefox58 --- wontfix
firefox59 + fixed
firefox60 + fixed


(Reporter: jesup, Assigned: jrmuizel)



(Keywords: csectype-bounds, sec-high, Whiteboard: [post-critsmash-triage][adv-main59+][adv-esr52.7+])


(3 files, 1 obsolete file)

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.
Attached patch Error-checkingSplinter Review
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.
Attachment #8951148 - Flags: review?(jmuizelaar)
Attachment #8951148 - Flags: review?(jmathies)
Assignee: nobody → rjesup
Attachment #8951372 - Flags: review?(rjesup)
Comment on attachment 8951148 [details] [diff] [review]

I'd prefer my approach
Attachment #8951148 - Flags: review?(jmuizelaar) → review-
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
Attachment #8951372 - Flags: review?(rjesup) → review+
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?
Flags: needinfo?(jmuizelaar)
Duplicate of this bug: 1282177
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.
Flags: needinfo?(jmuizelaar)
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)
Group: core-security → core-security-release
Please request Beta & ESR52 approval on the removal patch. Sounds like it's pretty safe to land everywhere.
Flags: needinfo?(rjesup)
Jeff actually wrote the patch
Assignee: rjesup → jmuizelaar
Flags: needinfo?(rjesup) → needinfo?(jmuizelaar)
Flags: needinfo?(jmuizelaar)
Attachment #8951148 - Flags: review?(jmathies)
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
Attachment #8951372 - Flags: approval-mozilla-esr52?
Attachment #8951372 - Flags: approval-mozilla-beta?
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.
Attachment #8951372 - Flags: approval-mozilla-esr52?
Attachment #8951372 - Flags: approval-mozilla-esr52+
Attachment #8951372 - Flags: approval-mozilla-beta?
Attachment #8951372 - Flags: approval-mozilla-beta+
Jeff, this is going to need a rebased patch for ESR52.
Flags: needinfo?(jmuizelaar)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Attached patch Remove DocumentRenderer (obsolete) — Splinter Review
Here's an ESR patch
Flags: needinfo?(jmuizelaar)
Attachment #8955646 - Flags: approval-mozilla-esr52+
Flags: needinfo?(ryanvm)
Here's a try attempt at something that builds on ESR:
Flags: needinfo?(jmuizelaar)
Jeff can you look at the WebGL parts of this (specifically the weird cycle collector change)?
Attachment #8956511 - Flags: review?(jgilbert)
Attachment #8956511 - Flags: review?(jgilbert) → review+
This should be ready to go.
Flags: needinfo?(ryanvm)
Attachment #8955646 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
Attachment #8955646 - Flags: approval-mozilla-esr52+
Comment on attachment 8956511 [details] [diff] [review]
Remove DocumentRenderer ESR

Thanks, Jeff!
Attachment #8956511 - Flags: approval-mozilla-esr52+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main59+][adv-esr52.7+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.