Last Comment Bug 657260 - "ASSERTION: nsTDependentString must wrap only null-terminated strings" with btoa()
: "ASSERTION: nsTDependentString must wrap only null-terminated strings" with b...
Status: RESOLVED FIXED
[sg:nse][qa+]
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Bobby Holley (PTO through June 13)
:
Mentors:
Depends on:
Blocks: 326633
  Show dependency treegraph
 
Reported: 2011-05-15 16:40 PDT by Jesse Ruderman
Modified: 2012-04-23 14:53 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
-
wontfix
+
wontfix
+
fixed
fixed
fixed
unaffected
wontfix


Attachments
testcase (hangs for a few seconds) (only for 32-bit Firefox) (95 bytes, text/html)
2011-05-15 16:40 PDT, Jesse Ruderman
no flags Details
stack trace (2.05 KB, text/plain)
2011-05-15 16:41 PDT, Jesse Ruderman
no flags Details
part 1 - Move CheckStringLength to JSString. v1 (7.30 KB, patch)
2011-09-29 10:02 PDT, Bobby Holley (PTO through June 13)
jwalden+bmo: review+
Details | Diff | Review
part 2 - Check JS string length against maximum in more places. v1 (2.70 KB, patch)
2011-09-29 10:02 PDT, Bobby Holley (PTO through June 13)
jwalden+bmo: review+
Details | Diff | Review
part 3 - Handle NULL return from JS_NewExternalString in XPCStringConvert::ReadableToJSVal. v1 (676 bytes, patch)
2011-09-29 10:40 PDT, Bobby Holley (PTO through June 13)
mrbkap: review+
Details | Diff | Review
part 1 - Move CheckStringLength to JSString. v2 r=Waldo (7.28 KB, patch)
2011-10-03 15:27 PDT, Bobby Holley (PTO through June 13)
bobbyholley: review+
Details | Diff | Review
part 2 - Check JS string length against maximum in more places. v2 r=Waldo (2.79 KB, patch)
2011-10-03 15:28 PDT, Bobby Holley (PTO through June 13)
bobbyholley: review+
Details | Diff | Review
part 1 - Move CheckStringLength to JSString. v3 r=Waldo (7.37 KB, patch)
2011-10-07 12:18 PDT, Bobby Holley (PTO through June 13)
bobbyholley: review+
Details | Diff | Review

Description Jesse Ruderman 2011-05-15 16:40:53 PDT
Created attachment 532541 [details]
testcase (hangs for a few seconds) (only for 32-bit Firefox)
Comment 1 Jesse Ruderman 2011-05-15 16:41:16 PDT
Created attachment 532542 [details]
stack trace
Comment 2 Daniel Veditz [:dveditz] 2011-06-08 17:32:48 PDT
worst case maybe sg:high if this leads to data from memory ending up in the output string where an attacker can read it. Or maybe it's not a vuln at all if we safely handle it based on length despite the assertion.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2011-09-23 16:46:24 PDT
Bobby, can you have a look here, this should be fairly straight forward.
Comment 4 Bobby Holley (PTO through June 13) 2011-09-23 17:21:03 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #3)
> Bobby, can you have a look here, this should be fairly straight forward.

Sure.
Comment 5 Bobby Holley (PTO through June 13) 2011-09-27 12:10:23 PDT
I built a 32-bit version of a nightly, and I don't get the assertion. I just get the following:

firefox(49581,0xac0e42c0) malloc: *** mmap(size=694489088) failed (error code=12)
*** error: can't allocate region
*** set a breakpoint in malloc_error_break to debug

This seems reasonable, since we're trying to allocate a ~800MB string on the sixty-something iteration where it fails. The main question is whether we're handling this failure correctly. The assertion would say no, but the failure to reproduce the assertion might mean we've since fixed it.

