725770 - (CVE-2012-3988) Firefox crashes by mozRequestFullScreen and history.back function

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Soroush Dalili]

Attachment: mozFullScreen-Back-Crash.html

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.46 Safari/535.11

Steps to reproduce:

When history.length is greater than 1 (back button is enabled) and user initiate the click button:
Calling the "mozRequestFullScreen()" function and then "history.go(-1)" can cause a crash or a DoS in Mozilla Firefox.
I have created a test case that will replicate the issue. 

As the Offset value can change, it can be a security issue as well. However, I could not find out the reason for this offset change.

Please also verify if it can be a case for the bug bounty.


Actual results:

Mozilla Firefox show the crash reporter
Or
It becomes unresponsive

If you cannot see this result, please use the provided attach file and do it again. Note: Your history should not be empty.


Expected results:

It should disable the full screen and then redirect the user to the previous page.

Comment 1

[Benjamin Smedberg]

If you get the crash reporter, can you please give us some crash report IDs (you can get them from about:crashes)?

Comment 2

[Soroush Dalili]

Some crash reports:
https://crash-stats.mozilla.com/report/index/bp-d8eb4d19-69b5-46e3-b254-162b12120209
https://crash-stats.mozilla.com/report/index/bp-bff089bc-cebf-40f1-8606-e4da62120209
https://crash-stats.mozilla.com/report/index/bp-f7973304-a6be-4432-aa04-723242120209

Comment 4

[Daniel Veditz [:dveditz]]

Added this to the bounty nomination queue, but at first glance those crashes look like null derefs and probably not exploitable. We need to run this under Valgrind or ASan to see if there's some worse memory abuse masquerading as a safe crash.

Comment 5

[chris hofmann]

who is a good candidate for the valgrind work?

Comment 6

[Bob Clary [:bc] (inactive)]

I can do the valgrind stuff. log in a bit.

Comment 7

[Christian Holler (:decoder)]

I can also have a look on Sunday when I'm back if that's early enough.

Comment 8

[Bob Clary [:bc] (inactive)]

OS X 10.5
Confirmed Firefox 10 crashes
Beta/11, Aurora/12, Nightly/13 release or debug do not crash

Beta Debug did not show anything but use and conditional jumps of uninitialized memory.
Nightly Debug under valgrind asserted

Fedora 16:
Confirmed Firefox 10 crashed
Beta/11, Aurora/12, Nightly/13 release or debug do not crash

Beta Debug under valgrind did not show anything but use and conditional jumps of uninitialized memory.

I don't have a debug build of Firefox 10 available. Do you need a valgrind report on it?

Comment 9

[Daniel Veditz [:dveditz]]

Bob: yes, please run Valgrind on Fx10. (also Christian, start with Fx10 rather than trunk if ASan runs on the older builds). If you still have them please also attach your Valgrind output for one of the beta runs (or both if they're different).

Looks like the crash may be something we fixed already, but it could just different symptoms due to the way code has moved around. If Fx10 turns up a different memory problem we should file a separate bug on the uninitialized memory.

Comment 10

[Christian Holler (:decoder)]

Ok, I took an initial look at this. Because I did not have a Fx10 ASan build handy, I started with trunk which was readily available. And it turns out that at least on trunk, this is neither fixed, nor a simple null deref.

Rather, I see a use-after-free in ASan both on debug and opt builds (64 bit). However, I'm having problems getting a symbolized trace (likely unrelated to this particular issue though). I'll try to get symbols into the trace and will attach it then. I'll also try to take another look with Valgrind because both tools should be able to see this.

Comment 11

[Christian Holler (:decoder)]

Attachment: Symbolized ASAN trace for trunk (m-c rev 307a032f2007)

Okay, there seems to be a bug in the ASan symbolizer code but I managed to work around and wrote my own symbolizer script. Attached is the symbolized log for this testcase on Firefox Trunk (m-c rev 307a032f2007).

Strangely enough, Valgrind does not see this issue at all, although it should.

Comment 12

[Christian Holler (:decoder)]

Confirming this for now by comment 11.

Comment 13

[Daniel Veditz [:dveditz]]

CC'ing :njn in case he's interested in comment 11

Comment 14

[Nicholas Nethercote [inactive]]

Valgrind's use-after-free tracking is imperfect -- it holds onto freed blocks for a while in order to detect them, but eventually the blocks are recycled.

Christian, does Valgrind detect it if you run it with --freelist-vol=200000000?  That makes Valgrind hold onto blocks for 10x longer than the default.

Comment 15

[Christian Holler (:decoder)]

(In reply to Nicholas Nethercote [:njn] from comment #14)

> Christian, does Valgrind detect it if you run it with
> --freelist-vol=200000000? 

You were right :) Now I see the issue also in Valgrind:

###!!! ASSERTION: unexpected parallel nsIWebProgress OnStateChange and/or OnLocationChange notification: 'mOnStateLocationChangeReentranceDetection == 1', file /srv/repos/browser/mozilla-central/security/manager/boot/src/nsSecureBrowserUIImpl.cpp, line 622
==28683== Invalid read of size 8
==28683==    at 0x73A3E82: nsDocShell::RestoreFromHistory() (nsDocShell.cpp:11783)
==28683==    by 0x73A4167: nsDocShell::RestorePresentationEvent::Run() (nsDocShell.cpp:6754)
==28683==    by 0x77B0113: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:657)
==28683==    by 0x776EC79: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:245)
==28683==    by 0x76BAE1F: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:110)
==28683==    by 0x77DA4E5: MessageLoop::RunInternal() (message_loop.cc:208)
==28683==    by 0x77DA50D: MessageLoop::Run() (message_loop.cc:201)
==28683==    by 0x75E04B6: nsBaseAppShell::Run() (nsBaseAppShell.cpp:189)
==28683==    by 0x742D557: nsAppStartup::Run() (nsAppStartup.cpp:295)
==28683==    by 0x69CF238: XRE_main (nsAppRunner.cpp:3564)
==28683==    by 0x40187B: main (nsBrowserApp.cpp:189)
==28683==  Address 0x28cb9cd8 is 8 bytes inside a block of size 152 free'd
==28683==    at 0x4C282E0: free (vg_replace_malloc.c:366)
==28683==    by 0x6FC9C08: nsView::~nsView() (nsView.cpp:228)
==28683==    by 0x6FC9CDB: nsView::~nsView() (nsView.cpp:275)
==28683==    by 0x6C24DDF: nsFrame::DestroyFrom(nsIFrame*) (nsFrame.cpp:658)
==28683==    by 0x6D3B729: nsBoxFrame::RemoveFrame(mozilla::layout::FrameChildListID, nsIFrame*) (nsIFrame.h:582)
==28683==    by 0x6BB77E9: nsFrameManager::RemoveFrame(mozilla::layout::FrameChildListID, nsIFrame*) (nsFrameManager.cpp:531)
==28683==    by 0x6B8E9CA: nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags, bool*) (nsCSSFrameConstructor.cpp:7542)
==28683==    by 0x6B8D79C: nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, bool) (nsCSSFrameConstructor.cpp:9133)
==28683==    by 0x6B8ED33: nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList&) (nsCSSFrameConstructor.cpp:7977)
==28683==    by 0x6B8F410: nsCSSFrameConstructor::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::css::RestyleTracker&, bool) (nsCSSFrameConstructor.cpp:8090)
==28683==    by 0x6B7A57B: mozilla::css::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint) (RestyleTracker.cpp:157)
==28683==    by 0x6B7B008: mozilla::css::RestyleTracker::DoProcessRestyles() (RestyleTracker.cpp:242)
==28683== 
###!!! ASSERTION: Thaw called on an unfrozen refresh driver: 'mFrozen', file /srv/repos/browser/mozilla-central/layout/base/nsRefreshDriver.cpp, line 498

Comment 16

[Bob Clary [:bc] (inactive)]

I tried a release debug build on OS X 10.5, Fedora 16 32 and 64 bit though I could reproduce the crash without valgrind I was never able to reproduce the crash with valgrind. I think timing issues prevent the crash with valgrind. On Mac and and Linux 32 bit I was able to get a similar invalid read though. I used an old svn 3.7 build on Mac and a current svn 3.8 build on Linux without needing the --freelist option.

