Issue metadata
Sign in to add a comment
|
Security: OOB access in RegExp Stubs |
||||||||||||||||||||||
Issue description
There is an out-of-bounds access in RegExp.prototype.exec and RegExp.prototype.test. The code defined in BranchIfFastRegExp checks whether a regular expression object has the default map, however, it is possible to alter the map after this check has been performed. This can cause inline fields, such as lastIndex to be changed to dictionary properties. This will cause out-of-bounds reads and writes the next time lastIndex is accessed on the fast path.
A minimal PoC is as follows, and two full PoCs (one for test and one for exec) are attached.
var re;
function f(){
for(var i = 0; i < 100; i++){
re["test" + i] = 0x77777777; // make a dict
}
return 0;
}
re = /-/g;
var str = '2016-01-02';
re.lastIndex = {valueOf : f};
result = re.exec(str);
This PoC crashes on google-chrome-beta on Linux.
Please note there are several situations in the RegExp class where user script can change the map during the fast path (for example, in replace and split), but it wasn't obvious how to cause a crash. I recommend cleaning up this code so that the slow path is taken whenever user script is executed.
This bug is subject to a 90 day disclosure deadline. After 90 days elapse
or a patch has been made broadly available, the bug report will become
visible to the public.
,
Apr 5 2017
,
Apr 5 2017
Jakob, do you mind taking a look?
,
Apr 5 2017
Looking, got a local d8 repro.
,
Apr 5 2017
The problem is that we can call ToLength(lastIndex) after deciding that we're on the fast path. ToLength can execute arbitrary user code, possibly changing the shape of the regexp object. We currently don't recheck the object shape after ToLength calls and continue on the fast path, leading to possible OOB writes when performing an offset-based store to lastIndex. There are several spots where the spec requires calling ToLength: * RegExpBuiltinExec (https://tc39.github.io/ecma262/#sec-regexpbuiltinexec) * @@match (https://tc39.github.io/ecma262/#sec-regexp.prototype-@@match) * @@replace (https://tc39.github.io/ecma262/#sec-regexp.prototype-@@replace) * @@split (https://tc39.github.io/ecma262/#sec-regexp.prototype-@@split) Both exec and @@match CSA builtins contain this bug. A possible solution would be to extend the fast-path check to go slow-path if lastIndex is not a non-negative smi, i.e. ensure that the fast-path never contains a ToLength call. Other spec operations with possible side-effects are ToString, ToUint32, ToInteger, ToBoolean. Let's audit RegExp builtins to ensure we don't have a similar issue elsewhere.
,
Apr 5 2017
I wonder whether it makes sense to add some mechanism to CSA to detect this class of problems.
,
Apr 5 2017
A DisallowJavascriptExecution scope for CSA would definitely make sense.
,
Apr 5 2017
That should be doable. It just reads a bit from the isolate, sets a new value for the bit, and later on restores the bit. Probably easier is to somehow check that no call to JS is produced by CSA in a certain range.
,
Apr 5 2017
Now that I think about this, porting DisallowJavascriptExecution to CSA does not help at all since it's checked at the C++ to JS boundary.
,
Apr 5 2017
Made a pass through all RegExp CSA builtins and filed a couple of bugs: crbug.com/v8/6209 (AdvanceStringIndex with non-smi argument) crbug.com/v8/6210 (Shape changes on fast path through ToString and ToUint32) And less urgent: crbug.com/v8/6207 (Possible spec violation)
,
Apr 5 2017
Got a CL in progress, landing ETA early tomorrow.
,
Apr 5 2017
,
Apr 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/ae459356468ae81955a1298acd255794ac901b4c commit ae459356468ae81955a1298acd255794ac901b4c Author: jgruber <jgruber@chromium.org> Date: Thu Apr 06 08:12:56 2017 [regexp] Ensure there are no shape changes on the fast path BUG= v8:5437 , chromium:708247 Review-Url: https://codereview.chromium.org/2797993002 Cr-Commit-Position: refs/heads/master@{#44428} [modify] https://crrev.com/ae459356468ae81955a1298acd255794ac901b4c/src/builtins/builtins-regexp-gen.cc [modify] https://crrev.com/ae459356468ae81955a1298acd255794ac901b4c/src/builtins/builtins-regexp-gen.h [modify] https://crrev.com/ae459356468ae81955a1298acd255794ac901b4c/src/builtins/builtins-string-gen.cc [modify] https://crrev.com/ae459356468ae81955a1298acd255794ac901b4c/src/regexp/regexp-utils.cc [add] https://crrev.com/ae459356468ae81955a1298acd255794ac901b4c/test/mjsunit/regress/regress-708247.js
,
Apr 6 2017
,
Apr 6 2017
,
Apr 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/1ccf6c0943e328183cb670e14d718b7461cbcb93 commit 1ccf6c0943e328183cb670e14d718b7461cbcb93 Author: jgruber <jgruber@chromium.org> Date: Thu Apr 06 15:52:21 2017 [regexp] Fix two more possible shape changes on fast path This CL fixes two more cases in which a regexp could unintentionally transition to slow mode while on the fast path, leading to possible OOB accesses of lastIndex. In both cases, the fix is to re-check the shape and possibly bail to runtime. BUG= chromium:708247 ,v8:6210 Review-Url: https://codereview.chromium.org/2803603005 Cr-Commit-Position: refs/heads/master@{#44451} [modify] https://crrev.com/1ccf6c0943e328183cb670e14d718b7461cbcb93/src/builtins/builtins-regexp-gen.cc [add] https://crrev.com/1ccf6c0943e328183cb670e14d718b7461cbcb93/test/mjsunit/regress/regress-6210.js
,
Apr 6 2017
The CLs in #14 and #17 fix all known shape changes on the fast path. Re #7: There's a design doc floating around describing such a mechanism for Arrays/TypedArrays that will likely also be applicable to this case. (Not posting a link since it sounds like this bug will be made public at some point).
,
Apr 6 2017
A friendly reminder that M58 Stable is launch is coming soon (less than 2 weeks)! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP so it gets enough baking time in Beta (before Stable promotion). Thank you!
,
Apr 7 2017
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 7 2017
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 8 2017
,
Apr 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/b5de87fef565d5e264e9babdcc46742d1cfc7bd9 commit b5de87fef565d5e264e9babdcc46742d1cfc7bd9 Author: jgruber <jgruber@chromium.org> Date: Mon Apr 10 14:21:15 2017 [regexp] Ensure there are no shape changes on the fast path Backmerge of commit ae459356468ae81955a1298acd255794ac901b4c and 1ccf6c0943e328183cb670e14d718b7461cbcb93. BUG= v8:5437 , chromium:708247 NOPRESUBMIT=true NOTRY=true Review-Url: https://codereview.chromium.org/2808023002 Cr-Commit-Position: refs/branch-heads/5.8@{#56} Cr-Branched-From: eda659cc5e307f20ac1ad542ba12ab32eaf4c7ef-refs/heads/5.8.283@{#1} Cr-Branched-From: 4310cd02d2160b1457baed81a2f40063eb264a21-refs/heads/master@{#43429} [modify] https://crrev.com/b5de87fef565d5e264e9babdcc46742d1cfc7bd9/src/builtins/builtins-regexp.cc [modify] https://crrev.com/b5de87fef565d5e264e9babdcc46742d1cfc7bd9/src/builtins/builtins-regexp.h [modify] https://crrev.com/b5de87fef565d5e264e9babdcc46742d1cfc7bd9/src/builtins/builtins-string.cc [modify] https://crrev.com/b5de87fef565d5e264e9babdcc46742d1cfc7bd9/src/regexp/regexp-utils.cc [add] https://crrev.com/b5de87fef565d5e264e9babdcc46742d1cfc7bd9/test/mjsunit/regress/regress-6210.js [add] https://crrev.com/b5de87fef565d5e264e9babdcc46742d1cfc7bd9/test/mjsunit/regress/regress-708247.js
,
Apr 10 2017
,
Apr 11 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 14 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 15 2017
,
Apr 28 2017
Issue 716166 has been merged into this issue.
,
Apr 28 2017
,
Jul 15 2017
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 20 2018
,
Jun 26 2018
,
Jun 26 2018
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by hablich@chromium.org
, Apr 4 2017