657260 - "ASSERTION: nsTDependentString must wrap only null-terminated strings" with btoa()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (hangs for a few seconds) (only for 32-bit Firefox)

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Daniel Veditz [:dveditz]]

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)]

Bobby, can you have a look here, this should be fairly straight forward.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #3)
> Bobby, can you have a look here, this should be fairly straight forward.

Sure.

Comment 5

[Bobby Holley (:bholley)]

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]

I can still reproduce.

Comment 7

[Bobby Holley (:bholley)]

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 (:bholley)]

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 (:bholley)]

Attachment: part 1 - Move CheckStringLength to JSString. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: part 2 - Check JS string length against maximum in more places. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: part 3 - Handle NULL return from JS_NewExternalString in XPCStringConvert::ReadableToJSVal. v1

Comment 12

[Bobby Holley (:bholley)]

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]]

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]]

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 (:bholley)]

(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 (:bholley)]

Attachment: part 1 - Move CheckStringLength to JSString. v2 r=Waldo

Added an updated patch with validateLength(). Carrying over review.

Comment 17

[Bobby Holley (:bholley)]

Attachment: part 2 - Check JS string length against maximum in more places. v2 r=Waldo

Updated part 2. Carrying over review.

Comment 18

[Bobby Holley (:bholley)]

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 (:bholley)]

Attachment: 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 (:bholley)]

This looks sufficiently green. Pushed to mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/fa6d799dfba7
(and ancestors)

Comment 21

[Bobby Holley (:bholley)]

Kyle merged this to m-c:

http://hg.mozilla.org/mozilla-central/rev/702c4c6fc60d
http://hg.mozilla.org/mozilla-central/rev/8f10a82cf2de
http://hg.mozilla.org/mozilla-central/rev/fa6d799dfba7

Resolving fixed.

Comment 22

[Daniel Veditz [:dveditz]]

We should get these fixes into Fx9-aurora

Comment 23

[Alex Keybl [:akeybl]]

[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 (:bholley)]

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]]

(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]]

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]]

Tried reproducing this bug on Firefox 8, but did not have any luck seeing the assertion appear.

Comment 28

[Jason Smith [:jsmith]]

Firefox 8 Debug Build*

Comment 29

[Jason Smith [:jsmith]]

Also cannot reproduce on a Firefox 9 Debug Build.

Comment 30

[Bobby Holley (:bholley)]

IIRC it was only reproducible on linux.

Comment 31

[Virgil Dicu [:virgil] [QA]]

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]]

The test case hangs Fx12 on my Mac for nearly 10 seconds