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

[Steinar H. Gunderson]

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 */
-- 
Homepage: https://www.sesse.net/