[FFmpeg-devel] speedhq: make sure the block index is not negative

Submitted by Andreas Cadhalpun on Jan. 30, 2017, 1:31 a.m.

Details

Message ID 7b5404a4-fae1-44b0-b97c-6c249ea03280@googlemail.com
State New
Headers show

Commit Message

Andreas Cadhalpun Jan. 30, 2017, 1:31 a.m.
Fixes out-of-bounds writes.

Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavcodec/speedhq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Steinar H. Gunderson Jan. 30, 2017, 8:23 a.m.
On Mon, Jan 30, 2017 at 02:31:08AM +0100, Andreas Cadhalpun wrote:
> Fixes out-of-bounds writes.

Hi,

How can you get a negative run, which would be required for this to happen?

/* Steinar */
Andreas Cadhalpun Jan. 30, 2017, 11:49 p.m.
On 30.01.2017 09:23, Steinar H. Gunderson wrote:
> How can you get a negative run, which would be required for this to happen?

Some values in ff_dc_alpha_run_vlc_le.table are negative, e.g.:
 ff_dc_alpha_run_vlc_le.table[32] = {-1, 0}

Best regards,
Andreas
Steinar H. Gunderson Jan. 30, 2017, 11:59 p.m.
On Tue, Jan 31, 2017 at 12:49:56AM +0100, Andreas Cadhalpun wrote:
>> How can you get a negative run, which would be required for this to happen?
> Some values in ff_dc_alpha_run_vlc_le.table are negative, e.g.:
>  ff_dc_alpha_run_vlc_le.table[32] = {-1, 0}

This sounds like a strangeness in constructing the table, which shouldn't be
papered over in the inner loop of the decoder. I might be missing something
in how this table is used internally in the bitstream reader, but a code of
32 should just immediately hit 0 in the LSB and be interpreted as run=0.
(There are no invalid codes in this VLC, so nothing like run=-1 should ever
happen.)

Do you have an actual input where your code makes a difference?

/* Steinar */
Andreas Cadhalpun Jan. 31, 2017, 12:57 a.m.
On 31.01.2017 00:59, Steinar H. Gunderson wrote:
> On Tue, Jan 31, 2017 at 12:49:56AM +0100, Andreas Cadhalpun wrote:
>>> How can you get a negative run, which would be required for this to happen?
>> Some values in ff_dc_alpha_run_vlc_le.table are negative, e.g.:
>>  ff_dc_alpha_run_vlc_le.table[32] = {-1, 0}
> 
> This sounds like a strangeness in constructing the table, which shouldn't be
> papered over in the inner loop of the decoder.

Maybe, I don't know what the contents of the table should be, but the following
are {-1, 0}: 32, 33, 64, 96, 128

> I might be missing something
> in how this table is used internally in the bitstream reader, but a code of
> 32 should just immediately hit 0 in the LSB and be interpreted as run=0.
> (There are no invalid codes in this VLC, so nothing like run=-1 should ever
> happen.)
> 
> Do you have an actual input where your code makes a difference?

Yes, without this patch ubsan reports:
src/libavcodec/speedhq.c:206:13: runtime error: index -1 out of bounds for type 'uint8_t [128]'

Best regards,
Andreas
Steinar H. Gunderson Jan. 31, 2017, 8:43 a.m.
On Tue, Jan 31, 2017 at 01:57:31AM +0100, Andreas Cadhalpun wrote:
>> This sounds like a strangeness in constructing the table, which shouldn't be
>> papered over in the inner loop of the decoder.
> Maybe, I don't know what the contents of the table should be, but the following
> are {-1, 0}: 32, 33, 64, 96, 128

Seemingly they are, indeed.

>> Do you have an actual input where your code makes a difference?
> Yes, without this patch ubsan reports:
> src/libavcodec/speedhq.c:206:13: runtime error: index -1 out of bounds for type 'uint8_t [128]'

Would you mind sharing an input where this actually triggers? None of the
samples I have seem to trigger this, so I suppose it's some sort of fuzzed
input.

/* Steinar */
Andreas Cadhalpun Feb. 1, 2017, 1:17 a.m.
On 31.01.2017 09:43, Steinar H. Gunderson wrote:
> On Tue, Jan 31, 2017 at 01:57:31AM +0100, Andreas Cadhalpun wrote:
>>> This sounds like a strangeness in constructing the table, which shouldn't be
>>> papered over in the inner loop of the decoder.
>> Maybe, I don't know what the contents of the table should be, but the following
>> are {-1, 0}: 32, 33, 64, 96, 128
> 
> Seemingly they are, indeed.
> 
>>> Do you have an actual input where your code makes a difference?
>> Yes, without this patch ubsan reports:
>> src/libavcodec/speedhq.c:206:13: runtime error: index -1 out of bounds for type 'uint8_t [128]'
> 
> Would you mind sharing an input where this actually triggers? None of the
> samples I have seem to trigger this, so I suppose it's some sort of fuzzed
> input.

