[FFmpeg-devel,3/4] avcodec/vorbisdec: Check vlc for floor0 dec vector offset

Submitted by Michael Niedermayer on July 7, 2019, 11:18 p.m.

Details

Message ID 20190707231805.29267-3-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer July 7, 2019, 11:18 p.m.
Fixes: out of array access
Fixes: 15649/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VORBIS_fuzzer-5729191309344768

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/vorbisdec.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Michael Niedermayer July 21, 2019, 9:25 a.m.
On Mon, Jul 08, 2019 at 01:18:04AM +0200, Michael Niedermayer wrote:
> Fixes: out of array access
> Fixes: 15649/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VORBIS_fuzzer-5729191309344768
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/vorbisdec.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

will apply

[...]
Paul B Mahol July 21, 2019, 9:27 a.m.
On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Mon, Jul 08, 2019 at 01:18:04AM +0200, Michael Niedermayer wrote:
>> Fixes: out of array access
>> Fixes:
>> 15649/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VORBIS_fuzzer-5729191309344768
>>
>> Found-by: continuous fuzzing process
>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>>  libavcodec/vorbisdec.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> will apply

Are you sure returning that value is correct approach?

Correct value to return is usually INVALIDDATA.

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The educated differ from the uneducated as much as the living from the
> dead. -- Aristotle
>
Michael Niedermayer July 21, 2019, 10:33 a.m.
On Sun, Jul 21, 2019 at 11:27:44AM +0200, Paul B Mahol wrote:
> On 7/21/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > On Mon, Jul 08, 2019 at 01:18:04AM +0200, Michael Niedermayer wrote:
> >> Fixes: out of array access
> >> Fixes:
> >> 15649/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VORBIS_fuzzer-5729191309344768
> >>
> >> Found-by: continuous fuzzing process
> >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> ---
> >>  libavcodec/vorbisdec.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > will apply
> 
> Are you sure returning that value is correct approach?
> 
> Correct value to return is usually INVALIDDATA.

the only call of this function i see does not use the value if it is
negative and uses INVALIDDATA instead.

        ret = floor->decode(vc, &floor->data, floor_ptr[i]);

        if (ret < 0) {
            av_log(vc->avctx, AV_LOG_ERROR, "Invalid codebook in vorbis_floor_decode.\n");
            return AVERROR_INVALIDDATA;
        }

but ill replace the code in this patch by an explicit AVERROR_INVALIDDATA
before pushing

Thanks
        
[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
index 7f2d27d1a2..d6c6a56e0c 100644
--- a/libavcodec/vorbisdec.c
+++ b/libavcodec/vorbisdec.c
@@ -1128,8 +1128,10 @@  static int vorbis_floor0_decode(vorbis_context *vc,
             ff_dlog(NULL, "floor0 dec: maximum depth: %d\n", codebook.maxdepth);
             /* read temp vector */
             vec_off = get_vlc2(&vc->gb, codebook.vlc.table,
-                               codebook.nb_bits, codebook.maxdepth)
-                      * codebook.dimensions;
+                               codebook.nb_bits, codebook.maxdepth);
+            if (vec_off < 0)
+                return vec_off;
+            vec_off *= codebook.dimensions;
             ff_dlog(NULL, "floor0 dec: vector offset: %d\n", vec_off);
             /* copy each vector component and add last to it */
             for (idx = 0; idx < codebook.dimensions; ++idx)