=12622== Invalid read of size 4
==12622==    at 0x680E99D: nsIView::GetViewManager() const (nsIView.h:115)
==12622==    by 0x75EE12A: nsDocShell::RestoreFromHistory() (nsDocShell.cpp:7235)
==12622==    by 0x75EE570: nsDocShell::RestorePresentationEvent::Run() (nsDocShell.cpp:6716)
==12622==    by 0x7D21BFD: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:631)
==12622==    by 0x7CAFC83: NS_ProcessPendingEvents_P(nsIThread*, unsigned int) (nsThreadUtils.cpp:195)
==12622==    by 0x79D1C66: nsBaseAppShell::NativeEventCallback() (nsBaseAppShell.cpp:130)
==12622==    by 0x7978704: nsAppShell::ProcessGeckoEvents(void*) (nsAppShell.mm:424)
==12622==    by 0x45F3C4: CFRunLoopRunSpecific (in /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation)
==12622==    by 0x45FAA7: CFRunLoopRunInMode (in /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation)
==12622==    by 0x21252AB: RunCurrentEventLoopInMode (in /System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox)
==12622==    by 0x2124FFD: ReceiveNextEventCommon (in /System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox)
==12622==    by 0x2124F38: BlockUntilNextEventMatchingListInMode (in /System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox)
==12622==  Address 0x1f214544 is 4 bytes inside a block of size 108 free'd
==12622==    at 0x1A07D: free (vg_replace_malloc.c:369)
==12622==    by 0x6079EDA: moz_free (mozalloc.cpp:97)
==12622==    by 0x6F03F3E: nsView::operator delete(void*) (mozalloc.h:252)
==12622==    by 0x6F030AA: nsView::~nsView() (nsView.cpp:277)
==12622==    by 0x6F002E9: nsIView::Destroy() (nsView.cpp:342)
==12622==    by 0x6F02F3A: nsView::~nsView() (nsView.cpp:230)
==12622==    by 0x6F002E9: nsIView::Destroy() (nsView.cpp:342)
==12622==    by 0x68CEFB5: nsFrame::DestroyFrom(nsIFrame*) (nsFrame.cpp:581)
==12622==    by 0x693BC9D: nsSubDocumentFrame::DestroyFrom(nsIFrame*) (nsSubDocumentFrame.cpp:805)
==12622==    by 0x67C9FC2: nsIFrame::Destroy() (nsIFrame.h:576)
==12622==    by 0x6A9DA57: nsBoxFrame::RemoveFrame(mozilla::layout::FrameChildListID, nsIFrame*) (nsBoxFrame.cpp:1013)
==12622==    by 0x6818985: nsFrameManager::RemoveFrame(mozilla::layout::FrameChildListID, nsIFrame*) (nsFrameManager.cpp:537)

Comment 17

[Nicholas Nethercote [inactive]]

