GNOME Bugzilla – Bug 724464
mpegts: does not check data sizes when parsing descriptors
Last modified: 2014-02-25 09:22:43 UTC
See summary, I noticed this while reviewing bug #724069. As these descriptors come from an untrusted source I would say that checking at least the size before parsing would make a lot of sense to prevent crashes and other interesting effects.
There's even a FIXME in the code about it :) /* * TODO * * * Add common validation code for data presence and minimum/maximum expected * size.
commit 142233d9171c6bcc93717bba48a11841c01da20c Author: Edward Hervey <edward@collabora.com> Date: Thu Feb 20 19:08:33 2014 +0100 mpegts: Add size guards for descriptors where neeeded Fixes https://bugzilla.gnome.org/show_bug.cgi?id=724464
I don't think g_return_val_if_fail() are good for this. It's for catching programming mistakes, not for catching invalid input streams.
Whoops, good point. I'll change all those g_return_val_if_fail in descriptor parsing to regular return FALSE.
Actually ... I'm wondering if all the following checks in descriptor parsing shouldn't be moved to regular returns: * descriptor presence (desc != NULL) * non-empty descriptor (desc->data != NULL) * correct descriptor type (desc->-tag == <expected descriptor type>) * descriptor length (desc->length ...) Maybe the first one can be kept as a g_return_val_if_fail (it's an argument), but the others are all dependent on the data stream.
I'm thinking about something like this, which would reduce code, yet still checks for API arguments. (gstmpegts-private.h) #define __common_desc_checks(desc, tagtype, minlen) \ ((desc)->data != NULL && (desc)->tag == (tagtype) && (desc)->length >= (minlen)) #define __common_desc_checks_exact(desc, tagtype, len) \ ((desc)->data != NULL && (desc)->tag == (tagtype) && (desc)->length == (len)) (gstmpegtsdescriptor.c) gboolean gst_mpegts_descriptor_parse_iso_639_language (const GstMpegTsDescriptor * descriptor, GstMpegTsISO639LanguageDescriptor * res) ... g_return_val_if_fail (descriptor != NULL && res != NULL, FALSE); if (!__common_desc_checks(descriptor, 0x0A, 0)) return FALSE;
Created attachment 270136 [details] [review] mpegts: Fix descriptor checks Only use g_return_val_if_fail on provided direct arguments. The others get checked all the time.
Review of attachment 270136 [details] [review]: ::: gst-libs/gst/mpegts/gstmpegts-private.h @@ +52,3 @@ + ((desc)->data != NULL && (desc)->tag == (tagtype) && (desc)->length >= (minlen)) +#define __common_desc_checks_exact(desc, tagtype, len) \ + ((desc)->data != NULL && (desc)->tag == (tagtype) && (desc)->length == (len)) I think these should be logged with GST_WARNING() or something
Good point. And I'll also add a 'return value' argument to the macro to avoid having to constantly repeat the if(!..) return retval; in the calling code. I'll rename the macro acccordingly.
Created attachment 270223 [details] [review] mpegts: Fix descriptor checks Only use g_return_val_if_fail on provided direct arguments. The others get checked all the time.
Comment on attachment 270223 [details] [review] mpegts: Fix descriptor checks Looks good :)
commit 63145f4576ef9e910f8a4cbf9317c255a2729bca Author: Edward Hervey <bilboed@bilboed.com> Date: Mon Feb 24 15:52:53 2014 +0100 mpegts: Fix descriptor checks Only use g_return_val_if_fail on provided direct arguments. The others get checked all the time. https://bugzilla.gnome.org/show_bug.cgi?id=724464