GNOME Bugzilla – Bug 775048
mpegts decoder: Out of bounds read in gst_mpegts_section_new
Last modified: 2016-11-26 10:32:46 UTC
Created attachment 340712 [details] poc file The attached file will cause an out of bounds read in the mpegts decoder. It seems the code assumes that section_size is at least 5 bytes and that those can be read from the data pointer, but it is not checked. I have attached a proposed fix as well. Found with afl, current git code. Here's the error message from address sanitizer:==30898==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200002cf73 at pc 0x7f828378abc1 bp 0x7f82834028c0 sp 0x7f82834028b8 READ of size 2 at 0x60200002cf73 thread T2 (tsdemux0:sink) #0 0x7f828378abc0 in __gst_fast_read_swap16 /usr/include/gstreamer-1.0/gst/gstutils.h:128:10 #1 0x7f828378abc0 in gst_mpegts_section_new /f/gstreamer/gst-plugins-bad/gst-libs/gst/mpegts/gstmpegtssection.c:1208 #2 0x7f8283a23b9b in mpegts_packetizer_parse_section_header /f/gstreamer/gst-plugins-bad/gst/mpegtsdemux/mpegtspacketizer.c:527:7 #3 0x7f8283a23b9b in mpegts_packetizer_push_section /f/gstreamer/gst-plugins-bad/gst/mpegtsdemux/mpegtspacketizer.c:1047 #4 0x7f8283a3788c in mpegts_base_chain /f/gstreamer/gst-plugins-bad/gst/mpegtsdemux/mpegtsbase.c:1419:17 #5 0x7f8283a341e7 in mpegts_base_loop /f/gstreamer/gst-plugins-bad/gst/mpegtsdemux/mpegtsbase.c:1589:13 #6 0x7f8290ab25c3 in gst_task_func /f/gstreamer/gstreamer/gst/gsttask.c:334:5 #7 0x7f828fcb1867 (/usr/lib64/libglib-2.0.so.0+0x70867) #8 0x7f828fcb0ed4 (/usr/lib64/libglib-2.0.so.0+0x6fed4) #9 0x7f828f72e443 in start_thread (/lib64/libpthread.so.0+0x7443) #10 0x7f828f25d92c in clone (/lib64/libc.so.6+0xe792c) 0x60200002cf74 is located 0 bytes to the right of 4-byte region [0x60200002cf70,0x60200002cf74) allocated by thread T2 (tsdemux0:sink) here: #0 0x4d4e28 in malloc (/usr/bin/gst-discoverer-1.0+0x4d4e28) #1 0x7f828fc903a8 in g_malloc (/usr/lib64/libglib-2.0.so.0+0x4f3a8) Thread T2 (tsdemux0:sink) created by T1 (typefind:sink) here: #0 0x42e26d in __interceptor_pthread_create (/usr/bin/gst-discoverer-1.0+0x42e26d) #1 0x7f828fcceadf (/usr/lib64/libglib-2.0.so.0+0x8dadf) Thread T1 (typefind:sink) created by T0 here: #0 0x42e26d in __interceptor_pthread_create (/usr/bin/gst-discoverer-1.0+0x42e26d) #1 0x7f828fcceadf (/usr/lib64/libglib-2.0.so.0+0x8dadf) SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/include/gstreamer-1.0/gst/gstutils.h:128:10 in __gst_fast_read_swap16 Shadow bytes around the buggy address: 0x0c047fffd990: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fffd9a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fffd9b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fffd9c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fffd9d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa =>0x0c047fffd9e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa[04]fa 0x0c047fffd9f0: fa fa 00 06 fa fa fd fd fa fa fd fa fa fa fd fd 0x0c047fffda00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fffda10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fffda20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fffda30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap right redzone: fb Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==30898==ABORTING
Created attachment 340713 [details] [review] proposed patch
Review of attachment 340713 [details] [review]: Thanks for the patch, and for finding this :) ::: gst-libs/gst/mpegts/gstmpegtssection.c @@ +1184,3 @@ /* Check for length */ section_length = GST_READ_UINT16_BE (data + 1) & 0x0FFF; + if (section_length < 5) { It probably makes sense to check before reading the section length already if data_size >= 3. I'm also not sure where the 5 comes from. We first 3 bytes, and then *if* it is not a short section we read another 5 bytes (which should then be checked if we don't have a short section only).
Sorry, patch was probably too sloppy. I just added all the data++ /+=, maybe I miscalculated somewhere.
The same problem probably exists in similar places here in the mpegts lib, needs a review of the other code.
Also in the docs: * Note: Ensuring @data is big enough to contain the full section is the * responsibility of the caller. If it is not big enough, %NULL will be * returned. Maybe the caller(s) should also do something here.
Created attachment 340788 [details] [review] mpegtssection: Add more section size checks The smallest section ever needs to be at least 3 bytes (i.e. just the short header). Non-short headers need to be at least 11 bytes long (3 for the minimum header, 5 for the non-short header, and 4 for the CRC).
commit d58f668ece8795bddb3316832e1848c7b7cf38ac Author: Edward Hervey <edward@centricular.com> Date: Sat Nov 26 10:44:43 2016 +0100 mpegtssection: Add more section size checks The smallest section ever needs to be at least 3 bytes (i.e. just the short header). Non-short headers need to be at least 11 bytes long (3 for the minimum header, 5 for the non-short header, and 4 for the CRC). https://bugzilla.gnome.org/show_bug.cgi?id=775048