(In reply to Bob Clary [:bc:] from comment #16)
> I could reproduce the crash without valgrind I was never able to reproduce the
> crash with valgrind. I think timing issues prevent the crash with valgrind.

It could be due to timing issues or different memory layout.  See http://www.valgrind.org/docs/manual/faq.html#faq.crashes

Comment 18

[Julian Seward [:jseward]]

(In reply to Nicholas Nethercote [:njn] from comment #17)
> > crash with valgrind. I think timing issues prevent the crash with valgrind.

Might also be worth trying --fair-sched=yes, which changes the running order
of threads to be more fine-grainedly interleaved.  Only works with V trunk 
on Linux, not on OSX.

Comment 19

[Julian Seward [:jseward]]

Here's a bit of analysis w.r.t. the trace shown below (which is the
same as in comment #16).  It has to do with nsDocShell.cpp:

7272    // Insert the new root view at the correct location in the view tree.
7273    if (rootViewParent) {
7274        nsIViewManager *parentVM = rootViewParent->GetViewManager();
7275
7276        if (parentVM && newRootView) {
...
7284            parentVM->InsertChild(rootViewParent, newRootView,
7285                                  rootViewSibling,
7286                                  rootViewSibling ? true : false);

It appears that at 7273, rootViewParent points at an object that has
already been freed.  At 7274, the second word of the object is read,
triggering the complaint.  The word is passed tp GetViewManager() and
that returns it unchanged -- from reading the object code, is the
identity fn.

So we now have parentVM being junk hauled out of a potentially
reallocated object, could be any value.  If it's nonzero then
at 7284 we're very likely to crash when doing something with it.

Crashy?  Definitely.  Exploitable?  I don't know.


Invalid read of size 8
   at 0x69D8CA2: nsDocShell::RestoreFromHistory()+3058 (nsDocShell.cpp:7274)
   by 0x69D8FF1: nsDocShell::RestorePresentationEvent::Run()+17 (nsDocShell.cpp:6754)
   by 0x6D90FED: nsThread::ProcessNextEvent(bool, bool*)+429 (nsThread.cpp:657)
   by 0x6D58EC9: NS_ProcessNextEvent_P(nsIThread*, bool)+25 (nsThreadUtils.cpp:245)
   by 0x6D91AFE: nsThread::Shutdown()+270 (nsThread.cpp:499)
   by 0x60AAF2D: nsRunnableMethodImpl<void (nsPACMan::*)(), true>::Run()+29 (nsThreadUtils.h:345)
   by 0x6D90FED: nsThread::ProcessNextEvent(bool, bool*)+429 (nsThread.cpp:657)
   by 0x6D58EC9: NS_ProcessNextEvent_P(nsIThread*, bool)+25 (nsThreadUtils.cpp:245)
   by 0x6CB1C6A: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)+106 (MessagePump.cpp:110)
   by 0x6DB97E1: MessageLoop::Run()+65 (message_loop.cc:208)
   by 0x6BDDD1F: nsBaseAppShell::Run()+47 (nsBaseAppShell.cpp:189)
   by 0x6A48BFD: nsAppStartup::Run()+45 (nsAppStartup.cpp:295)
 Address 0x21809fe8 is 8 bytes inside a block of size 152 free'd
   at 0x40297AF: free+135 (vg_replace_malloc.c:427)
   by 0x6619780: nsView::~nsView()+224 (nsView.cpp:342)
   by 0x66197E8: nsView::~nsView()+8 (nsView.cpp:275)
   by 0x62A7F8F: nsFrame::DestroyFrom(nsIFrame*)+127 (nsFrame.cpp:658)
   by 0x63A6399: nsBoxFrame::RemoveFrame(mozilla::layout::FrameChildListID, nsIFrame*)+89 (nsIFrame.h:582)
   by 0x6249ACF: nsFrameManager::RemoveFrame(mozilla::layout::FrameChildListID, nsIFrame*)+79 (nsFrameManager.cpp:531)
   by 0x6221C68: nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags, bool*)+328 (nsCSSFrameConstructor.cpp:7542)
   by 0x622049F: nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, bool)+431 (nsCSSFrameConstructor.cpp:9133)
   by 0x6222129: nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList&)+233 (nsCSSFrameConstructor.cpp:7977)
   by 0x6222712: nsCSSFrameConstructor::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::css::RestyleTracker&, bool)+130 (nsCSSFrameConstructor.cpp:8090)
   by 0x620B82C: mozilla::css::RestyleTracker::DoProcessRestyles()+1948 (RestyleTracker.cpp:157)
   by 0x6222683: nsCSSFrameConstructor::ProcessPendingRestyles()+179 (RestyleTracker.h:101)


