| Message ID | 7b5404a4-fae1-44b0-b97c-6c249ea03280@googlemail.com |
|---|---|
| State | New |
| Headers | show |
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 */
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
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 */
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
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 */
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
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 */
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 [...]
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);
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(-)