344759 - EIP hijack in FF 1.5.0.5 -- crash [@ 0x20202113] called by JS_SetPrivate

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[Darin Fisher]

From H D Moore <[e-mail]>:
----------------------------------------------------------------------------

This bug is really annoying to track down, it seems like changing anything
in the script prevents the EIP hijack from working. Hope it works for
you, I can reliably reproduce here (Windows XP SP2 - FF 1.5.0.4).

-HD

(a18.470): Access violation - code c0000005 (!!! second chance !!!)
eax=20202020 ebx=00000000 ecx=20202020 edx=022e5308 esi=022e5310
edi=022e5308
eip=20202113 esp=0012d7c4 ebp=0012d7e8 iopl=0         nv up ei pl nz na pe
nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000
efl=00000202
*** ERROR: Module load completed but symbols could not be loaded for C:
\WINDOWS\system32\xpsp2res.dll
xpsp2res+0x202113:
20202113 008000000606     add     [eax+0x6060000],al
ds:0023:26262020=??
0:000> k
ChildEBP RetAddr
WARNING: Stack unwind information not available. Following frames may be
wrong.
0012d7e8 60082df1 xpsp2res+0x202113
0012d834 6009c92d js3250!JS_SetPrivate+0x47
0012d864 60082a79 js3250!js_GetSrcNoteOffset+0x400f
0012d8a4 6009c148 js3250!JS_InitClass+0xa3
0012d8e0 60081fd4 js3250!js_GetSrcNoteOffset+0x382a
0012d90c 60082321 js3250!JS_SetGlobalObject+0xc4
*** WARNING: Unable to verify checksum for C:\Program Files\Mozilla
Firefox\firefox.exe
*** ERROR: Symbol file could not be found.  Defaulted to export symbols
for C:\Program Files\Mozilla Firefox\firefox.exe -
0012d928 0041021f js3250!JS_ResolveStandardClass+0x17c
0012d940 600addd2 firefox!NS_RegistryGetFactory+0x5ee0
0012d980 600ada90 js3250!js_LookupProperty+0x35b
0012d9a0 600ae29d js3250!js_LookupProperty+0x19
0012d9e0 004162e7 js3250!js_FindProperty+0x31b
0012da20 00416243 firefox!NS_RegistryGetFactory+0xbfa8
0012da4c 0040ce48 firefox!NS_RegistryGetFactory+0xbf04
0012dae0 0040cfb3 firefox!NS_RegistryGetFactory+0x2b09
0012db98 005364c5 firefox!NS_RegistryGetFactory+0x2c74
0012dbd8 00536e34 firefox!nsPrintOptions::GetNewPrintSettings+0x9baa
0012dbf4 005f298c firefox!nsPrintOptions::GetNewPrintSettings+0xa519
0012dc24 00732737 firefox!DeviceContextImpl::GetCanonicalPixelScale+0x9c53
0012dc4c 007247a5 firefox!nsPrintSettings::AddRef+0xb752
*** ERROR: Symbol file could not be found.  Defaulted to export symbols
for C:\Program Files\Mozilla Firefox\xpcom_core.dll -
0012dc90 60311954 firefox!nsPrintSettings::GetPrintOptionsBits+0x14d0

----------------------------------------------------------------------------

See attached testcase.

I was able to reproduce this exploit using FF 1.5.0.4 and the pre-release version of FF 1.5.0.5 from today (July 14).

Comment 1

[Darin Fisher]

Attachment: testcase

When successful, this webpage causes a find dialog to appear.  In other cases, it has caused by browser to crash or hang.

Comment 2

[Bernd]

this got fixed on trunk after  2006-01-11

Comment 3

[Martijn Wargers (dead)]

Ok, I crash with a 2006-03-23 build, but don't crash with a 2006-03-24 trunk build. I tested this multiple times and on 2 different machines.
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-03-23+05&maxdate=2006-03-24+06&cvsroot=%2Fcvsroot

