[FFmpeg-devel] [PATCH 1/2] avcodec/htmlsubtitles: Protect very slow redundant sscanf() calls by optimized use of strchr()

[Uoti Urpala]

On Sat, 2017-06-10 at 16:18 +0200, Michael Niedermayer wrote:
> So we are guranteed that anyone who wants to exploit this has the
> ability to do so as long as they can use the search mask and are able
> to remux the data into whatever format they need.
> And i belive this publication of issues is the right thing to do.

It looks like the current code has quadratic time requirements. Is
everything else in FFmpeg actually guaranteed to not need quadratic
time? Can anything really rely on FFmpeg decoding of hostile data not
taking a long time? To the degree that it would be reasonable to have a
system using FFmpeg on untrusted data without timeouts or capacity to
kill the process? To me being slow on malicious data doesn't seem like
a real security issue.


> Do you have a better idea than the next_closep code to fix this ?
> If not, do you agree that we push this fix ?

Since the slow behavior seems unlikely to occur except with
intentionally malicious or completely corrupted data, I'm not sure it's
worth actually fixing it. But I think it could be done somewhat more
cleanly as follows:
Instead of using scanf to find matches for "{Y:xxx}" or "{\xxx}", where
"xxx" is arbitrarily long, match for "{Y:" or "{\", and then do the
"skip until next }" as a separate step after you've confirmed a match.
Then you can optimize that to avoid quadratic behavior by setting a
flag when you find no "}", and not searching again if it's already set.

Also, the current code seems buggy, or at least I don't see why you'd
want it to behave like it now does. It skips "{\xxx}" tags when the
"an" variable is not set to 1. I assume the intent was to only allow
the first "\an" tag through. But as implemented now it seems to allow
ANY "{\xxx}" tags through after the first \an and before possible
second one. I don't think that's intentional?