[FFmpeg-devel] [PATCH v2] avformat/jpegxl_anim_dec: avoid overrun with jxlp boxes in container

[FFmpeg-devel] [PATCH v2] avformat/jpegxl_anim_dec: avoid overrun with jxlp boxes in container

[Leo Izen]

This switches the jpegxl_collect_codestream_header function to use
avcodec/bytestream2, which better enforces barriers, and should avoid
overrunning buffers with jxlp boxes if the size is zero or if the size
is so small the box is invalid.

Signed-off-by: Leo Izen <leo.izen@gmail.com>
---
 libavformat/jpegxl_anim_dec.c | 56 +++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/libavformat/jpegxl_anim_dec.c b/libavformat/jpegxl_anim_dec.c
index 6ea6c46d8f..ec400c955c 100644
--- a/libavformat/jpegxl_anim_dec.c
+++ b/libavformat/jpegxl_anim_dec.c
@@ -28,6 +28,7 @@
 #include <stdint.h>
 #include <string.h>
 
+#include "libavcodec/bytestream.h"
 #define BITSTREAM_READER_LE
 #include "libavcodec/get_bits.h"
 
@@ -48,62 +49,65 @@ typedef struct JXLAnimDemuxContext {
  * returns the number of bytes consumed from input, may be greater than input_len
  * if the input doesn't end on an ISOBMFF-box boundary
  */
-static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len, uint8_t *buffer, int buflen, int *copied) {
-    const uint8_t *b = input_buffer;
+static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len,
+                                            uint8_t *buffer, int buflen, int *copied) {
+    GetByteContext gb;
     *copied = 0;
+    bytestream2_init(&gb, input_buffer, input_len);
 
     while (1) {
         uint64_t size;
         uint32_t tag;
         int head_size = 8;
 
-        if (b - input_buffer >= input_len - 16)
+        if (bytestream2_get_bytes_left(&gb) < 16)
             break;
 
-        size = AV_RB32(b);
-        b += 4;
+        size = bytestream2_get_be32(&gb);
         if (size == 1) {
-            size = AV_RB64(b);
-            b += 8;
+            size = bytestream2_get_be64(&gb);
             head_size = 16;
         }
         /* invalid ISOBMFF size */
-        if (size > 0 && size <= head_size)
+        if (size && size <= head_size)
             return AVERROR_INVALIDDATA;
-        if (size > 0)
+        if (size)
             size -= head_size;
 
-        tag = AV_RL32(b);
-        b += 4;
+        tag = bytestream2_get_le32(&gb);
         if (tag == MKTAG('j', 'x', 'l', 'p')) {
-            b += 4;
-            size -= 4;
+            if (bytestream2_get_bytes_left(&gb) < 4)
+                break;
+            bytestream2_skip(&gb, 4);
+            if (size) {
+                if (size <= 4)
+                    return AVERROR_INVALIDDATA;
+                size -= 4;
+            }
         }
+        /*
+         * size = 0 means "until EOF". this is legal but uncommon
+         * here we just set it to the remaining size of the probe buffer
+         */
+        if (!size)
+            size = bytestream2_get_bytes_left(&gb);
 
         if (tag == MKTAG('j', 'x', 'l', 'c') || tag == MKTAG('j', 'x', 'l', 'p')) {
-            /*
-             * size = 0 means "until EOF". this is legal but uncommon
-             * here we just set it to the remaining size of the probe buffer
-             * which at this point should always be nonnegative
-             */
-            if (size == 0 || size > input_len - (b - input_buffer))
-                size = input_len - (b - input_buffer);
-
             if (size > buflen - *copied)
                 size = buflen - *copied;
             /*
              * arbitrary chunking of the payload makes this memcpy hard to avoid
              * in practice this will only be performed one or two times at most
              */
-            memcpy(buffer + *copied, b, size);
-            *copied += size;
+            *copied += bytestream2_get_buffer(&gb, buffer + *copied, size);
+        } else {
+            bytestream2_skip(&gb, size);
         }
-        b += size;
-        if (b >= input_buffer + input_len || *copied >= buflen)
+        if (bytestream2_get_bytes_left(&gb) <= 0 || *copied >= buflen)
             break;
     }
 
-    return b - input_buffer;
+    return bytestream2_tell(&gb);
 }
 
 static int jpegxl_anim_probe(const AVProbeData *p)
