[FFmpeg-devel] [PATCH 1/2] avcodec/wmalosslessdec: Check channels after setting them

[FFmpeg-devel] [PATCH 1/2] avcodec/wmalosslessdec: Check channels after setting them

[Michael Niedermayer]

Fixes: 46194/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WMALOSSLESS_fuzzer-580292873827123
Fixes: stack-buffer-overflow on address 0x7ffc0ce69b30 at pc 0x00000062fb03 bp 0x7ffc0ce69af0 sp 0x7ffc0ce69ae8
Fixes: 46205/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WMALOSSLESS_fuzzer-5354894996930560

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

diff --git a/libavcodec/wmalosslessdec.c b/libavcodec/wmalosslessdec.c
index d4f3462804..fd16f3f9bc 100644
--- a/libavcodec/wmalosslessdec.c
+++ b/libavcodec/wmalosslessdec.c
@@ -191,13 +191,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
         return AVERROR(EINVAL);
     }
 
-    av_assert0(avctx->ch_layout.nb_channels >= 0);
-    if (avctx->ch_layout.nb_channels > WMALL_MAX_CHANNELS) {
-        avpriv_request_sample(avctx,
-                              "More than " AV_STRINGIFY(WMALL_MAX_CHANNELS) " channels");
-        return AVERROR_PATCHWELCOME;
-    }
-
     if (avctx->extradata_size >= 18) {
         s->decode_flags    = AV_RL16(edata_ptr + 14);
         channel_mask       = AV_RL32(edata_ptr +  2);
@@ -226,6 +219,12 @@ static av_cold int decode_init(AVCodecContext *avctx)
         av_channel_layout_uninit(&avctx->ch_layout);
         av_channel_layout_from_mask(&avctx->ch_layout, channel_mask);
     }
+    av_assert0(avctx->ch_layout.nb_channels >= 0);
+    if (avctx->ch_layout.nb_channels > WMALL_MAX_CHANNELS) {
+        avpriv_request_sample(avctx,
+                            "More than " AV_STRINGIFY(WMALL_MAX_CHANNELS) " channels");
+        return AVERROR_PATCHWELCOME;
+    }
 
     s->num_channels = avctx->ch_layout.nb_channels;
 
-- 
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/matroskadec: avoid integer overflows in SAR computation

[Michael Niedermayer]

This ignores >64bit
Alternatively we could support that if it occurs in reality

Fixes: negation of -9223372036854775808
Fixes: integer overflows
Fixes: 46072/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-5029840966778880

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

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index d97fc33d44..069fba6cf7 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -2886,11 +2886,14 @@ static int matroska_parse_tracks(AVFormatContext *s)
                 mkv_stereo_mode_display_mul(track->video.stereo_mode, &display_width_mul, &display_height_mul);
 
             if (track->video.display_unit < MATROSKA_VIDEO_DISPLAYUNIT_UNKNOWN) {
-                av_reduce(&st->sample_aspect_ratio.num,
-                          &st->sample_aspect_ratio.den,
-                          st->codecpar->height * track->video.display_width  * display_width_mul,
-                          st->codecpar->width  * track->video.display_height * display_height_mul,
-                          INT_MAX);
+                if (track->video.display_width && track->video.display_height &&
+                    st->codecpar->height * (int64_t)display_width_mul  < INT64_MAX / track->video.display_width &&
+                    st->codecpar->width  * (int64_t)display_height_mul < INT64_MAX / track->video.display_height)
+                    av_reduce(&st->sample_aspect_ratio.num,
+                              &st->sample_aspect_ratio.den,
+                              st->codecpar->height * track->video.display_width  * display_width_mul,
+                              st->codecpar->width  * track->video.display_height * display_height_mul,
+                              INT_MAX);
             }
             if (st->codecpar->codec_id != AV_CODEC_ID_HEVC)
                 sti->need_parsing = AVSTREAM_PARSE_HEADERS;
-- 
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/matroskadec: avoid integer overflows in SAR computation

[Anton Khirnov]

Quoting Michael Niedermayer (2022-04-01 12:46:08)
> This ignores >64bit
> Alternatively we could support that if it occurs in reality
> 
> Fixes: negation of -9223372036854775808
> Fixes: integer overflows
> Fixes: 46072/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-5029840966778880
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/matroskadec.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index d97fc33d44..069fba6cf7 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -2886,11 +2886,14 @@ static int matroska_parse_tracks(AVFormatContext *s)
>                  mkv_stereo_mode_display_mul(track->video.stereo_mode, &display_width_mul, &display_height_mul);
>  
>              if (track->video.display_unit < MATROSKA_VIDEO_DISPLAYUNIT_UNKNOWN) {
> -                av_reduce(&st->sample_aspect_ratio.num,
> -                          &st->sample_aspect_ratio.den,
> -                          st->codecpar->height * track->video.display_width  * display_width_mul,
> -                          st->codecpar->width  * track->video.display_height * display_height_mul,
> -                          INT_MAX);
> +                if (track->video.display_width && track->video.display_height &&
> +                    st->codecpar->height * (int64_t)display_width_mul  < INT64_MAX / track->video.display_width &&
> +                    st->codecpar->width  * (int64_t)display_height_mul < INT64_MAX / track->video.display_height)

Why not move display_{width,height}_mul to the other side of the
comparison and avoid wordsize assumptions? This is header parsing, so
division performance impact should be negligible.

-- 
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/matroskadec: avoid integer overflows in SAR computation

[Michael Niedermayer]

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

On Tue, Apr 05, 2022 at 11:59:44AM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2022-04-01 12:46:08)
> > This ignores >64bit
> > Alternatively we could support that if it occurs in reality
> > 
> > Fixes: negation of -9223372036854775808
> > Fixes: integer overflows
> > Fixes: 46072/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-5029840966778880
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/matroskadec.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > index d97fc33d44..069fba6cf7 100644
> > --- a/libavformat/matroskadec.c
> > +++ b/libavformat/matroskadec.c
> > @@ -2886,11 +2886,14 @@ static int matroska_parse_tracks(AVFormatContext *s)
> >                  mkv_stereo_mode_display_mul(track->video.stereo_mode, &display_width_mul, &display_height_mul);
> >  
> >              if (track->video.display_unit < MATROSKA_VIDEO_DISPLAYUNIT_UNKNOWN) {
> > -                av_reduce(&st->sample_aspect_ratio.num,
> > -                          &st->sample_aspect_ratio.den,
> > -                          st->codecpar->height * track->video.display_width  * display_width_mul,
> > -                          st->codecpar->width  * track->video.display_height * display_height_mul,
> > -                          INT_MAX);
> > +                if (track->video.display_width && track->video.display_height &&
> > +                    st->codecpar->height * (int64_t)display_width_mul  < INT64_MAX / track->video.display_width &&
> > +                    st->codecpar->width  * (int64_t)display_height_mul < INT64_MAX / track->video.display_height)
> 
> Why not move display_{width,height}_mul to the other side of the
> comparison and avoid wordsize assumptions? This is header parsing, so
> division performance impact should be negligible.

will do

thx

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

Take away the freedom of one citizen and you will be jailed, take away
the freedom of all citizens and you will be congratulated by your peers
in Parliament.

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