319872 - CVE-2006-0297 probably an integer overflow in jsxml.c

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[georgi - hopefully not receiving bugspam]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

in jsxml.c:2351 EscapeAttributeValue
    if (c == '"')
            newlength += 5;
        else if (c == '<')
            newlength += 3;
        else if (c == '&' || c == '\n' || c == '\r' || c == '\t')
            newlength += 4;

this seems somewhat suspicous, because of "int too big" problem (probably
after multiplication with sizeof(jschar) ).

some empirical evidence suggests SEGV in free() after hitting this code,
though the exact codepath is not determined yet.

exploitability will be determined later.

debug builds die from abort(), but 1.5 release crashes in free.

testcase to follow.

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1220606272 (LWP 16742)]
0xb753e339 in free () from /lib/tls/libc.so.6

#0  0xb753e339 in free () from /lib/tls/libc.so.6
#1  0xb7fc2ea9 in js_ReportCompileErrorNumberUC ()
   from /opt/firefox15/libmozjs.so
#2  0xb7fc2ef7 in js_FinishStringBuffer () from /opt/firefox15/libmozjs.so
#3  0xb7fd342d in js_IsXMLName () from /opt/firefox15/libmozjs.so
#4  0xb7fd3954 in js_IsXMLName () from /opt/firefox15/libmozjs.so
#5  0xb7fd9b62 in js_IsXMLName () from /opt/firefox15/libmozjs.so
#6  0xb7f989b9 in js_Invoke () from /opt/firefox15/libmozjs.so
#7  0xb7fa1717 in js_Interpret () from /opt/firefox15/libmozjs.so
#8  0xb7f99011 in js_Execute () from /opt/firefox15/libmozjs.so
#9  0xb7f78a28 in JS_EvaluateUCScriptForPrincipals ()
   from /opt/firefox15/libmozjs.so

(gdb) x/i $eip
0xb753e339 <free+73>:   mov    0x4(%ecx),%eax
(gdb) p/x $eax
$1 = 0xb76037e4
(gdb) p/x $ecx
$2 = 0xfffffff9



Reproducible: Always

Steps to Reproduce:
testcase
Actual Results:  
crash in free()

Expected Results:  
crash free

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: crash in free()

crash in free()

Comment 2

[georgi - hopefully not receiving bugspam]

no action, so adding management without offensing words.
management knows better.
good management.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

I'm sorry for not commenting earlier. I have a patch in hand and am currently running it through tests to ensure that it does not introduce any regressions. I didn't bother commenting because I feel that, in general, the "Status" of a bug is sufficient to indicate that I am working on a fix. So there has certainly been action on this bug.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

