Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for nested incomplete tag sanitization #473

Merged
merged 3 commits into from Sep 22, 2022

Conversation

vin01
Copy link
Contributor

@vin01 vin01 commented Aug 30, 2022

Hi, this is just a test demonstrating nested incomplete tags resulting in XSS. As there. have been multiple bypasses since I reported #285, I still think introducing Bleach might be worthwhile. Perhaps in a major version as it might involve breaking changes for rendering. Bleach is a bit more aggressive than current implementation for escaping and replacing in markdown2.

>>> bleach.clean('<blah/<img src="" onerror=alert(/XSS/)')
''

@Crozzers
Copy link
Contributor

I think this can be fixed relatively easily without breaking too much.

Safe mode calls upon _encode_incomplete_tags to deal with tags that aren't properly closed, which uses a regex to match such tags. Passing in the following line from the test case you added:

<img <img src="" onerror=alert(/XSS/)

The regex will match the substring of 'img <img '. Currently the function returns like so:

return self._incomplete_tags_re.sub("&lt;\\1", text)

Which only replaces the initial < and not the nested one. I think if the function was changed to return like so:

        def incomplete_tags_sub(match):
            return match.group().replace('<', '&lt;')

        return self._incomplete_tags_re.sub(incomplete_tags_sub, text)

Then this would fix the problem. From the tests I've done, this does seem to work as intended, with the output of the above line being:

&lt;img &lt;img src="" onerror=alert(/XSS/) 

@nicholasserra
Copy link
Collaborator

Another example Vin emailed to me, might as well put it in here:

<blah/<img src="" onerror=alert(/XSS/)


>>> markdown('<blah/<img src="" onerror=alert(/XSS/) ', safe_mode=True)
'<p>&lt;blah/<img src="" onerror=alert(/XSS/) </p>\n'

I think it's fine we keep chasing these assuming they don't take too much time. Maybe we add a warning for users to use bleach if they want better xss handling.

@Crozzers
Copy link
Contributor

Just tested this:

<blah/<img src="" onerror=alert(/XSS/)

And my suggested fix does cover this with no breakages of the test suite.

@nicholasserra
Copy link
Collaborator

Nice. I was planning on doing a release this week. If you wanna try to sneak that in let me know. Then going to probably cut another release after that with your setup.py changes PR.

Remove trailing whitespaces in `nested_incomplete_tags_xss.html` to make tests pass
@Crozzers
Copy link
Contributor

I've opened a PR on @vin01's fork so that we can keep this inside the same PR here, instead of spinning up another one and closing this one.

Don't mind when this is released but I have got a PR for adding wavedrom support (#467) queued up after the setup.py stuff is merged

…ation

Fix sanitization for nested incomplete tags (see trentm#473)
@nicholasserra nicholasserra merged commit 9b6fc0b into trentm:master Sep 22, 2022
@nicholasserra
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants