laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.51k stars 11.02k forks source link

Parsedown dependency - Security concerns #23304

Closed Numline1 closed 6 years ago

Numline1 commented 6 years ago

Description:

erusev/parsedown, a package used by Laravel, currently has unfixed security issues (see https://github.com/erusev/parsedown/pull/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.

tillkruss commented 6 years ago

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)
);
aidantwoods commented 6 years ago

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.

https://github.com/Roave/SecurityAdvisories/issues/44#issuecomment-368594409

Parsedown has an option ->setMarkupEscaped(true). Among other things, the Wiki states that the goal of this is escaping HTML. Except, take a look at this v.s. what it should do (with the patch applied) screen shot 2018-02-26 at 17 51 02

As you can see, it is possible to get Parsedown to output arbitrary HTML when using this option (this is a bug with security consequences). There are multiple other issues fixed in https://github.com/erusev/parsedown/pull/495 too.

paragonie-scott commented 6 years ago

If the maintainer of Parsedown is unwilling or unable to continue, should it be replaced with laravel/parsedown instead? (i.e. let the community run its own fork, moving forward).

aidantwoods commented 6 years ago

If the maintainer of Parsedown is unwilling or unable to continue, should it be replaced with laravel/parsedown instead? (i.e. let the community run its own fork, moving forward).

Personally I'd like to give the maintainer a little bit longer to respond to https://github.com/erusev/parsedown/pull/495#issuecomment-368555723, 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 https://github.com/erusev/parsedown/issues/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 🤷‍♂️).

garygreen commented 6 years ago

Out of interest, why does https://github.com/ircmaxell/Stauros parse the HTML in PHP manually? Can't it use DOMDocument or some other form to sanitize / remove possible malicious HTML code? That seems far more efficient and safer.

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.

aidantwoods commented 6 years ago

@garygreen

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?

@taylorotwell has since revised his opinion, see https://github.com/FriendsOfPHP/security-advisories/pull/257#issuecomment-368592698

garygreen commented 6 years ago

@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

aidantwoods commented 6 years ago

XSS is a complex problem, I suspect a lot of PRs to Markdown parser will be needed to fix all known examples

This is why in https://github.com/erusev/parsedown/pull/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 javascript:, javascr\0ipt:, etc... and even newscriptingscheme: because it is a whitelist, and not a blacklist).

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).

vpratfr commented 6 years ago

I am running into a related composer update issue:

I have added roave/security-advisories as a dependency for security purpose (looks like a good practice to me and to the people at EAPhpInspections plugin for PHPStorm).

Installation on a new machine was ok (I had a composer.lock file committed). But, composer update failed to execute:

  Problem 1
    - Conclusion: remove roave/security-advisories dev-master
    - laravel/framework v5.6.0 requires erusev/parsedown ~1.6 -> satisfiable by erusev/parsedown[1.6.4, 1.6.0, 1.6.1, 1.6.2, 1.6.3].
    - laravel/framework v5.6.1 requires erusev/parsedown ~1.6 -> satisfiable by erusev/parsedown[1.6.4, 1.6.0, 1.6.1, 1.6.2, 1.6.3].
    - laravel/framework v5.6.2 requires erusev/parsedown ~1.6 -> satisfiable by erusev/parsedown[1.6.4, 1.6.0, 1.6.1, 1.6.2, 1.6.3].
    - laravel/framework v5.6.3 requires erusev/parsedown ~1.6 -> satisfiable by erusev/parsedown[1.6.4, 1.6.0, 1.6.1, 1.6.2, 1.6.3].
    - laravel/framework v5.6.4 requires erusev/parsedown ~1.6 -> satisfiable by erusev/parsedown[1.6.4, 1.6.0, 1.6.1, 1.6.2, 1.6.3].
    - laravel/framework v5.6.5 requires erusev/parsedown ~1.6 -> satisfiable by erusev/parsedown[1.6.4, 1.6.0, 1.6.1, 1.6.2, 1.6.3].
    - laravel/framework v5.6.6 requires erusev/parsedown ~1.6 -> satisfiable by erusev/parsedown[1.6.4, 1.6.0, 1.6.1, 1.6.2, 1.6.3].
    - laravel/framework v5.6.0 requires erusev/parsedown ~1.6 -> satisfiable by erusev/parsedown[1.6.4, 1.6.0, 1.6.1, 1.6.2, 1.6.3].
    - erusev/parsedown 1.6.0 conflicts with roave/security-advisories[dev-master].
    - erusev/parsedown 1.6.1 conflicts with roave/security-advisories[dev-master].
    - erusev/parsedown 1.6.2 conflicts with roave/security-advisories[dev-master].
    - erusev/parsedown 1.6.3 conflicts with roave/security-advisories[dev-master].
    - erusev/parsedown 1.6.4 conflicts with roave/security-advisories[dev-master].
    - roave/security-advisories dev-master conflicts with erusev/parsedown[1.6.4].
    - Installation request for roave/security-advisories dev-master -> satisfiable by roave/security-advisories[dev-master].
    - Installation request for laravel/framework 5.6.* -> satisfiable by laravel/framework[v5.6.0, v5.6.1, v5.6.2, v5.6.3, v5.6.4, v5.6.5, v5.6.6].

No way to update to the latest Laravel 5.6 version.

My only workaround is to temporarly remove roave/security-advisories but I don't really like that.

Any better workaround for now (until parsedown is fixed and/or you switch to another MD implementation)? I currently use michelf/php-markdown on some of projects.

mfn commented 6 years ago

Can confirm, composer update is now broken for everyone using roave/security-advisories too.

Sad fact: I'm not even using that capability but it still prevents me from updating Laravel ATM.

JeppeKnockaert commented 6 years ago

The PR that fixes the security issue has been merged, so executing composer update "erusev/parsedown" in your project will solve the issue.