454113 - e4x/extensions/regress-374025.js - invalid write

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

This is fixed on the trunk and is a known crasher on 1.8.1, but I just realized it is a security issue.

==30135== Invalid write of size 2
==30135==    at 0x40D6CA4: js_RepeatChar (jsscan.c:845)
==30135==    by 0x40F5597: XMLToXMLString (jsxml.c:2709)
==30135==    by 0x40F6246: XMLToXMLString (jsxml.c:3004)
==30135==    by 0x40F6624: ToXMLString (jsxml.c:3082)
==30135==    by 0x4102289: xml_toXMLString (jsxml.c:7168)
==30135==    by 0x406B298: js_Invoke (jsinterp.c:1387)
==30135==    by 0x406B6CB: js_InternalInvoke (jsinterp.c:1481)
==30135==    by 0x40A78ED: js_TryMethod (jsobj.c:4686)
==30135==    by 0x40E8B98: js_ValueToSource (jsstr.c:2718)
==30135==    by 0x40E1CA2: str_uneval (jsstr.c:470)
==30135==    by 0x406B298: js_Invoke (jsinterp.c:1387)
==30135==    by 0x407E78D: js_Interpret (jsinterp.c:3964)

Comment 1

[Daniel Veditz [:dveditz]]

Is bug 374025 the trunk fix or is that unrelated?

Comment 2

[Daniel Veditz [:dveditz]]

mm, no, that's closed WFM -- something else fixed it.

Comment 3

[Mike Shaver (:shaver emeritus)]

Adding sayrer for assignment help.

Comment 4

[Brian Crowder]

Igor:  Any idea what trunk work might've fixed this?  I'll be happy to port.  If not, don't spend any more time on it, I'll track it down.

Comment 5

[Brian Crowder]

Fixed on trunk by bug 410192 (thanks, hg bisect), more shortly.

Comment 6

[Brian Crowder]

Attachment: the backported patch

Took the interesting code from the fix in bug 410192 and ported to 1.8, asking mrbkap for review since he wrote the original code.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 341958 [details] [diff] [review]
the backported patch

- In AppendAttributeValue(JSContext *cx, JSStringBuffer *sb, JSString *valstr)
 {
-    js_AppendCString(sb, "=\"");
-    valstr = js_EscapeAttributeValue(cx, valstr);
+    js_AppendChar(sb, '=');
+    valstr = js_EscapeAttributeValue(cx, valstr, JS_TRUE);
     if (!valstr) {
         free(sb->base);
         sb->base = STRING_BUFFER_ERROR_BASE;
         return;
     }
     js_AppendJSString(sb, valstr);
     js_AppendChar(sb, '"');

The last js_AppendChar dies in brendan's patch but lives on here. I see you removing the first " (for the one in js_EscapeAttributeValue) so it seems like this one should go too.

r- until that's addressed.

Comment 8

[Brian Crowder]

Attachment: v2

Thanks!

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 342113 [details] [diff] [review]
v2

Looks good. Also feel free to commit the .cvsignore change if it makes life easier for you when doing backports like this.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 342113 [details] [diff] [review]
v2

Approved for 1.8.1.18, a=dveditz for release-drivers

Comment 11

[Daniel Veditz [:dveditz]]

qawanted: We of course have the testcase that this is fixing, but I'm concerned that we test heavily the things this might break.

Comment 12

[Brian Crowder]

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.104; previous revision: 3.181.2.103
done
Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.72; previous revision: 3.50.2.71
done
Checking in jsxml.h;
/cvsroot/mozilla/js/src/jsxml.h,v  <--  jsxml.h
new revision: 3.10.4.5; previous revision: 3.10.4.4
done

Comment 13

[Bob Clary [:bc] (inactive)]

v 1.8.1.18

Comment 14

[Bob Clary [:bc] (inactive)]

crowder: this appears to have fixed e4x/extensions/regress-410192.js on 1.8.1 as well. Does that make sense to you?

Comment 15

[Brian Crowder]

It makes sense that my change -could- have fixed that, but I didn't expect it and I don't know for sure exactly why.  Let's call it serendipity, as I don't really have time to investigate it.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

I looked into it -- this is because toSource on an XML attribute is expected to produce quotes around the value and toString isn't. This change enforces that (if you follow the TO_SOURCE_FLAG propagation through XMLToXMLString). So in summary: it makes sense for this to have fixed that test.

Comment 17

[Alexander Sack]

Comment on attachment 342113 [details] [diff] [review]
v2

a=asac for 1.8.0 (needs context adjustments to apply cleanly AND if possible without .cvsignore cruft :-P )

Comment 18

[Alexander Sack]

Attachment: clean 1.8.0 patch

Comment 19

[Joshua Mitchell (Inactive)]

Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task