GNOME Bugzilla – Bug 774859
flic decoder: Invalid memory read in flx_decode_chunks
Last modified: 2016-11-23 10:47:01 UTC
Created attachment 340550 [details] poc file The attached file will cause a read access to invalid memory and a crash in the flic decoder of gst-plugins-good. Tested with the latest git code, found with the tool american fuzzy lop. With a build with address sanitizer I get this error message: ==14717==ERROR: AddressSanitizer: SEGV on unknown address 0x61d0303670be (pc 0x7f10f1f76910 bp 0x7f10f3dec770 sp 0x7f10f3dec520 T1) ==14717==The signal is caused by a READ memory access. #0 0x7f10f1f7690f in flx_decode_chunks /f/gstreamer/gst-plugins-good/gst/flx/gstflxdec.c:255:9 #1 0x7f10f1f7690f in gst_flxdec_chain /f/gstreamer/gst-plugins-good/gst/flx/gstflxdec.c:636 #2 0x7f11009a90b7 in gst_pad_chain_data_unchecked /f/gstreamer/gstreamer/gst/gstpad.c:4206:11 #3 0x7f11009ac887 in gst_pad_push_data /f/gstreamer/gstreamer/gst/gstpad.c:4458:9 #4 0x7f11009abeff in gst_pad_push /f/gstreamer/gstreamer/gst/gstpad.c:4577:9 #5 0x7f10f5633af9 in gst_type_find_element_loop /f/gstreamer/gstreamer/plugins/elements/gsttypefindelement.c:1180:11 #6 0x7f1100a735c3 in gst_task_func /f/gstreamer/gstreamer/gst/gsttask.c:334:5 #7 0x7f10ffd80867 (/usr/lib64/libglib-2.0.so.0+0x70867) #8 0x7f10ffd7fed4 (/usr/lib64/libglib-2.0.so.0+0x6fed4) #9 0x7f10ff6ef443 in start_thread (/lib64/libpthread.so.0+0x7443) #10 0x7f10ff21e92c in clone (/lib64/libc.so.6+0xe792c) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV /f/gstreamer/gst-plugins-good/gst/flx/gstflxdec.c:255:9 in flx_decode_chunks Thread T1 (typefind:sink) created by T0 here: #0 0x42e81d in __interceptor_pthread_create (/usr/bin/gst-launch-1.0+0x42e81d) #1 0x7f10ffd9dadf (/usr/lib64/libglib-2.0.so.0+0x8dadf) ==14717==ABORTING
Created attachment 340576 [details] [review] flxdec: Just remove the whole element The code is terrible, it should be entirely re-written using GstByteReader.
Created attachment 340577 [details] [review] flxdec: rewrite logic based on GstByteReader/Writer
Comment on attachment 340576 [details] [review] flxdec: Just remove the whole element ... which happened now :)
Review of attachment 340577 [details] [review]: Generally looks good (just sanity-checked the parsing/writing). Just some comments that would be easy to fix, then merge please ::: gst/flx/flx_color.c @@ -102,3 @@ memcpy (&flxpal->palvec[start * 3], newpal, grab * 3); } - There's a size calculation in this file: guint size = flxpal->width * flxpal->height Do we check that this a) can't overflow and b) we have that much data available in the caller? ::: gst/flx/gstflxdec.c @@ +709,3 @@ + available = gst_adapter_available (flxdec->adapter); + input = gst_adapter_get_buffer (flxdec->adapter, available); + if (!gst_buffer_map (input, &map_info, GST_MAP_READ)) { Just to be sure: get_buffer() is what is wanted here? It removes the whole data from the adapter, i.e. if it contains less than a complete frame, we would throw that away? @@ +728,1 @@ gst_adapter_flush (flxdec->adapter, FlxHeaderSize); You removed all above, so flushing here won't work @@ +747,2 @@ GST_LOG ("size : %d", flxh->size); GST_LOG ("frames : %d", flxh->frames); A bit further down, flxdec->size = ((guint) flxh->width * (guint) flxh->height) And width/height come directly from the header. Do we make sure that a) this can't overflow (sanity check width/height) and later b) size * 4 can't (that's what we allocate)
(In reply to Sebastian Dröge (slomo) from comment #4) > Review of attachment 340577 [details] [review] [review]: > > Generally looks good (just sanity-checked the parsing/writing). Just some > comments that would be easy to fix, then merge please > > ::: gst/flx/flx_color.c > @@ -102,3 @@ > memcpy (&flxpal->palvec[start * 3], newpal, grab * 3); > } > - > > There's a size calculation in this file: guint size = flxpal->width * > flxpal->height > > Do we check that this a) can't overflow and b) we have that much data > available in the caller? width/height are from the header and are at maximum 16-bit integers and ((2**16-1)**2) fits in 32-bits. We always allocate width * height so yes, always available. > ::: gst/flx/gstflxdec.c > @@ +709,3 @@ > + available = gst_adapter_available (flxdec->adapter); > + input = gst_adapter_get_buffer (flxdec->adapter, available); > + if (!gst_buffer_map (input, &map_info, GST_MAP_READ)) { > > Just to be sure: get_buffer() is what is wanted here? It removes the whole > data from the adapter, i.e. if it contains less than a complete frame, we > would throw that away? get_buffer() doesn't remove, take_buffer() does though. > @@ +728,1 @@ > gst_adapter_flush (flxdec->adapter, FlxHeaderSize); > > You removed all above, so flushing here won't work See above. > @@ +747,2 @@ > GST_LOG ("size : %d", flxh->size); > GST_LOG ("frames : %d", flxh->frames); > > A bit further down, flxdec->size = ((guint) flxh->width * (guint) > flxh->height) > > And width/height come directly from the header. Do we make sure that a) this > can't overflow (sanity check width/height) and later b) size * 4 can't > (that's what we allocate) size = width * height doesn't overflow (as above). size * 4 will overflow with 32-bit gsize.
Ok, so the "out = gst_buffer_new_and_alloc (flxdec->size * 4);" is problematic (and we should probably prevent it from the beginning by sanity-checking width/height when parsing the header). Everything else seems fine then
commit 153a8ae752c90d07190ef45803422a4f71ea8bff Author: Matthew Waters <matthew@centricular.com> Date: Wed Nov 23 07:09:06 2016 +1100 flxdec: rewrite logic based on GstByteReader/Writer Solves overreading/writing the given arrays and will error out if the streams asks to do that. Also does more error checking that the stream is valid and won't overrun any allocated arrays. Also mitigate integer overflow errors calculating allocation sizes. https://bugzilla.gnome.org/show_bug.cgi?id=774859
http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/?id=153a8ae752c90d07190ef45803422a4f71ea8bff
1.8 7c7a9f8df1b545eec1ef4461b742c3696d4d0e9e 1.10 be670f0daf67304fb92c76aa09c30cae0bfd1fe4