[FFmpeg-devel] Re: [PATCH] pngdec: abort if x_offset * bpp >= image_linesize

[Andreas Cadhalpun]

On 03.05.2015 20:09, Michael Niedermayer wrote:
> On Sun, May 03, 2015 at 05:11:17PM +0200, Andreas Cadhalpun wrote:
>> In this case ptr could be set to a position outside the image_buf in
>> png_handle_row, leading to memory corruption and thus crashes.
>>
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
>> ---
>>  libavcodec/pngdec.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
>> index 26de09d..4488726 100644
>> --- a/libavcodec/pngdec.c
>> +++ b/libavcodec/pngdec.c
>> @@ -655,6 +655,12 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s,
>>                  s->row_size, s->crow_size);
>>          s->image_buf      = p->data[0];
>>          s->image_linesize = p->linesize[0];
>> +        if (s->x_offset * s->bpp >= s->image_linesize) {
> 
> this doesnt look correct, the linesize could be larger than the
> width in pixels yet still x_offset shouldnt point outside the width
> and x_offset is being checked where its read

The problem is that the original check assumes linesize >= width.
However this is not the case for AV_PIX_FMT_MONOBLACK, which has the
AV_PIX_FMT_FLAG_BITSTREAM flag. Thus the linesize is divided by 8 in
image_get_linesize:
    if (desc->flags & AV_PIX_FMT_FLAG_BITSTREAM)
        linesize = (linesize + 7) >> 3;

> maybe theres some missing check on chunk order or something changes
> thats assumed not to ...

Alternatively one could just not accept AV_PIX_FMT_MONOBLACK in
combination with AV_CODEC_ID_APNG. Patch for that attached.

Best regards,
Andreas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-pngdec-don-t-use-AV_PIX_FMT_MONOBLACK-for-apng.patch
Type: text/x-diff
Size: 1518 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150503/8fd26cab/attachment.bin>