After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 775048 - mpegts decoder: Out of bounds read in gst_mpegts_section_new
mpegts decoder: Out of bounds read in gst_mpegts_section_new
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.10.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-24 20:22 UTC by Hanno Böck
Modified: 2016-11-26 10:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
poc file (832 bytes, text/vnd.trolltech.linguist)
2016-11-24 20:22 UTC, Hanno Böck
  Details
proposed patch (613 bytes, patch)
2016-11-24 20:23 UTC, Hanno Böck
needs-work Details | Review
mpegtssection: Add more section size checks (2.34 KB, patch)
2016-11-26 09:46 UTC, Edward Hervey
committed Details | Review

Description Hanno Böck 2016-11-24 20:22: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
Comment 1 Hanno Böck 2016-11-24 20:23:03 UTC
Created attachment 340713 [details] [review]
proposed patch
Comment 2 Sebastian Dröge (slomo) 2016-11-24 20:44:08 UTC
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).
Comment 3 Hanno Böck 2016-11-24 21:00:38 UTC
Sorry, patch was probably too sloppy. I just added all the data++ /+=, maybe I miscalculated somewhere.
Comment 4 Sebastian Dröge (slomo) 2016-11-24 21:27:35 UTC
The same problem probably exists in similar places here in the mpegts lib, needs a review of the other code.
Comment 5 Sebastian Dröge (slomo) 2016-11-24 21:29:06 UTC
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.
Comment 6 Edward Hervey 2016-11-26 09:46:14 UTC
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).
Comment 7 Edward Hervey 2016-11-26 10:15:25 UTC
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