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

Issue 862566 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jul 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

CHECK failure: pos + sizeof(U) <= size() in buffer_view.h

Project Member Reported by ClusterFuzz, Jul 11

Issue description

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

Fuzzer: libFuzzer_zucchini_disassembler_dex_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  pos + sizeof(U) <= size() in buffer_view.h
  zucchini::internal::BufferViewBase<>::read<>
  zucchini::ReadTargetOffset32
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=572200:572205

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

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.
 
Project Member

Comment 1 by ClusterFuzz, Jul 11

Cc: ckitagawa@chromium.org
Labels: ClusterFuzz-Auto-CC
Automatically adding ccs based on OWNERS file / target commit history.

If this is incorrect, please add ClusterFuzz-Wrong label.
Project Member

Comment 2 by ClusterFuzz, Jul 11

Components: Internals>Core Internals>Installer>Diff
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 3 by ClusterFuzz, Jul 11

Labels: Test-Predator-Auto-Owner
Owner: ckitagawa@chromium.org
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/c2a778621cbcd812e2687269ba3f10132a31df12 ([Zucchini] Add dissassembler_dex Fuzzer).

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.
Owner: hua...@chromium.org
Reassigning to huangs@ as the issue is related to the atomicity assumption of references in Zucchini. Specifically, this assumption isn't upheld in the case of bad input trying to read past the file boundary.

See components/zucchini/disassembler_dex.cc
Lines: 423 and 660.
The invalid field being read is ClassDefItem::static_values_off, which straddles EOF. A cause is the lack of validation for fixed-length MapItems. So I'll fix this bug by implementing a stricter check for these items (note that variable-length MapItems such as for CodeItem would undergo more involved parsing that should root out these problems).

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 11

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

commit 1f63bf220bcb93560358fbe593ebccc4d64f4baa
Author: Samuel Huang <huangs@chromium.org>
Date: Wed Jul 11 20:02:23 2018

[Zucchini] DEX parsing: Implement stricter size checks for MapItem.

This CL fixes a bug discovered by the Fuzzer, where DEX parsing
triggers CHECK() failure due to an attempt to read a 4-byte value that
straddles EOF.

Tracing (Zucchini-read) using the Fuzzer-provided DEX show that the
faulty read attempt is for ClassDefItem::static_values_off, which is
a field in a fixed-length item.

Our fix is to validate MapItem entries for fixed-length items. Details:
- Add GetItemBaseSize() to return an item size bound. For fixed-length
  items, this is exactly the item size. Moreover, no 4-byte alignment
  is needed, since the item sizes are already multiples of 4 bytes.
- In DisassemblerDex::ParseHeader(), verify each MapItem fits in the
  image. For fixed-length items, this check enables safe read for
  items whose index are within bound (and avoid similar bugs). For
  variable-length items, this serves as a sanity check to quickly
  reject obviously bad inputs. Subsequent parsing will perform a more
  refined check. For unhandled items, sizes are assumed to be 1.
- Not checked: Whether MapItems ranges overlap. May do this in the
  future (more refactoring will be needed).

Bug:  862566 
Change-Id: If8efce122979fa1a36d1d445556d414eb499d273
Reviewed-on: https://chromium-review.googlesource.com/1133713
Reviewed-by: Samuel Huang <huangs@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Commit-Queue: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574294}
[modify] https://crrev.com/1f63bf220bcb93560358fbe593ebccc4d64f4baa/components/zucchini/disassembler_dex.cc

Status: Fixed (was: Assigned)
Project Member

Comment 8 by ClusterFuzz, Jul 12

ClusterFuzz has detected this issue as fixed in range 574290:574302.

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

Fuzzer: libFuzzer_zucchini_disassembler_dex_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  pos + sizeof(U) <= size() in buffer_view.h
  zucchini::internal::BufferViewBase<>::read<>
  zucchini::ReadTargetOffset32
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=572200:572205
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=574290:574302

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

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md 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 9 by ClusterFuzz, Jul 12

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

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

Sign in to add a comment