Bug 159238 (CVE-2016-4767) - Throw exceptions for invalid number of channels for ConvolverNode
Summary: Throw exceptions for invalid number of channels for ConvolverNode
Status: RESOLVED FIXED
Alias: CVE-2016-4767
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 159232
Blocks:
  Show dependency treegraph
 
Reported: 2016-06-28 17:17 PDT by David Kilzer (:ddkilzer)
Modified: 2017-10-11 10:29 PDT (History)
10 users (show)

See Also:


Attachments
Patch v1 (9.68 KB, patch)
2016-06-28 17:41 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch (10.61 KB, patch)
2016-07-05 21:06 PDT, Brent Fulgham
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2016-06-28 17:17:35 PDT
The ConvolverNode only supports 1, 2 or 4 channels and must throw an exception for any other number of channels.
Comment 1 David Kilzer (:ddkilzer) 2016-06-28 17:19:18 PDT
This causes a heap-buffer-overflow read in com.apple.WebKit.WebContent at com.apple.WebCore: WebCore::AudioArray<float>::copyToRange + 76 under ASan.

Application Specific Information:
================================================================
==30955==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200030a3f4 at pc 0x00010e94734a bp 0x7fff5360ed70 sp 0x7fff5360e528
READ of size 256 at 0x60200030a3f4 thread T0
    #0 0x10e947349 in __asan_memcpy (/Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.11.xctoolchain/usr/lib/clang/7.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib+0x39349)
    #1 0x11398935b in WebCore::AudioArray<float>::copyToRange(float const*, unsigned int, unsigned int) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x97f35b)
    #2 0x11503b0bf in WebCore::ReverbConvolverStage::ReverbConvolverStage(float const*, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, WebCore::ReverbAccumulationBuffer*, bool) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x20310bf)
    #3 0x11503b4b8 in WebCore::ReverbConvolverStage::ReverbConvolverStage(float const*, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, WebCore::ReverbAccumulationBuffer*, bool) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x20314b8)
    #4 0x11503921a in WebCore::ReverbConvolver::ReverbConvolver(WebCore::AudioChannel*, unsigned long, unsigned long, unsigned long, bool) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x202f21a)
    #5 0x115037bba in WebCore::Reverb::initialize(WebCore::AudioBus*, unsigned long, unsigned long, unsigned long, bool) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x202dbba)
    #6 0x1150376cc in WebCore::Reverb::Reverb(WebCore::AudioBus*, unsigned long, unsigned long, unsigned long, bool, bool) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x202d6cc)
    #7 0x115037d57 in WebCore::Reverb::Reverb(WebCore::AudioBus*, unsigned long, unsigned long, unsigned long, bool, bool) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x202dd57)
    #8 0x1133ca260 in WebCore::ConvolverNode::setBuffer(WebCore::AudioBuffer*, int&) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x3c0260)
    #9 0x114073996 in WebCore::setJSConvolverNodeBuffer(JSC::ExecState*, long long, long long) (/Users/local/OpenSource/WebKitBuild/Release/WebCore.framework/Versions/A/WebCore+0x1069996)
    #10 0x110359368 in JSC::callCustomSetter(JSC::ExecState*, bool (*)(JSC::ExecState*, long long, long long), bool, JSC::JSValue, JSC::JSValue) (/Users/local/OpenSource/WebKitBuild/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x574368)
[...]
Comment 2 David Kilzer (:ddkilzer) 2016-06-28 17:20:06 PDT
<rdar://problem/27020410>
Comment 3 David Kilzer (:ddkilzer) 2016-06-28 17:41:26 PDT
Created attachment 282306 [details]
Patch v1
Comment 4 David Kilzer (:ddkilzer) 2016-06-28 17:42:31 PDT
Please note that the patch for Bug 159232 needs to land before this one since it adds support to use anonymous functions and descriptive messages with shouldNotThrow() and shouldThrow().
Comment 5 Brent Fulgham 2016-06-28 17:54:41 PDT
Comment on attachment 282306 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=282306&action=review

Looks good. You'll need to land this manually, since the commit-queue doesn't process security bugs.

> Source/WebCore/ChangeLog:4
> +        <https://webkit.org/b/159238>

<rdar://problem/27020410>

> LayoutTests/ChangeLog:4
> +        <https://webkit.org/b/159238>

<rdar://problem/27020410>

> LayoutTests/webaudio/convolver-channels.html:18
> +    for (var count = 1; count <= 32; ++count) {

Just curious: Is 32 an arbitrary value? Or do we have Audio elements that use 32 channels?
Comment 6 David Kilzer (:ddkilzer) 2016-06-29 02:40:48 PDT
Comment on attachment 282306 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=282306&action=review

>> LayoutTests/webaudio/convolver-channels.html:18
>> +    for (var count = 1; count <= 32; ++count) {
> 
> Just curious: Is 32 an arbitrary value? Or do we have Audio elements that use 32 channels?

I suspect it was chosen as a practical limit for the test.
Comment 7 David Kilzer (:ddkilzer) 2016-06-29 03:20:31 PDT
Committed r202617: <http://trac.webkit.org/changeset/202617>
Comment 8 Eric Carlson 2016-06-29 07:52:14 PDT
Comment on attachment 282306 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=282306&action=review

> Source/WebCore/Modules/webaudio/ConvolverNode.cpp:137
> +    bool isChannelCountGood = numberOfChannels == 1 || numberOfChannels == 2 || numberOfChannels == 4;

Is it safe to drop the bufferLength check?
Comment 9 David Kilzer (:ddkilzer) 2016-06-29 13:16:26 PDT
This will be rolled out as the fix wasn't complete.
Comment 10 Ryan Haddad 2016-06-29 13:18:08 PDT
Reverted r202617 for reason:

The LayoutTest from this change crashes under GuardMalloc

Committed r202645: <http://trac.webkit.org/changeset/202645>
Comment 11 Brent Fulgham 2016-07-05 21:06:40 PDT
Created attachment 282848 [details]
Patch
Comment 12 Brent Fulgham 2016-07-05 21:08:09 PDT
Comment on attachment 282848 [details]
Patch

Re-landing after finding the corresponding Blink change to correct the buffer overrun found during our testing.
Comment 13 Brent Fulgham 2016-07-05 21:12:20 PDT
Committed r202845: <http://trac.webkit.org/changeset/202845>