[FFmpeg-devel] [PATCH 1/2] avformat/vividas: Check packet size

[FFmpeg-devel] [PATCH 1/2] avformat/vividas: Check packet size

[Michael Niedermayer]

Fixes: signed integer overflow: 119760682 - -2084600173 cannot be represented in type 'int'
Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-6745781167587328

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/vividas.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/libavformat/vividas.c b/libavformat/vividas.c
index e9954f73ed0..e8efe49a5c0 100644
--- a/libavformat/vividas.c
+++ b/libavformat/vividas.c
@@ -683,6 +683,7 @@ static int viv_read_packet(AVFormatContext *s,
 
     if (viv->sb_entries[viv->current_sb_entry].flag == 0) {
         uint64_t v_size = ffio_read_varlen(pb);
+        int last, last_start;
 
         if (!viv->num_audio)
             return AVERROR_INVALIDDATA;
@@ -704,14 +705,22 @@ static int viv_read_packet(AVFormatContext *s,
             start = ffio_read_varlen(pb);
             pcm_bytes = ffio_read_varlen(pb);
 
-            if (i > 0 && start == 0)
-                break;
+            if (i > 0) {
+                if (start == 0)
+                    break;
+                if (start < last || start - (unsigned)last > INT_MAX)
+                    return AVERROR_INVALIDDATA;
+            }
 
             viv->n_audio_subpackets = i + 1;
+            last =
             viv->audio_subpackets[i].start = start;
             viv->audio_subpackets[i].pcm_bytes = pcm_bytes;
         }
+        last_start =
         viv->audio_subpackets[viv->n_audio_subpackets].start = (int)(off - avio_tell(pb));
+        if (last_start < last || last_start - (unsigned)last > INT_MAX)
+            return AVERROR_INVALIDDATA;
         viv->current_audio_subpacket = 0;
 
     } else {
-- 
2.17.1

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

[FFmpeg-devel] [PATCH 2/2] avformat/westwood_vqa: Check chunk size

[Michael Niedermayer]

the type is also changed to int as it is interpreted as int in av_get_packet()

Fixes: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_WSVQA_fuzzer-6593408795279360

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/westwood_vqa.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/libavformat/westwood_vqa.c b/libavformat/westwood_vqa.c
index e3d2e2668c4..03b2d9e03c3 100644
--- a/libavformat/westwood_vqa.c
+++ b/libavformat/westwood_vqa.c
@@ -178,13 +178,15 @@ static int wsvqa_read_packet(AVFormatContext *s,
     int ret = -1;
     uint8_t preamble[VQA_PREAMBLE_SIZE];
     uint32_t chunk_type;
-    uint32_t chunk_size;
-    int skip_byte;
+    int chunk_size;
+    unsigned skip_byte;
 
     while (avio_read(pb, preamble, VQA_PREAMBLE_SIZE) == VQA_PREAMBLE_SIZE) {
         chunk_type = AV_RB32(&preamble[0]);
         chunk_size = AV_RB32(&preamble[4]);
 
+        if (chunk_size < 0)
+            return AVERROR_INVALIDDATA;
         skip_byte = chunk_size & 0x01;
 
         if (chunk_type == VQFL_TAG) {
@@ -193,9 +195,9 @@ static int wsvqa_read_packet(AVFormatContext *s,
              * so it can be combined with the next VQFR packet. This way each packet
              * includes a whole frame as expected. */
             wsvqa->vqfl_chunk_pos = avio_tell(pb);
-            wsvqa->vqfl_chunk_size = (int)(chunk_size);
-            if (wsvqa->vqfl_chunk_size < 0 || wsvqa->vqfl_chunk_size > 3 * (1 << 20))
+            if (chunk_size > 3 * (1 << 20))
                 return AVERROR_INVALIDDATA;
+            wsvqa->vqfl_chunk_size = chunk_size;
             /* We need a big seekback buffer because there can be SNxx, VIEW and ZBUF
              * chunks (<512 KiB total) in the stream before we read VQFR (<256 KiB) and
              * seek back here. */
-- 
2.17.1

_______________________________________________
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 2/2] avformat/westwood_vqa: Check chunk size

[Anton Khirnov]

Quoting Michael Niedermayer (2022-09-22 20:08:52)
> the type is also changed to int as it is interpreted as int in av_get_packet()
> 
> Fixes: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
> Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_WSVQA_fuzzer-6593408795279360
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/westwood_vqa.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Looks good, thank you.

-- 
Anton Khirnov
_______________________________________________
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 2/2] avformat/westwood_vqa: Check chunk size

[Michael Niedermayer]

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

On Wed, Sep 28, 2022 at 05:17:44PM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2022-09-22 20:08:52)
> > the type is also changed to int as it is interpreted as int in av_get_packet()
> > 
> > Fixes: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
> > Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_WSVQA_fuzzer-6593408795279360
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/westwood_vqa.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> Looks good, thank you.

will apply

thx

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

Dictatorship: All citizens are under surveillance, all their steps and
actions recorded, for the politicians to enforce control.
Democracy: All politicians are under surveillance, all their steps and
actions recorded, for the citizens to enforce control.

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

Re: [FFmpeg-devel] [PATCH 1/2] avformat/vividas: Check packet size

[Anton Khirnov]

Quoting Michael Niedermayer (2022-09-22 20:08:51)
> Fixes: signed integer overflow: 119760682 - -2084600173 cannot be represented in type 'int'
> Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-6745781167587328
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/vividas.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/vividas.c b/libavformat/vividas.c
> index e9954f73ed0..e8efe49a5c0 100644
> --- a/libavformat/vividas.c
> +++ b/libavformat/vividas.c
> @@ -683,6 +683,7 @@ static int viv_read_packet(AVFormatContext *s,
>  
>      if (viv->sb_entries[viv->current_sb_entry].flag == 0) {
>          uint64_t v_size = ffio_read_varlen(pb);
> +        int last, last_start;
>  
>          if (!viv->num_audio)
>              return AVERROR_INVALIDDATA;
> @@ -704,14 +705,22 @@ static int viv_read_packet(AVFormatContext *s,
>              start = ffio_read_varlen(pb);
>              pcm_bytes = ffio_read_varlen(pb);
>  
> -            if (i > 0 && start == 0)
> -                break;
> +            if (i > 0) {
> +                if (start == 0)
> +                    break;
> +                if (start < last || start - (unsigned)last > INT_MAX)

What is the second condition for?

-- 
Anton Khirnov
_______________________________________________
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 1/2] avformat/vividas: Check packet size

[Michael Niedermayer]

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

On Wed, Sep 28, 2022 at 05:16:05PM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2022-09-22 20:08:51)
> > Fixes: signed integer overflow: 119760682 - -2084600173 cannot be represented in type 'int'
> > Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-6745781167587328
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/vividas.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavformat/vividas.c b/libavformat/vividas.c
> > index e9954f73ed0..e8efe49a5c0 100644
> > --- a/libavformat/vividas.c
> > +++ b/libavformat/vividas.c
> > @@ -683,6 +683,7 @@ static int viv_read_packet(AVFormatContext *s,
> >  
> >      if (viv->sb_entries[viv->current_sb_entry].flag == 0) {
> >          uint64_t v_size = ffio_read_varlen(pb);
> > +        int last, last_start;
> >  
> >          if (!viv->num_audio)
> >              return AVERROR_INVALIDDATA;
> > @@ -704,14 +705,22 @@ static int viv_read_packet(AVFormatContext *s,
> >              start = ffio_read_varlen(pb);
> >              pcm_bytes = ffio_read_varlen(pb);
> >  
> > -            if (i > 0 && start == 0)
> > -                break;
> > +            if (i > 0) {
> > +                if (start == 0)
> > +                    break;
> > +                if (start < last || start - (unsigned)last > INT_MAX)
> 
> What is the second condition for?

start is signed int so are "copyies" of it
"start < last" would allow a negative last with a large start
the 2nd check handles that.
the difference of consequtive values are stored as int later

The patch tried to leave the storage types and check for it.
The types could be changed or some other checks could be used
I was undecided on this patch a bit too. I picked this mainly
to keep changes more minimal but maybe this was not the best
choice

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

Re: [FFmpeg-devel] [PATCH 1/2] avformat/vividas: Check packet size

[Anton Khirnov]

Quoting Michael Niedermayer (2022-09-29 00:35:09)
> On Wed, Sep 28, 2022 at 05:16:05PM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2022-09-22 20:08:51)
> > > Fixes: signed integer overflow: 119760682 - -2084600173 cannot be represented in type 'int'
> > > Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-6745781167587328
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/vividas.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libavformat/vividas.c b/libavformat/vividas.c
> > > index e9954f73ed0..e8efe49a5c0 100644
> > > --- a/libavformat/vividas.c
> > > +++ b/libavformat/vividas.c
> > > @@ -683,6 +683,7 @@ static int viv_read_packet(AVFormatContext *s,
> > >  
> > >      if (viv->sb_entries[viv->current_sb_entry].flag == 0) {
> > >          uint64_t v_size = ffio_read_varlen(pb);
> > > +        int last, last_start;
> > >  
> > >          if (!viv->num_audio)
> > >              return AVERROR_INVALIDDATA;
> > > @@ -704,14 +705,22 @@ static int viv_read_packet(AVFormatContext *s,
> > >              start = ffio_read_varlen(pb);
> > >              pcm_bytes = ffio_read_varlen(pb);
> > >  
> > > -            if (i > 0 && start == 0)
> > > -                break;
> > > +            if (i > 0) {
> > > +                if (start == 0)
> > > +                    break;
> > > +                if (start < last || start - (unsigned)last > INT_MAX)
> > 
> > What is the second condition for?
> 
> start is signed int so are "copyies" of it
> "start < last" would allow a negative last with a large start
> the 2nd check handles that.
> the difference of consequtive values are stored as int later
> 
> The patch tried to leave the storage types and check for it.
> The types could be changed or some other checks could be used
> I was undecided on this patch a bit too. I picked this mainly
> to keep changes more minimal but maybe this was not the best
> choice

Checking that start >= 0 would fix this as well, wouldn't it? I don't
think it makes sense for it to be negative.

-- 
Anton Khirnov
_______________________________________________
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 1/2] avformat/vividas: Check packet size

[Michael Niedermayer]

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

On Thu, Sep 29, 2022 at 04:10:22PM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2022-09-29 00:35:09)
> > On Wed, Sep 28, 2022 at 05:16:05PM +0200, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2022-09-22 20:08:51)
> > > > Fixes: signed integer overflow: 119760682 - -2084600173 cannot be represented in type 'int'
> > > > Fixes: 50993/clusterfuzz-testcase-minimized-ffmpeg_dem_VIVIDAS_fuzzer-6745781167587328
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavformat/vividas.c | 13 +++++++++++--
> > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/libavformat/vividas.c b/libavformat/vividas.c
> > > > index e9954f73ed0..e8efe49a5c0 100644
> > > > --- a/libavformat/vividas.c
> > > > +++ b/libavformat/vividas.c
> > > > @@ -683,6 +683,7 @@ static int viv_read_packet(AVFormatContext *s,
> > > >  
> > > >      if (viv->sb_entries[viv->current_sb_entry].flag == 0) {
> > > >          uint64_t v_size = ffio_read_varlen(pb);
> > > > +        int last, last_start;
> > > >  
> > > >          if (!viv->num_audio)
> > > >              return AVERROR_INVALIDDATA;
> > > > @@ -704,14 +705,22 @@ static int viv_read_packet(AVFormatContext *s,
> > > >              start = ffio_read_varlen(pb);
> > > >              pcm_bytes = ffio_read_varlen(pb);
> > > >  
> > > > -            if (i > 0 && start == 0)
> > > > -                break;
> > > > +            if (i > 0) {
> > > > +                if (start == 0)
> > > > +                    break;
> > > > +                if (start < last || start - (unsigned)last > INT_MAX)
> > > 
> > > What is the second condition for?
> > 
> > start is signed int so are "copyies" of it
> > "start < last" would allow a negative last with a large start
> > the 2nd check handles that.
> > the difference of consequtive values are stored as int later
> > 
> > The patch tried to leave the storage types and check for it.
> > The types could be changed or some other checks could be used
> > I was undecided on this patch a bit too. I picked this mainly
> > to keep changes more minimal but maybe this was not the best
> > choice
> 
> Checking that start >= 0 would fix this as well, wouldn't it?

i agree


>  I don't
> think it makes sense for it to be negative.

well no, but i tried to minimize the chance of breaking something
so the fix ended up looking a bit wonky
ill change it to check for start >= 0

thx

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

The worst form of inequality is to try to make unequal things equal.
-- 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".