Potential XSS Vulnerability With Filter and Function Components

[psmoros]

Hello 👋

I run a security community that finds and fixes vulnerabilities in OSS. A researcher (@Rudloff) has found a potential issue, which I would be eager to share with you.

Could you add a SECURITY.md file with an e-mail address for me to send further details to? GitHub recommends a security policy to ensure issues are responsibly disclosed, and it would help direct researchers in the future.

Looking forward to hearing from you 👍

(cc @huntr-helper)

[jralph]

Hi,

Thanks for getting in touch.

Would you be able to add the issue here? I don't have an email I would be putting in the SECURITY.md file and would prefer any issues to be handled through GitHub issues as this project is fairly old and somewhat abandoned due to myself not having worked with PHP in a fair few years now.

If there are any security issues I would prefer them to be visible to anyone thinking of using the project so they can make their own judgment if I am unable to find the time to resolve the problems.

[Rudloff]

The report is about a potential XSS vulnerability.

Description

The |markdown filter and markdown() function don't sanitize HTML passed to them.
Filters and functions that use is_safe need to make sure the HTML they output is actually safe.

Proof of Concept

{# Dangerous user-provided value #}
{% set payload = '<script>alert(`XSS`);</script>' %}

{# Here it is correctly escaped #}
{{ payload }}

{# Here too #}
{% markdown %}{{payload}}{% endmarkdown %}

{# Here it is not #}
{{ markdown(payload) }}
{{ payload|markdown }}

A solution could be to use 'pre_escape' => 'html' so that Twig escapes the HTML before passing it to the filter. (It would then have a similar behaviour to the {% markdown %} tag.) However the pre_escape setting only exists on filters and not functions.

Impact

If a website allows users to enter Markdown and then renders it with this filter, it could allow an attacker to inject arbitrary JS into the page.

[jralph]

Thank you for this! I'll see if I can get an update in to fix this. Or if anyone is able to do a PR, feel free to and I'll get it approved and merged!

I'll changed the title of this issue to reflect the vulnerability found so people are easily able to see what the security issue is.

[jralph]

For now, I have updated the readme to include documentation under both the filter and function to state that the input is not sanitized.

[jralph]

I've fixed the XSS issues and added in some documentation around this, as well as added in tests to confirm the XSS fixes work.

@Rudloff If you would like a look, the PR is here: #12

[jralph]

As an update to this issue, I've chosen not to directly fix the potential problems as they would cause a breaking change to the current way the plugin works. (It is fully valid to use HTML in Markdown, so changing the functions to remove HTML would technically break the potential current uses people have.)

I plan to make a v3 release in the future that supports twig 3, and I will likely look changing the functionality to be sanitised by default, but with an option to allow raw parsing if the user desires. This will change usage slightly, and will not be fully compatible with the current setup, but will follow a safe by default approach which I prefer.

For now, I've updated the docs to explain that the user should ensure they are sanitising their input when using the function, filter, or global. But do not need to sanitise input when using the tag.