Bug (CVE-2016-4623) – JSObject::putByIndexBeyondVectorLengthWithoutAttributes needs to go to the sparse map based on MAX_STORAGE_VECTOR_INDEX

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 271241 [details]
the patch

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/196524

[Geoffrey Garen]

Comment on attachment 271241 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271241&action=review

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1208
> +                throwOutOfMemoryError(exec);
> +                return JSValue::encode(jsUndefined());

A few thoughts here:

(1) There are two other places in this function where we might exceed MAX_STORAGE_VECTOR_INDEX. I think we want consistent behavior in those places.

(2) It is unlikely this limit will harm web compatibility. I suspect that total process memory usage at the point of MAX_STORAGE_VECTOR_INDEX is greater than 1GB. Chrome unconditionally kills any process over ~1GB. So does iOS. 

(3) Perhaps a more obvious behavior for web developers would be to prohibit strings larger than MAX_STORAGE_VECTOR_INDEX? ECMAScript specifies that a string can be as large as 134 million GBs, but that is obviously absurd. 

(4) It's surprising that the array that results from a split can cause problems when the input string to split did not cause problems. I think the length of the array will at worst be equal to the length of the string. (Of course, the array can take up 8X more memory, if it uses one 64bit pointer per 8bit character. Still, surprising that 8X makes the difference between OK and not OK.)

[Filip Pizlo]

(In reply to comment #3)
> Comment on attachment 271241 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271241&action=review
> 
> > Source/JavaScriptCore/runtime/StringPrototype.cpp:1208
> > +                throwOutOfMemoryError(exec);
> > +                return JSValue::encode(jsUndefined());
> 
> A few thoughts here:
> 
> (1) There are two other places in this function where we might exceed
> MAX_STORAGE_VECTOR_INDEX. I think we want consistent behavior in those
> places.

I agree.  We should do something for those, too, if we think it was a good idea to do it here.

> 
> (2) It is unlikely this limit will harm web compatibility. I suspect that
> total process memory usage at the point of MAX_STORAGE_VECTOR_INDEX is
> greater than 1GB. Chrome unconditionally kills any process over ~1GB. So
> does iOS. 
> 
> (3) Perhaps a more obvious behavior for web developers would be to prohibit
> strings larger than MAX_STORAGE_VECTOR_INDEX? ECMAScript specifies that a
> string can be as large as 134 million GBs, but that is obviously absurd. 

That's a good idea.

> 
> (4) It's surprising that the array that results from a split can cause
> problems when the input string to split did not cause problems. I think the
> length of the array will at worst be equal to the length of the string. (Of
> course, the array can take up 8X more memory, if it uses one 64bit pointer
> per 8bit character. Still, surprising that 8X makes the difference between
> OK and not OK.)

I think that MAX_STORAGE_VECTOR_INDEX is too small right now, which exacerbates the problem.

I wouldn't be surprised if split() gave an OOME for a very large string because I would assume that the split() has to allocate the array and the substrings. Some developers will realize that the engine should optimize the substrings by either doing the classic substring optimization, but even that will allocate some kind of object. Some developers will realize that the engine should use single-character-string optimizations, but even then we're still allocating the array. So even if the array takes the same amount of memory as the string, it's not surprising that you had enough memory for the string but not enough for the array. I don't think anyone would expect the GC to kill the string the moment that the array was allocated, even if the string was logically dead after the split.