Re: [FFmpeg-devel] [PATCH] avcodec/jpegxl_parser: check ANS cluster alphabet size vs bundle size

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 2911 bytes --]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No great genius has ever existed without some touch of madness. -- Aristotle

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".