Unrooted strings in jsstr.c

VERIFIED FIXED in mozilla1.8rc1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
12 years ago
6 years ago

People

(Reporter: Igor Bukanov, Assigned: brendan)

Tracking

({js1.6, verified1.7.13, verified1.8})

Trunk
mozilla1.8rc1
js1.6, verified1.7.13, verified1.8
Points:
---
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
blocking1.8rc1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(4 attachments)

(Reporter)

Description

12 years ago
The following functions in jsstr.c do not root the results of js_ValueToString
before calling GC-triggering operations:

str_substring
str_decodeURI
str_decodeURI_Component
str_encodeURI
str_encodeURI_Component

The encode/decode cases is only exploitable if one can create a situation when
GC is triggered in js_NewString that is called later in Decode/Encode after the
return from js_ValueToString. 

The case of str_substring is trivially exploitable to read arbitrary region in
address space via JSString->double subversion like the following example
demonstrates:

obj = {
	toString: function() {
		return "*TEST*".substr(1, 4);
	}
};

var TMP = 1e200;

likeZero = {
	valueOf: function() {
		if (typeof gc == "function") gc();
		for (var i = 0; i != 40000; ++i) {
			var tmp = 2 / TMP;
			tmp = null;
		}
		return 0;	
	}
}

var expected = "TEST";
var actual = String.prototype.substr.call(obj, likeZero);

print("Substring length: "+actual.length);
print(expected === actual);

On my box executing it gives in js shell:
 
before 3690, after 3654, break 08a40000
Substring length: 357496748
Segmentation fault

where 357496748 comes from 1e-200 interpreted as JSString.
Flags: blocking1.8rc1?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Whiteboard: [sg:critical?]

Comment 1

12 years ago
->brendan. 
Assignee: general → brendan
Flags: blocking1.8rc1? → blocking1.8rc1+
(Assignee)

Comment 2

12 years ago
Asa: I'm relying on mrbkap (thanks and praise to him) for fixing.  This may not
be fair (hey, I didn't write str_decodeURI, str_decodeURI_Component,
str_encodeURI, or str_encodeURI_Component -- mccabe or rogerl did), but that's
life.  I am to blame for the lack of automatic local roots.

FYI for future reassigning.

/be
Assignee: brendan → mrbkap
Keywords: js1.6
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.8rc1
This goes back to brendan. Thanks!
Assignee: mrbkap → brendan
(Reporter)

Comment 4

12 years ago
str_resolve and str_enumerate in jsstr.c also do not root results of
js_ValueToString which bring them to the same company where str_decodeURL etc.
are. That is, they are exploitable only if js_NewDependentString that follows
js_ValueToString would cause GC.
(Assignee)

Comment 5