I cannot reproduce the crash on my machine (though it's brought almost to its knees), but this should fix the overflow.

I also found a bug in the use of StringBuffer::grow (please excuse my dirty C++ syntax here!) where we were too agressively growing the string.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 205755 [details] [diff] [review]
patch v1

> static JSBool
> GrowStringBuffer(JSStringBuffer *sb, size_t newlength)
> {
>     ptrdiff_t offset;
>     jschar *bp;
> 
>     offset = PTRDIFF(sb->ptr, sb->base, jschar);
>-    newlength += offset;
>-    bp = (jschar *) realloc(sb->base, (newlength + 1) * sizeof(jschar));
>+    newlength += offset + 1;
>+    if (newlength > offset && newlength < ~(size_t)0 / sizeof(jschar))

Put this in number-line order:

>+    if (offset < newlength && newlength < ~(size_t)0 / sizeof(jschar))

Also, you'll need to cast offset to (size_t) to shut up an MSVC warning.

Pick these nits and r=me (new patch in bug only for branch landing and verification).

Thanks for fixing these(! what was I thinking adding the offset in the callers?) bugs,

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch for checkin

I added the cast and an assertion to make sure that casting |offset| wasn't going to do the wrong thing.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 205762 [details] [diff] [review]
Patch for checkin

r=me, nominate for branches once this bakes a bit on the trunk.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 9

[Bob Clary [:bc] (inactive)]

Although there is a testcase outlined in this bug, it is one that doesn't complete in a time short enough to allow it to be placed in the normal JavaScript Test Library so I am marking this testcase-.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 205762 [details] [diff] [review]
Patch for checkin

Another security fix that has been thoroughly baked on the trunk.

/be

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 205762 [details] [diff] [review]
Patch for checkin

a=dveditz for drivers

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the branches.

Comment 13

[Bob Clary [:bc] (inactive)]

I've rethought the time limit and think we can put this in the automated tests after all.

no crash in 1.8.0.1, 1.8.1, 1.9a1 on winxp 20060112

no crash in 1.8.0.1 linux 20060112

crash in 1.8.1 linux 20060112 TB13900472

libc.so.6 + 0x61e79 (0x00c3de79)
FreeStringBuffer()  [/builds/tinderbox/Fx-Mozilla1.8/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/jsscan.c, line 823]

crash in 1.9a1 linux 20060112 TB13900588

libc.so.6 + 0x61e79 (0x00e29e79)
FreeStringBuffer()  [/builds/tinderbox/Fx-Trunk/Linux_2.4.21-27.0.4.ELsmp_Depend/mozilla/js/src/jsscan.c, line 823]

Comment 14

[Bob Clary [:bc] (inactive)]

Attachment: e4x/Regress/regress-319872.js

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix the bad call

In a debug build, you would have noticed an assertion in jsscan.c. The problem is fairly obvious: in the case that !STRING_BUFFER_OK(&sb), we were reporting oom and jumping to out:, where we would only test for str != null (with no assignments to str outside of null) and try to free the sentinal value. Rearranging the label won't work, so here's the patch to not free when we don't want to.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 208787 [details] [diff] [review]
Fix the bad call

r=me, thanks for fixing this.

/be

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on trunk, again.

Comment 18

[Daniel Veditz [:dveditz]]

sorry for the bugspam, saw the checkin and thought the missing fixed1.8.1 was as oversight, but it was removed with comment 13

Comment 19

[Bob Clary [:bc] (inactive)]

There are other issues with this that I need to sort out. I verified based on the results of the browser based tests which don't necessarily run long enough to show  crashes. I am rebuilding with the patch in attachment 208787 [details] [diff] [review] to get a better idea of what is happening.

Comment 20

[Bob Clary [:bc] (inactive)]

(In reply to comment #19)
with  attachment 208787 [details] [diff] [review] I crash in windows firefox 1.5 (1.8)

JSHashNumber
js_HashString(JSString *str)
{
    JSHashNumber h;
    const jschar *s;
    size_t n;

    h = 0;
    for (s = JSSTRING_CHARS(str), n = JSSTRING_LENGTH(str); n; s++, n--)
->        h = (h >> (JS_HASH_BITS - 4)) ^ (h << 4) ^ *s;
    return h;
}


	h	0x00000000
	n	0x2fa6001a
+	s	0x00000001 ""
-	str	0x0013dc48
	length	0x2fa6001a
+	chars	0x00000001 ""


js_HashString(JSString * 0x0013dc48) line 2811 + 19 bytes
js_AtomizeString(JSContext * 0x00032dd8, JSString * 0x0013dc48, unsigned int 0x00000080) line 650 + 9 bytes
js_AtomizeChars(JSContext * 0x00032dd8, const unsigned short * 0x00000001, unsigned int 0x2fa6001a, unsigned int 0x00000000) line 752 + 19 bytes
js_GetToken(JSContext * 0x00032dd8, JSTokenStream * 0x004a9af8) line 1271 + 42 bytes
js_MatchToken(JSContext * 0x00032dd8, JSTokenStream * 0x004a9af8, int 0x00000043) line 2130 + 13 bytes
XMLTagContent(JSContext * 0x00032dd8, JSTokenStream * 0x004a9af8, JSTreeContext * 0x0013e308, int 0x0000003d, JSAtom * * 0x0013e244) line 3407 + 15 bytes
XMLElementOrList(JSContext * 0x00032dd8, JSTokenStream * 0x004a9af8, JSTreeContext * 0x0013e308, int 0x00000000) line 3534 + 23 bytes
XMLElementContent(JSContext * 0x00032dd8, JSTokenStream * 0x004a9af8, JSParseNode * 0x004a9dd0, JSTreeContext * 0x0013e308) line 3486 + 19 bytes
XMLElementOrList(JSContext * 0x00032dd8, JSTokenStream * 0x004a9af8, JSTreeContext * 0x0013e308, int 0x00000000) line 3584 + 21 bytes
XMLElementOrListRoot(JSContext * 0x00032dd8, JSTokenStream * 0x004a9af8, JSTreeContext * 0x0013e308, int 0x00000000) line 3671 + 21 bytes
js_ParseXMLTokenStream(JSContext * 0x00032dd8, JSObject * 0x000387c8, JSTokenStream * 0x004a9af8, int 0x00000000) line 3717 + 24 bytes
ParseXMLSource(JSContext * 0x00032dd8, JSString * 0x008d7ff8) line 1943 + 25 bytes
ToXML(JSContext * 0x00032dd8, long 0x008d7ffc) line 2042 + 13 bytes
XML(JSContext * 0x00032dd8, JSObject * 0x008d86a8, unsigned int 0x00000001, long * 0x0041db8c, long * 0x0013e524) line 7051 + 13 bytes
js_Invoke(JSContext * 0x00032dd8, unsigned int 0x00000001, unsigned int 0x00000001) line 1177 + 23 bytes
js_Interpret(JSContext * 0x00032dd8, unsigned char * 0x00420771, long * 0x0013ee20) line 3095 + 15 bytes
js_Execute(JSContext * 0x00032dd8, JSObject * 0x000387c8, JSScript * 0x00420670, JSStackFrame * 0x00000000, unsigned int 0x00000000, long * 0x0013fecc) line 1423 + 19 bytes
JS_ExecuteScript(JSContext * 0x00032dd8, JSObject * 0x000387c8, JSScript * 0x00420670, long * 0x0013fecc) line 3999 + 25 bytes
Process(JSContext * 0x00032dd8, JSObject * 0x000387c8, char * 0x00032d2a) line 223 + 22 bytes
ProcessArgs(JSContext * 0x00032dd8, JSObject * 0x000387c8, char * * 0x00032cc4, int 0x00000003) line 470 + 17 bytes
main(int 0x00000003, char * * 0x00032cc4, char * * 0x00033268) line 2565 + 21 bytes
JS! mainCRTStartup + 227 bytes
KERNEL32! 7c816d4f()

I am ASSuming this is related to the overlong string crash in bug 342422 and the int overflow is indeed resolved with this patch. I'll try the patch in bug 324422 combined with this.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Bob, would you file a new bug on that crash? If it turns out to be a dupe, we'll mark it then.

Comment 22

[Bob Clary [:bc] (inactive)]

(In reply to comment #21)
Filed Bug 324533 

Comment 23

[georgi - hopefully not receiving bugspam]

regarding CVE in the summary:

i consider mitre/CVE corporate/government's puppies and have low opinion of
them.

mitre's  "Steven M. Christey" <[e-mail]>
co-authored the "responsible disclosure draft rfc" which was
obviously a corporate plot and was ditched by even the IETF - check the
ietf archives for this corporate fiasco.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 208787 [details] [diff] [review]
Fix the bad call

approving for 1.8 and 1.8.0 branch checkin.

/be

Comment 25

[Bob Clary [:bc] (inactive)]

I don't have a stack for this, but this now crashes in 1.8/1.8.0.1 linux only in browser tests on e4x/Regress/regress-319872.js

linux-1.8_2006020704
linux-1.8.0.1_2006020704

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Isn't that just bug 324533?

Comment 27

[Bob Clary [:bc] (inactive)]

(In reply to comment #26)
> Isn't that just bug 324533?
> 

yeah.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Second fix checked into the 1.8.0.2 branch.

Comment 29

[Dave Liebreich [:davel]]

Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates) since in-testsuite+ indicates a test case exists in the js test library.

Comment 30

[Brendan Eich [:brendan]]

(In reply to comment #29)
> Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)
> since in-testsuite+ indicates a test case exists in the js test library.

Are these status whiteboard marks documented somewhere?  Thanks,

/be

Comment 31

[Bob Clary [:bc] (inactive)]

v ff 1.8.0.1/1.8/1.9 20060302 win/linux/mac

Comment 32

[Bob Clary [:bc] (inactive)]

Although I wasn't able to reproduce it with my local builds, I saw a crash in the trunk shell on windows from 20060328. No crashes in the browser versions of the tests or on mac or linux.

Comment 33

[georgi - hopefully not receiving bugspam]

(In reply to comment #32)
> No crashes in the browser versions of
> the tests or on mac or linux.
> 

for me linux trunk also seems safe.

Comment 34

[Bob Clary [:bc] (inactive)]

Checking in regress-319872.js;
/cvsroot/mozilla/js/tests/e4x/Regress/regress-319872.js,v  <--  regress-319872.js
initial revision: 1.1