Parsedown dependency - Security concerns #23304
Comments
|
The "security issue" here is that generated HTML may contain XSS, which can be solved by escaping output yourself. See example blow. FriendsOfPHP/security-advisories has included it as well. We considered switching to CommonMark a while back, but it doesn't support tables (GFM). Parsedown has already been forked, maybe we can switch to the community maintained version if the original author of Parsedown doesn't get his act together. This is what I personally use for escaping Parsedown output (@JosephSilber suggested using Stauros a while back): use Parsedown;
use Stauros\Stauros;
use Stauros\HTML\Config;
$parsedown = (new Parsedown)->setMarkupEscaped(true);
$stauros = new Stauros(
new Config([
'a' => ['href' => true, 'title' => true],
'b' => [],
'br' => [],
'blockquote' => ['cite' => true],
'cite' => [],
'code' => [],
'em' => [],
'i' => [],
'p' => [],
'q' => ['cite' => true],
's' => [],
'strike' => [],
'strong' => [],
])
);
$html = $stauros->scanHTML(
$parsedown->text($markdown)
); |
|
I'm including a reference here to an example of a security issue in Parsedown, since a lot of people seem to keep jumping the gun a bit on whether or not a markdown parser with XSS is a security issue (given markdown permits HTML). The following demonstrates an AST rendering bug. Roave/SecurityAdvisories#44 (comment)
|
|
If the maintainer of Parsedown is unwilling or unable to continue, should it be replaced with |
Personally I'd like to give the maintainer a little bit longer to respond to erusev/parsedown#495 (comment), maybe we can get some more maintainers added to the core repo so that we don't have to count on everyone changing a dependency. In the interim there is some progress on applying updates and bug fixes proposed in erusev/parsedown#553, so that would at least be a good place to start moving forward regardless. Or at the very least get something ready if it does come to maintaining a fork (if it comes to it I'll transfer the repo to wherever makes sense if there's some consensus on where it should go, just want to get some work started for now – or just fork the work, its open-source so 🤷♂️). |
|
Out of interest, why does https://github.com/ircmaxell/Stauros parse the HTML in PHP manually? Can't it use I kind of agree with what Taylor has said on twitter about this which is why is the Markdown parser responsible for taking care of XSS attacks? It takes markdown and returns HTML. Is it sensible to sanitise the output? Absolutely, but that should be an additional package with sensible defaults and ways to override, no? In my mind, it's not really a markdown parser problem, it's more of a "general html sanitization" problem. |
@taylorotwell has since revised his opinion, see FriendsOfPHP/security-advisories#257 (comment) |
Fair enough. I think Taylor's original stance on it made a lot of sense, though. I guess the author of the markdown parser shot himself in the foot when they begun supporting escaping / sanitising markdown - I personally would of had that as a separate package, it should be in the realm of userland, as Taylor said. There's a few sanitation packages out there, html purifier seems the most advanced at detecting XXS. XSS is a complex problem, I suspect a lot of PRs to Markdown parser will be needed to fix all known examples - see RSnake's famous cheatsheet of XSS attacks http://htmlpurifier.org/live/smoketests/xssAttacks.php |
This is why in erusev/parsedown#495 I simplified the problem considerably by assuming all element content and attribute values are hostile and addressed most issues by applying proper contextual encoding to Parsedown's AST output (individual handlers no longer need apply encoding manually, where it can be forgotten – in-fact this was literally one of the bugs). Essentially this is context isolation to ensure that data cannot cross syntax borders that would affect its interpretation (like quotes when data is meant to be in an attribute, or angle brackets when data is meant to be within a tag). For sanitisation problems (like scripting via certain schemes in link destinations) the patch allows Parsedown to maintain a whitelist of allowed schemes, and in the event one is not used, additional encoding will be applied to prevent the contents of an attribute from containing a valid scheme (essentially only relative paths are permitted in the case that an unknown scheme is used – which prevents All that said, I'm not perfect, and I invite anyone to try and break the patch so I can make it better if they succeed. (As mentioned in the other thread) as a defence in depth measure, you can additionally run something like HTMLPurifier on Parsedown's output, and deploy a CSP (I strongly recommend doing the latter in any case). |
|
I am running into a related composer update issue: I have added Installation on a new machine was ok (I had a composer.lock file committed). But, No way to update to the latest Laravel 5.6 version. My only workaround is to temporarly remove Any better workaround for now (until parsedown is fixed and/or you switch to another MD implementation)? I currently use |
|
Can confirm, composer update is now broken for everyone using Sad fact: I'm not even using that capability but it still prevents me from updating Laravel ATM. |
|
The PR that fixes the security issue has been merged, so executing |
Description:
erusev/parsedown, a package used by Laravel, currently has unfixed security issues (see erusev/parsedown#495). These have been open since mid-ish-2017, the maintainer seems to have abandoned the project, leaving them unfixed. roave/security-advisories has recently included it in their vulnerable packages list (people using security-advisories on CI/CD systems may end up with errors during tests/deployment).
Steps To Reproduce:
N/A
Suggestions
It'd suggest moving away from this package to a more actively maintained MD parser.
The text was updated successfully, but these errors were encountered: