| Message ID | 20200415210614.29152-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 889ad93c8839e5ac1ec28bc8e1fea6df71b9bf80 |
| Headers | show |
| Series | [FFmpeg-devel,1/2] fftools/ffmpeg_opt: Check attachment filesize | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Wed, Apr 15, 2020 at 11:06:13PM +0200, Andreas Rheinhardt wrote: > The data of an attachment file is put into an AVCodecParameter's > extradata. The corresponding size field has type int, yet there was no > check for the size to fit into an int. As a consequence, it was possible > to create extradata with negative size (by using a big enough max_alloc). > > Other errors were also possible: If SIZE_MAX < INT64_MAX (e.g. on 32bit > systems) then the file size might be truncated before the allocation; > and avio_read() takes an int, too, so one would not have read as much > as one desired. > > Furthermore, the extradata is now padded as is required. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > fftools/ffmpeg_opt.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) LGTM thx [...]
Michael Niedermayer: > On Wed, Apr 15, 2020 at 11:06:13PM +0200, Andreas Rheinhardt wrote: >> The data of an attachment file is put into an AVCodecParameter's >> extradata. The corresponding size field has type int, yet there was no >> check for the size to fit into an int. As a consequence, it was possible >> to create extradata with negative size (by using a big enough max_alloc). >> >> Other errors were also possible: If SIZE_MAX < INT64_MAX (e.g. on 32bit >> systems) then the file size might be truncated before the allocation; >> and avio_read() takes an int, too, so one would not have read as much >> as one desired. >> >> Furthermore, the extradata is now padded as is required. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> fftools/ffmpeg_opt.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) > > LGTM > > thx > > [...] > Applied, thanks. - Andreas
diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c index 93b3d96205..680f0f1dfb 100644 --- a/fftools/ffmpeg_opt.c +++ b/fftools/ffmpeg_opt.c @@ -2432,12 +2432,14 @@ loop_end: o->attachments[i]); exit_program(1); } - if (!(attachment = av_malloc(len))) { - av_log(NULL, AV_LOG_FATAL, "Attachment %s too large to fit into memory.\n", + if (len > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE || + !(attachment = av_malloc(len + AV_INPUT_BUFFER_PADDING_SIZE))) { + av_log(NULL, AV_LOG_FATAL, "Attachment %s too large.\n", o->attachments[i]); exit_program(1); } avio_read(pb, attachment, len); + memset(attachment + len, 0, AV_INPUT_BUFFER_PADDING_SIZE); ost = new_attachment_stream(o, oc, -1); ost->stream_copy = 0;
The data of an attachment file is put into an AVCodecParameter's extradata. The corresponding size field has type int, yet there was no check for the size to fit into an int. As a consequence, it was possible to create extradata with negative size (by using a big enough max_alloc). Other errors were also possible: If SIZE_MAX < INT64_MAX (e.g. on 32bit systems) then the file size might be truncated before the allocation; and avio_read() takes an int, too, so one would not have read as much as one desired. Furthermore, the extradata is now padded as is required. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- fftools/ffmpeg_opt.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)