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

Issue 603495 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug

Blocking:
issue 591845



Sign in to add a comment

Undefined-shift in ff_get_pcm_codec_id

Project Member Reported by ClusterFuzz, Apr 14 2016

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=4798839747248128

Fuzzer: libfuzzer_media_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Undefined-shift
Crash Address: 
Crash State:
  ff_get_pcm_codec_id
  ff_wav_codec_get_id
  ff_get_wav_header
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_ubsan&range=386932:386961

Minimized Testcase (0.04 Kb): https://cluster-fuzz.appspot.com/download/AMIfv95pd33-zlETWG7M-mJs3lnJlT9eq4lskh2nkrn3SccnK29nogDQHG544Wt16XMci2cZYtvdjUNGrVXe1WFlNxiPDRtBU2xjjHmbANQgTdewqp38foEJnVufYCF7P_2b-wLEkFPSvJcLT9SS_q4EjxMsiBbTpA

Filer: mmoroz

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 

Comment 1 by mmoroz@chromium.org, Apr 14 2016

Cc: kcc@chromium.org aizatsky@chromium.org
Owner: xhw...@chromium.org

Comment 2 by xhw...@chromium.org, Apr 14 2016

Cc: xhw...@chromium.org
Owner: wolenetz@chromium.org
Assign to wolenetz@ who is working on the FFmpeg roll. The fix probably should be upstreamed.
Blocking: 591845
Cc: chcunningham@chromium.org tguilbert@chromium.org
Cc: -tguilbert@chromium.org wolenetz@chromium.org
Labels: M-50
Owner: tguilbert@chromium.org
Status: Assigned (was: Available)
Sharding this one to tguilbert@. Thanks Thomas!

This one is probably also in M-50. Please check as part of fixing this.
Status: Started (was: Assigned)
This repros in a UBSan build (using ./configure --toolchain=clang-usan ...) using the latest ffplay.

It seems like this line might be the culprit:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg/libavformat/riffdec.c&sq=package:chromium&type=cs&l=66&rcl=1462461499

I do not know if there is a valid "reasonable" maximum bits per sample for the WAVE RIFF format.

I will send an email to Michael Niedermayer once I am done investigating 600959.
Project Member

Comment 7 by bugdroid1@chromium.org, May 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/77fdc79ab48570f5018cb4d4086909e8b7297743

commit 77fdc79ab48570f5018cb4d4086909e8b7297743
Author: Michael Niedermayer <michael@niedermayer.cc>
Date: Tue May 10 22:00:52 2016

avformat/utils: Check bps before using it in a shift in ff_get_pcm_codec_id()

We did not check that the bits per seconds received was a reasonable
number (64 bps or less). This caused undefined behavior when we
later attempted to left shift by "bits per second" (which ended up
shifting by more than 64 bits).

Fixes undefined shift
Fixes: usan_shift

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
(cherry picked from commit ea791c080dd5494b3bee0c618a3f52e371b5f320)

BUG= 603495 

Change-Id: Ice2b7a9b5c3dbd15d0d9806bf456885a25cbe190

[modify] https://crrev.com/77fdc79ab48570f5018cb4d4086909e8b7297743/libavformat/utils.c
[modify] https://crrev.com/77fdc79ab48570f5018cb4d4086909e8b7297743/chromium/patches/README

Project Member

Comment 8 by bugdroid1@chromium.org, May 12 2016

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

commit 79fb282a377bd92f2a679babae5c627a4f4f118a
Author: tguilbert <tguilbert@chromium.org>
Date: Thu May 12 03:21:23 2016

Roll src/third_party/ffmpeg/ 20d74768d..77fdc79ab (3 commits).

https://chromium.googlesource.com/chromium/third_party/ffmpeg.git/+log/20d74768dcd9..77fdc79ab485