Indeed it is. I've sent you a sample.

Best regards,
Andreas
Steinar H. Gunderson Feb. 1, 2017, 8:56 a.m.
On Wed, Feb 01, 2017 at 02:17:05AM +0100, Andreas Cadhalpun wrote:
>> Would you mind sharing an input where this actually triggers? None of the
>> samples I have seem to trigger this, so I suppose it's some sort of fuzzed
>> input.
> Indeed it is. I've sent you a sample.

Thanks; I see what is happening now (and I should have fuzzed SHQ1 too, not
just SHQ0 :-) ).

The relevant part is the construction of the (little-endian) alpha VLC:

        if (!run) {
            /* 0 -> 0. */
            run_code[run] = 0;
            run_bits[run] = 1;
        } else if (run <= 4) {
            /* 10xx -> xx plus 1. */
            run_code[run] = ((run - 1) << 2) | 1;
            run_bits[run] = 4;
        } else {
            /* 111xxxxxxx -> xxxxxxx. */
            run_code[run] = (run << 3) | 7;
            run_bits[run] = 10;
        }

The sample in question encodes 1110000000, which is a legal code for 0,
but we haven't told the VLC this (since simply 0 is a much more logical
way of doing it), so it returns -1 (signaling invalid). We will see the same
problem in level_code/level_bits (a few lines further down), but it's not
used for indexing, so it's not a crash issue.

My preference would be to simply decode this as 0 instead of returning;
it would be both the safest and the fastest. Is there a way we can do this?
Failing that, I would suppose the best fix is

-           if (run == 128) break;
+           if (run < 0 || run >= 128) break;

treating these as EOB codes (with no performance penalty, as it becomes an
unsigned compare -- as in your patch). The reason I'm not too keen on putting
this on the check for i is that if we hit end of stream and read the same
code over and over again (due to the checked bitstream reader), an attacker
_might_ be able to construct a file where run=-1 infinitely and we go into an
infinite loop.

Optionally we can just do

            if (run < 0) return AVERROR_INVALIDDATA;

because I don't really think these formats are speed critical :-)

/* Steinar */
Michael Niedermayer Feb. 1, 2017, 9:53 a.m.
On Wed, Feb 01, 2017 at 09:56:41AM +0100, Steinar H. Gunderson wrote:
> On Wed, Feb 01, 2017 at 02:17:05AM +0100, Andreas Cadhalpun wrote:
> >> Would you mind sharing an input where this actually triggers? None of the
> >> samples I have seem to trigger this, so I suppose it's some sort of fuzzed
> >> input.
> > Indeed it is. I've sent you a sample.
> 
> Thanks; I see what is happening now (and I should have fuzzed SHQ1 too, not
> just SHQ0 :-) ).
> 
> The relevant part is the construction of the (little-endian) alpha VLC:
> 
>         if (!run) {
>             /* 0 -> 0. */
>             run_code[run] = 0;
>             run_bits[run] = 1;
>         } else if (run <= 4) {
>             /* 10xx -> xx plus 1. */
>             run_code[run] = ((run - 1) << 2) | 1;
>             run_bits[run] = 4;
>         } else {
>             /* 111xxxxxxx -> xxxxxxx. */
>             run_code[run] = (run << 3) | 7;
>             run_bits[run] = 10;
>         }
> 
> The sample in question encodes 1110000000, which is a legal code for 0,
> but we haven't told the VLC this (since simply 0 is a much more logical
> way of doing it), so it returns -1 (signaling invalid). We will see the same
> problem in level_code/level_bits (a few lines further down), but it's not
> used for indexing, so it's not a crash issue.
> 

> My preference would be to simply decode this as 0 instead of returning;
> it would be both the safest and the fastest. Is there a way we can do this?

yes

also please check if there are more "holes" in the VLC table
(this is easy to check by summing the ranges covered by codes ...
 which  would sum to 1 if there are no holes)

ff_init_vlc_sparse()
we seem to be missing a INIT_*VLC_STATIC for that but that should be
trivial to add

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/speedhq.c b/libavcodec/speedhq.c
index 385f779f83..6ae1e0f8df 100644
--- a/libavcodec/speedhq.c
+++ b/libavcodec/speedhq.c
@@ -198,7 +198,7 @@  static inline int decode_alpha_block(const SHQContext *s, GetBitContext *gb, uin
 
             if (run == 128) break;
             i += run;
-            if (i >= 128)
+            if (i < 0 || i >= 128)
                 return AVERROR_INVALIDDATA;
 
             UPDATE_CACHE_LE(re, gb);