Security: V8: Empty BytecodeJumpTable may lead to OOB read |
||||||||||||||||||||||||
Issue descriptionIn the current implementation, the bytecode generator also emits empty jump tables. https://cs.chromium.org/chromium/src/v8/src/interpreter/bytecode-array-writer.cc?rcl=111e990462823c9faeee06b67c0dcf05749d4da8&l=89 So the bytecode for the example code would be generated as follows: Code: function* opt() { for (;;) if (true) { } else { yield; // never richs, never hits BindJumpTableEntry } } Bytecode: ... 0x35dda532a2a5 @ 75 : 90 04 01 01 SwitchOnSmiNoFeedback [4], [1], [1] { } <<--- SIZE: 1, but EMPTY ... Here's a snippet of JumpTableTargetOffsets::iterator::UpdateAndAdvanceToValid which is used to enumerate a jump table. void JumpTableTargetOffsets::iterator::UpdateAndAdvanceToValid() { if (table_offset_ >= table_end_) return; current_ = accessor_->GetConstantAtIndex(table_offset_); Isolate* isolate = accessor_->bytecode_array()->GetIsolate(); while (current_->IsTheHole(isolate)) { ++table_offset_; ++index_; current_ = accessor_->GetConstantAtIndex(table_offset_); } } If the jump table is empty, table_offset_ may exceed table_end_. As a result, out-of-bounds reads occur. PoC: function* opt() { for (;;) if (true) { } else { yield; } for (;;) if (true) { } else { yield; yield; yield; yield; yield; yield; yield; yield; } } for (let i = 0; i < 100000; i++) opt(); 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.
,
Dec 14 2017
,
Dec 14 2017
Detailed report: https://clusterfuzz.com/testcase?key=4968984012390400 Job Type: linux_asan_chrome_mp Crash Type: Null-dereference READ Crash Address: 0x000000000008 Crash State: v8::internal::compiler::UpdateOutLiveness v8::internal::compiler::BytecodeAnalysis::Analyze v8::internal::compiler::BytecodeGraphBuilder::VisitBytecodes Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=477198:477224 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4968984012390400 See https://github.com/google/clusterfuzz-tools for more information.
,
Dec 14 2017
Automatically applying components based on crash stacktrace and information from OWNERS files. If this is incorrect, please apply the Test-Predator-Wrong-Components label.
,
Dec 14 2017
,
Dec 14 2017
Applying Security_Severity-Medium since this could be used to OOB read in a renderer process. Clusterfuzz sees it as only a null deref, which might downgrade this to Low if it can't be used for arbitrary reads.
,
Dec 15 2017
,
Dec 15 2017
Making Mythri the owner. I think the solution could be to just always bind entries in the table (e.g. to the end of the loop) to make sure there aren't empty entries.
,
Dec 15 2017
,
Dec 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/e9770dfeb885a82190ec48555a422c4afca34508 commit e9770dfeb885a82190ec48555a422c4afca34508 Author: Mythri <mythria@chromium.org> Date: Fri Dec 15 17:17:46 2017 Add bounds check when iterating over bytecode jump table Bytecode jump table for switch statements can have holes when the corresponding case statements do not exist (either because the case was missing or was eliminated because it was dead code). The iterator deals with this by skipping over the holes and setting the iterator to the next valid entry. Bounds check was missing during this skipping over if the last element is a hole. Bug: chromium:794825 Change-Id: Ifdb63257e2997d2fd2868467a56da72b68feb47e Reviewed-on: https://chromium-review.googlesource.com/829774 Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Commit-Queue: Mythri Alle <mythria@chromium.org> Cr-Commit-Position: refs/heads/master@{#50137} [modify] https://crrev.com/e9770dfeb885a82190ec48555a422c4afca34508/src/interpreter/bytecode-array-accessor.cc
,
Dec 15 2017
There is one more cl in pipeline to fix this bug: https://chromium-review.googlesource.com/c/v8/v8/+/830013. After the earlier fix, it fails in instruction-selector.
,
Dec 17 2017
ClusterFuzz has detected this issue as fixed in range 524603:524604. Detailed report: https://clusterfuzz.com/testcase?key=4968984012390400 Job Type: linux_asan_chrome_mp Crash Type: Null-dereference READ Crash Address: 0x000000000008 Crash State: v8::internal::compiler::UpdateOutLiveness v8::internal::compiler::BytecodeAnalysis::Analyze v8::internal::compiler::BytecodeGraphBuilder::VisitBytecodes Sanitizer: address (ASAN) Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=477198:477224 Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_mp&range=524603:524604 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4968984012390400 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.
,
Dec 17 2017
ClusterFuzz testcase 4968984012390400 is verified as fixed, so closing issue as verified. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
,
Dec 17 2017
,
Dec 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/f2d85ff1637e5451cdacf93247a5a7463e7b5d90 commit f2d85ff1637e5451cdacf93247a5a7463e7b5d90 Author: Mythri <mythria@chromium.org> Date: Mon Dec 18 10:17:08 2017 [Turbofan] Fix instruction selector to handle switch with no case Instruction selector computes the range of case statement labels to choose between a table or lookup based switch. We need to special case this when there are no case statements. Bug: chromium:794825 Change-Id: I46ef57d17f5e2b99a3570f7f3c4ff06e75d78fab Reviewed-on: https://chromium-review.googlesource.com/830013 Reviewed-by: Benedikt Meurer <bmeurer@chromium.org> Commit-Queue: Mythri Alle <mythria@chromium.org> Cr-Commit-Position: refs/heads/master@{#50153} [modify] https://crrev.com/f2d85ff1637e5451cdacf93247a5a7463e7b5d90/src/compiler/instruction-selector.cc [add] https://crrev.com/f2d85ff1637e5451cdacf93247a5a7463e7b5d90/test/mjsunit/regress/regress-794825.js
,
Jan 10 2018
Has this been merged into M64?
,
Jan 10 2018
,
Jan 10 2018
,
Jan 10 2018
I am sorry, didn't merge them to 64. I added a merge request, if it isn't too late will merge both fixes now.
,
Jan 10 2018
This bug requires manual review: We are only 12 days from stable. 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
,
Jan 10 2018
awhalley@ Please take a look!
,
Jan 10 2018
,
Jan 10 2018
cmasso@ - yep, would be good to get this into 64, thanks!
,
Jan 11 2018
Thanks, I have a merged cl ready here: https://chromium-review.googlesource.com/c/v8/v8/+/860659. I am waiting for the label to be updated to approved before I merge this to 6.4.
,
Jan 11 2018
,
Jan 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/d08ae1854ac14879cfdc2900d9e7055c1d1e0777 commit d08ae1854ac14879cfdc2900d9e7055c1d1e0777 Author: Mythri Alle <mythria@chromium.org> Date: Thu Jan 11 16:27:40 2018 Merged: Squashed multiple commits. Merged: Add bounds check when iterating over bytecode jump table Revision: e9770dfeb885a82190ec48555a422c4afca34508 Merged: [Turbofan] Fix instruction selector to handle switch with no case Revision: f2d85ff1637e5451cdacf93247a5a7463e7b5d90 BUG= chromium:794825 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true R=rmcilroy@chromium.org Change-Id: I665b1b61640d126c611875b1c29dec75a40e7c98 Reviewed-on: https://chromium-review.googlesource.com/860659 Reviewed-by: Ross McIlroy <rmcilroy@chromium.org> Cr-Commit-Position: refs/branch-heads/6.4@{#45} Cr-Branched-From: 0407506af3d9d7e2718be1d8759296165b218fcf-refs/heads/6.4.388@{#1} Cr-Branched-From: a5fc4e085ee543cb608eb11034bc8f147ba388e1-refs/heads/master@{#49724} [modify] https://crrev.com/d08ae1854ac14879cfdc2900d9e7055c1d1e0777/src/compiler/instruction-selector.cc [modify] https://crrev.com/d08ae1854ac14879cfdc2900d9e7055c1d1e0777/src/interpreter/bytecode-array-accessor.cc [add] https://crrev.com/d08ae1854ac14879cfdc2900d9e7055c1d1e0777/test/mjsunit/regress/regress-794825.js
,
Jan 12 2018
,
Jan 22 2018
,
Mar 25 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
,
Mar 27 2018
|
||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||
Comment 1 by ClusterFuzz
, Dec 14 2017