$ git log 20d74768d..77fdc79ab --date=short --no-merges --format='%ad %ae %s'
2016-05-11 michael avformat/utils: Check bps before using it in a shift in ff_get_pcm_codec_id()
2016-05-09 chcunningham libavformat/oggdec: Free stream private when header parsing fails.
2016-05-10 michael avformat/oggparseopus: Check that granule pos is within the supported range

BUG= 600959 , 602185 , 603495 
R=wolenetz

Review-Url: https://codereview.chromium.org/1969993003
Cr-Commit-Position: refs/heads/master@{#393167}

[modify] https://crrev.com/79fb282a377bd92f2a679babae5c627a4f4f118a/DEPS

Labels: Merge-Request-51

Comment 10 by tin...@google.com, May 13 2016

Labels: -Merge-Request-51 Merge-Review-51 Hotlist-Merge-Review
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
Labels: -Merge-Review-51
Er, hold off on approving this one. The fix updates utils.c but not autorename_libavformat_utils.c, which is the file we generate and actually include to hack around mac toolchain badness. See  Issue 495833 . I'll land a fix and re-add the request label.
Project Member

Comment 12 by bugdroid1@chromium.org, May 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/0ba7f6830534bc2afbbe24200cc51602272fc706

commit 0ba7f6830534bc2afbbe24200cc51602272fc706
Author: Chris Cunningham <chcunningham@chromium.org>
Date: Fri May 13 21:06:15 2016

Update autorename_libavformat_utils.c to match utils.c

In a follow up cl I will update our generate_gyp.py to #include
the original file (pre-rename), avoiding this in the future.

BUG= 603495 , 495833 

Change-Id: Id0a531e39a7940d0ec69c3056cdf7a4846757188

[modify] https://crrev.com/0ba7f6830534bc2afbbe24200cc51602272fc706/libavformat/autorename_libavformat_utils.c

Project Member

Comment 13 by bugdroid1@chromium.org, May 14 2016

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

commit daf0b69488f463afc5dade7c391bb0beb9ff5776
Author: chcunningham <chcunningham@chromium.org>
Date: Sat May 14 00:37:29 2016

Roll src/third_party/ffmpeg 77fdc79:0ba7f68

Brings in change to (autorename_libavformat_)utils.c to fix bug.

Summary of changes available at:
https://chromium.googlesource.com/chromium/third_party/ffmpeg/+log/77fdc79..0ba7f68

BUG= 603495 

Review-Url: https://codereview.chromium.org/1974293003
Cr-Commit-Position: refs/heads/master@{#393707}

[modify] https://crrev.com/daf0b69488f463afc5dade7c391bb0beb9ff5776/DEPS

Project Member

Comment 14 by ClusterFuzz, May 14 2016

ClusterFuzz has detected this issue as fixed in range 393601:393724.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=4798839747248128

Fuzzer: libfuzzer_media_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Undefined-shift
Crash Address: 
Crash State:
  ff_get_pcm_codec_id
  ff_wav_codec_get_id
  ff_get_wav_header
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_ubsan&range=386932:386961
Fixed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_ubsan&range=393601:393724

Minimized Testcase (0.04 Kb): https://cluster-fuzz.appspot.com/download/AMIfv95pd33-zlETWG7M-mJs3lnJlT9eq4lskh2nkrn3SccnK29nogDQHG544Wt16XMci2cZYtvdjUNGrVXe1WFlNxiPDRtBU2xjjHmbANQgTdewqp38foEJnVufYCF7P_2b-wLEkFPSvJcLT9SS_q4EjxMsiBbTpA

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.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 15 by ClusterFuzz, May 15 2016

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5865324007653376

Fuzzer: libfuzzer_media_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Undefined-shift
Crash Address: 
Crash State:
  ff_get_pcm_codec_id
  ff_wav_codec_get_id
  ff_get_wav_header
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_ubsan&range=386932:386961

Minimized Testcase (0.03 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97KYcf0nYt8PEf6yAga4KFWSQDfBSGq5Ep9oztf8EtTgC_nfq76ouu_lmC9q24zeeGY0oRvfVwRjGqnEbh1IslZo79l3h0nyh8yRliKzdHChusFy5asP6v2vuabeAUC9YDY-WeJDXp3mBp9ThU3B_0XyOCzLQ

Filer: mmoroz

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reproducing.md for more information.
It seems the original undefined shift (too large) is fixed, but now we're hitting new undefined behavior with a shift that is negative. Will have a fix for this soon.
Project Member

Comment 17 by bugdroid1@chromium.org, May 20 2016

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

commit 1648fd90bce53f8a16514d6053d08289c2b50f9d
Author: chcunningham <chcunningham@chromium.org>
Date: Fri May 20 02:34:41 2016

Roll src/third_party/ffmpeg fa382f2:7f03319

Summary of changes available at:
https://chromium.googlesource.com/chromium/third_party/ffmpeg/+log/fa382f2..7f03319

Brings in security fix and basename collision fixes.
7f03319 avformat/utils: Check negative bps before shifting in ff_get_pcm_codec_id()
33e5416 Merge "Improve strategy for autorename_* basename collision hack."
1e4e65d Improve strategy for autorename_* basename collision hack.

BUG= 603495 
TBR=wolenetz@chromium.org

Review-Url: https://codereview.chromium.org/1994313002
Cr-Commit-Position: refs/heads/master@{#394962}

[modify] https://crrev.com/1648fd90bce53f8a16514d6053d08289c2b50f9d/DEPS

Project Member

Comment 18 by ClusterFuzz, May 20 2016

ClusterFuzz has detected this issue as fixed in range 394859:395005.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5865324007653376

Fuzzer: libfuzzer_media_pipeline_integration_fuzzer
Job Type: libfuzzer_chrome_ubsan
Platform Id: linux

Crash Type: Undefined-shift
Crash Address: 
Crash State:
  ff_get_pcm_codec_id
  ff_wav_codec_get_id
  ff_get_wav_header
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_ubsan&range=386932:386961
Fixed: https://cluster-fuzz.appspot.com/revisions?job=libfuzzer_chrome_ubsan&range=394859:395005

Minimized Testcase (0.03 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97KYcf0nYt8PEf6yAga4KFWSQDfBSGq5Ep9oztf8EtTgC_nfq76ouu_lmC9q24zeeGY0oRvfVwRjGqnEbh1IslZo79l3h0nyh8yRliKzdHChusFy5asP6v2vuabeAUC9YDY-WeJDXp3mBp9ThU3B_0XyOCzLQ

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

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Cc: mbarbe...@chromium.org
I had a chat to mbarbella@ about this. There's not a strong security argument to merge this into M51, so from a security standpoint this can go with M52.
Labels: Merge-Request-52

Comment 21 by tin...@google.com, May 24 2016

Labels: -Merge-Request-52 Merge-Review-52
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
Cc: -kcc@chromium.org tinazh@chromium.org
Hi Tina, ping for merge review.
Cc: kcc@chromium.org
(sorry, didn't mean to remove cc)
Before we approve merge to M52, Could you please confirm whether this change is baked/verified in Canary and safe to merge?
Yes, this change has been in Canary for over a week. Its a very low risk change.
Labels: -Merge-Review-52 Merge-Approved-52
Approving merge to M52 branch 2743 based on comment #25. Please merge ASAP. Thank you.
Project Member

Comment 27 by bugdroid1@chromium.org, Jun 7 2016

Labels: -merge-approved-52 merge-merged-2743
The following revision refers to this bug:
  http://goto.ext.google.com/viewvc/chrome-internal?view=rev&revision=88663

------------------------------------------------------------------
r88663 | chcunningham@google.com | 2016-06-07T23:45:31.695277Z

-----------------------------------------------------------------
Status: Fixed (was: Started)
Project Member

Comment 29 by sheriffbot@chromium.org, Nov 22 2016

Labels: -Restrict-View-EditIssue
Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment