Re: [FFmpeg-devel] [PATCH] avcodec/zmbv: Check decomp_size

[Michael Niedermayer]

On Thu, Aug 17, 2017 at 10:19:18AM +0200, Tomas Härdin wrote:
> On 2017-08-16 16:03, Michael Niedermayer wrote:
> >Fixes: OOM
> >Fixes: 2710/clusterfuzz-testcase-minimized-4750001420894208
> >
> >Found-by: continuous fuzzing process 
> >https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >Signed-off-by: Michael Niedermayer 
> >---
> >  libavcodec/zmbv.c | 5 +
> >  1 file changed, 5 insertions(+)
> >
> >diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
> >index f126515bd1..b09dc41ebd 100644
> >--- a/libavcodec/zmbv.c
> >+++ b/libavcodec/zmbv.c
> >@@ -589,6 +589,11 @@ static av_cold int decode_init(AVCodecContext *avctx)
> >  // Needed if zlib unused or init aborted before inflateInit
> >  memset(>zstream, 0, sizeof(z_stream));
> >+if ((avctx->width + 255ULL) * (avctx->height + 64ULL) > 
> >FFMIN(avctx->max_pixels, INT_MAX / 4) ) {
> >+av_log(avctx, AV_LOG_ERROR, "Internal buffer (decomp_size) larger 
> >than max_pixels or too large\n");
> >+return AVERROR_INVALIDDATA;
> >+}
> >+
> 
> Looks like a decent solution

applied

thx

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

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/zmbv: Check decomp_size

[Tomas Härdin]

On 2017-08-16 16:03, Michael Niedermayer wrote:

Fixes: OOM
Fixes: 2710/clusterfuzz-testcase-minimized-4750001420894208

Found-by: continuous fuzzing process 
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer 
---
  libavcodec/zmbv.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
index f126515bd1..b09dc41ebd 100644
--- a/libavcodec/zmbv.c
+++ b/libavcodec/zmbv.c
@@ -589,6 +589,11 @@ static av_cold int decode_init(AVCodecContext *avctx)
  // Needed if zlib unused or init aborted before inflateInit
  memset(>zstream, 0, sizeof(z_stream));
  
+if ((avctx->width + 255ULL) * (avctx->height + 64ULL) > FFMIN(avctx->max_pixels, INT_MAX / 4) ) {

+av_log(avctx, AV_LOG_ERROR, "Internal buffer (decomp_size) larger than 
max_pixels or too large\n");
+return AVERROR_INVALIDDATA;
+}
+


Looks like a decent solution

/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/zmbv: Check decomp_size

[Tomas Härdin]

On 2017-08-16 14:35, Michael Niedermayer wrote:

On Wed, Aug 16, 2017 at 09:48:43AM +0200, Tomas Härdin wrote:

On 2017-08-16 05:04, Michael Niedermayer wrote:

Fixes: OOM
Fixes: 2710/clusterfuzz-testcase-minimized-4750001420894208

Found-by: continuous fuzzing process 
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer 
---
  libavcodec/zmbv.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
index f126515bd1..861098a0f2 100644
--- a/libavcodec/zmbv.c
+++ b/libavcodec/zmbv.c
@@ -589,8 +589,12 @@ static av_cold int decode_init(AVCodecContext *avctx)
  // Needed if zlib unused or init aborted before inflateInit
  memset(>zstream, 0, sizeof(z_stream));
-c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64);
+if ((avctx->width + 255ULL) * (avctx->height + 64ULL) > avctx->max_pixels) 
{

Are width and height constrained somewhere? If both end up around
1<<32 then the multiplication can overflow.

width and height are signed integers and checked in avcodec_open2()

I can replicate the check here if you think depending on a distant
check is too fragile.


Nah, so long as it's checked. Too bad C doesn't have range typed 
integers like Ada



+av_log(avctx, AV_LOG_ERROR, "Internal buffer larger than 
max_pixels\n");
+return AVERROR_INVALIDDATA;
+}
+c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64);

Use 255ULL and 64ULL maybe? It's almost conceivable that you could
have a file constructed to be (1<<31 - 255)x1 that passes the
max_pixels check

such file could exist but avcodec_open2() will reject this currently.
I think adding an explicit check of some kind would make sense
255ULL and 64ULL wont help as  decomp_size is just a unsigned int and
its passed into zlib which uses  a uInt ...

ill send a new patch

thx


Right, it might pass max_pixels but then overflow uInt..

/Tomas

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/zmbv: Check decomp_size

[Michael Niedermayer]

On Wed, Aug 16, 2017 at 09:48:43AM +0200, Tomas Härdin wrote:
> On 2017-08-16 05:04, Michael Niedermayer wrote:
> >Fixes: OOM
> >Fixes: 2710/clusterfuzz-testcase-minimized-4750001420894208
> >
> >Found-by: continuous fuzzing process 
> >https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >Signed-off-by: Michael Niedermayer 
> >---
> >  libavcodec/zmbv.c | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> >diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
> >index f126515bd1..861098a0f2 100644
> >--- a/libavcodec/zmbv.c
> >+++ b/libavcodec/zmbv.c
> >@@ -589,8 +589,12 @@ static av_cold int decode_init(AVCodecContext *avctx)
> >  // Needed if zlib unused or init aborted before inflateInit
> >  memset(>zstream, 0, sizeof(z_stream));
> >-c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64);
> >+if ((avctx->width + 255ULL) * (avctx->height + 64ULL) > 
> >avctx->max_pixels) {
> 
> Are width and height constrained somewhere? If both end up around
> 1<<32 then the multiplication can overflow.

width and height are signed integers and checked in avcodec_open2()

I can replicate the check here if you think depending on a distant
check is too fragile.


> 
> >+av_log(avctx, AV_LOG_ERROR, "Internal buffer larger than 
> >max_pixels\n");
> >+return AVERROR_INVALIDDATA;
> >+}
> >+c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64);
> 
> Use 255ULL and 64ULL maybe? It's almost conceivable that you could
> have a file constructed to be (1<<31 - 255)x1 that passes the
> max_pixels check