Jesse, can you still reproduce?
Comment 6 Jesse Ruderman 2011-09-27 15:37:30 PDT
I can still reproduce.
Comment 7 Bobby Holley (PTO through June 13) 2011-09-28 12:08:13 PDT
I've managed to reproduce this. I instrumented the test case a little bit, and here's what we see:

...
Iteration 58 - string length is 146492984
Iteration 59 - string length is 195323980
Iteration 60 - string length is 260431976
Iteration 61 - string length is 78807180
###!!! ASSERTION: nsTDependentString must wrap only null-terminated strings: 'mData[mLength] == 0', file ../../dist/include/nsTDependentString.h, line 67
Iteration 62 - string length is 105076240
Iteration 63 - string length is 140101656
Iteration 64 - string length is 186802208
Iteration 65 - string length is 249069612
Iteration 66 - string length is 63657360
###!!! ASSERTION: nsTDependentString must wrap only null-terminated strings: 'mData[mLength] == 0', file ../../dist/include/nsTDependentString.h, line 67
Iteration 67 - string length is 84876480
Iteration 68 - string length is 113168640
Iteration 69 - string length is 150891520

So something is going wrong here on iteration 61 and 66, because the string should never decrease in size. I'll dig in here and see what's going on.
Comment 8 Bobby Holley (PTO through June 13) 2011-09-28 15:27:36 PDT
So the problem here is that JSString stores flags in the same word it uses to store the length, and thus have a maximum length limited to 4 bits fewer than the number of bits in size_t. This is defined as JSString::MAX_LENGTH = JS_BIT(32 - LENGTH_SHIFT) - 1.

Unfortunately, the length of the string passed to JS_NewExternalString is never checked against MAX_LENGTH. And while MAX_LENGTH is defined in terms of 32 bits, the practical limitation is sizeof(size_t), which is why this bug only shows up on 32-bit systems.

So in buildLengthAndFlags, we end up left-shifting the input length by 4. So if the 4 highest bits are non-zero, the length we end up recording is missing its most significant bit(s).

The underlying buffer is still intact and null-terminated, but doing mData[mLength] won't give us a NUL byte, because mLength of garbage.

But...it's bounded garbage - in particular, it can only be too small, never too large. So I'm not convinced there's a security threat. But I haven't considered all of the possible implications here.

I'll get a patch up tomorrow and flag Waldo for review.
Comment 9 Bobby Holley (PTO through June 13) 2011-09-29 10:02:07 PDT
Created attachment 563446 [details] [diff] [review]
part 1 - Move CheckStringLength to JSString. v1
Comment 10 Bobby Holley (PTO through June 13) 2011-09-29 10:02:25 PDT
Created attachment 563447 [details] [diff] [review]
part 2 - Check JS string length against maximum in more places. v1
Comment 11 Bobby Holley (PTO through June 13) 2011-09-29 10:40:30 PDT
Created attachment 563456 [details] [diff] [review]
part 3 - Handle NULL return from JS_NewExternalString in XPCStringConvert::ReadableToJSVal. v1
Comment 12 Bobby Holley (PTO through June 13) 2011-09-29 10:42:14 PDT
The attached fixes to the js engine and to xpconnect fix this assertion.

With the above patches, we'll return NULL here:
http://hg.mozilla.org/mozilla-central/file/1aa8a5876058/js/src/xpconnect/src/xpcstring.cpp#l109

Which in turn causes us to return JSVAL_NULL from nsXPCWrappedJSClass::CheckForException, which makes XPCConvert::NativeData2JS return false, which makes XPCWrappedNative::GatherAndConvertResults throw and XPC exception.
Comment 13 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-30 15:27:07 PDT
Comment on attachment 563446 [details] [diff] [review]
part 1 - Move CheckStringLength to JSString. v1

Review of attachment 563446 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/String-inl.h
@@ +47,4 @@
>  #include "jsgcinlines.h"
>  
> +JS_ALWAYS_INLINE bool
> +JSString::CheckStringLength(JSContext *cx, size_t length)

