Skip to content

Commit 5d4aca7

Browse files
ricobbeMikeHolman
authored andcommitted
[CVE-2017-11894] An OOB Access in Chakra Engine - Individual
Avoid integer overflow in implementation of `String.prototype.replace`: - Failfast in Chakra regex parser if a regex contains more than 2^15 capturing groups, thus avoiding overflow when passing the matching string's to `replace`'s second argument. - Add comments to `RegexHelper::ReplaceFormatString` explaining why the capture index cannot overflow; add asserts to guard against it.
1 parent 66b9abb commit 5d4aca7

File tree

5 files changed

+40
-3
lines changed

5 files changed

+40
-3
lines changed

lib/Parser/RegexParser.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2961,6 +2961,8 @@ namespace UnifiedRegex
29612961
this->scriptContext->ProfileEnd(Js::RegexCompilePhase);
29622962
#endif
29632963

2964+
AssertOrFailFast(0 < pattern->NumGroups() && pattern->NumGroups() <= MAX_NUM_GROUPS);
2965+
29642966
return pattern;
29652967
}
29662968

@@ -2993,6 +2995,8 @@ namespace UnifiedRegex
29932995

29942996
program->numGroups = nextGroupId;
29952997

2998+
AssertOrFailFast(0 < program->numGroups && program->numGroups <= MAX_NUM_GROUPS);
2999+
29963000
// Remaining to set during compilation: litbuf, litbufLen, numLoops, insts, instsLen, entryPointLabel
29973001
}
29983002

lib/Parser/RegexParser.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,16 @@ namespace UnifiedRegex
7777
const EncodedChar* next;
7878
bool inBody;
7979

80+
// Maximum number of capturing groups allowed, including the entire regexp, which is always
81+
// considered a capturing group. Using INT16_MAX allows us to pass one value for each
82+
// group, plus a few additional values, to a JavaScript function without overflowing the
83+
// number of arguments. This is important, for example, in the implementation of
84+
// String.prototype.replace, where the second argument is a function.
85+
//
86+
// This should really be an unsigned, but we compare it against numGroups, so make it
87+
// an int for now to avoid a bunch of compiler warnings until we can go back and clean this up.
88+
static const int MAX_NUM_GROUPS = INT16_MAX;
89+
8090
int numGroups; // determined in first parse
8191
int nextGroupId;
8292
// Buffer accumulating all literals.

