[FFmpeg-devel,6/7] lavf/flacenc: avoid buffer overread with unexpected extradata sizes

Submitted by James Almer on Aug. 1, 2017, 11:25 p.m.

Details

Message ID f909af46-b297-896f-585a-3a3922e1d054@gmail.com
State New
Headers show

Commit Message

James Almer Aug. 1, 2017, 11:25 p.m.
On 8/1/2017 3:33 AM, Rodger Combs wrote:
> ---
>  libavformat/flacenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
> index 9768b6a..1906aee 100644
> --- a/libavformat/flacenc.c
> +++ b/libavformat/flacenc.c
> @@ -322,7 +322,7 @@ static int flac_write_trailer(struct AVFormatContext *s)
>      if (!c->write_header || !streaminfo)
>          return 0;
>  
> -    if (pb->seekable & AVIO_SEEKABLE_NORMAL) {
> +    if (pb->seekable & AVIO_SEEKABLE_NORMAL && (c->streaminfo || s->streams[0]->codecpar->extradata_size == FLAC_STREAMINFO_SIZE)) {

In what situation would stream extradata or c->streaminfo be smaller
than FLAC_STREAMINFO_SIZE? Nothing changes par->extradata anywhere, and
c->streaminfo is always allocated with FLAC_STREAMINFO_SIZE bytes of memory.
I have the feeling i already asked this before, but not sure if you
answered it. Apologies if you did.

You can also simplify this by just rewriting the STREAMINFO header only
if c->streaminfo is allocated, meaning updated extradata showed up in a
packet. Otherwise, you'd be rewriting the same STREAMINFO header you
already wrote at the beginning.
You could also even use ff_flac_write_header(), which already does a
buffer size check.

         avio_flush(pb);
     } else {

Comments

Rodger Combs Aug. 1, 2017, 11:35 p.m.
> On Aug 1, 2017, at 18:25, James Almer <jamrial@gmail.com> wrote:
> 
> On 8/1/2017 3:33 AM, Rodger Combs wrote:
>> ---
>> libavformat/flacenc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
>> index 9768b6a..1906aee 100644
>> --- a/libavformat/flacenc.c
>> +++ b/libavformat/flacenc.c
>> @@ -322,7 +322,7 @@ static int flac_write_trailer(struct AVFormatContext *s)
>>     if (!c->write_header || !streaminfo)
>>         return 0;
>> 
>> -    if (pb->seekable & AVIO_SEEKABLE_NORMAL) {
>> +    if (pb->seekable & AVIO_SEEKABLE_NORMAL && (c->streaminfo || s->streams[0]->codecpar->extradata_size == FLAC_STREAMINFO_SIZE)) {
> 
> In what situation would stream extradata or c->streaminfo be smaller
> than FLAC_STREAMINFO_SIZE? Nothing changes par->extradata anywhere, and
> c->streaminfo is always allocated with FLAC_STREAMINFO_SIZE bytes of memory.
> I have the feeling i already asked this before, but not sure if you
> answered it. Apologies if you did.

It shouldn't happen in normal cases, but you could imagine a case with remuxing a corrupted input file.

> 
> You can also simplify this by just rewriting the STREAMINFO header only
> if c->streaminfo is allocated, meaning updated extradata showed up in a
> packet. Otherwise, you'd be rewriting the same STREAMINFO header you
> already wrote at the beginning.

Ah, good idea! Done.

> You could also even use ff_flac_write_header(), which already does a
> buffer size check.

I don't think this is necessary when coupled with your previous suggestion?

> 
> diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
> index b894f9ef61..7392cf13c1 100644
> --- a/libavformat/flacenc.c
> +++ b/libavformat/flacenc.c
> @@ -141,17 +141,15 @@ static int flac_write_trailer(struct
> AVFormatContext *s)
>     AVIOContext *pb = s->pb;
>     int64_t file_size;
>     FlacMuxerContext *c = s->priv_data;
> -    uint8_t *streaminfo = c->streaminfo ? c->streaminfo :
> -
> s->streams[0]->codecpar->extradata;
> 
> -    if (!c->write_header || !streaminfo)
> +    if (!c->write_header || !c->streaminfo)
>         return 0;
> 
>     if (pb->seekable & AVIO_SEEKABLE_NORMAL) {
>         /* rewrite the STREAMINFO header block data */
>         file_size = avio_tell(pb);
> -        avio_seek(pb, 8, SEEK_SET);
> -        avio_write(pb, streaminfo, FLAC_STREAMINFO_SIZE);
> +        avio_seek(pb, 0, SEEK_SET);
> +        ff_flac_write_header(pb, c->streaminfo, FLAC_STREAMINFO_SIZE, 0);
>         avio_seek(pb, file_size, SEEK_SET);
>         avio_flush(pb);
>     } else {
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Patch hide | download patch | download mbox

diff --git a/libavformat/flacenc.c b/libavformat/flacenc.c
index b894f9ef61..7392cf13c1 100644
--- a/libavformat/flacenc.c
+++ b/libavformat/flacenc.c
@@ -141,17 +141,15 @@  static int flac_write_trailer(struct
AVFormatContext *s)
     AVIOContext *pb = s->pb;
     int64_t file_size;
     FlacMuxerContext *c = s->priv_data;
-    uint8_t *streaminfo = c->streaminfo ? c->streaminfo :
-
s->streams[0]->codecpar->extradata;

-    if (!c->write_header || !streaminfo)
+    if (!c->write_header || !c->streaminfo)
         return 0;

     if (pb->seekable & AVIO_SEEKABLE_NORMAL) {
         /* rewrite the STREAMINFO header block data */
         file_size = avio_tell(pb);
-        avio_seek(pb, 8, SEEK_SET);
-        avio_write(pb, streaminfo, FLAC_STREAMINFO_SIZE);
+        avio_seek(pb, 0, SEEK_SET);
+        ff_flac_write_header(pb, c->streaminfo, FLAC_STREAMINFO_SIZE, 0);
         avio_seek(pb, file_size, SEEK_SET);