JS engine uses camelCaps, not InterCaps, so this should be JSString::checkStringLength if you were to use this name.  But then you repeat yourself with "string" in both parts.  So let's make this |JSString::checkLength|.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-30 15:36:19 PDT
Comment on attachment 563447 [details] [diff] [review]
part 2 - Check JS string length against maximum in more places. v1

Review of attachment 563447 [details] [diff] [review]:
-----------------------------------------------------------------

Hm, so I guess JSString::checkStringLength is redundant only outside JSString methods.  And inside them, it's not clear whether "checkLength" means to check the string's length, or to check the passed-in length, if you're not calling the method with clearly-static syntax.  (That is, |JSString::checkLength(cx, len)| versus |checkLength(cx, len)|.)  Better thought than JSString::checkLength: how about |JSString::isValidLength(cx, len)|?

Which is all just a longer way of saying you should ignore the naming suggestion for part 1, and use the one here instead.  :-)

::: js/src/vm/String-inl.h
@@ +141,5 @@
>      JS_ASSERT(length <= MAX_LENGTH);
>      JS_ASSERT(chars[length] == jschar(0));
>  
> +    if (!CheckStringLength(cx, length))
> +        return NULL;

Hm, so this contradicts the |JS_ASSERT(length <= MAX_LENGTH)| just above it.  Either that needs to go, or this needs to stay.  Given how rare too-big-length is, and given that auditing callers and making sure they *stay* audited is hard, let's remove the assertion and keep the length-check.
Comment 15 Bobby Holley (PTO through June 13) 2011-10-03 15:10:17 PDT
(In reply to Jeff Walden (remove +bmo to email) from comment #14)
> Comment on attachment 563447 [details] [diff] [review] [diff] [details] [review]
> part 2 - Check JS string length against maximum in more places. v1
> 
> Review of attachment 563447 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Hm, so I guess JSString::checkStringLength is redundant only outside
> JSString methods.  And inside them, it's not clear whether "checkLength"
> means to check the string's length, or to check the passed-in length, if
> you're not calling the method with clearly-static syntax.  (That is,
> |JSString::checkLength(cx, len)| versus |checkLength(cx, len)|.)  Better
> thought than JSString::checkLength: how about |JSString::isValidLength(cx,
> len)|?

Hm, I don't like isValidLength(..) so much, because the function has some pretty strong side effects (we report an allocation overflow if the check fails). I'm going to go with validateLength(..) unless you object.
Comment 16 Bobby Holley (PTO through June 13) 2011-10-03 15:27:24 PDT
Created attachment 564353 [details] [diff] [review]
part 1 - Move CheckStringLength to JSString. v2 r=Waldo

Added an updated patch with validateLength(). Carrying over review.
Comment 17 Bobby Holley (PTO through June 13) 2011-10-03 15:28:02 PDT
Created attachment 564354 [details] [diff] [review]
part 2 - Check JS string length against maximum in more places. v2 r=Waldo

Updated part 2. Carrying over review.
Comment 18 Bobby Holley (PTO through June 13) 2011-10-07 11:12:34 PDT
pushed this to try: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=2766ae8057ba

If it goes green, we can land.
Comment 19 Bobby Holley (PTO through June 13) 2011-10-07 12:18:40 PDT
Created attachment 565605 [details] [diff] [review]
part 1 - Move CheckStringLength to JSString. v3 r=Waldo

Gah, this fails to build in optimized configurations due to some inline/linkage weirdness. Attached a fix, which Waldo r+ed on IRC.

Canceled the old try push, and pushed this one to try again: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=a13151c0f749
Comment 20 Bobby Holley (PTO through June 13) 2011-10-07 16:38:00 PDT
This looks sufficiently green. Pushed to mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/fa6d799dfba7
(and ancestors)
Comment 22 Daniel Veditz [:dveditz] 2011-10-13 13:40:23 PDT
We should get these fixes into Fx9-aurora
Comment 23 Alex Keybl [:akeybl] 2011-12-05 13:51:28 PST
[Triage Comment]
Is this still desired for FF9? If so, please nominate a patch for approval and provide reasoning as to why this should still be considered at this point in the cycle.
Comment 24 Bobby Holley (PTO through June 13) 2011-12-05 21:13:45 PST
My assessment is that this isn't an obvious security risk, as mentioned in comment 8. dveditz is the one who nominated, so he'll have a better idea.

In general, was I at fault here for letting this languish? I'm not clear on the my responsibilities (if any) when someone flags one of my bugs as tracking-firefoxX.
Comment 25 Alex Keybl [:akeybl] 2011-12-06 17:33:47 PST
(In reply to Bobby Holley (:bholley) from comment #24)
> My assessment is that this isn't an obvious security risk, as mentioned in
> comment 8. dveditz is the one who nominated, so he'll have a better idea.
> 
> In general, was I at fault here for letting this languish? I'm not clear on
> the my responsibilities (if any) when someone flags one of my bugs as
> tracking-firefoxX.

When a bug has a fix and is being tracked for an affected branch, my experience is that the committer prepares a patch.

You're right that whether this is still worth the risk at this point is more a  question for Dan since he marked it for tracking originally.
Comment 26 Daniel Veditz [:dveditz] 2012-01-12 16:25:11 PST
On the 1.9.2 branch a debug build eventually dies

0   JS_Assert + 67 (jsutil.cpp:69)
1   JSString::initFlat(unsigned short*, unsigned long) + 54 (jsstr.h:237)
2   JS_NewExternalString + 187 (jsapi.cpp:2630)
3   XPCStringConvert::ReadableToJSVal(JSContext*, nsAString_internal const&) + 284 (xpcstring.cpp:108)
4   XPCConvert::NativeData2JS(XPCLazyCallContext&, long*, void const*, nsXPTType const&, nsID const*, JSObject*, unsigned int*) + 1620 (xpcconvert.cpp:333)
5   XPCConvert::NativeData2JS(XPCCallContext&, long*, void const*, nsXPTType const&, nsID const*, JSObject*, unsigned int*) + 87 (xpcprivate.h:2985)
6   XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) + 7528 (xpcwrappednative.cpp:2808)
7   XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, long*, long*) + 498
8   js_Invoke + 2441 (jsinterp.cpp:1360)
9   js_Interpret + 98209 (jsops.cpp:2240)
10  js_Execute + 781 (jsinterp.cpp:1601)
 ... more ...

Code looks like
   void initFlat(jschar *chars, size_t length) {
        JS_ASSERT(length <= MAX_LENGTH);
        mLength = length;
        mChars = chars;
   }

That sounds like it corresponds to Bobby's analysis in comment 8 and we shouldn't need to fix this on the 1.9.2 branch.
Comment 27 Jason Smith [:jsmith] 2012-03-13 18:02:47 PDT
Tried reproducing this bug on Firefox 8, but did not have any luck seeing the assertion appear.
Comment 28 Jason Smith [:jsmith] 2012-03-13 18:03:11 PDT
Firefox 8 Debug Build*
Comment 29 Jason Smith [:jsmith] 2012-03-13 18:09:25 PDT
Also cannot reproduce on a Firefox 9 Debug Build.
Comment 30 Bobby Holley (PTO through June 13) 2012-03-13 18:22:27 PDT
IIRC it was only reproducible on linux.
Comment 31 Virgil Dicu [:virgil] [QA] 2012-04-19 07:25:38 PDT
Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20120417 Firefox/12.0 (debug build)

Could reproduce the hang (more than 15 seconds) with a Firefox 8 debug build, but did not receive any assertion.

With a F12 beta debug build the hang is a lot shorter (3-4 seconds). And no assertions displayed.

Would this be enough to call it verified here?
Comment 32 Tracy Walker [:tracy] 2012-04-23 14:53:20 PDT
The test case hangs Fx12 on my Mac for nearly 10 seconds

Note You need to log in before you can comment on or make changes to this bug.