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

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 {

> 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

On 2 August 2017 at 00:35, Rodger Combs <rodger.combs@gmail.com> wrote:

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

Shouldn't the demuxer handle this?

> On Nov 21, 2017, at 19:03, Rostislav Pehlivanov <atomnuker@gmail.com> wrote:
> 
> On 2 August 2017 at 00:35, Rodger Combs <rodger.combs@gmail.com <mailto:rodger.combs@gmail.com>> wrote:
> 
>> 
>>> 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.
>> 
> 
> Shouldn't the demuxer handle this?

Even in e.g. MKV?

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel <http://ffmpeg.org/mailman/listinfo/ffmpeg-devel>