Issue metadata
Sign in to add a comment
|
Security: Possible arbitrary heap access through RegExp.prototype[@@match] |
||||||||||||||||||||||
Issue descriptionMoved from crbug.com/v8/6209. VULNERABILITY DETAILS AdvanceStringIndex [0] is called internally as part of the RegExp.prototype[@@match] builtin. AdvanceStringIndex assumes a Smi index argument but can be called with a HeapNumber on the @@match slow path. If index is a HeapNumber, we essentially perform (unintended) pointer arithmetic instead of integer addition. In more detail: * First we SmiAdd(index, 1), where index is the tagged pointer to the lastIndex value. * The resulting invalid pointer is then stored into lastIndex, and is accessible to the user. * This can be repeated to make the resulting lastIndex point at any spot on the heap that is located at (index + k * i), where index is the pointer to the HeapNumber, k is a positive integer, and i is the value of Smi::FromInt(1). [0] https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-regexp-gen.cc?q=AdvanceStringIn&l=1499 VERSION V8 M57 until current ToT (7ba4789d88a2f7af6f7aac20af8a901985e54e61) REPRODUCTION CASE let exec_count = 0; let last_last_index = -1; let fake_re = { exec: () => { return (exec_count++ == 0) ? [""] : null }, // This condition can be changed to control how often addition is performed. [Symbol.match]: RegExp.prototype[Symbol.match], get lastIndex() { return 4294967295; // HeapNumber. }, set lastIndex(value) { last_last_index = value }, get global() { return true; }, get flags() { return "g"; } }; fake_re[Symbol.match]("abc") I'm currently working on a CL to fix this, should land at some point later today.
,
Apr 6 2017
,
Apr 7 2017
,
Apr 7 2017
,
Apr 7 2017
Is this serious enough to require a merge to M57?
,
Apr 7 2017
Benedikt, Yang, what's your opinion regarding a merge to M57? As far as I was aware, there were no more merges to M57 since the branch point is next week.
,
Apr 7 2017
Up to hablich@ to decide.
,
Apr 7 2017
No need to merge it to M57.
,
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
Though I believe the commit in #1 is to 59, I think this would be good to get into 58.
,
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/+/bd8da83ff492de02d922abd5c08ad8f762bc7fba commit bd8da83ff492de02d922abd5c08ad8f762bc7fba Author: jgruber <jgruber@chromium.org> Date: Mon Apr 10 14:18:58 2017 [regexp] Properly handle HeapNumbers in AdvanceStringIndex This fixes behavior for HeapNumber {index} arguments passed to AdvanceStringIndex. Previously, we'd blindly treat {index} as a Smi. Passing a HeapNumber instead would result in a Smi addition on the tagged HeapNumber pointer. Backmerge of commit ed5496f3cd1b410e1632531799edddfad8afe4f6. BUG= chromium:709015 NOPRESUBMIT=true NOTRY=true Review-Url: https://codereview.chromium.org/2808033002 Cr-Commit-Position: refs/branch-heads/5.8@{#55} Cr-Branched-From: eda659cc5e307f20ac1ad542ba12ab32eaf4c7ef-refs/heads/5.8.283@{#1} Cr-Branched-From: 4310cd02d2160b1457baed81a2f40063eb264a21-refs/heads/master@{#43429} [modify] https://crrev.com/bd8da83ff492de02d922abd5c08ad8f762bc7fba/src/builtins/builtins-regexp.cc [modify] https://crrev.com/bd8da83ff492de02d922abd5c08ad8f762bc7fba/src/builtins/builtins-regexp.h [add] https://crrev.com/bd8da83ff492de02d922abd5c08ad8f762bc7fba/test/mjsunit/regress/regress-6209.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 11 2017
,
Apr 12 2017
,
Apr 18 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 |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by bugdroid1@chromium.org
, Apr 6 2017