#5809 closed defect (fixed)

av_parser_parse2 overflows input buffer (mpegaudio)

Reported by: driegel Owned by:
Priority: minor Component: documentation
Version: git-master Keywords:
Cc: Blocked By:
Blocking: Reproduced by developer: yes
Analyzed by developer: no

Description

Summary of the bug:
When parsing mpegaudio, av_parser_parse2 doesn't respect input buffer size and read beyond its limit. This overflow can lead to crashes.

FFmpeg version:
n3.1.2 (built from sources)

How to reproduce:
Compile with make (adjusted the Makefile if necessary), and run output binary with valgrind.

Valgrind output:

$ valgrind ./test 
==9820== Memcheck, a memory error detector
==9820== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==9820== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==9820== Command: ./test
==9820== 
==9820== Invalid read of size 8
==9820==    at 0x4C326C8: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9820==    by 0x55CED85: memcpy (string3.h:53)
==9820==    by 0x55CED85: ff_combine_frame (parser.c:298)
==9820==    by 0x55657F8: mpegaudio_parse (mpegaudio_parser.c:111)
==9820==    by 0x55CE9A0: av_parser_parse2 (parser.c:182)
==9820==    by 0x400D0A: main (test.c:67)
==9820==  Address 0x76a2258 is 0 bytes after a block of size 40 alloc'd
==9820==    at 0x4C2FFC6: memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9820==    by 0x4C300D1: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9820==    by 0x63016CF: av_malloc (mem.c:97)
==9820==    by 0x400C4D: main (test.c:53)
==9820== 
==9820== Invalid read of size 8
==9820==    at 0x4C326D6: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9820==    by 0x55CED85: memcpy (string3.h:53)
==9820==    by 0x55CED85: ff_combine_frame (parser.c:298)
==9820==    by 0x55657F8: mpegaudio_parse (mpegaudio_parser.c:111)
==9820==    by 0x55CE9A0: av_parser_parse2 (parser.c:182)
==9820==    by 0x400D0A: main (test.c:67)
==9820==  Address 0x76a2f80 is 8 bytes after a block of size 40 alloc'd
==9820==    at 0x4C2FFC6: memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9820==    by 0x4C300D1: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9820==    by 0x63016CF: av_malloc (mem.c:97)
==9820==    by 0x400C4D: main (test.c:53)
==9820== 
==9820== 
==9820== HEAP SUMMARY:
==9820==     in use at exit: 40 bytes in 1 blocks
==9820==   total heap usage: 734 allocs, 733 frees, 67,939 bytes allocated
==9820== 
==9820== LEAK SUMMARY:
==9820==    definitely lost: 0 bytes in 0 blocks
==9820==    indirectly lost: 0 bytes in 0 blocks
==9820==      possibly lost: 0 bytes in 0 blocks
==9820==    still reachable: 40 bytes in 1 blocks
==9820==         suppressed: 0 bytes in 0 blocks
==9820== Rerun with --leak-check=full to see details of leaked memory
==9820== 
==9820== For counts of detected and suppressed errors, rerun with: -v
==9820== ERROR SUMMARY: 92 errors from 2 contexts (suppressed: 0 from 0)

Quick link to checkout the code and run test case:
https://github.com/d-k-c/ffmpeg-av_parser_parse2-bug

Attachments (4)

encoded_audio.c (164.8 KB) - added by driegel 22 months ago.
mp2-encoded audio dump
test.c (1.6 KB) - added by driegel 22 months ago.
test case to reproduce the bug
Makefile (455 bytes) - added by driegel 22 months ago.
patchparserpadding.diff (995 bytes) - added by cehoyos 22 months ago.

Download all attachments as: .zip

Change History (10)

Changed 22 months ago by driegel

mp2-encoded audio dump

Changed 22 months ago by driegel

test case to reproduce the bug

Changed 22 months ago by driegel

comment:1 follow-up: Changed 22 months ago by cehoyos

  • Component changed from avcodec to undetermined
  • Resolution set to invalid
  • Status changed from new to closed

I believe the documentation requires you to pad the input buffer.
For future tickets, please remember to test current FFmpeg git head before reporting issues here.

comment:2 in reply to: ↑ 1 Changed 22 months ago by driegel

Replying to cehoyos:

I believe the documentation requires you to pad the input buffer.

The documentation doesn't state any requirement about the input buffer. Compilation was broken with git head and the file (mpegaudio_parser.c) hasn't changed since the release I used, so I supposed it was a good approximate.

comment:3 Changed 22 months ago by cehoyos

  • Resolution invalid deleted
  • Status changed from closed to reopened

The documentation of av_parser_parse2() doesn't mention padding, I don't know if there is an issue.

comment:4 Changed 22 months ago by cehoyos

Not reproducible with ffmpeg because av_grow_packet() does indeed pad each AVPacket with AV_INPUT_BUFFER_PADDING_SIZE.

Changed 22 months ago by cehoyos

comment:5 Changed 22 months ago by michael

IIRC the parsers always were written on the assumtation that the input is padded. So to me this looks like a documentation issue

Of course if someone wants to change all parsers to not require padding that would e welcome if its clean and has no speed penalty

comment:6 Changed 22 months ago by cehoyos

  • Component changed from undetermined to documentation
  • Priority changed from normal to minor
  • Reproduced by developer set
  • Resolution set to fixed
  • Status changed from reopened to closed
  • Version changed from unspecified to git-master

Documentation improved in 7a8e5ff1fd06c0363ed4bb26cda5262fcd925b74, thank you for the report!

Note: See TracTickets for help on using tickets.