diff mbox

[FFmpeg-devel,4/5] avcodec/hevc_mp4toannexb_bsf: check that nalu size doesnt overflow

Message ID 20191213002810.6440-4-michael@niedermayer.cc
State Accepted
Headers show

Commit Message

Michael Niedermayer Dec. 13, 2019, 12:28 a.m. UTC 
Fixes: Out of array access
Fixes: 19299/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_MP4TOANNEXB_fuzzer-5169193398042624

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

Comments

Andriy Gelman Dec. 13, 2019, 12:36 a.m. UTC | #1 
On Fri, 13. Dec 01:28, Michael Niedermayer wrote:
> Fixes: Out of array access
> Fixes: 19299/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_MP4TOANNEXB_fuzzer-5169193398042624
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/hevc_mp4toannexb_bsf.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c
> index d0f1b94f0e..baa93628ed 100644
> --- a/libavcodec/hevc_mp4toannexb_bsf.c
> +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> @@ -152,8 +152,7 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
>          extra_size    = add_extradata * ctx->par_out->extradata_size;
>          got_irap     |= is_irap;
>  
> -        if (SIZE_MAX - nalu_size < 4 ||
> -            SIZE_MAX - 4 - nalu_size < extra_size) {
> +        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size) {
>              ret = AVERROR_INVALIDDATA;
>              goto fail;
>          }
> -- 
> 2.24.0

I already sent a patch for this:
http://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/253972.html
Michael Niedermayer Dec. 13, 2019, 5:51 p.m. UTC | #2 
On Thu, Dec 12, 2019 at 07:36:50PM -0500, Andriy Gelman wrote:
> On Fri, 13. Dec 01:28, Michael Niedermayer wrote:
> > Fixes: Out of array access
> > Fixes: 19299/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_MP4TOANNEXB_fuzzer-5169193398042624
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/hevc_mp4toannexb_bsf.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c
> > index d0f1b94f0e..baa93628ed 100644
> > --- a/libavcodec/hevc_mp4toannexb_bsf.c
> > +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> > @@ -152,8 +152,7 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
> >          extra_size    = add_extradata * ctx->par_out->extradata_size;
> >          got_irap     |= is_irap;
> >  
> > -        if (SIZE_MAX - nalu_size < 4 ||
> > -            SIZE_MAX - 4 - nalu_size < extra_size) {
> > +        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size) {
> >              ret = AVERROR_INVALIDDATA;
> >              goto fail;
> >          }
> > -- 
> > 2.24.0
> 
> I already sent a patch for this:
> http://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/253972.html

yes but it depends on "distant" code behaving in specific ways. I prefer
a single self contained check. Because its more robust to someone changing
that distant code.
So i would prefer to apply some "dumb" check. Do you agree ?


Thanks

[...]
Andriy Gelman Dec. 13, 2019, 8:09 p.m. UTC | #3 
On Fri, 13. Dec 18:51, Michael Niedermayer wrote:
> On Thu, Dec 12, 2019 at 07:36:50PM -0500, Andriy Gelman wrote:
> > On Fri, 13. Dec 01:28, Michael Niedermayer wrote:
> > > Fixes: Out of array access
> > > Fixes: 19299/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_MP4TOANNEXB_fuzzer-5169193398042624
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/hevc_mp4toannexb_bsf.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c
> > > index d0f1b94f0e..baa93628ed 100644
> > > --- a/libavcodec/hevc_mp4toannexb_bsf.c
> > > +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> > > @@ -152,8 +152,7 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
> > >          extra_size    = add_extradata * ctx->par_out->extradata_size;
> > >          got_irap     |= is_irap;
> > >  
> > > -        if (SIZE_MAX - nalu_size < 4 ||
> > > -            SIZE_MAX - 4 - nalu_size < extra_size) {
> > > +        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size) {
> > >              ret = AVERROR_INVALIDDATA;
> > >              goto fail;
> > >          }
> > > -- 
> > > 2.24.0
> > 
> > I already sent a patch for this:
> > http://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/253972.html
> 
> yes but it depends on "distant" code behaving in specific ways. I prefer
> a single self contained check. Because its more robust to someone changing
> that distant code.
> So i would prefer to apply some "dumb" check. Do you agree ?

I agree it's more robust. I'll keep this in mind for future

Thanks,
Michael Niedermayer Jan. 18, 2020, 5:25 p.m. UTC | #4 
On Fri, Dec 13, 2019 at 03:09:25PM -0500, Andriy Gelman wrote:
> On Fri, 13. Dec 18:51, Michael Niedermayer wrote:
> > On Thu, Dec 12, 2019 at 07:36:50PM -0500, Andriy Gelman wrote:
> > > On Fri, 13. Dec 01:28, Michael Niedermayer wrote:
> > > > Fixes: Out of array access
> > > > Fixes: 19299/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_MP4TOANNEXB_fuzzer-5169193398042624
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavcodec/hevc_mp4toannexb_bsf.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c
> > > > index d0f1b94f0e..baa93628ed 100644
> > > > --- a/libavcodec/hevc_mp4toannexb_bsf.c
> > > > +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> > > > @@ -152,8 +152,7 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
> > > >          extra_size    = add_extradata * ctx->par_out->extradata_size;
> > > >          got_irap     |= is_irap;
> > > >  
> > > > -        if (SIZE_MAX - nalu_size < 4 ||
> > > > -            SIZE_MAX - 4 - nalu_size < extra_size) {
> > > > +        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size) {
> > > >              ret = AVERROR_INVALIDDATA;
> > > >              goto fail;
> > > >          }
> > > > -- 
> > > > 2.24.0
> > > 
> > > I already sent a patch for this:
> > > http://ffmpeg.org/pipermail/ffmpeg-devel/2019-December/253972.html
> > 
> > yes but it depends on "distant" code behaving in specific ways. I prefer
> > a single self contained check. Because its more robust to someone changing
> > that distant code.
> > So i would prefer to apply some "dumb" check. Do you agree ?
> 
> I agree it's more robust. I'll keep this in mind for future

will apply

thx

[...]
diff mbox

Patch

diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c
index d0f1b94f0e..baa93628ed 100644
--- a/libavcodec/hevc_mp4toannexb_bsf.c
+++ b/libavcodec/hevc_mp4toannexb_bsf.c
@@ -152,8 +152,7 @@  static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
         extra_size    = add_extradata * ctx->par_out->extradata_size;
         got_irap     |= is_irap;
 
-        if (SIZE_MAX - nalu_size < 4 ||
-            SIZE_MAX - 4 - nalu_size < extra_size) {
+        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size) {
             ret = AVERROR_INVALIDDATA;
             goto fail;
         }