1339566 - Use-after-free in nsDocShell::CreateAboutBlankViewer

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Andrew McCreight [:mccr8]]

AutoRestore<bool> creatingDocument(mCreatingDocument);
  mCreatingDocument = true;

  // mContentViewer->PermitUnload may release |this| docshell.
  nsCOMPtr<nsIDocShell> kungFuDeathGrip(this);

The grip gets released before the AutoRestore.

Comment 1

[Daniel Holbert [:dholbert]]

(and the AutoRestore is holding a pointer to this->mCreatingDocument, which means it will stomp on potentially-freed memory when it goes out of scope & tries to do its AutoRestore value-restoration)

Source link:
http://searchfox.org/mozilla-central/rev/ac8a72f2d5204ced2004c93a9c193430c63a6a71/docshell/base/nsDocShell.cpp#8026

I'll take this; seems trivial.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Please give this a sec rating too :)

Comment 4

[Andrew McCreight [:mccr8]]

I'm just going to mark this sec-moderate. I think almost no code runs between the destructor and the UAF, so it seems benign.

Comment 5

[Daniel Holbert [:dholbert]]

I'll land this tomorrow, then, unless you think I shouldn't (e.g. if there's another similar bug that would become more discoverable after this points an arrow at AutoRestore).

(Per https://wiki.mozilla.org/Security/Bug_Approval_Process , no sec-approval is needed if "the bug has a sec-low, sec-moderate, sec-other, or sec-want rating")

Comment 6

[Daniel Holbert [:dholbert]]

Per bug 1338772 comment 15, sounds like we're good to go. Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1afbc59a1cdd3012f1309bc64c0effaa9cae34

Also, marking "in-testsuite-" since we don't have a testcase & we're not aware that testcases could actually trigger bad behavior here, per comment 4 (and this issue was discovered via code inspection)

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0f1afbc59a1c

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8837295 [details] [diff] [review]
fix v1

Approval Request Comment
[Feature/Bug causing the regression]: bug 1031303 (which added the AutoRestore here)
[User impact if declined]: This fix addresses a potential security issue -- there's a possible use-after-free being fixed here, though we're not aware of any way to get up to mischief in this case, per comment 4.  So: user impact if declined is: users would remain exposed to a small security risk.
[Is this code covered by automated tests?]: I think so. This is in our about:blank setup code, which gets exercised on every pageload (since we start pages at about:blank before loading their actual contents).
[Has the fix been verified in Nightly?]: Yes. (It's landed on Nightly and hasn't caused any problems. "Verified" is a bit of a silly concept here, since we don't have a testcase -- this was discovered via code inspection -- and we don't know of any way for this to actually cause trouble. So this is just fixing a theoretical problem.)
[Needs manual test from QE? If yes, steps to reproduce]: Nope.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Extremely small tweak - just keeping an object alive slightly longer, so that we tweak its bool before it's potentially-destroyed instead of after it's potentially-destroyed.
[String changes made/needed]: None.

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8837295 [details] [diff] [review]
fix v1

fix a possible UAF, aurora53+, beta52+

Comment 11

[Daniel Holbert [:dholbert]]

Landed on Aurora/Beta:
  https://hg.mozilla.org/releases/mozilla-aurora/rev/db2e6ff03ae1c57ebd4b4dae874d744bd1b8a41b
  https://hg.mozilla.org/releases/mozilla-beta/rev/72ec61c54de394e550b56e6561ecf88e42e7c90e

Comment 12

[Daniel Holbert [:dholbert]]

ESR52 should get this as well (and IIUC doesn't need separate approval beyond beta52+). 

Based on recent treeherder, looks like RyanVM is merging everything from beta52 to esr52 in periodic large batches, so I assume this will make it over to esr52 that way.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/72ec61c54de3