Security: Arrow function scope fixing bug |
||||||||||||||||||||||
Issue descriptionWhen the parser parses the parameter list of an arrow function contaning destructuring assignments, it can't distinguish whether the assignments will be actually in the parameter list or just assignments until it meets a "=>" token. So it first assigns the destructuring assignments to the outer scope, and fixs the scope when it meets the "=>" token. Here's the methods used to fix the scope (https://cs.chromium.org/chromium/src/v8/src/parsing/parser-base.h?rcl=787ecbb389741d2b76131f9fa526374a0dbfcff6&l=407). void RewindDestructuringAssignments(int pos) { destructuring_assignments_to_rewrite_.Rewind(pos); } void SetDestructuringAssignmentsScope(int pos, Scope* scope) { for (int i = pos; i < destructuring_assignments_to_rewrite_.length(); ++i) { destructuring_assignments_to_rewrite_[i]->set_scope(scope); } } Since the SetDestructuringAssignmentsScope method changes the scope from "pos" to the end of the list, it needs to call the RewindDestructuringAssignments method after fixing the scope. But the RewindDestructuringAssignments method is only called when the arrow function's body starts with a "{" token (https://cs.chromium.org/chromium/src/v8/src/parsing/parser-base.h?rcl=787ecbb389741d2b76131f9fa526374a0dbfcff6&l=4418). So it can't properly handle the following case where a destructuring assignment expression containing a single line arrow function. It will set the scope of the inner destructuring assignments to the outer arrow function's scope. PoC: (({a = (async ({b = {a = c} = { a: 0x1234 }}) => 1)({})}, c) => 1)({}); Log: Received signal 10 BUS_ADRERR 12340000001f ==== C stack trace =============================== [0x00010edde85e] [0x7fff53e54f5a] [0x000000000000] [0x7eb48331b6d8] [0x7eb48331b6d8] [end of stack trace] Bus error: 10 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.
,
Jan 30 2018
Thanks for the report; fix should be straightforward, aiming to fix this week.
,
Jan 30 2018
Detailed report: https://clusterfuzz.com/testcase?key=5955339443503104 Job Type: linux_asan_chrome_mp Crash Type: UNKNOWN READ Crash Address: 0x12340000001f Crash State: v8::internal::Invoke v8::internal::Execution::Call v8::Script::Run Sanitizer: address (ASAN) Recommended Security Severity: Medium Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=511396:511405 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5955339443503104 See https://github.com/google/clusterfuzz-tools for more information. The recommended severity is different from what was assigned to the bug. Please double check the accuracy of the assigned severity.
,
Jan 30 2018
,
Jan 30 2018
Caitlin, here's the bug I mentioned this morning.
,
Feb 3 2018
I have what seems like a reasonable fix to this in progress. Aiming to get it out for review before I go home today, or first thing on Monday.
,
Feb 3 2018
,
Feb 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/f1a551800fa843b5c8d5bccc20aa74fb0fa8f7f5 commit f1a551800fa843b5c8d5bccc20aa74fb0fa8f7f5 Author: Adam Klein <adamk@chromium.org> Date: Wed Feb 07 18:14:28 2018 [parser] More carefully handle destructuring in arrow params This patch attempts to reduce the special handling of destructuring assignments in arrow function parameters by "adopting" them from wherever they were initially parsed into the arrow function's FunctionState/Scope. This avoids incorrectly re-setting the Scope of such assignments multiple times for arrow functions that are nested inside other arrow params themselves. It also generally seems better, in that we now only rewrite destructuring assignments for a single function at a time. Bug: chromium:807096 Change-Id: I6bef5613f99e3e8c130fc0aa2ee5d6fcf2efd34b Reviewed-on: https://chromium-review.googlesource.com/900168 Reviewed-by: Marja Hölttä <marja@chromium.org> Reviewed-by: Caitlin Potter <caitp@igalia.com> Commit-Queue: Adam Klein <adamk@chromium.org> Cr-Commit-Position: refs/heads/master@{#51155} [modify] https://crrev.com/f1a551800fa843b5c8d5bccc20aa74fb0fa8f7f5/src/parsing/parser-base.h [add] https://crrev.com/f1a551800fa843b5c8d5bccc20aa74fb0fa8f7f5/test/mjsunit/regress/regress-crbug-807096.js
,
Feb 8 2018
Marking this fixed, merges will be tracked by merge labels.
,
Feb 8 2018
ClusterFuzz has detected this issue as fixed in range 535282:535283. Detailed report: https://clusterfuzz.com/testcase?key=5955339443503104 Job Type: linux_asan_chrome_mp Crash Type: UNKNOWN READ Crash Address: 0x12340000001f Crash State: v8::internal::Invoke v8::internal::Execution::Call v8::Script::Run Sanitizer: address (ASAN) Recommended Security Severity: Medium Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=511396:511405 Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=535282:535283 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5955339443503104 See https://github.com/google/clusterfuzz-tools for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
,
Feb 8 2018
ClusterFuzz testcase 5955339443503104 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
,
Feb 8 2018
,
Feb 8 2018
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 8 2018
awhalley@ - how critical is this bug? Can we wait until 65?
,
Feb 8 2018
,
Feb 8 2018
This bug requires manual review: Less than 22 days to go before AppStore submit on M65 Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 8 2018
+awhalley@ for M65 merge review
,
Feb 9 2018
[Bulk Edit] +awhalley@ (Security TPM) for M65 merge review
,
Feb 12 2018
Ping!
,
Feb 12 2018
govind@ - good for 65
,
Feb 12 2018
Approving merge to M65 branch 3325 based on comment #21. Please merge ASAP so we can pick it up for this week Beta release. Thank you.
,
Feb 12 2018
confirmed with awhalley@ - it's not needed for M64. Rejecting merge.
,
Feb 12 2018
Merged in https://chromium.googlesource.com/v8/v8/+/d036460eb8b60700fd450baa69dae0b57154400c (I forgot to include the bug in the CL description).
,
Feb 12 2018
,
Mar 6 2018
,
Mar 20 2018
,
Mar 27 2018
,
May 17 2018
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 ClusterFuzz
, Jan 30 2018