lib/Runtime/Library/RegexHelper.cpp

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -724,15 +724,24 @@ namespace Js
724724
char16 currentChar = replaceStr[substitutionOffset + 1];
725725
if (currentChar >= _u('0') && currentChar <= _u('9'))
726726
{
727+
// We've found a substitution ref, like $32. In accordance with the standard (sec-getsubstitution),
728+
// we recognize at most two decimal digits after the dollar sign.
729+
730+
// This should be unsigned, but this would cause lots of compiler warnings unless we also make
731+
// numGroups unsigned, because of a comparison below.
727732
int captureIndex = (int)(currentChar - _u('0'));
733+
Assert(0 <= captureIndex && captureIndex <= 9); // numeric value of single decimal digit
734+
728735
offset = substitutionOffset + 2;
729736

730737
if (offset < replaceLength)
731738
{
732739
currentChar = replaceStr[substitutionOffset + 2];
733740
if (currentChar >= _u('0') && currentChar <= _u('9'))
734741
{
742+
// Should also be unsigned; see captureIndex above.
735743
int tempCaptureIndex = (10 * captureIndex) + (int)(currentChar - _u('0'));
744+
Assert(0 <= tempCaptureIndex && tempCaptureIndex < 100); // numeric value of 2-digit positive decimal number
736745
if (tempCaptureIndex < numGroups)
737746
{
738747
captureIndex = tempCaptureIndex;
@@ -741,6 +750,7 @@ namespace Js
741750
}
742751
}
743752

753+
Assert(0 <= captureIndex && captureIndex < 100); // as above, value of 2-digit positive decimal number
744754
if (captureIndex < numGroups && (captureIndex != 0))
745755
{
746756
Var group = getGroup(captureIndex, nonMatchValue);
@@ -1204,10 +1214,13 @@ namespace Js
12041214
JavascriptString* newString = nullptr;
12051215
const char16* inputStr = input->GetString();
12061216
CharCount inputLength = input->GetLength();
1207-
const int numGroups = pattern->NumGroups();
1217+
const int rawNumGroups = pattern->NumGroups();
12081218
Var nonMatchValue = NonMatchValue(scriptContext, false);
12091219
UnifiedRegex::GroupInfo lastMatch; // initially undefined
12101220

1221+
AssertOrFailFast(0 < rawNumGroups && rawNumGroups <= INT16_MAX);
1222+
const uint16 numGroups = uint16(rawNumGroups);
1223+
12111224
#if ENABLE_REGEX_CONFIG_OPTIONS
12121225
RegexHelperTrace(scriptContext, UnifiedRegex::RegexStats::Replace, regularExpression, input, scriptContext->GetLibrary()->CreateStringFromCppLiteral(_u("<replace function>")));
12131226
#endif
@@ -1265,7 +1278,6 @@ namespace Js
12651278
lastSuccessfulMatch = lastActualMatch;
12661279
for (int groupId = 0; groupId < numGroups; groupId++)
12671280
replaceArgs[groupId + 1] = GetGroup(scriptContext, pattern, input, nonMatchValue, groupId);
1268-
#pragma prefast(suppress:6386, "The write index numGroups + 1 is in the bound")
12691281
replaceArgs[numGroups + 1] = JavascriptNumber::ToVar(lastActualMatch.offset, scriptContext);
12701282

12711283
// The called function must see the global state updated by the current match
@@ -1278,7 +1290,7 @@ namespace Js
12781290
ThreadContext* threadContext = scriptContext->GetThreadContext();
12791291
Var replaceVar = threadContext->ExecuteImplicitCall(replacefn, ImplicitCall_Accessor, [=]()->Js::Var
12801292
{
1281-
return replacefn->CallFunction(Arguments(CallInfo((ushort)(numGroups + 3)), replaceArgs));
1293+
return replacefn->CallFunction(Arguments(CallInfo(UInt16Math::Add(numGroups, 3)), replaceArgs));
12821294
});
12831295
JavascriptString* replace = JavascriptConversion::ToString(replaceVar, scriptContext);
12841296
concatenated.Append(input, offset, lastActualMatch.offset - offset);

test/Regex/replace.baseline

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,10 @@ replace(/b/ /*lastIndex=0*/ , "abc", "$12345678");
3838
"a$12345678c"
3939
r.lastIndex=0
4040
RegExp.${_,1,...,9}=["abc","","","","","","","","",""]
41+
replace(/([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])([ab])/ /*lastIndex=0*/ , "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "$100");
42+
"a0"
43+
r.lastIndex=0
44+
RegExp.${_,1,...,9}=["aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb","a","a","a","a","a","a","a","a","a"]
4145

4246
replace(/b/ /*lastIndex=0*/ , "abc", []);
4347
"ac"

test/Regex/replace.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,13 @@ replace(/b/, "abc", "$`$&$'");
229229
replace(/b/, "abc", "1234567$");
230230
replace(/b/, "abc", "$12345678");
231231

232+
// Ensure that we only accept 2 digits after a $ in the replacement
233+
// string, as per standards (sec-getsubstitution); this avoids an
234+
// integer overflow in implementation of 'replace'. Correct behavior
235+
// is 10th char of input ('a') followed by '0'; incorrect behavior is
236+
// 100th char of input ('b').
237+
replace(new RegExp('([ab])'.repeat(101)), ("a".repeat(50) + "b".repeat(51)), "$100");
238+
232239
echo("");
233240

234241
replace(/b/, "abc", function () { return ""; });

0 commit comments

Comments
 (0)