such file could exist but avcodec_open2() will reject this currently.
I think adding an explicit check of some kind would make sense
255ULL and 64ULL wont help as  decomp_size is just a unsigned int and
its passed into zlib which uses  a uInt ...

ill send a new patch

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avcodec/zmbv: Check decomp_size

[Tomas Härdin]

On 2017-08-16 05:04, Michael Niedermayer wrote:

Fixes: OOM
Fixes: 2710/clusterfuzz-testcase-minimized-4750001420894208

Found-by: continuous fuzzing process 
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer 
---
  libavcodec/zmbv.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libavcodec/zmbv.c b/libavcodec/zmbv.c
index f126515bd1..861098a0f2 100644
--- a/libavcodec/zmbv.c
+++ b/libavcodec/zmbv.c
@@ -589,8 +589,12 @@ static av_cold int decode_init(AVCodecContext *avctx)
  // Needed if zlib unused or init aborted before inflateInit
  memset(>zstream, 0, sizeof(z_stream));
  
-c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64);

+if ((avctx->width + 255ULL) * (avctx->height + 64ULL) > avctx->max_pixels) 
{


Are width and height constrained somewhere? If both end up around 1<<32 
then the multiplication can overflow.



+av_log(avctx, AV_LOG_ERROR, "Internal buffer larger than 
max_pixels\n");
+return AVERROR_INVALIDDATA;
+}
  
+c->decomp_size = (avctx->width + 255) * 4 * (avctx->height + 64);


Use 255ULL and 64ULL maybe? It's almost conceivable that you could have 
a file constructed to be (1<<31 - 255)x1 that passes the max_pixels check


/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel