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/
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