-- 
2.41.0

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avformat/jpegxl_anim_dec: avoid overrun with jxlp boxes in container

[Leo Izen]

On 6/12/23 09:22, Leo Izen wrote:
> This switches the jpegxl_collect_codestream_header function to use
> avcodec/bytestream2, which better enforces barriers, and should avoid
> overrunning buffers with jxlp boxes if the size is zero or if the size
> is so small the box is invalid.
> 
> Signed-off-by: Leo Izen <leo.izen@gmail.com>
> ---
>   libavformat/jpegxl_anim_dec.c | 56 +++++++++++++++++++----------------
>   1 file changed, 30 insertions(+), 26 deletions(-)

Will push soon if there's no objections, as this fixes a bug.

- Leo Izen

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avformat/jpegxl_anim_dec: avoid overrun with jxlp boxes in container

[Leo Izen]

On 6/16/23 14:35, Leo Izen wrote:
> On 6/12/23 09:22, Leo Izen wrote:
>> This switches the jpegxl_collect_codestream_header function to use
>> avcodec/bytestream2, which better enforces barriers, and should avoid
>> overrunning buffers with jxlp boxes if the size is zero or if the size
>> is so small the box is invalid.
>>
>> Signed-off-by: Leo Izen <leo.izen@gmail.com>
>> ---
>>   libavformat/jpegxl_anim_dec.c | 56 +++++++++++++++++++----------------
>>   1 file changed, 30 insertions(+), 26 deletions(-)
> 
> Will push soon if there's no objections, as this fixes a bug.
> 
> - Leo Izen
> 

Pushed as 61047f0f82c76620cc42888958dd9834bcaa18fb.

- Leo Izen (Traneptora)
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avformat/jpegxl_anim_dec: avoid overrun with jxlp boxes in container

[Andreas Rheinhardt]

Leo Izen:
> This switches the jpegxl_collect_codestream_header function to use
> avcodec/bytestream2, which better enforces barriers, and should avoid
> overrunning buffers with jxlp boxes if the size is zero or if the size
> is so small the box is invalid.
> 
> Signed-off-by: Leo Izen <leo.izen@gmail.com>
> ---
>  libavformat/jpegxl_anim_dec.c | 56 +++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 26 deletions(-)
> 
> diff --git a/libavformat/jpegxl_anim_dec.c b/libavformat/jpegxl_anim_dec.c
> index 6ea6c46d8f..ec400c955c 100644
> --- a/libavformat/jpegxl_anim_dec.c
> +++ b/libavformat/jpegxl_anim_dec.c
> @@ -28,6 +28,7 @@
>  #include <stdint.h>
>  #include <string.h>
>  
> +#include "libavcodec/bytestream.h"
>  #define BITSTREAM_READER_LE
>  #include "libavcodec/get_bits.h"
>  
> @@ -48,62 +49,65 @@ typedef struct JXLAnimDemuxContext {
>   * returns the number of bytes consumed from input, may be greater than input_len
>   * if the input doesn't end on an ISOBMFF-box boundary
>   */
> -static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len, uint8_t *buffer, int buflen, int *copied) {
> -    const uint8_t *b = input_buffer;
> +static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len,
> +                                            uint8_t *buffer, int buflen, int *copied) {
> +    GetByteContext gb;
>      *copied = 0;
> +    bytestream2_init(&gb, input_buffer, input_len);
>  
>      while (1) {
>          uint64_t size;
>          uint32_t tag;
>          int head_size = 8;
>  
> -        if (b - input_buffer >= input_len - 16)
> +        if (bytestream2_get_bytes_left(&gb) < 16)
>              break;
>  
> -        size = AV_RB32(b);
> -        b += 4;
> +        size = bytestream2_get_be32(&gb);
>          if (size == 1) {
> -            size = AV_RB64(b);
> -            b += 8;
> +            size = bytestream2_get_be64(&gb);
>              head_size = 16;
>          }
>          /* invalid ISOBMFF size */
> -        if (size > 0 && size <= head_size)
> +        if (size && size <= head_size)
>              return AVERROR_INVALIDDATA;
> -        if (size > 0)
> +        if (size)
>              size -= head_size;
>  
> -        tag = AV_RL32(b);
> -        b += 4;
> +        tag = bytestream2_get_le32(&gb);
>          if (tag == MKTAG('j', 'x', 'l', 'p')) {
> -            b += 4;
> -            size -= 4;
> +            if (bytestream2_get_bytes_left(&gb) < 4)
> +                break;
> +            bytestream2_skip(&gb, 4);
> +            if (size) {
> +                if (size <= 4)
> +                    return AVERROR_INVALIDDATA;
> +                size -= 4;
> +            }
>          }
> +        /*
> +         * size = 0 means "until EOF". this is legal but uncommon
> +         * here we just set it to the remaining size of the probe buffer
> +         */
> +        if (!size)
> +            size = bytestream2_get_bytes_left(&gb);
>  
>          if (tag == MKTAG('j', 'x', 'l', 'c') || tag == MKTAG('j', 'x', 'l', 'p')) {
> -            /*
> -             * size = 0 means "until EOF". this is legal but uncommon
> -             * here we just set it to the remaining size of the probe buffer
> -             * which at this point should always be nonnegative
> -             */
> -            if (size == 0 || size > input_len - (b - input_buffer))
> -                size = input_len - (b - input_buffer);
> -
>              if (size > buflen - *copied)
>                  size = buflen - *copied;
>              /*
>               * arbitrary chunking of the payload makes this memcpy hard to avoid
>               * in practice this will only be performed one or two times at most
>               */
> -            memcpy(buffer + *copied, b, size);
> -            *copied += size;
> +            *copied += bytestream2_get_buffer(&gb, buffer + *copied, size);
> +        } else {
> +            bytestream2_skip(&gb, size);
>          }
> -        b += size;
> -        if (b >= input_buffer + input_len || *copied >= buflen)
> +        if (bytestream2_get_bytes_left(&gb) <= 0 || *copied >= buflen)
>              break;
>      }
>  
> -    return b - input_buffer;
> +    return bytestream2_tell(&gb);
>  }
>  
>  static int jpegxl_anim_probe(const AVProbeData *p)

Is there an actual (potential) overrun or is this just a precaution?

- Andreas

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v2] avformat/jpegxl_anim_dec: avoid overrun with jxlp boxes in container

[Leo Izen]

On 6/16/23 15:00, Andreas Rheinhardt wrote:
> Leo Izen:
>> This switches the jpegxl_collect_codestream_header function to use
>> avcodec/bytestream2, which better enforces barriers, and should avoid
>> overrunning buffers with jxlp boxes if the size is zero or if the size
>> is so small the box is invalid.
>>
>> Signed-off-by: Leo Izen <leo.izen@gmail.com>
>> ---
>>   libavformat/jpegxl_anim_dec.c | 56 +++++++++++++++++++----------------
>>   1 file changed, 30 insertions(+), 26 deletions(-)
>>
>> diff --git a/libavformat/jpegxl_anim_dec.c b/libavformat/jpegxl_anim_dec.c
>> index 6ea6c46d8f..ec400c955c 100644
>> --- a/libavformat/jpegxl_anim_dec.c
>> +++ b/libavformat/jpegxl_anim_dec.c
>> @@ -28,6 +28,7 @@
>>   #include <stdint.h>
>>   #include <string.h>
>>   
>> +#include "libavcodec/bytestream.h"
>>   #define BITSTREAM_READER_LE
>>   #include "libavcodec/get_bits.h"
>>   
>> @@ -48,62 +49,65 @@ typedef struct JXLAnimDemuxContext {
>>    * returns the number of bytes consumed from input, may be greater than input_len
>>    * if the input doesn't end on an ISOBMFF-box boundary
>>    */
>> -static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len, uint8_t *buffer, int buflen, int *copied) {
>> -    const uint8_t *b = input_buffer;
>> +static int jpegxl_collect_codestream_header(const uint8_t *input_buffer, int input_len,
>> +                                            uint8_t *buffer, int buflen, int *copied) {
>> +    GetByteContext gb;
>>       *copied = 0;
>> +    bytestream2_init(&gb, input_buffer, input_len);
>>   
>>       while (1) {
>>           uint64_t size;
>>           uint32_t tag;
>>           int head_size = 8;
>>   
>> -        if (b - input_buffer >= input_len - 16)
>> +        if (bytestream2_get_bytes_left(&gb) < 16)
>>               break;
>>   
>> -        size = AV_RB32(b);
>> -        b += 4;
>> +        size = bytestream2_get_be32(&gb);
>>           if (size == 1) {
>> -            size = AV_RB64(b);
>> -            b += 8;
>> +            size = bytestream2_get_be64(&gb);
>>               head_size = 16;
>>           }
>>           /* invalid ISOBMFF size */
>> -        if (size > 0 && size <= head_size)
>> +        if (size && size <= head_size)
>>               return AVERROR_INVALIDDATA;
>> -        if (size > 0)
>> +        if (size)
>>               size -= head_size;
>>   
>> -        tag = AV_RL32(b);
>> -        b += 4;
>> +        tag = bytestream2_get_le32(&gb);
>>           if (tag == MKTAG('j', 'x', 'l', 'p')) {
>> -            b += 4;
>> -            size -= 4;
>> +            if (bytestream2_get_bytes_left(&gb) < 4)
>> +                break;
>> +            bytestream2_skip(&gb, 4);
>> +            if (size) {
>> +                if (size <= 4)
>> +                    return AVERROR_INVALIDDATA;
>> +                size -= 4;
>> +            }
>>           }
>> +        /*
>> +         * size = 0 means "until EOF". this is legal but uncommon
>> +         * here we just set it to the remaining size of the probe buffer
>> +         */
>> +        if (!size)
>> +            size = bytestream2_get_bytes_left(&gb);
>>   
>>           if (tag == MKTAG('j', 'x', 'l', 'c') || tag == MKTAG('j', 'x', 'l', 'p')) {
>> -            /*
>> -             * size = 0 means "until EOF". this is legal but uncommon
>> -             * here we just set it to the remaining size of the probe buffer
>> -             * which at this point should always be nonnegative
>> -             */
>> -            if (size == 0 || size > input_len - (b - input_buffer))
>> -                size = input_len - (b - input_buffer);
>> -
>>               if (size > buflen - *copied)
>>                   size = buflen - *copied;
>>               /*
>>                * arbitrary chunking of the payload makes this memcpy hard to avoid
>>                * in practice this will only be performed one or two times at most
>>                */
>> -            memcpy(buffer + *copied, b, size);
>> -            *copied += size;
>> +            *copied += bytestream2_get_buffer(&gb, buffer + *copied, size);
>> +        } else {
>> +            bytestream2_skip(&gb, size);
>>           }
>> -        b += size;
>> -        if (b >= input_buffer + input_len || *copied >= buflen)
>> +        if (bytestream2_get_bytes_left(&gb) <= 0 || *copied >= buflen)
>>               break;
>>       }
>>   
>> -    return b - input_buffer;
>> +    return bytestream2_tell(&gb);
>>   }
>>   
>>   static int jpegxl_anim_probe(const AVProbeData *p)
> 
> Is there an actual (potential) overrun or is this just a precaution?
> 
> - Andreas
> 

Yea, michaelni sent a cvslog email about it earlier, mail ID 
<20230608002033.GF870501@pb2>. This fixes that and switches to 
bytestream2 at Anton's recommendation.

- Leo Izen

_______________________________________________
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".