erusev / parsedown

Better Markdown Parser in PHP
https://parsedown.org
MIT License
14.74k stars 1.12k forks source link

Large nested blockquote consumes huge amount of memory and crashes PHP #681

Closed lin-toto closed 5 years ago

lin-toto commented 5 years ago

My site has been experiencing attacks achieved by posting a large nested blockquote in the forum. It looks like this:

>>>>>>>>>>>>>>>>>>>>>>... (about 10k+ blockquote markings in one line)

This eventually exhausts all available memory and kills the PHP instance. I wonder if this is the nature of the parser or is it an issue?

This is sucessfully reproduced in the project demo (http://parsedown.org/demo). (sorry if this caused any damage)

NightScript370 commented 5 years ago

Can reproduce as well. This is most likely the nature of the parser, though it should be fixed.

aidantwoods commented 5 years ago

Thanks for reporting. Probably the best way to resolve this is for Parsedown to track and limit parse recursion depth to some sane limit that actual text isn't likely to reach, but also isn't going come close to consuming all of PHP's available memory. We can make this configurable in-case someone wants to lower or raise the limit depending on use cases.

I think in principle you'd still need to have some submission limit when accepting user data though: an attacker could just submit the > character n - 1 times (where n is the recursion limit) and submit it across multiple lines to achieve a similar effect even with a maximum depth in place.

Probably the preferable thing for Parsedown to do if the recursion limit is exceeded is to cease sub-parsing and output potentially deeper remaining text as if it were regular text, though perhaps having Parsedown stop and report failure might be useful too?

cebe commented 5 years ago

this has been reported here: https://github.com/erusev/parsedown/pull/86 but never got merged.

Probably the preferable thing for Parsedown to do if the recursion limit is exceeded is to cease sub-parsing and output potentially deeper remaining text as if it were regular text

:+1:

though perhaps having Parsedown stop and report failure might be useful too?

when converting markdown to html there is no failure condition, everything that is not valid markdown will remain plain text. So I think also in this case continuing without error is the best solution. Afaik this is what other parsers do in such cases. E.g. https://github.com/thephpleague/commonmark/blob/1804ff378ff685b239ef29bdc0b176979f41b064/tests/functional/MaximumNestingLevelTest.php

aidantwoods commented 5 years ago

9eb6a02 adds a recursion limiter to the 2.0.x branch. Bear in mind that accepting large input could still cause PHP to crash due to not having enough memory to, for example, copy a string.