Note, I had to disable Java, because otherwise it would crash on the Java part in trunk (I think that's a known bug on trunk, at least there are some known Java crashers on trunk).

Comment 4

[Martijn Wargers (dead)]

I managed to crash once with Firefox1.5RC3 (talkback ID: TB20767470W), but not anymore, afterwards.

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase2

I think I may have reduced it to this. However, I'm not sure (since I can't really reproduce the crash with the original testcase in FF1.5.0.5RC3).
But I am sure, that the first button is crashing also Firefox1.5.0.5RC3.
The second button is only crashing current trunk builds.

Comment 6

[Martijn Wargers (dead)]

A typical talkback I get from testcase2 in Firefox1.5 is TB20978230Z:
kernel32.dll + 0x1eb33 (0x7c81eb33)
_CxxThrowException
_com_error::_com_error
_com_issue_error
CommonConstructor  [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/XPCIDispatchExtension.cpp, line 90]
ActiveXConstructor 
etc...

That's quite different from the original testcase, so that's why I'm not sure it's the same bug.

Comment 7

[timeless]

afaik we don't normally build w/ activex support. the build i have doesn't seem to:

 --enable-application=browser --enable-update-channel=release --enable-optimize --disable-debug --disable-tests --enable-static --disable-shared --enable-official-branding --enable-svg --enable-canvas --enable-update-packaging

Comment 8

[Jesse Ruderman]

CCing the reporter, H D Moore.

Comment 9

[Jesse Ruderman]

Attachment: testcase 2, trunk-only version

This version doesn't hang Firefox and Mac OS X for 30 seconds while it loads ;)  I haven't actually tested it, because I don't have ActiveX enabled.

Comment 10

[Jesse Ruderman]

Filed bug 344804 with a reduced testcase for a Java crash this turns up.  I don't know if it's the same Java crash Martijn mentioned in comment 3.

Comment 11

[Jesse Ruderman]

I can't reproduce the crash in comment 0.  With Firefox 1.5.0.4 on both Windows XP and Mac, as well as with yesterday's 1.5.0.x nightly on Mac, I get a few alert-like things and a Find dialog, and my browser window is resized, but Firefox does not crash.

Comment 12

[Jesse Ruderman]

Crashing [@ 0x20202113] in the JavaScript engine probably means this is a GC hazard bug.  GC hazard bugs tend to be hard to reduce :(  I think the fastest way to get this fixed on 1.5.0.x is to determine what trunk patch fixed it.

Martijn, is the "fixed on trunk" period you gave in comment 3 for the GC-hazard-looking crash in comment 0, or is it for the ActiveX crash in comment 5?  I'm hoping it's the latter, since there weren't any changes to the JavaScript engine or XPConnect on that day that look like GC hazard fixes.

Comment 13

[Daniel Veditz [:dveditz]]

I couldn't reproduce the original testcase either until I created a new profile, then *boom*. So if you're having trouble reproducing, try that. There's probably some common power-user pref setting a lot of us have that's interfering.

I died in JS_SetPrivate a lot (different call stacks, though): TB20989667, TB20989639, TB20989449

Once under ActiveXConstructor (TB20987029), similar to Martijn's comment 6.

Have not been able to reproduce with a debug build. I get a single "NATIVE:window" line added each time I press the button, and on the console "An error occurred updating the cmd_copy command". Otherwise nothing.

Comment 14

[Martijn Wargers (dead)]

(In reply to comment #12)
> Martijn, is the "fixed on trunk" period you gave in comment 3 for the
> GC-hazard-looking crash in comment 0, or is it for the ActiveX crash in comment
> 5?
It's not for the ActiveX crash, I just tested that. Not sure what it is.
A typical talkback ID, I get is: TB20990092X
js3250.dll + 0x44f0e (0x60104f0e)
firefox.exe + 0x48f9c4 (0x0088f9c4)
xpcom_core.dll + 0x354e9 (0x604954e9)
firefox.exe + 0x1de24 (0x0041de24)

Comment 15

[Daniel Veditz [:dveditz]]

> I'm hoping it's the latter, since there weren't any changes to the
> JavaScript engine or XPConnect on that day that look like GC hazard fixes.

The bonsai link doesn't go back far enough, just misses Igor's 3:50 check-in for bug 330692 ("GC_MARK_DEBUG: using GC_MARK, not JS_MarkGCThing internally"). Let me see if applying that does the trick (may need to drag bug 324278 along, too).

Comment 16

[Daniel Veditz [:dveditz]]

Igor's patches don't apply cleanly to my tree (not unexpected). I'm too sleepy to fix it up tonight and travelling most of tomorrow, can anyone else look at it? If not I'll try again Monday.

Comment 17

[Mike Schroepfer]

Blake - can you take a look?

Comment 18

[Martijn Wargers (dead)]

Ok, the "fixed on trunk" period I gave in comment 3 appears to come from the fix from bug 330900. I've checked that by building a 2006-03-23 build and manually applying the patch from that bug.
That means that the regression range in comment 3 is not relevant for this bug :(

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do we have a fixed-on-trunk window for testcase 2?

Comment 20

[Martijn Wargers (dead)]

Comment on attachment 229352 [details]
testcase2

(In reply to comment #19)
> Do we have a fixed-on-trunk window for testcase 2?

Sorry, testcase 2 is most likely not related to this bug (I thought it was, first).
Testcase2 is not fixed on trunk, I'll file a new bug on it.

Comment 21

[Martijn Wargers (dead)]

Sorry, I somehow reset the blocking1.8.1+ back to blocking1.8.1?
I filed bug 344957 and bug 344960 for testcase2, which isn't related to this bug.

Comment 22

[Daniel Veditz [:dveditz]]

(In reply to comment #18)
> Ok, the "fixed on trunk" period I gave in comment 3 appears to come from the
> fix from bug 330900. I've checked that by building a 2006-03-23 build and
> manually applying the patch from that bug.
> That means that the regression range in comment 3 is not relevant for this bug

Why do you say that? The stacks from bug 330900 were different from this bug, and in addition the current 1.8/1.8.0 branches also have the 330900 fix but still crash on the first testcase here. I've not been able to reproduce this crash with any debug build.

I confirm your regression range in comment 3 (2006-03-23-05 broken, 2006-03-24-05 fixed) but my money's still on Igor's GC patch.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #13)
> Have not been able to reproduce with a debug build. I get a single

I saw the same problem.  mrbkap explained it to me:  the string representations have some more information in them, which includes the word "native".  So this change makes it run in a DEBUG build:

@@ -26,7 +26,7 @@

 function msftestIsNative(obj) {
        var txt = obj + '';
-       if (txt.indexOf('native') != -1) {
+       if (txt.indexOf('[native') != -1) {
                return true;
        }
        return false;

Comment 24

[Martijn Wargers (dead)]

(In reply to comment #22)
> Why do you say that? The stacks from bug 330900 were different from this bug,
> and in addition the current 1.8/1.8.0 branches also have the 330900 fix but
> still crash on the first testcase here. I've not been able to reproduce this
> crash with any debug build.

I don't crash with 1.8/1.8.0 branches (only very sporadically). When I add  && e.indexOf('CRMFRequest') == -1 to the testcase on the relevant spots, then I don't crash anymore with a 2006-03-23 trunk build.

Comment 25

[Bob Clary [:bc] (inactive)]

Attachment: testcase with dbaron's 'native->'[native mod

Comment 26

[Bob Clary [:bc] (inactive)]

Attachment: reduced testcase invoke window.find on buf

Comment 27

[Bob Clary [:bc] (inactive)]

(In reply to comment #26)
> Created an attachment (id=229642) [edit]
> reduced testcase invoke window.find on buf

works in 1.8.0.5, 1.8, 1.9 winxp

Comment 28

[Igor Bukanov]

(In reply to comment #13)
> I couldn't reproduce the original testcase either until I created a new
> profile, then *boom*. So if you're having trouble reproducing, try that.
> There's probably some common power-user pref setting a lot of us have that's
> interfering.
> 
> I died in JS_SetPrivate a lot (different call stacks, though): TB20989667,
> TB20989639, TB20989449

The stack traces shows 

JS_SetPrivate  [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/js/src/jsapi.c, line 2164]
js_DefineFunction  [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/js/src/jsfun.c, line 2125]
JS_InitClass  [c:/builds/tinderbox/Fx-Mozilla1.8.0-Release/WINNT_5.2_Depend/mozilla/js/src/jsapi.c, line 2045]

but js_DefineFunction does not call JS_SetPrivate directly, so the stack traces are not complete. Why it is so?

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, I ran mrbkap's variant of the original testcase under WAY_TOO_MUCH_GC on my 1.8.0 branch Windows build, and it didn't crash.

Comment 30

[Daniel Veditz [:dveditz]]

> this change makes it run in a DEBUG build:

Thank you (and mrbkap) for that!

> > reduced testcase invoke window.find on buf
> works in 1.8.0.5, 1.8, 1.9 winxp

Define "works"? I don't crash using that testcase on either trunk or 1.5.0.4

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Potential fix

This is the only object that could possibly be the cause of the crash based on the stacks that we have seen. The basic principal here is that we cannot trust newborn roots!

Comment 32

[Daniel Veditz [:dveditz]]

Attachment: merged to 1.8 branches

Comment 33

[Daniel Veditz [:dveditz]]

This fixes the problem in my 1.8.0.5 builds (debug and optimized). Reproducing the original crash in the debug build was iffy, but optimized was a consistent crasher (despite the occassional need to create new profiles, or delete the prefs.js file) and it's fixed now.

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 229759 [details] [diff] [review]
Potential fix

The comment might say "any potential GC callback."

r=me, looks good.

/be

Comment 35

[Brendan Eich [:brendan]]

Comment on attachment 229765 [details] [diff] [review]
merged to 1.8 branches

r=me with same comment suggestion as for the trunk/1.8-branch.

/be

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 37

[Daniel Veditz [:dveditz]]

Comment on attachment 229765 [details] [diff] [review]
merged to 1.8 branches

approved for 1.8.0 branch, a=dveditz

Comment 38

[Daniel Veditz [:dveditz]]

Fix checked into 1.8.0 branch

Comment 39

[Brendan Eich [:brendan]]

(In reply to comment #31)
> The basic principal here is that we cannot trust newborn roots!

But that's a bug in itself, and a regression at some point that we did not catch.  We need to fix it, too, and centrally.  Short of fixing bug 313437, we should not allow JS_ClearNewbornRoots to do anything when called from the GC allocator.

Would you please file that bug?

/be

Comment 40

[Brendan Eich [:brendan]]

(In reply to comment #39)
> (In reply to comment #31)
> > The basic principal here is that we cannot trust newborn roots!
> 
> But that's a bug in itself, and a regression at some point that we did not
> catch.  We need to fix it, too, and centrally.  Short of fixing bug 313437, we
> should not allow JS_ClearNewbornRoots to do anything when called from the GC
> allocator.
> 
> Would you please file that bug?

This JS_ClearNewbornRoots hazard arose due to the null-script branch callback case, added for bug 203278.  But it exists also in theory due to the JSGC_END mode GC-callback, used by Venkman and other modules.

/be

Comment 41

[Daniel Veditz [:dveditz]]

(In reply to comment #39)
> Would you please file that bug?

bug 345230 has been filed on this.

Comment 42

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.5) Gecko/20060719 Firefox/1.5.0.5, no crash with testcase.  I was able to reliably crash with rc3 with the following steps:

1. launch with profile manager flag (-P)
2. create new profile
3. go to this bug, login to bugzilla
4. open testcase and click button
5. boom!

I crashed about 5-6 times in a row with the previous build (rc3), but have not been able to reproduce with latest rc4 build after about 15 tries (same basic steps above with a few variations thrown in).

Comment 43

[Daniel Veditz [:dveditz]]

In Firefox 1.0.8 this particular testcase crashes on bug 330900 instead. Once that's fixed on that branch this should be retested. It may or may not crash due to the timing issues involved.

Comment 44

[Daniel Veditz [:dveditz]]

Not needed on the aviary or moz1.7 branches since bug 203278 was not fixed until after those branches.

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branch.

Comment 46

[Alexander Sack]

Unfortunately, this bug does exist for 1.7.x and 1.0.x. 

Dan, are you sure this was introduced with fix for bug #203278 ? I have not backported that, nevertheless *all* testcases do apply, so probably it was a different fix that triggered this. Any idea?

Comment 47

[Christopher Aillon (sabbatical, not receiving bugmail)]

Did anyone verify this on Linux?  Our QA seems to suggest that this is still not fixed on Linux... Will try to get more details.

Comment 48

[Christopher Aillon (sabbatical, not receiving bugmail)]

(To clarify, they say that 1.5.0.5 still exhibits this bug on Linux...)  Will update with details as I get them.

Comment 49

[Alexander Sack]

(In reply to comment #48)
> (To clarify, they say that 1.5.0.5 still exhibits this bug on Linux...)  Will
> update with details as I get them.
> 

At least backporting the branches fix attached did not help for 1.0.x ... so probably its really not fixed for linux.

Comment 50

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #49)
> At least backporting the branches fix attached did not help for 1.0.x ... so
> probably its really not fixed for linux.

Do you have backtraces of any crashes you see? The testcase for this bug is *extremely* generic, and it's possible that fixing this bug is exposing another bug.

Comment 51

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: strace output

So I went and had a look at the testing boxes, and it appears to not be a crash anymore.  It's simply an exit(1) which is an improvement,  but we should still get it to work properly.  Anyway, here's the strace output.

Comment 52

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #51)
> So I went and had a look at the testing boxes, and it appears to not be a crash
> anymore.  It's simply an exit(1) which is an improvement,  but we should still
> get it to work properly.  Anyway, here's the strace output.

If I'm reading the strace correctly, it seems that X becomes unhappy with us for whatever reason (the end of the log is opening XErrorDB and writing out something about 'Gecko') and kills us. I think that deserves its own bug.

Comment 53

[Alexander Sack]

Attachment: Backtrace 1.0.x (without patch)

This is a backtrace for 1.0.8 without the backported patch.

Further there was no patch for bug 203278 applied - see comment #44
In response to comment 43  I used the patch from bug 330900 ... but it makes no difference at all. The backtrace looks similar.

Comment 54

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #53)
> Further there was no patch for bug 203278 applied - see comment #44
> In response to comment 43  I used the patch from bug 330900 ... but it makes no
> difference at all. The backtrace looks similar. 

The backtrace looks similar to what? That looks like a very different problem, presumably fixed by some other bug on the trunk (and probably on the 1.8 branch). This particular crash is actually controlled, it will assert (abort in debug builds, and return false, silently halting the script's execution).

Comment 55

[Alexander Sack]

(In reply to comment #54)
> (In reply to comment #53)
> > Further there was no patch for bug 203278 applied - see comment #44
> > In response to comment 43  I used the patch from bug 330900 ... but it makes no
> > difference at all. The backtrace looks similar. 
> 
> The backtrace looks similar to what? 

I meant similar to the backtrace from a build without the patch from bug 330900. 


> branch). This particular crash is actually controlled, it will assert (abort in
> debug builds, and return false, silently halting the script's execution).
> 

Fine, that's good enough for me I guess.