Skip to content

Commit

Permalink
[CVE-2017-0252] CopyNativeIntArrayElementsToVar overflow
Browse files Browse the repository at this point in the history 
Array length mutation introduced by copy-from-prototype
reentrancy can cause heap overflow in concat fast paths.
Fix by excluding copy-from-prototype cases from fast paths.
Adjust unit tests to avoid excessively long run time.
  • Loading branch information
@rajatd
Suwei Chen authored and rajatd committed May 10, 2017
1 parent 9587507 commit f95aa76
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 33 deletions.
24 changes: 15 additions & 9 deletions lib/Runtime/Library/JavascriptArray.cpp
View file
Expand Up @@ -3066,7 +3066,7 @@ namespace Js
}

if (pDestArray && JavascriptArray::IsDirectAccessArray(aItem) && JavascriptArray::IsDirectAccessArray(pDestArray)
&& BigIndex(idxDest + JavascriptArray::FromVar(aItem)->length).IsSmallIndex()) // Fast path
&& BigIndex(idxDest + JavascriptArray::FromVar(aItem)->length).IsSmallIndex() && !JavascriptArray::FromVar(aItem)->IsFillFromPrototypes()) // Fast path
{
if (JavascriptNativeIntArray::Is(aItem))
{
Expand Down Expand Up @@ -3200,10 +3200,11 @@ namespace Js
for (uint idxArg = 0; idxArg < args.Info.Count; idxArg++)
{
Var aItem = args[idxArg];
BOOL isConcatSpreadable = false;

if (scriptContext->GetConfig()->IsES6IsConcatSpreadableEnabled())
{
JS_REENTRANT(jsReentLock, BOOL isConcatSpreadable = JavascriptOperators::IsConcatSpreadable(aItem));
JS_REENTRANT(jsReentLock, isConcatSpreadable = JavascriptOperators::IsConcatSpreadable(aItem));
if (!JavascriptNativeIntArray::Is(pDestArray))
{
ConcatArgs<uint>(pDestArray, remoteTypeIds, args, scriptContext, idxArg, idxDest);
Expand All @@ -3222,7 +3223,7 @@ namespace Js
}
}

if (JavascriptNativeIntArray::Is(aItem)) // Fast path
if (JavascriptNativeIntArray::Is(aItem) && !JavascriptNativeIntArray::FromVar(aItem)->IsFillFromPrototypes()) // Fast path
{
JavascriptNativeIntArray* pItemArray = JavascriptNativeIntArray::FromVar(aItem);

Expand Down Expand Up @@ -3258,7 +3259,9 @@ namespace Js
else
{
JavascriptArray *pVarDestArray = JavascriptNativeIntArray::ConvertToVarArray(pDestArray);
JS_REENTRANT(jsReentLock, ConcatArgs<uint>(pVarDestArray, remoteTypeIds, args, scriptContext, idxArg, idxDest));
BigIndex length;
JS_REENTRANT(jsReentLock, length = OP_GetLength(aItem, scriptContext),
ConcatArgs<uint>(pVarDestArray, remoteTypeIds, args, scriptContext, idxArg, idxDest, isConcatSpreadable, length));
return pVarDestArray;
}
}
Expand All @@ -3276,10 +3279,11 @@ namespace Js
for (uint idxArg = 0; idxArg < args.Info.Count; idxArg++)
{
Var aItem = args[idxArg];
BOOL isConcatSpreadable = false;

if (scriptContext->GetConfig()->IsES6IsConcatSpreadableEnabled())
{
JS_REENTRANT(jsReentLock, BOOL isConcatSpreadable = JavascriptOperators::IsConcatSpreadable(aItem));
JS_REENTRANT(jsReentLock, isConcatSpreadable = JavascriptOperators::IsConcatSpreadable(aItem));
if (!JavascriptNativeFloatArray::Is(pDestArray))
{
ConcatArgs<uint>(pDestArray, remoteTypeIds, args, scriptContext, idxArg, idxDest);
Expand All @@ -3299,18 +3303,18 @@ namespace Js
}
}

bool converted;
bool converted = false;
if (JavascriptArray::IsAnyArray(aItem) || remoteTypeIds[idxArg] == TypeIds_Array)
{
if (JavascriptNativeIntArray::Is(aItem)) // Fast path
if (JavascriptNativeIntArray::Is(aItem) && !JavascriptArray::FromVar(aItem)->IsFillFromPrototypes()) // Fast path
{
JavascriptNativeIntArray *pIntArray = JavascriptNativeIntArray::FromVar(aItem);

JS_REENTRANT(jsReentLock, converted = CopyNativeIntArrayElementsToFloat(pDestArray, idxDest, pIntArray));

idxDest = idxDest + pIntArray->length;
}
else if (JavascriptNativeFloatArray::Is(aItem))
else if (JavascriptNativeFloatArray::Is(aItem) && !JavascriptArray::FromVar(aItem)->IsFillFromPrototypes())
{
JavascriptNativeFloatArray* pItemArray = JavascriptNativeFloatArray::FromVar(aItem);

Expand All @@ -3322,7 +3326,9 @@ namespace Js
{
JavascriptArray *pVarDestArray = JavascriptNativeFloatArray::ConvertToVarArray(pDestArray);

JS_REENTRANT(jsReentLock, ConcatArgs<uint>(pVarDestArray, remoteTypeIds, args, scriptContext, idxArg, idxDest));
BigIndex length;
JS_REENTRANT(jsReentLock, length = OP_GetLength(aItem, scriptContext),
ConcatArgs<uint>(pVarDestArray, remoteTypeIds, args, scriptContext, idxArg, idxDest, isConcatSpreadable, length));

return pVarDestArray;
}
Expand Down
40 changes: 20 additions & 20 deletions test/Array/concat2.baseline
View file
@@ -1,26 +1,26 @@
-------concat Small-------------
- concat 101, 102, 103, 104, 105
length: 2147483647
2147483641: 100
2147483642: 101
2147483643: 102
2147483644: 103
2147483645: 104
2147483646: 105
length: 505
499: 100
500: 101
501: 102
502: 103
503: 104
504: 105
- arr.concat(arr)
length: 4294967294
2147483641: 100
2147483642: 101
2147483643: 102
2147483644: 103
2147483645: 104
2147483646: 105
4294967288: 100
4294967289: 101
4294967290: 102
4294967291: 103
4294967292: 104
4294967293: 105
length: 1010
499: 100
500: 101
501: 102
502: 103
503: 104
504: 105
1004: 100
1005: 101
1006: 102
1007: 103
1008: 104
1009: 105
-------test prototype lookup-------------
a: 200,101,202,203,204,105,106,207
r: 200,101,202,203,204,105,106,207,300,301,302,303,304
Expand Down
2 changes: 1 addition & 1 deletion test/Array/concat2.js
View file
Expand Up @@ -38,7 +38,7 @@ function test_concat(size) {
}

echo("-------concat Small-------------");
test_concat(2147483642);
test_concat(500);

// Fix for MSRC 33319 changes concat to skip a fast path if the index we're copying into is a BigIndex.
// Disable the large portion of this test since this change makes such a test run for hours.
Expand Down
6 changes: 3 additions & 3 deletions test/es6/es6toLength.js
View file
Expand Up @@ -12,12 +12,12 @@ var tests = [
{
var c = [];
c[0] = 1;
c[4294967293] = 2;
c[100] = 2;
var oNeg = { length : -1, 0 : 3, 1: 4, [Symbol.isConcatSpreadable] : true};
c = c.concat(oNeg);
assert.areEqual(1, c[0], "confirm indices of array concated to did not change")
assert.areEqual(2, c[4294967293], "confirm indices of array concated to did not change");
assert.areEqual(undefined, c[4294967294], "Length of oNeg is coerced to 0 nothing is concated here");
assert.areEqual(2, c[100], "confirm indices of array concated to did not change");
assert.areEqual(undefined, c[101], "Length of oNeg is coerced to 0 nothing is concated here");
}
},
{
Expand Down

0 comments on commit f95aa76

Please sign in to comment.