GNOME Bugzilla – Bug 737591
rtpgstdepay: buffer overread
Last modified: 2018-01-23 00:43:11 UTC
Created attachment 287354 [details] [review] fix buffer overread This ensures we don't read a string past the end of the buffer. One could make an argument that the terminating NUL should be included in the data instead. This seems safer (and would have to be done anyway since the data might be corrupt).
Comment on attachment 287354 [details] [review] fix buffer overread Makes sense and should be fixed IMHO, but would be nice to avoid the dup if not required (these strings can be really huge).
Shouldn't we have a gst_structure_from_string_n(gchar *str, gint size) ? Having a function that deals with strings without checking them seems dangerous to start with.
Created attachment 287569 [details] [review] add gst_structure_from_string_n Like this ?
Created attachment 287570 [details] [review] fix buffer overead
Well, if it's not 0-terminated it's by definition not a valid string, so arguably in the 0.0000001% of the cases where we process untrusted data we might just as well go through the trouble of checking. It would also be ok to just ignore the input then in such code, arguably. So not sure if we really need to add new API for this one use case. If we do, do we want to keep the end out argument? (Which is only used internally by gstreamer when deserializing caps I think) (And which shouldn't have been in the from_string() API in the first place).
I think this is the best patch, check if the delimiter is where it is expected and refuse all other input. For the caps caps with the 0-terminator, this is ok but I'm unsure if it's possible to escape the ; somewhere in a valid event.. commit 9d2902d978905acf3ecc88dd331b20072dc9eab0 Author: Wim Taymans <wtaymans@redhat.com> Date: Thu Nov 20 12:40:28 2014 +0100 rtpgstdepay: avoid buffer overread. Check that a caps event string is 0 terminated and the event string is terminated with a ; to avoid buffer overreads.
A little better but still dangerous because core will read past the 0-byte when \ is the last char of the string. commit 3d7b0f30d748ad1c8e2c5d3528bfbdfe21cfde60 Author: Wim Taymans <wtaymans@redhat.com> Date: Thu Nov 20 13:14:14 2014 +0100 rtpgstpay: put 0-byte at the end of events Put a 0-byte at the end of the event string. Does not break ABI because old depayloaders will skip the 0 byte (which is included in the length). Expect a 0-byte at the end of the event string or a ; for old payloaders. See https://bugzilla.gnome.org/show_bug.cgi?id=737591
Is this complete with this commit? Also should we backport the rtgst* changes to 1.4 too? Seems relatively safe, I'm just a little bit worried that we missed a backwards incompatible change in here. commit 5cf630697c55c49c60862494712c9f32f55fcf0f Author: Wim Taymans <wtaymans@redhat.com> Date: Thu Nov 20 13:34:32 2014 +0100 structure: don't overread input when searching for " When searching for the string terminator don't read past the ending 0-byte when escaping characters. Add unit test for various escaping cases.
(In reply to comment #8) > Is this complete with this commit? Also should we backport the rtgst* changes > to 1.4 too? Seems relatively safe, I'm just a little bit worried that we missed > a backwards incompatible change in here. I think it's safe to backport. I can't see a scenario that would cause failure. the compatibility matrix is like: * old pay -> old depay: no change, with upgraded core it would not crash on certain malformed input. * old pay -> new depay: new depay finds ; at end and parses the message. core change doesn't matter. * new pay -> old depay: new depay puts extra 0 after string. old depay just parses and skips the extra 0 byte (because it is included in the length). With new core, certain malformed payloads are not crashing. * new pay -> new depay: new pay adds extra 0 after string, new depay checks for 0 and decodes. new core will avoid certain buffer overreads.
Backported :) Can we close this bug now or does anybody think the other things should be merged too?