328839 - [FIX]print preview causes memory corruption at least via <xul:editor src='javascript

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[georgi - hopefully not receiving bugspam]

print preview causes memory corruption at least via <xul:editor src='javascript

probably because of deleted object or bad cast.

stack:
#7  0x080ac2f9 in nsProfileLock::FatalSignalHandler (signo=11)
    at nsProfileLock.cpp:210
#8  <signal handler called>

#9  0x096f7d29 in ?? ()
#10 0x08482e85 in nsBoxObject::GetFrame (this=0x9686a44)
    at /opt/firefox-cvs/mozilla/layout/xul/base/src/nsBoxObject.cpp:169
#11 0x08483b90 in nsBoxObject::GetDocShell (this=0x9686a44, aResult=0xbfffca40)
    at /opt/firefox-cvs/mozilla/layout/xul/base/src/nsBoxObject.cpp:558
#12 0x084810f1 in nsEditorBoxObject::GetDocShell (this=0x9686a40, 
    aResult=0xbfffca40)
    at /opt/firefox-cvs/mozilla/layout/xul/base/src/nsEditorBoxObject.cpp:87
#13 0xb7ecb0c5 in XPTC_InvokeByIndex ()
    at /opt/firefox-cvs/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_gcc_x86_unix.cpp:69
#14 0x080f9d6e in XPCWrappedNative::CallMethod (ccx=@0xbfffcb20, 
    mode=XPCWrappedNative::CALL_METHOD)

(gdb) frame 9
#9  0x096f7d29 in ?? ()
(gdb) x/i $eip
0x96f7d29:      insb   (%dx),%es:(%edi)

exploitability is not clear, but seems easier than integer overflow imho.

1.5.0.1 doesn't seem affected by this.

testcase to follow.

the testcase may need to be print previewed several times.

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: xblv1

Comment 2

[georgi - hopefully not receiving bugspam]

Attachment: htmlv1

Comment 3

[Daniel Veditz [:dveditz]]

There's definite memory mangling here. The crash I got on windows (eventually -- hung up for a while) was in nsBoxObject::GetFrame on a null pointer, but the rest of the mPressShell object looked like a mixture of deleted and allocated stuff. Oh, and the null pointer was the vtbl which is another not-good sign.

Comment 4

[georgi - hopefully not receiving bugspam]

(In reply to comment #3)
> There's definite memory mangling here. The crash I got on windows (eventually
> -- hung up for a while) was in nsBoxObject::GetFrame on a null pointer, but the
> rest of the mPressShell object looked like a mixture of deleted and allocated
> stuff. Oh, and the null pointer was the vtbl which is another not-good sign.
> 

i am ready to bet this is not null dereference on linux.
seems dependent on previous actions and i managed to get $eip == 0xdadadada, though not reliably.

the "hung" is probably the creation of a js string several megabytes long.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, so box objects suck.  News at 11.

This is the usual "let's make a virtual call on a deleted object and let content control exactly when the object gets deleted" sorta thing.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Crash testcase that doesn't need print preview -- crashes on load

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

On trunk I want to do a followup to nuke mPresShell altogether, but for now let's just deal with it dying.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Er, the real trunk patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for 1.8/1.8.0

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for the 1.7 branch

Please push the approvals out to the next security release if we've shipped already, but otherwise this is pretty much a guaranteed remote execution flaw, I suspect.

Comment 11

[georgi - hopefully not receiving bugspam]

confirming bz's testcase.

running it 3 times (the first url at startup), always get:


#9  0xdadadada in ?? ()
#10 0x08482e85 in nsBoxObject::GetFrame (this=0x929bea0)

Comment 12

[georgi - hopefully not receiving bugspam]

bz: if i understand correctly, the most exciting part in your testcase is
node.style.display = "none";
which causes something equivalent of delete of a C++ object, right?

Comment 13

[[e-mail]]

Comment on attachment 213832 [details] [diff] [review]
Er, the real trunk patch

/me needs an "Edit All" link...

Comment 14

[Boris Zbarsky [:bzbarsky]]

Georgi, yes.  Setting the display of an iframe to none deletes the presentation (not the DOM, just the layout obects) inside that iframe.

Comment 15

[georgi - hopefully not receiving bugspam]

managed to overwrite mPresShell with js string, but not reliably and depending on previous actions.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 213834 [details] [diff] [review]
Patch for 1.8/1.8.0

approved for 1.8.0 branch pending sr=, a=dveditz for drivers

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 213832 [details] [diff] [review]
Er, the real trunk patch

sr=jst

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 213834 [details] [diff] [review]
Patch for 1.8/1.8.0

sr=jst

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 213837 [details] [diff] [review]
Patch for the 1.7 branch

r=jst

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk and both 1.8 branches.

Comment 21

[georgi - hopefully not receiving bugspam]

today's trunk doesn't crash

Comment 22

[Marcia Knous [:marcia]]

verified on the 1.8.0.2 branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060306 Firefox/1.5.0.2. No crash using all three of the testcases listed in the attachment section. Adding keyword.

Comment 23

[georgi - hopefully not receiving bugspam]

Bug 330818 is more boxObject fun