12 years ago
(In reply to comment #4)
> str_resolve and str_enumerate in jsstr.c also do not root results of
> js_ValueToString which bring them to the same company where str_decodeURL etc.
> are. That is, they are exploitable only if js_NewDependentString that follows
> js_ValueToString would cause GC.

These are ok.

First, it would be the OBJ_DEFINE_PROPERTY, or some later operation that might GC, that would cause the js_ValueToString(cx, OBJECT_TO_JSVAL(obj)) result to be collected, if it had no other strong refs.  This is because that result is protected by the newborn string across the js_NewDependentString call, even though that call may overwrite that newborn (if both strings are the same GCX_* type).

Second, in these cases (str_resolve and str_enumerate), js_NewDependentString creates a rooted new string that strongly refs its base string, the result of the js_ValueToString.  So the chain of refs from newborn for dependent, through dependent base, keeps the first string alive.  Once the property value being defined is committed, the dependent is protected by more than *its* newborn.

/be
Status: NEW → ASSIGNED
(Assignee)

Comment 6

12 years ago
str_substring uses the argv[-1] explicit local root to protect the result of its js_ValueToString(cx, OBJECT_TO_JSVAL(obj)), so it is ok.

There are a number of other bad native methods than the Edition 3 encode/decode URi ones, however.  Patch anon.

/be
(Assignee)

Comment 7

12 years ago
Created attachment 200481 [details] [diff] [review]
simple use of explicit local roots

per http://www.mozilla.org/js/spidermonkey/gctips.html -- sigh.  Fragile model that is easy to forget to use.  We will put it to rest on the trunk shortly.  This is good for 1.8.  mrbkap feel free to review as well.

/be
Attachment #200481 - Flags: superreview?(shaver)
Attachment #200481 - Flags: review?(igor.bukanov)

Comment 8

12 years ago
Created attachment 200482 [details]
ecma_3/String/regress-313276.js

Updated

12 years ago
Flags: testcase+
(Reporter)

Comment 9

12 years ago
(In reply to comment #5)
> These are ok.

Counter example:

var prepared_string = String(1);
String(2); // to remove prepared_string from newborn

var strObj = new String("");
strObj.toString  = function() {
	var tmp = prepared_string;
	prepared_string = null;
	return tmp;
}

var not_one_count = 0;
for each (var s in strObj) { 
	if (s !== '1') 
	        ++not_one_count;
}

print("not_one_count="+not_one_count);

If I recompile js shell with TOO_MUCH_GC defined, its execution instead of printing expected 1 coming from "toString" property, produces on my box:

Assertion failure: flags != GCF_FINAL, at jsgc.c:1040
Trace/breakpoint trap
(Reporter)

Comment 10

12 years ago
Comment on attachment 200481 [details] [diff] [review]
simple use of explicit local roots

The patch does not solve all the problems per comment 9 :(
Attachment #200481 - Flags: review?(igor.bukanov) → review-
(Assignee)

Comment 11

12 years ago
Comment on attachment 200481 [details] [diff] [review]
simple use of explicit local roots

This patch is correct, but insufficient.  The str_enumerate and str_resolve problems Igor points out (old-borns cut off from the live thing graph) remain.  New patch to fix those in addition, I hope.

/be
Attachment #200481 - Flags: superreview?(shaver)
Attachment #200481 - Flags: review-
(Assignee)

Comment 12

12 years ago
Created attachment 200524 [details] [diff] [review]
first patch plus newborn root hacking in str_enumerate and str_resolve

The alternative is the cx->lastInvokeResult root Igor proposed, which we were trying not to take for 1.8/fx1.5 for fear of entrainment problems, and assuming we can spot-fix problems more safely.  Not clear that is a good assumption, but here are two more spot (hack) fixes, plus the saner explicit local root fixes from the first patch.

/be
Attachment #200524 - Flags: superreview?(shaver)
Attachment #200524 - Flags: review?(igor.bukanov)
(Reporter)

Comment 13

12 years ago
With the patch applied only the following functions in jsstr.c contain calls to ValueToString without immediate rooting of the result:

str_getProperty -- this is safe
str_localeCompare -- safe if cx->localeCallbacks->localeCompare safe
match_or_replace -- need some time to check
find_replen      -- need some time to check

But perhaps it is better to add explicit rooting in all the places just to make sure?
(Reporter)

Comment 14

12 years ago
Unrelated to the bug curiosity question: why str_getProperty, str_enumerate and str_resolve use ValueToString and not simply OBJ_GET_SLOT(cx, obj, JSSLOT_PRIVATE) like str_valueOf and str_toString do?
(Assignee)

Comment 15

12 years ago
I fixed str_localeCompare to root thatStr using argv[0] -- thanks.

(In reply to comment #14)
> Unrelated to the bug curiosity question: why str_getProperty, str_enumerate and
> str_resolve use ValueToString and not simply OBJ_GET_SLOT(cx, obj,
> JSSLOT_PRIVATE) like str_valueOf and str_toString do?

Good question.  Only the getter (and a setter, if there were one) needs to be generic:

s = new String('hello');
o = {__proto__: s};
print(o[0]);

You can inspect the engine for resolve/newresolve and enumerate calls and show that their obj param is guaranteed to be of the class whose hook is being called -- not so with getters and setters.

Thanks, an even better patch coming up.

/be
(Assignee)

Comment 16

12 years ago
Created attachment 200552 [details] [diff] [review]
best patch yet, hope this is it for jsstr.c
Attachment #200552 - Flags: superreview?(shaver)
Attachment #200552 - Flags: review?(igor.bukanov)
(Reporter)

Comment 17

12 years ago
Comment on attachment 200524 [details] [diff] [review]
first patch plus newborn root hacking in str_enumerate and str_resolve

If remaining cases turned out to be problematic, I will report them as a separated bug.
Attachment #200524 - Flags: review?(igor.bukanov) → review+
(Assignee)

Comment 18

12 years ago
Comment on attachment 200524 [details] [diff] [review]
first patch plus newborn root hacking in str_enumerate and str_resolve

Igor, if you are convinced the next patch is better, please r+ it and I'll check it into the trunk.  Thanks,

/be
Attachment #200524 - Flags: superreview?(shaver)
(Reporter)

Comment 19

12 years ago
Comment on attachment 200552 [details] [diff] [review]
best patch yet, hope this is it for jsstr.c

Yes, this is better :)

Although IMO even for getProperty for consistency it would be better to get length from the string from the private slot of the right string, this is unrelated 100% to this bug.
Attachment #200552 - Flags: review?(igor.bukanov) → review+
Comment on attachment 200552 [details] [diff] [review]
best patch yet, hope this is it for jsstr.c

sr=shaver
Attachment #200552 - Flags: superreview?(shaver) → superreview+
(Assignee)

Comment 21

12 years ago
(In reply to comment #19)
> (From update of attachment 200552 [details] [diff] [review] [edit])
> Yes, this is better :)
> 
> Although IMO even for getProperty for consistency it would be better to get
> length from the string from the private slot of the right string, this is
> unrelated 100% to this bug.

ECMA-262 makes most string and array methods generic, so any object can use 'em (via Function.prototype.call/.apply, or in SpiderMonkey now via generic static methods of the same name, e.g. String.slice).  So str_getProperty does likewise and converts from object to string.

Fixed on trunk.

/be
(Assignee)

Comment 22

12 years ago
Comment on attachment 200552 [details] [diff] [review]
best patch yet, hope this is it for jsstr.c

Time is running out for tonight.

/be
Attachment #200552 - Flags: approval1.8rc1?
Comment on attachment 200552 [details] [diff] [review]
best patch yet, hope this is it for jsstr.c

a=dveditz
Attachment #200552 - Flags: approval1.8rc1? → approval1.8rc1+
(Assignee)

Comment 24

12 years ago
Fixed on branch and trunk.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
(Assignee)

Comment 25

12 years ago
This fix caused bug 313565.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 26

12 years ago
Comment on attachment 200552 [details] [diff] [review]
best patch yet, hope this is it for jsstr.c

>@@ -543,19 +550,20 @@ str_enumerate(JSContext *cx, JSObject *o
...
>-    str = js_ValueToString(cx, OBJECT_TO_JSVAL(obj));
>+    str = (JSString *) JS_GetPrivate(cx, obj);
...
>@@ -570,19 +578,20 @@ str_resolve(JSContext *cx, JSObject *obj
>             JSObject **objp)
> {
...
>-    str = js_ValueToString(cx, OBJECT_TO_JSVAL(obj));
>+    str = (JSString *) JS_GetPrivate(cx, obj);

Note that this is a change in functinality if someone relied on the ability to redefine String object "string" value. But why would anybody want this?
(Assignee)

Comment 27

12 years ago
(In reply to comment #26)
> (From update of attachment 200552 [details] [diff] [review] [edit])
> >@@ -543,19 +550,20 @@ str_enumerate(JSContext *cx, JSObject *o
> ...
> >-    str = js_ValueToString(cx, OBJECT_TO_JSVAL(obj));
> >+    str = (JSString *) JS_GetPrivate(cx, obj);
> ...
> >@@ -570,19 +578,20 @@ str_resolve(JSContext *cx, JSObject *obj
> >             JSObject **objp)
> > {
> ...
> >-    str = js_ValueToString(cx, OBJECT_TO_JSVAL(obj));
> >+    str = (JSString *) JS_GetPrivate(cx, obj);

First, this was just totally broken: JS_GetPrivate returns null for any private data slot value that's not int-tagged.  OBJ_GET_SLOT(cx, obj, JSSLOT_PRIVATE) and then JSVAL_TO_STRING if not JSVAL_VOID would be required.

> Note that this is a change in functinality if someone relied on the ability to
> redefine String object "string" value. But why would anybody want this?

I don't think any Firefox code does, based on some debugging code I've added.  But who knows?  Compatibility is king.  I don't think we can change this.  At least, not right not for Mozilla1.8/Firefox1.5/JS1.6.

/be
Status: REOPENED → ASSIGNED
(Assignee)

Comment 28

12 years ago
Let's take this resolve/enumerate js_ValueToString issue up in the followup bug Igor filed, bug 313567.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Comment 29

12 years ago
on winxp

1.8b5_2005102412:  ecma_3/String/regress-313276.js failed, 
Expected exit code 0, got 3

STATUS: Root strings
before 5346, after 5283, break 00000000
STATUS: Substring length: 4
./ecma_3/shell.js:101: TypeError: msg.split is not a function 

1.9a1_2005102413: crashed.

Comment 30

12 years ago
(In reply to comment #29)
The failure in the testcase on branch is bug in the testcase line 69 where it passes a boolean to the printStatus function.

After repulling and rebuilding the shell, I can not reproduce the crash.

Sorry for the false alarm.
Fix checked into 1.7 branch.
Keywords: fixed-aviary1.0.8, fixed1.7.13

Comment 32

12 years ago
v ff on 1.7.13, 1.8.0.1, 1.8, 1.9a1 on windows/linux/mac 20060221 builds, moz on 1.7.13 on windows 20060221 builds.
Status: RESOLVED → VERIFIED
Keywords: fixed-aviary1.0.8, fixed1.7.13, fixed1.8 → verified-aviary1.0.8, verified1.7.13, verified1.8
Group: security

Comment 33

11 years ago
RCS file: /cvsroot/mozilla/js/tests/js1_5/GC/regress-313276.js,v
done
Checking in regress-313276.js;
/cvsroot/mozilla/js/tests/js1_5/GC/regress-313276.js,v  <--  regress-313276.js
initial revision: 1.1
done
You need to log in before you can comment on or make changes to this bug.