[FFmpeg-devel] libavformat/flacdec: Workaround for truncated metadata picture size

lavf flacenc could previously write truncated metadata picture size if
the picture data did not fit in 24 bits. Detect this by truncting the
size found inside the picture block and if it matches the block size
use it and read rest of picture data.

flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
---
 libavformat/flac_picture.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

Mattias Wadman:
> lavf flacenc could previously write truncated metadata picture size if
> the picture data did not fit in 24 bits. Detect this by truncting the
> size found inside the picture block and if it matches the block size
> use it and read rest of picture data.
> 
> flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> ---
>  libavformat/flac_picture.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> index 81ddf80465..17be497f95 100644
> --- a/libavformat/flac_picture.c
> +++ b/libavformat/flac_picture.c
> @@ -32,11 +32,12 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
>      const CodecMime *mime = ff_id3v2_mime_tags;
>      enum AVCodecID id = AV_CODEC_ID_NONE;
>      AVBufferRef *data = NULL;
> -    uint8_t mimetype[64], *desc = NULL;
> +    uint8_t mimetype[64], *desc = NULL, *picturebuf = NULL;
>      GetByteContext g;
>      AVStream *st;
>      int width, height, ret = 0;
> -    unsigned int len, type;
> +    unsigned int type;
> +    uint32_t len, left, lendiff;
>  
>      if (buf_size < 34) {
>          av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
> @@ -114,6 +115,23 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
>  
>      /* picture data */
>      len = bytestream2_get_be32u(&g);
> +
> +    // Workaround lavf flacenc bug that allowed writing truncated metadata picture block size if
> +    // picture size did not fit in 24 bits
> +    left = bytestream2_get_bytes_left(&g);
> +    if (len > left && (len & 0xffffff) == left) {
> +        av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size from %d to %d\n", left, len);
> +
> +        picturebuf = av_malloc(len);
> +        if (!picturebuf)
> +            RETURN_ERROR(AVERROR(ENOMEM));
> +        lendiff = len - left;
> +        bytestream2_get_buffer(&g, picturebuf, left);
> +        if (avio_read(s->pb, picturebuf+left, lendiff) < lendiff)
> +            RETURN_ERROR(AVERROR_INVALIDDATA);
> +        bytestream2_init(&g, picturebuf, len);
> +    }
> +
>      if (len <= 0 || len > bytestream2_get_bytes_left(&g)) {
>          av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
>          if (s->error_recognition & AV_EF_EXPLODE)
> @@ -155,6 +173,7 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
>  fail:
>      av_buffer_unref(&data);
>      av_freep(&desc);
> +    av_freep(&picturebuf);
>  
>      return ret;
>  }
> 
1. This fixes ticket 6333 (you will now no longer run into the flac
parser bug), doesn't it? If so, you should mention it.
2. ff_flac_parse_picture() is called from two places: The flac demuxer
as well as the vorbis-in-ogg demuxer. In the latter case the picture
data is base64-encoded and reading anything via the AVIOContext is a
very bad idea (ff_vorbis_comment() is for e.g. used to parse
VorbisComments contained in the CodecPrivate of FLAC tracks (for channel
masks) and if such a CodecPrivate also contained an embedded picture
that happens to trigger this scenario, you would read more data
believing it comes from immediately after the buffer you have received,
but that is just not true). You need to add a parameter to distinguish
these cases.

- Andreas

On Wed, Apr 22, 2020 at 5:18 PM Andreas Rheinhardt
<andreas.rheinhardt@gmail.com> wrote:
>
> Mattias Wadman:
> > lavf flacenc could previously write truncated metadata picture size if
> > the picture data did not fit in 24 bits. Detect this by truncting the
> > size found inside the picture block and if it matches the block size
> > use it and read rest of picture data.
> >
> > flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> > ---
> >  libavformat/flac_picture.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> > index 81ddf80465..17be497f95 100644
> > --- a/libavformat/flac_picture.c
> > +++ b/libavformat/flac_picture.c
> > @@ -32,11 +32,12 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> >      const CodecMime *mime = ff_id3v2_mime_tags;
> >      enum AVCodecID id = AV_CODEC_ID_NONE;
> >      AVBufferRef *data = NULL;
> > -    uint8_t mimetype[64], *desc = NULL;
> > +    uint8_t mimetype[64], *desc = NULL, *picturebuf = NULL;
> >      GetByteContext g;
> >      AVStream *st;
> >      int width, height, ret = 0;
> > -    unsigned int len, type;
> > +    unsigned int type;
> > +    uint32_t len, left, lendiff;
> >
> >      if (buf_size < 34) {
> >          av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
> > @@ -114,6 +115,23 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> >
> >      /* picture data */
> >      len = bytestream2_get_be32u(&g);
> > +
> > +    // Workaround lavf flacenc bug that allowed writing truncated metadata picture block size if
> > +    // picture size did not fit in 24 bits
> > +    left = bytestream2_get_bytes_left(&g);
> > +    if (len > left && (len & 0xffffff) == left) {
> > +        av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size from %d to %d\n", left, len);
> > +
> > +        picturebuf = av_malloc(len);
> > +        if (!picturebuf)
> > +            RETURN_ERROR(AVERROR(ENOMEM));
> > +        lendiff = len - left;
> > +        bytestream2_get_buffer(&g, picturebuf, left);
> > +        if (avio_read(s->pb, picturebuf+left, lendiff) < lendiff)
> > +            RETURN_ERROR(AVERROR_INVALIDDATA);
> > +        bytestream2_init(&g, picturebuf, len);
> > +    }
> > +
> >      if (len <= 0 || len > bytestream2_get_bytes_left(&g)) {
> >          av_log(s, AV_LOG_ERROR, "Attached picture metadata block too short\n");
> >          if (s->error_recognition & AV_EF_EXPLODE)
> > @@ -155,6 +173,7 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> >  fail:
> >      av_buffer_unref(&data);
> >      av_freep(&desc);
> > +    av_freep(&picturebuf);
> >
> >      return ret;
> >  }
> >
> 1. This fixes ticket 6333 (you will now no longer run into the flac
> parser bug), doesn't it? If so, you should mention it.

Yes this patch should fixat that. Didn't know there was a ticket, will
mention it.

> 2. ff_flac_parse_picture() is called from two places: The flac demuxer
> as well as the vorbis-in-ogg demuxer. In the latter case the picture
> data is base64-encoded and reading anything via the AVIOContext is a
> very bad idea (ff_vorbis_comment() is for e.g. used to parse
> VorbisComments contained in the CodecPrivate of FLAC tracks (for channel
> masks) and if such a CodecPrivate also contained an embedded picture
> that happens to trigger this scenario, you would read more data
> believing it comes from immediately after the buffer you have received,
> but that is just not true). You need to add a parameter to distinguish
> these cases.

Ok will look into that. Thanks for the speedy review!

-Mattias

>
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".