| Message ID | 20231223025733.85366-1-leo.izen@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/jpegxl_parser: check ANS cluster alphabet size vs bundle size | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Fri, Dec 22, 2023 at 09:57:33PM -0500, Leo Izen wrote: > The specification doesn't mention that clusters cannot have alphabet > sizes greater than 1 << bundle->log_alphabet_size, but the reference > implementation rejects these entropy streams as invalid, so we should > too. Refusing to do so can overflow a stack variable on line 556 that > should be large enough otherwise. > > Fixes #10738. > > Reported-by: Michael Niedermayer <michael@niedermayer.cc> The issue has been discovered by Zeng Yunxiang and Li Zeyuan. as mentioned in the ticket > Signed-off-by: Leo Izen <leo.izen@gmail.com> > --- > libavcodec/jpegxl_parser.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/jpegxl_parser.c b/libavcodec/jpegxl_parser.c > index 006eb6b295..c9832e4393 100644 > --- a/libavcodec/jpegxl_parser.c > +++ b/libavcodec/jpegxl_parser.c > @@ -388,7 +388,6 @@ static int populate_distribution(GetBitContext *gb, JXLSymbolDistribution *dist, > > if (get_bits1(gb)) { > /* simple code */ > - dist->alphabet_size = 256; > if (get_bits1(gb)) { > uint8_t v1 = jxl_u8(gb); > uint8_t v2 = jxl_u8(gb); > @@ -398,10 +397,12 @@ static int populate_distribution(GetBitContext *gb, JXLSymbolDistribution *dist, > dist->freq[v2] = (1 << 12) - dist->freq[v1]; > if (!dist->freq[v1]) > dist->uniq_pos = v2; > + dist->alphabet_size = 1 + FFMAX(v1, v2); > } else { > uint8_t x = jxl_u8(gb); > dist->freq[x] = 1 << 12; > dist->uniq_pos = x; > + dist->alphabet_size= 1 + x; > } > return 0; > } > @@ -880,6 +881,8 @@ static int read_distribution_bundle(GetBitContext *gb, JXLEntropyDecoder *dec, > ret = populate_distribution(gb, &bundle->dists[i], bundle->log_alphabet_size); > if (ret < 0) > return ret; > + if (bundle->dists[i].alphabet_size > (1 << bundle->log_alphabet_size)) > + return AVERROR_INVALIDDATA; i think alphabet_size should be checked before it is stored in the struct or at least before it is used. ATM the value is unchecked and substantial processing is done with it in populate_distribution() before this check Also log_alphabet_size for use_prefix_code == 0 is limited to a max of 8 which limits alphabet_size to 256 in that codepath with the new check. There are also various arrays that can be reduced in size when alphabet_size is limited in this codepath. But thats for a different time and patch. For now i think just moving the alphabet_size check, is fine thx [...]
diff --git a/libavcodec/jpegxl_parser.c b/libavcodec/jpegxl_parser.c index 006eb6b295..c9832e4393 100644 --- a/libavcodec/jpegxl_parser.c +++ b/libavcodec/jpegxl_parser.c @@ -388,7 +388,6 @@ static int populate_distribution(GetBitContext *gb, JXLSymbolDistribution *dist, if (get_bits1(gb)) { /* simple code */ - dist->alphabet_size = 256; if (get_bits1(gb)) { uint8_t v1 = jxl_u8(gb); uint8_t v2 = jxl_u8(gb); @@ -398,10 +397,12 @@ static int populate_distribution(GetBitContext *gb, JXLSymbolDistribution *dist, dist->freq[v2] = (1 << 12) - dist->freq[v1]; if (!dist->freq[v1]) dist->uniq_pos = v2; + dist->alphabet_size = 1 + FFMAX(v1, v2); } else { uint8_t x = jxl_u8(gb); dist->freq[x] = 1 << 12; dist->uniq_pos = x; + dist->alphabet_size= 1 + x; } return 0; } @@ -880,6 +881,8 @@ static int read_distribution_bundle(GetBitContext *gb, JXLEntropyDecoder *dec, ret = populate_distribution(gb, &bundle->dists[i], bundle->log_alphabet_size); if (ret < 0) return ret; + if (bundle->dists[i].alphabet_size > (1 << bundle->log_alphabet_size)) + return AVERROR_INVALIDDATA; if (get_bits_left(gb) < 0) return AVERROR_BUFFER_TOO_SMALL; }
The specification doesn't mention that clusters cannot have alphabet sizes greater than 1 << bundle->log_alphabet_size, but the reference implementation rejects these entropy streams as invalid, so we should too. Refusing to do so can overflow a stack variable on line 556 that should be large enough otherwise. Fixes #10738. Reported-by: Michael Niedermayer <michael@niedermayer.cc> Signed-off-by: Leo Izen <leo.izen@gmail.com> --- libavcodec/jpegxl_parser.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)