New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 813376 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug-Security

Blocked on:
issue 804084



Sign in to add a comment

Crash in v8::internal::Invoke

Project Member Reported by ClusterFuzz, Feb 17 2018

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=4980429950812160

Fuzzer: inferno_twister_c
Job Type: linux_asan_chrome_media
Platform Id: linux

Crash Type: UNKNOWN WRITE
Crash Address: 0x7eadcd6b0000
Crash State:
  v8::internal::Invoke
  v8::internal::Execution::Call
  v8::Script::Run
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_media&range=498140:498349

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4980429950812160

Additional requirements: Requires HTTP

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Project Member

Comment 1 by ClusterFuzz, Feb 17 2018

Components: Blink>JavaScript
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Project Member

Comment 2 by ClusterFuzz, Feb 17 2018

Cc: jbroman@chromium.org petermarshall@chromium.org
Labels: Test-Predator-Auto-CC
Automatically adding ccs based on suspected regression changelists:

[cleanup] Replace List with std::vector in cctests and d8. by petermarshall@chromium.org - https://chromium.googlesource.com/v8/v8/+/abaece06d211bc0c4e5fbbda3999a1d1d7736055

[snapshot] Make the API external reference table const. by jbroman@chromium.org - https://chromium.googlesource.com/v8/v8/+/779bb1b1724c02a7b88480db857d8d4c3f5ede74

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label.
Project Member

Comment 3 by sheriffbot@chromium.org, Feb 18 2018

Labels: M-64
Project Member

Comment 4 by sheriffbot@chromium.org, Feb 18 2018

Labels: Pri-1
Owner: petermarshall@chromium.org
Status: Started (was: Untriaged)
I can't reproduce this with the build and repro given on the clusterfuzz page.
Tried to repro with a fresh build with the provided gn args, and again on the provided build but no luck. Apparently the crash is flaky. I get the same error in the console each time:

Uncaught RuntimeError: memory access out of bounds
    at wasm-function[1]:8
    at call (.../mozilla/tests/wasm/js/harness/index.js:25:46)
    at assert_trap (.../mozilla/tests/wasm/js/memory_trap.wast.js:2:19)
    at assert_trap (.../mozilla/tests/wasm/js/harness/index.js:28:18)
    at .../mozilla/tests/wasm/js/memory_trap.wast.js:2:1

Not sure what could be going wrong here. The CL only touches d8 (which is not used here, because this repro is from chrome), the cpu profiler (also not used here) and cctests, which are not being run.
Cc: yangguo@chromium.org jgruber@chromium.org
Now it is pointing to other CLs (see the CF page):
[csa] Add and use ToInteger_Inline by jgruber@chromium.org
Leave spaces between instance types. by yangguo@chromium.org

I don't think we can actually solve this without a repro though - any ideas?
Project Member

Comment 9 by sheriffbot@chromium.org, Mar 7 2018

Labels: -M-64 M-65
I was able to repro this locally with the following steps:

1) Download the unminified test case and ASAN build (asan-linux-release-537537) from the clusterfuzz test case.
2) Extract the test case and copy the "resources" folder from fuzzer-common-data-bundles/LayoutTests/ into data-bundles/ltests/moz_tests/testing/web-platform/mozilla/tests/wasm/.
3) From the wasm/ folder, start an HTTP server (`python -m SimpleHTTPServer`).
4) Start the ASAN build using the same flags as clusterfuzz:
`./chrome --user-data-dir=/tmp/clusterfuzz537537 --js-flags="--expose-gc --verify-heap" --no-first-run --use-gl=swiftshader --disable-in-process-stack-traces http://127.0.0.1:8000/fuzz-http-72.html`

Without adding the additional flags this did not repro for me. With the flags, I get the following ASAN result:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==1==ERROR: AddressSanitizer: SEGV on unknown address 0x7eac7e720000 (pc 0x7e9fd923203f bp 0x7fff7fc27ac0 sp 0x7fff7fc27aa0 T0)
==1==The signal is caused by a WRITE memory access.
SCARINESS: 30 (wild-addr-write)
Owner: jgruber@chromium.org
Status: Assigned (was: Started)
Thanks cthomp for the detailed steps. I still can't repro it :( I get the same "Uncaught RuntimeError: memory access out of bounds" in the js console.

The CF issue isn't pointing at my CL anymore, Jakob, could you see if you can repro it? Maybe there is an edge-case in ToInteger_Inline.
Got a repro in d8: 

1. extract the two JS files from the minified repro,
2. build a debug asan d8

is_component_build = true
is_debug = true
use_goma = true
v8_enable_backtrace = true
v8_enable_disassembler = true
v8_enable_object_print = true
v8_enable_slow_dchecks = true
v8_enable_verify_heap = true
v8_optimized_debug = false

is_asan = true
is_lsan = true

3. run: 

out/debug-asan/d8 ~/Downloads/repro/index.js ~/Downloads/repro/memory_trap.wast.js 
wasm-function[1]:8: RuntimeError: memory access out of bounds
RuntimeError: memory access out of bounds
    at wasm-function[1]:8
    at call (index.js:25:46)
    at assert_trap (memory_trap.wast.js:2:19)
    at assert_trap (index.js:28:18)
    at memory_trap.wast.js:2:1
Oh, never mind - I just realized this is what Peter was also seeing.
Cc: mstarzinger@chromium.org ahaas@chromium.org clemensh@chromium.org titzer@chromium.org
Owner: ----
Status: Available (was: Assigned)
I also couldn't repro, neither with debug/release d8 builds on ToT, not with the instructions from #10.

I'm fairly confident that ToInteger_Inline is not the culprit as the change is a rather mechanic refactoring. 

Unassigning myself & ccing a few wasm folks since this happens in a wasm test case. Maybe this rings a bell for someone? Did we have other similar UNKNOWN WRITE crashes lately?
Owner: eholk@chromium.org
Status: Assigned (was: Available)
Assigning to eholk@ as per offline discussion with ahaas@, ptal!
Project Member

Comment 16 by sheriffbot@chromium.org, Mar 26 2018

eholk: Uh oh! This issue still open and hasn't been updated in the last 36 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

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

Comment 17 by eholk@chromium.org, Mar 26 2018

Does Chrome have the WebAssemblyTrapHandler feature enabled?

If so, the fix is to change the ASAN_OPTIONS to have allow_user_segv_handler=1.

The trap handler feature catches out of bounds memory accesses with a signal handler and reports this as an out of bounds exception. It's set up to call the existing signal handler if it does not recognize the failure (i.e. it was not generated by an instruction that came from wasm memory reference).
Blockedon: 804084
Components: -Blink>JavaScript Blink>JavaScript>WebAssembly
This is a dupe of 804084. We had some ideas there (#17-19) to either statically (using preprocessor macros), or dynamically detect whether a user segfault handler is allowed, and disabling the trap handler feature if not. To be on the safe side, I would propose a combination of both.

So basically, in RegisterDefaultSignalHandler, adding something like this:

#if defined(ADDRESS_SANITIZER) || defined(MEMORY_SANITIZER) || \
    defined(THREAD_SANITIZER) || defined(LEAK_SANITIZER) ||    \
    defined(UNDEFINED_SANITIZER)
struct sigaction installed_handler;
CHECK(sigaction(SIGSEGV, NULL, &installed_handler) == 0);
// If the installed handler does not point to HandleSignal, then
// allow_user_segv_handler is 0.
if (installed_handler.sa_sigaction != HandleSignal) return false;
#endif

Glad to see some progress here. Just a reminder from the Security Sheriff that Chrome aims to deploy patches for issues like this one to all Chrome users in under 60 days. This bug is 40 days old.

https://chromium.googlesource.com/chromium/src/+/master/docs/security/severity-guidelines.md#high-severity

Comment 20 by eholk@chromium.org, Mar 30 2018

I think the best option for fixing this is to either set allow_user_segv_handler to 1 or arrange for the sanitizers to call V8::TryHandleSignal in their own signal handler.

Disabling trap handlers on sanitizers hurts our test coverage for Wasm's fast, trap-based bounds checks. Because this feature relies on low-level features like memory protection and signal handling, this is exactly the kind of place where we want to be sure we have thorough sanitizer coverage.

If we can't do that, the solution Clemens proposed is tolerable. I have a CL for it at https://crrev.com/c/987628 that I will make sure passes on all the trybots.
 Issue 830174  has been merged into this issue.
Project Member

Comment 22 by bugdroid1@chromium.org, Apr 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/1fac51a85bd520c03949fc3e2f530cd0d3755745

commit 1fac51a85bd520c03949fc3e2f530cd0d3755745
Author: Eric Holk <eholk@chromium.org>
Date: Mon Apr 09 21:23:43 2018

[trap handler] verify signal handler successfully installed on sanitizer builds

Bug:  chromium:813376 
Change-Id: I7d32f2ea09f7e8a4b75b9826695e129adac69e50
Reviewed-on: https://chromium-review.googlesource.com/987628
Commit-Queue: Eric Holk <eholk@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52495}
[modify] https://crrev.com/1fac51a85bd520c03949fc3e2f530cd0d3755745/src/trap-handler/handler-outside.cc

Project Member

Comment 23 by ClusterFuzz, Apr 11 2018

ClusterFuzz has detected this issue as fixed in range 549440:549441.

Detailed report: https://clusterfuzz.com/testcase?key=4980429950812160

Fuzzer: inferno_twister_c
Job Type: linux_asan_chrome_media
Platform Id: linux

Crash Type: UNKNOWN WRITE
Crash Address: 0x7eb8e32b0000
Crash State:
  v8::internal::Invoke
  v8::internal::Execution::Call
  v8::Script::Run
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_media&range=525321:525392
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_media&range=549440:549441

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4980429950812160

Additional requirements: Requires HTTP

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.
Project Member

Comment 24 by ClusterFuzz, Apr 11 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase 4980429950812160 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Project Member

Comment 25 by sheriffbot@chromium.org, Apr 11 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 26 by sheriffbot@chromium.org, Jul 18

Labels: -Restrict-View-SecurityNotify allpublic
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