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

Issue 868651 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Aug 1
Cc:
EstimatedDays: ----
NextAction: 2018-08-01
OS: Linux
Pri: 1
Type: Bug

Blocking:
issue 868463



Sign in to add a comment

Pointer-overflow in aom_read_obu_header_and_size

Project Member Reported by ClusterFuzz, Jul 28

Issue description

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

Fuzzer: libFuzzer_mediasource_MP4_AV1_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Pointer-overflow
Crash Address: 
Crash State:
  aom_read_obu_header_and_size
  decoder_peek_si_internal
  decode_one
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=578774:578804

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

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 28

Labels: Test-Predator-Auto-Owner
Owner: johannko...@google.com
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/f2813ce352ac65a2d4259ae7aba9fa739d3903aa (enable av1 playback by default).

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.
Blocking: 868463
Cc: jzern@chromium.org huisu@google.com wtc@google.com urvang@chromium.org
Cc: -wtc@google.com johannko...@google.com
Owner: wtc@google.com
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 31

The following revision refers to this bug:
  https://aomedia.googlesource.com/aom/+/bc484c485277bc19c7a1b273c8cf5472f741b73a

commit bc484c485277bc19c7a1b273c8cf5472f741b73a
Author: Wan-Teh Chang <wtc@google.com>
Date: Tue Jul 31 03:37:16 2018

Validate payload_size in decoder_peek_si_internal.

If payload_size is greater than data_sz, return an error.

Remove the local variable payload_start. (It is always equal to 'data'.)

BUG= aomedia:2060 
BUG= chromium:868651 

Change-Id: Id8e0b5a35db3d1ff452507545f47066c52e1388c

[modify] https://crrev.com/bc484c485277bc19c7a1b273c8cf5472f741b73a/av1/av1_dx_iface.c

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 31

Labels: merge-merged-m69-3497
The following revision refers to this bug:
  https://aomedia.googlesource.com/aom/+/e96c7350ab1e217090515ef64b2839634a309971

commit e96c7350ab1e217090515ef64b2839634a309971
Author: Wan-Teh Chang <wtc@google.com>
Date: Tue Jul 31 16:24:44 2018

Validate payload_size in decoder_peek_si_internal.

If payload_size is greater than data_sz, return an error.

Remove the local variable payload_start. (It is always equal to 'data'.)

BUG= aomedia:2060 
BUG= chromium:868651 

Change-Id: Id8e0b5a35db3d1ff452507545f47066c52e1388c
(cherry picked from commit bc484c485277bc19c7a1b273c8cf5472f741b73a)

[modify] https://crrev.com/e96c7350ab1e217090515ef64b2839634a309971/av1/av1_dx_iface.c

Labels: -merge-merged-m69-3497
Removing wrong label: this has been cherry-picked in libaom's branch, not chromium's.

I'll roll libaom in chromium master now.
(And then in chromium branch tomorrow).
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 31

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

commit c26a4173cd6f0eb4762563603c6ff34ab7baa385
Author: Urvang Joshi <urvang@google.com>
Date: Tue Jul 31 19:35:24 2018

Roll src/third_party/libaom/source/libaom/ 4f1fd9640..bc484c485 (38 commits)

https://aomedia.googlesource.com/aom.git/+log/4f1fd9640433..bc484c485277

$ git log 4f1fd9640..bc484c485 --date=short --no-merges --format='%ad %ae %s'
2018-07-30 wtc Validate payload_size in decoder_peek_si_internal.
2018-06-28 grant.hsu Reduce interpolation filter search
2018-07-26 binpengsmail Cleanup warp motion param in MB_MODE_INFO
2018-07-30 huisu Fix encoder multi thread test failures
2018-07-30 huisu Add const to input parameters of update_state()
2018-07-30 yaowu Fix include order: av1/encoder/encoder.c
2018-07-30 ilie.halip Correctly set symbol file header when using gcc on Windows.
2018-07-30 yaowu Simplify range check of max_gf_interval
2018-07-30 huisu Code cleanup in rd_pick_partition()
2018-07-28 martin arm: Fix building neon txfm with clang
2018-07-27 sanampudi.venkatarao Speed improvement in av1_convolve_y_sr_neon for 32-bit Neon
2018-07-30 yaowu Fix include order
2018-07-27 wtc Move CONFIG_SIZE_LIMIT check in yv12config.c.
2018-07-30 yaowu Remove effectless code
2018-07-30 ddvfinite Rename compute_stats
2018-07-28 jonathan.matthews [normative]Reset film grain params if monochrome
2018-06-23 lu_zero Update the VSX support
2018-07-27 weitinglin Comment out a debug output
2018-07-27 huisu code cleanup in encode_rd_sb_row()
2018-07-27 huisu Remove code about COLLECT_TX_SIZE_DATA
(...)

Created with:
  roll-dep src/third_party/libaom/source/libaom
R=johannkoenig@google.com

