nsBoxObject::GetPresShell flushes without keeping the doc alive

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

({csectype-uaf, sec-critical})

unspecified
mozilla22
Points:
---

Firefox Tracking Flags

(firefox19 wontfix, firefox20+ fixed, firefox21+ fixed, firefox22+ fixed, firefox-esr1720+ fixed, b2g1820+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 affected)

Details

(Whiteboard: [adv-main20+][adv-esr1705+])

Attachments

(1 attachment)

Posted patch patchSplinter Review
nsBoxObject::GetPresShell keeps a raw pointer to document and calls flush.
The flush operation itself should be safe these days, but the raw document pointer may
point to a deleted object after the flush, yet we use the document.
Attachment #727143 - Flags: review?(bzbarsky)
Comment on attachment 727143 [details] [diff] [review]
patch

r=me
Attachment #727143 - Flags: review?(bzbarsky) → review+
Comment on attachment 727143 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It can be a bit tricky, since the code is for XUL elements and one have to figure out a way to make chrome to adopt
mContent element to another document or something similar.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
It is quite obvious what is going wrong, so need to be creative when figuring out the commit message.
Perhaps I just leave out the commit message. Have just bug number and r=...

Which older supported branches are affected by this flaw?
All

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Same code everywhere. old code.

How likely is this patch to cause regressions; how much testing does it need?
Extremely likely to cause regressions
Attachment #727143 - Flags: sec-approval?
Attachment #727143 - Flags: approval-mozilla-esr17?
Attachment #727143 - Flags: approval-mozilla-beta?
Attachment #727143 - Flags: approval-mozilla-aurora?
Since this is tricky to exploit, internally reported, and highly likely to cause regressions we should hold off until earlier in FF21 when it gets on Beta and not take this in FF20.
(In reply to lsblakk@mozilla.com from comment #3)
> Since this is tricky to exploit, internally reported, and highly likely to
> cause regressions we should hold off until earlier in FF21 when it gets on
> Beta and not take this in FF20.

Er oops, I missed quite important part from the word. *un*. Highly unlikely to cause regressions.
Sorry.
Comment on attachment 727143 [details] [diff] [review]
patch

sec-approval+ for checkin after 4/9.
Attachment #727143 - Flags: sec-approval? → sec-approval+
Whiteboard: [checkin after 4/9]
Going to clear the check-in embargo and try to get this into 20 based on comment 4.

Olli, please wait to land until Lukas (or another driver) says whether they'll agree to take this on beta or not.
tracking-b2g18: --- → +
Whiteboard: [checkin after 4/9]
Comment on attachment 727143 [details] [diff] [review]
patch

Thanks for the correction, in that case we can take this sec-critical fix for the last FF20 beta - please land to branches asap.
Attachment #727143 - Flags: approval-mozilla-esr17?
Attachment #727143 - Flags: approval-mozilla-esr17+
Attachment #727143 - Flags: approval-mozilla-beta?
Attachment #727143 - Flags: approval-mozilla-beta+
Attachment #727143 - Flags: approval-mozilla-aurora?
Attachment #727143 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/a0448fd67f91
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Whiteboard: [adv-main20+][adv-esr1705+]
Ryan - would you mind landing this to mozilla-b2g18?
Flags: needinfo?(ryanvm)
Attachment #727143 - Flags: approval-mozilla-b2g18+
Group: core-security
You need to log in before you can comment on or make changes to this bug.