380359 - Crash [@ nsEventStateManager::GetContentState] [@ nsNativeTheme::CheckBooleanAttr] with -moz-appearance and position: fixed

Status: VERIFIED FIXED

(Core :: Widget, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase in Firefox trunk on Mac (opt or debug) makes it crash.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Frame dump + stack

The native theme code makes bad assumptions about the frame tree...

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Add null-checks.

Comment 3

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

How can a null frame, or a frame with null content, get passed in to native theme APIs?

Comment 4

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

Comment on attachment 264528 [details] [diff] [review]
Patch rev. 1

clearing request pending comments

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(I tested this on latest-trunk mozilla-central nightly build on WinXP SP3)

Turning security-sensitive and nominating blocking1.9.1? just-in-case, due to !exploitable PROBABLY_EXPLOITABLE result.


0:000> !exploitable -v
HostMachine\HostUser
Executing Processor Architecture is x86
Debuggee is in User Mode
Debuggee is a live user mode debugging session on the local machine
Event Type: Exception
Exception Faulting Address: 0x0
First Chance Exception Type: STATUS_ACCESS_VIOLATION (0xC0000005)
Exception Sub-Type: Read Access Violation

Faulting Instruction:1019945f mov eax,dword ptr [esi]

Basic Block:
    1019945f mov eax,dword ptr [esi]
       Tainted Input Operands: esi
    10199461 mov edx,dword ptr [eax+3ch]
       Tainted Input Operands: eax
    10199464 push 80h
    10199469 mov ecx,esi
       Tainted Input Operands: esi
    1019946b call edx
       Tainted Input Operands: ecx, edx

Exception Hash (Major/Minor): 0x6c0b5600.0xb3e226e

Stack Trace:
xul!nsNativeTheme::CheckBooleanAttr+0xf
xul!nsNativeTheme::GetCheckedOrSelected+0x33c0dc
xul!nsNativeThemeWin::GetThemePartAndState+0xf0
xul!nsNativeThemeWin::GetMinimumWidgetSize+0x23cf14
xul!nsCSSOffsetState::InitOffsets+0x184
xul!nsCOMPtr_base::assign_from_qi+0x19
xul!nsAbsoluteContainingBlock::Reflow+0x132
xul!ViewportFrame::Reflow+0x1a0
xul!PresShell::DoReflow+0x249
xul!PresShell::ProcessReflowCommands+0x11e
xul!PresShell::DoFlushPendingNotifications+0x12f
xul!PresShell::ReflowEvent::Run+0x37
xul!nsThread::ProcessNextEvent+0x213
xul!nsBaseAppShell::Run+0x4a
xul!nsAppStartup::Run+0x1e
xul!XRE_main+0xcb9
Unknown
Instruction Address: 0x1019945f

Description: Data from Faulting Address controls Code Flow
Short Description: TaintedDataControlsCodeFlow
Exploitability Classification: PROBABLY_EXPLOITABLE
Recommended Bug Title: Probably Exploitable - Data from Faulting Address controls Code Flow starting at xul!nsNativeTheme::CheckBooleanAttr+0xf (Hash=0x6c0b5600.0xb3e226e)

The data from the faulting address is later used as the target for a branch.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Any response to roc's question?

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

So the aFrame passed in to nsNativeTheme::GetContentState has a mContent, but there's this chunk of code:

77	  PRBool isXULCheckboxRadio = 
78	    (aWidgetType == NS_THEME_CHECKBOX ||
79	     aWidgetType == NS_THEME_RADIO) &&
80	    aFrame->GetContent()->IsNodeOfType(nsINode::eXUL);
81	  if (isXULCheckboxRadio)
82	    aFrame = aFrame->GetParent();

isXULCheckboxRadio is TRUE, so we grab the frame's parent.  The frame's parent doesn't have mContent, and has a mRect of 0,0,65820,59880 (which I think is 2194x1996 pixels?)  No idea what type it is.

We grab this parent frame for checkbox/radio in a few different places.

roc, any ideas as to what we should do here?

Comment 9

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

oof, it's the viewport. that makes sense I guess.

Where we do
  if (isXULCheckboxRadio)
    aFrame = aFrame->GetParent();
we should check that the new aFrame's content is non-null and bail if it is.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch

This fixes the testcase for me; just protects the places where we grab a parent and then grab its content.

Comment 11

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

We'll want the crashtest checked in of course...

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hmm.. I'll hold off on checking in the crashtest actually, since this bug is old enough that it might be in 3.0.x.  Sam, do you know if that's the case?

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Fixed on trunk and 1.9.1.

Comment 14

[Samuel Sidler (old account; do not CC)]

This, indeed, crashes 1.9.0. I tested with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.10pre) Gecko/2009041404 GranParadiso/3.0.10pre

Comment 15

[Daniel Veditz [:dveditz]]

If this is fixed by a simple null-check is it really sg:critical, or only sg:dos?

Comment 16

[Daniel Veditz [:dveditz]]

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

Approved for 1.9.0.10, a=dveditz for release-drivers

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verfied fixed on trunk and 1.9.1 with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090421 Minefield/3.6a1pre ID:20090421032809

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090421 Shiretoko/3.5b4pre ID:20090421030848

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

checked in on 1.9.0.10

Comment 19

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