Bug:  868651 
Change-Id: I16b309f12078b5eb1924c8c3d3b978cb644a761f
Reviewed-on: https://chromium-review.googlesource.com/1157033
Reviewed-by: Johann Koenig <johannkoenig@google.com>
Commit-Queue: Urvang Joshi <urvang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579529}
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/DEPS
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/README.chromium
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/config/aom_version.h
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/arm-neon-cpu-detect/config/aom_config.asm
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/arm-neon-cpu-detect/config/aom_config.c
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/arm-neon-cpu-detect/config/aom_config.h
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/arm-neon/config/aom_config.asm
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/arm-neon/config/aom_config.c
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/arm-neon/config/aom_config.h
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/arm/config/aom_config.asm
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/arm/config/aom_config.c
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/arm/config/aom_config.h
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/arm64/config/aom_config.asm
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/arm64/config/aom_config.c
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/arm64/config/aom_config.h
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/generic/config/aom_config.asm
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/generic/config/aom_config.h
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/ia32/config/aom_config.asm
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/ia32/config/aom_config.c
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/ia32/config/aom_config.h
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/x64/config/aom_config.asm
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/linux/x64/config/aom_config.h
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/win/ia32/config/aom_config.asm
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/win/ia32/config/aom_config.c
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/win/ia32/config/aom_config.h
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/win/x64/config/aom_config.asm
[modify] https://crrev.com/c26a4173cd6f0eb4762563603c6ff34ab7baa385/third_party/libaom/source/config/win/x64/config/aom_config.h

NextAction: 2018-08-01
Will request merge tomorrow.
For future reference, here is some info on the bug.

The following code in decoder_peek_si_internal() third_party/libaom/source/libaom/av1/av1_dx_iface.c has a bug:

 266     // skip past any unread OBU header data
 267     data = payload_start + payload_size;
 268     data_sz -= payload_size;
 269     if (data_sz <= 0) break;  // exit if we're out of OBUs
 270     status = aom_read_obu_header_and_size(
 271         data, data_sz, si->is_annexb, &obu_header, &payload_size, &bytes_read);

The relevant variables are data_sz and payload_size, both of which are of the size_t type.

Before line 268 is executed, we have:
  data_sz = 4358
  payload_size = 16006

After line 268 is executed, we have:
  data_sz = 18446744073709539968
  (long)data_sz = -11648

The fix is to check for the error condition data_sz < payload_size before executing the subtraction in line 268. Now that data_sz < payload_size is handled as an error, line 269 should check for data_sz == 0 instead.
Project Member

Comment 10 by ClusterFuzz, Aug 1

ClusterFuzz has detected this issue as fixed in range 579526:579532.

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

Fuzzer: libFuzzer_mediasource_MP4_AV1_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Pointer-overflow
Crash Address: 
Crash State:
  aom_read_obu_header_and_size
  decoder_peek_si_internal
  decode_one
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=578774:578804
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_ubsan&range=579526:579532

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

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 11 by ClusterFuzz, Aug 1

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

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
The NextAction date has arrived: 2018-08-01
Labels: Merge-Request-69
Requesting merge, as this has been in canary for nearly 24 hours.
Project Member

Comment 14 by sheriffbot@chromium.org, Aug 1

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
@Krishna: Wan-Teh wrote the bugfix, and explained the fix above:  https://crbug.com/868651#c9  

In short, the fix is checking for an error condition that wasn't checked before.
Without the fix, an unsigned integer underflow can occur, which can make a pointer invalid.

So, overall, the fix seems to be necessary and also safe.
Labels: -Merge-Review-69 Merge-Approved-69
Approving merge to M69 branch 3497 based on comment #15 and per https://bugs.chromium.org/p/chromium/issues/detail?id=868463#c23. Pls merge ASAP. Thank you.
Project Member

Comment 17 by bugdroid1@chromium.org, Aug 1

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ad7b7712b06c843e260bb3133ce1dce27ab08943

commit ad7b7712b06c843e260bb3133ce1dce27ab08943
Author: Urvang Joshi <urvang@google.com>
Date: Wed Aug 01 21:56:43 2018

Roll src/third_party/libaom/source/libaom/ e56f1db26..e96c7350a (1 commit)

https://aomedia.googlesource.com/aom.git/+log/e56f1db26bd5..e96c7350ab1e

$ git log e56f1db26..e96c7350a --date=short --no-merges --format='%ad %ae %s'
2018-07-30 wtc Validate payload_size in decoder_peek_si_internal.

Created with:
  roll-dep src/third_party/libaom/source/libaom
R=johannkoenig@google.com

Bug:  868651 
Change-Id: Iab844afb9d53df0cb9fe18c8c805ef859d550116
Reviewed-on: https://chromium-review.googlesource.com/1157192
Reviewed-by: Johann Koenig <johannkoenig@google.com>
Cr-Commit-Position: refs/branch-heads/3497@{#322}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/ad7b7712b06c843e260bb3133ce1dce27ab08943/DEPS
[modify] https://crrev.com/ad7b7712b06c843e260bb3133ce1dce27ab08943/third_party/libaom/README.chromium
[modify] https://crrev.com/ad7b7712b06c843e260bb3133ce1dce27ab08943/third_party/libaom/source/config/config/aom_version.h

Sign in to add a comment