852923 - nsBoxObject::GetPresShell flushes without keeping the doc alive

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 727143 [details] [diff] [review]
patch

r=me

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to [e-mail] 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 5

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

Comment on attachment 727143 [details] [diff] [review]
patch

sec-approval+ for checkin after 4/9.

Comment 6

[Daniel Veditz [:dveditz]]

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.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0448fd67f91
https://hg.mozilla.org/releases/mozilla-aurora/rev/c3107b4bd85a
https://hg.mozilla.org/releases/mozilla-beta/rev/3c440715a9b2
https://hg.mozilla.org/releases/mozilla-esr17/rev/5c097cbce381

Comment 9

[Ed Morley [:emorley]]

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

Comment 10

[Alex Keybl [:akeybl]]

Ryan - would you mind landing this to mozilla-b2g18?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/826f649a6dcf