7273    if (rootViewParent) {
  f88c9d:       4d 85 ed           test   %r13,%r13
  f88ca0:       74 12              je     f88cb4 
                                   <_ZN10nsDocShell18RestoreFromHistoryEv+0xc04>

7274        nsIViewManager *parentVM = rootViewParent->GetViewManager();
  f88ca2:       49 8b 7d 08        mov    0x8(%r13),%rdi
  f88ca6:       e8 b5 03 ff ff     callq  f79060
                                   <_ZNK7nsIView14GetViewManagerEv.isra.46>

Comment 20

[Johnny Stenback (:jst)]

Chris, this looks like a use-after-free, and likely something that was introduced with the full screen work that you did. Can you have a look here?

Comment 21

[Daniel Veditz [:dveditz]]

qawanted: Based on when this feature landed Firefox 9 and later is probably affected, but we should test to be sure if ESR needs patching.

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

I cannot reproduce this on [Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120301 Firefox/13.0a1].

Is this fixed on trunk?

I won't be able to try on other platforms until I go back into work on Monday, NZST.

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

So I can reproduce this on my x64 Linux box at the office and in my 32bit WinXP VM at least, but not on my primary Win7 x64 laptop.

If I apply the (review denied) patch in Bug 708553 the crash goes away. This implies to me that this crash is caused by reframing the document when we transition between fullscreen and normal mode. In fullscreen mode we apply a position:fixed CSS style which cause the document presentation to be recreated, which I'm guessing is messing with rootViewParent here?

So presumably fixing bug 708553 will fix this bug. To fix bug 708553 we're proposing to make frame reconstruction not destroy/recreate the subdocument presentation. That fix will be tricky, and I imagine not something we should rush onto beta/aurora.

bz, roc or tn may be able to figure out a simpler fix, I'm not a layout expert yet. ;)

Comment 24

[Timothy Nikkel (:tnikkel)]

Looks like the SetCurrentURI call in nsDocShell::RestoreFromHistory can call script (via nsDocLoader::FireOnLocationChange and nsBrowserStatusFilter::OnLocationChange) and hence lead to flushing and that can destroy anything.

Even if we fix the full screen problem this problem would still exist, no? So we should fix this too.

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ok, thanks for looking at this. Do you have a recommendation on how to fix this issue?

Comment 26

[Timothy Nikkel (:tnikkel)]

The view pointers in this function seem to be the only non-nsCOMPtrs, so everything else is safe. We'd either need to use weak views to tell when they get deleted, or store something else and reget the views we need from it, like the content node of the iframe containing this docshell. There are currently no uses of weak views in the tree, so if we can avoid that we can delete the weak view code.

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

I have a build with a fix coming through Try now here:

https://tbpl.mozilla.org/?tree=Try&rev=4a529f7578a7

Looks like it's going green.

The fix is based on Timothy's suggestion of using nsWeakViews instead of raw pointers in nsDocShell::RestoreFromHistory(). I couldn't get Timothy's other suggestion to work (storing parent frame's content node, retrieving view from that) the parent's content node kept being null for some reason I don't understand. I had to alter nsWeakView, since it couldn't handle the case where the view outlives nsWeakView, as nsWeakView wasn't clearing the nsIView->nsWeakView reference upon its destruction.

Comment 28

[Alex Keybl [:akeybl]]

Chris thinks it's unlikely that this is exploitable, so no need to track for FF12 or FF13. Please nominate for tracking if that doesn't end up being the case.

Comment 29

[Timothy Nikkel (:tnikkel)]

