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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: bc, Assigned: Brian Crowder)

Tracking

({testcase, valgrind, verified1.8.1.18})

1.8 Branch
x86
Linux
testcase, valgrind, verified1.8.1.18
Points:
---
Bug Flags:
blocking1.8.1.18 +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
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)
Flags: in-testsuite+
Flags: in-litmus-
Flags: blocking1.8.1.18?
Is bug 374025 the trunk fix or is that unrelated?
Flags: blocking1.8.1.18? → blocking1.8.1.18+
Whiteboard: [sg:critical]
mm, no, that's closed WFM -- something else fixed it.
Flags: wanted1.8.1.x+
Adding sayrer for assignment help.
Assignee: general → crowder
(Assignee)

Comment 4

9 years ago
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.
(Assignee)

Comment 5

9 years ago
Fixed on trunk by bug 410192 (thanks, hg bisect), more shortly.
(Assignee)

Comment 6

9 years ago
Created attachment 341958 [details] [diff] [review]
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.
Attachment #341958 - Flags: review?(mrbkap)
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.
Attachment #341958 - Flags: review?(mrbkap) → review-
(Assignee)

Comment 8

9 years ago
Created attachment 342113 [details] [diff] [review]
v2

Thanks!
Attachment #341958 - Attachment is obsolete: true
Attachment #342113 - Flags: review?(mrbkap)
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.
Attachment #342113 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

9 years ago
Attachment #342113 - Flags: approval1.8.1.18?
Comment on attachment 342113 [details] [diff] [review]
v2

Approved for 1.8.1.18, a=dveditz for release-drivers
Attachment #342113 - Flags: approval1.8.1.18? → approval1.8.1.18+
qawanted: We of course have the testcase that this is fixing, but I'm concerned that we test heavily the things this might break.
Keywords: qawanted
(Assignee)

Comment 12

9 years ago
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
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: fixed1.8.1.18
Resolution: --- → FIXED
(Reporter)

Comment 13

9 years ago
v 1.8.1.18
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.18 → verified1.8.1.18
(Reporter)

Comment 14

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

Comment 15

9 years ago
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.
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

9 years ago
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 )
Attachment #342113 - Flags: approval1.8.0.15+

Updated

9 years ago
Flags: blocking1.8.0.15+

Comment 18

9 years ago
Created attachment 347308 [details] [diff] [review]
clean 1.8.0 patch
Group: core-security
Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.