Verified for 1.9.0.11 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11pre) Gecko/2009051111 GranParadiso/3.0.11pre. No crash with testcase.

Comment 20

[Martin Stránský [:stransky] (ni? me)]

The testcase still crashes on Linux, there's a missing null pointer check at nsNativeThemeGTK::GetGtkWidgetAndState():

#0  0x012a244e in nsNativeThemeGTK::GetGtkWidgetAndState (this=0xb5619000, aWidgetType=3 '\003', aFrame=0xaf5284ac, aGtkWidgetType=@0xbfced668,	aState=0xbfced66c, aWidgetFlags=0xbfced664) at nsNativeThemeGTK.cpp:247

246
247  if (aFrame && aFrame->GetContent()->IsNodeOfType(nsINode::eXUL)) { 
248    // For these widget types, some element (either a child or parent)
249    // actually has element focus, so we check the focused attribute
250    // to see whether to draw in the focused state.
251    if (aWidgetType == NS_THEME_TEXTFIELD || 

(gdb) p	aFrame->mContent
$5 = (class nsIContent *) 0x0

Comment 21

[Martin Stránský [:stransky] (ni? me)]

(In reply to comment #20)
> The testcase still crashes on Linux

Forgot to mention, it's 1.9.0.11.

Comment 22

[Daniel Veditz [:dveditz]]

qawanted: Al, please check out Martin's claim in comment 21. If this is not fixed completely we need to pull it out of the advisories and finish in 1.9.0.12

Does not affect the 1.8 branch (tested on Mac 1.8.1.22pre)

Comment 23

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

Ack. It does crash on Linux with 1.9.0.11. 

This was listed as a Mac crash in comments so I had no reason to specifically look on Linux before now once it was fixed on Mac. Weird.

Comment 24

[Samuel Sidler (old account; do not CC)]

Does this crash on 1.9.1 on Linux too?

Comment 25

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

Yes, it crashes last night's 1.9.1.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1pre) Gecko/20090529 Shiretoko/3.5pre

Comment 26

[Samuel Sidler (old account; do not CC)]

I'm going to assume trunk is still crashing too then and reopen this. "yay"

We need to be sure this bug isn't listed in our advisories as well.

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

As per comments 20-26, marking this as Linux only (has anyone tested it on w32?). Vlad: do you still think this blocks?

Comment 28

[Martijn Wargers (dead)]

This doesn't crash for me on windowsxp, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009052005 GranParadiso/3.0.12pre (.NET CLR 3.5.30729)

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

My temptation is to unblock on this, but if all it needs is a nullcheck, can we get a patch whipped together?

Comment 30

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: linux 1.9.0 patch

This is on the try server now; linux 1.9.0 patch.

Comment 31

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: linux 1.9.1 patch

and the same patch ported to 1.9.1

Comment 32

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: linux trunk patch

and the same patch for the trunk, all slightly different!

Comment 33

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

All the patches had a compilation bug.  That's what I get for not using emacs.

Comment 34

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

+  nsIContent *content = aFrame ? nsnull : aFrame->GetContent();

So if aFrame is non-null, we set content to null, otherwise aFrame is null and we dereference it?

Comment 35

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

There's a tryserver build going of 190 with the fixed patch; I don't have linux easily accessible here, I'll post a link here when it's done.   Then I'll kick off 191 and 192 builds, given that the patches are slightly different for all three.

Comment 36

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

1.9.0 builds -- can someone with linux test?  https://build.mozilla.org/tryserver-builds/[e-mail]-vlad190/

Comment 37

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

Vlad, I tested your build on Ubuntu 8.10. It doesn't crash anymore. Firefox 3.0.10 on the same machine does.

Comment 38

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

Can you attach the fixed patch?

Comment 39

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated patch, 1.9.0

Updated patch, the 1.9.0 flavour; the others are similar.

Comment 40

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checking in nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.161; previous revision: 1.160
done

http://hg.mozilla.org/mozilla-central/rev/78c6f6a6ecb5
http://hg.mozilla.org/mozilla-central/rev/2b3ff1a9ad49


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/60e46f62c924

marking fixed 1.9.0.12, not sure if 11 or 12 is right.

Comment 41

[Samuel Sidler (old account; do not CC)]

Comment on attachment 380989 [details] [diff] [review]
updated patch, 1.9.0

/me subtly notes that patches on 1.9.0 need explicit approval before landing and retroactively approves.

Approved for 1.9.0.12. a=ss

Comment 42

[Daniel Veditz [:dveditz]]

Fix checked into 1.9.0.11 relbranch

Checking in nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.160.14.1; previous revision: 1.160

Comment 43

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

Verified for 1.9.0.11 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11) Gecko/2009060214 Firefox/3.0.11.

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry that I missed to verify the fix on Linux. But now I can also verify that it is fixed on trunk and 1.9.1 with

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090602 Minefield/3.6a1pre ID:20090602031649

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1pre) Gecko/20090603 Shiretoko/3.5pre ID:20090603031326

Comment 45

[Jesse Ruderman]

Crashtest added:

http://hg.mozilla.org/mozilla-central/rev/bf3a4f5dd798

Comment 46

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

Verified again for 1.9.0.12 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.12pre) Gecko/2009070104 GranParadiso/3.0.12pre.