(In reply to Chris Pearce (:cpearce) from comment #27)
> I couldn't get Timothy's other
> suggestion to work (storing parent frame's content node, retrieving view
> from that) the parent's content node kept being null for some reason I don't
> understand.

rootViewParent is actually the anonymous inner view of the subdocument frame, meaning it does not have a corresponding frame (its client data is null). So you wouldn't be able to get a content node from it. You'd have to look at rootViewParent's parent.

Comment 30

[Daniel Veditz [:dveditz]]

(In reply to Alex Keybl [:akeybl] from comment #28)
> Chris thinks it's unlikely that this is exploitable, so no need to track for
> FF12 or FF13. Please nominate for tracking if that doesn't end up being the
> case.

Based on comment 10 and the description of the fix in comment 27 this definitely looks exploitable in theory. We can argue about how hard it might be to turn into a reliable attack but we should save the energy and just fix it.

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: use nsWeakView

Use nsWeakView in nsDocShell::RestoreFromHistory().

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: re-retrieve the view references

As an alternative to my other patch, instead re-retrieve the view pointers in nsDocShell::RestoreFromHistory() by storing references to the parent/sibling views' content nodes.

Note I need to guard against the newRootView already having been inserted in the rootViewParent's tree because transitioning to/from fullscreen mode can cause the containing frame to be re-framed, which creates the view. (For a call stack of the newRootView being inserted, see: http://pastebin.mozilla.org/1514716 ).

Greenish on Try: https://tbpl.mozilla.org/?tree=Try&rev=65d9e9a5f1c5

Once this on nightlies, we'll want some QA testing on browser going back/forward in history, in particular (but not only) from pages that are in fullscreen mode (for example an HTML5 video running in fullscreen mode).

Comment 33

[Jesse Ruderman]

DOM fuzzer improvements to catch bugs like this: b3b8ce832e2e, 20108e594838

Comment 34

[Timothy Nikkel (:tnikkel)]

Ok, thanks for the stack. I want to think a little bit about whether the reconstruct reinserting the view is okay here because this is one of the few key points where we mess with inserting/removing root views.

Comment 35

[Timothy Nikkel (:tnikkel)]

Boris, in nsDocShell::RestoreFromHistory we fetch and save a view parent pointer and a view sibling pointer to know where to insert the root view for the document that we are navigating to. It then does some stuff and then inserts the new root view. However the SetCurrentURI call in RestoreFromHistory can run script, and its possible to destroy those views. In this testcase we destroy and reconstruct the subdocument frame holding this docshell. This inserts the root view of the document so when we get to insert the root view we find it is already there. Boris, do you have an opinion if this is an okay thing to happen?

Comment 36

[Boris Zbarsky [:bzbarsky]]

Which part of it?  SetCurrentURI running script, or the reconstruct per se?

SetCurrentURI is triggering chrome-only script via onLocationChange, right?

In a sane world there would be some scriptblockers here or something, but I'm worried about changing the code flow...  We could try telling SetCurrentURI to not fire onLocationChange, and do it ourselves after we hook up the views.  Or we could try to detect when we actually get reframed during onLocationChange and skip the view hookup.

Comment 37

[Timothy Nikkel (:tnikkel)]

(In reply to Boris Zbarsky (:bz) from comment #36)
> Or we could try to detect when we actually get reframed
> during onLocationChange and skip the view hookup.

This is what Chris' patch does, it just checks if the new root view is inserted into the view manager hierarchy already. We could try your other suggestion, not sure which approach is better.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Chris' patch is almost certainly less regression-prone than the other approach.

Comment 39

[Timothy Nikkel (:tnikkel)]

(The second of the two patches is what I was referring to in case there is any confusion.)

Comment 40

[Chris Pearce [:cpearce (Not reading bugmail)]]

tn/bz: which approach should we take here? We should get this fixed.

Comment 41

[Boris Zbarsky [:bzbarsky]]

My personal preference is the safer thing, but tn knows this view stuff better than I do.....

Comment 42

[Timothy Nikkel (:tnikkel)]

Oh sorry, I was of the impression that the next step was for me to review the patch "Patch: re-retrieve the view references" (the general approach seems like the way to go).

Comment 43

[Chris Pearce [:cpearce (Not reading bugmail)]]

tn: OK, can you review the second patch please?

Comment 44

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 604800 [details] [diff] [review]
Patch: re-retrieve the view references

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

bz, can you look at this?

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 604800 [details] [diff] [review]
Patch: re-retrieve the view references

Why are we assuming that rootViewSibling is the root view of its nsIFrame's document's presentation?  We _are_ assuming that, right?

Comment 46

[Chris Pearce [:cpearce (Not reading bugmail)]]

Perhaps because we're transitioning between two documents in the <iframe>, and so there's two documents in the <iframe>, each of which has it's own nsIFrame and view?

I'm not sure... I don't know this code very well... Perhaps we should just take my earlier nsWeakViews patch and be done with this? We *do* need to do something here though, this crash is apparently exploitable.

Comment 47

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Log of conversation with tn regarding views

Here's my IRC logs of my conversations with tn regarding this, in case it helps anyone figure out what view and view siblings are all about...

Comment 48

[Boris Zbarsky [:bzbarsky]]

OK.  Let me rephrase.  I think that the rootViewSibling parts of the attached patch are just wrong.  The right thing is probably to verify that it is the view of its frame and then to hold on to the _content_ pointer and reget its primary frame (if any) and its view (if any).  But I'm not sure what the right thing to do is in cases when those end up null (or in the weakview case when the weakview goes null).  :(

Comment 49

[David Bolter [:davidb] (NeedInfo me for attention)]

Hi Chris, Boris, did you have any further thoughts to comment 48?

Comment 50

[Boris Zbarsky [:bzbarsky]]

Not I.

Comment 51

[Boris Zbarsky [:bzbarsky]]

OK, Timothy explained to me what's going on with rootViewSibling.  The patch now makes sense, but needs a lot more comments to explain what's going on (in particular, that rootViewSibling is from some random other document).

Comment 52

[Alex Keybl [:akeybl]]

Untracking for upcoming releases since we've shipped many times with this bug in. We would still of course consider uplifting a fix when found, but no need to track.

Comment 53

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3: More comments...

Rebased, and added a few more comments. bz: if you think the comments are insufficient, perhaps you could suggest improvements? I don't know the moving parts here very well.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 648592 [details] [diff] [review]
Patch v3: More comments...

I think what we should document in addition to this is the following:

1)  rootViewParent is the inner view of the parent iframe, if any.
2)  rootViewSibling is the root view for another document loaded in the same
    iframe as us; this is only non-null in the middle of a page transition.

#2 is important because it explains why getting rootViewSibling off an nsIDocument as the root view of it's presshell is reasonable.

r=me with those bits.

Comment 55

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3.1: More comments, updated with bz's comments

Comment 56

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 651636 [details] [diff] [review]
Patch v3.1: More comments, updated with bz's comments

Patch updated with bz's suggested comments. r=bz.

Comment 57

[Chris Pearce [:cpearce (Not reading bugmail)]]

Since this bug has been designated as exploitable, how should I land this fix?

Should I strip the comments from my patch and land that with a comment containing only the bug number, and come back and land the comments once it ships? Or should I just land the patch as is?

We should at least take this patch on Aurora as well, we're perhaps too close to the end of the beta cycle to risk landing it on beta.

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Chris Pearce (:cpearce) from comment #57)
> Should I strip the comments from my patch and land that with a comment
> containing only the bug number, and come back and land the comments once it
> ships?

Let's do that.

Comment 59

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1 of 2: Code changes

Code changes with comments stripped.

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=4924799181ab

Comment 60

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2 of 2: Comments

Adds comments. To be landed after patch 1 ships in release code.

Comment 61

[Chris Pearce [:cpearce (Not reading bugmail)]]

Landed commentless patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74649a7a17c8

Comment 62

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/74649a7a17c8

Comment 63

[Lukas Blakk [:lsblakk] use ?needinfo]

Still don't need to track this, but please nominate for aurora approval when ready.  If this lands on aurora it will need to be tracked for esr10 16+, otherwise 17+

Comment 64

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 651891 [details] [diff] [review]
Patch 1 of 2: Code changes

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Fullscreen API
User impact if declined: This patch fixes an exploitable crash, which can ocur when navigating "back" away from a fullscreen page.
Testing completed (on m-c, etc.): Local testing, been on m-c for several days.
Risk to taking this patch (and alternatives if risky): Could cause regressions. We should take this on Aurora and monitor for crashes when pressing the UI back button. I wouldn't feel comfortable taking this on Beta this close to the end of the cycle.
String or UUID changes made by this patch:  None.

Comment 65

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80af99a29753

Comment 66

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: code changes, backported to ESR10.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Exploitable crash.
Fix Landed on Version: FF16+
Risk to taking this patch (and alternatives if risky): low-ish, there's no known regressions since landing on FF16.
String or UUID changes made by this patch:  None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 67

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/edc5438e0009

Comment 68

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old