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

HTML injection via unescaped attachment filename #554

Closed
elrido opened this issue Dec 25, 2019 · 3 comments
Closed

HTML injection via unescaped attachment filename #554

elrido opened this issue Dec 25, 2019 · 3 comments

Comments

@elrido
Copy link
Contributor

elrido commented Dec 25, 2019

I investigated the error condition found in the property based unit test with RNG state 8b8f0d4ec2a67139b5 for the AttachmentViewer class in an unrelated change.

The error occurs in the AttachmentViewer.moveAttachmentTo() method, were we can provide a string that will have the filename inserted in place of any %s in that string and which then becomes the text node of the link to download the attachment. The error condition that jsverify found was when it generated a prefix <a for that label (so the label generated in that test case became <a%s). Since we insert that label untouched, it attempts to set another link into text node of a link, which isn't allowed in HTML and in the jsdom based environment gets silently ignored - Hence the test failed only for <a /> tags, as other tags are allowed inside a link.

While the label itself is from a trusted source (hardcoded in the translation files and privatebin.js), the filename that is injected isn't. If one would create a paste with attachment with a name containing HTML code (that can be easily done using one of the CLI clients), that paste would have that snippet injected when the paste is displayed.

I have prepared a patch and updated unit test case that does the HTML entity encoding to address this and will push this ASAP. While file uploads are disabled by default, I would still suggest that we publish a release with this fix.

Your opinions?

@elrido
Copy link
Contributor Author

elrido commented Dec 25, 2019

Sorry, while the fix addresses the issue, lets keep it open until we discussed if this warrants the publication of a release. I think it does, but would like to get some more brains and eyeballs on the change and opinions on the scope of the issue.

@rugk
Copy link
Member

rugk commented Dec 27, 2019

Maybe next time we should discuss this internally first before publicly fixing and releasing this? Responsible disclosure?

@elrido
Copy link
Contributor Author

elrido commented Jan 11, 2020

For the record, this issue had been created after I created and tested the initial fix and I had therefore assumed this to be in line with responsible disclosure, as it was only made public after already having been addressed. I'll discuss such cases in the future in the "Maintainers" github team first, in order to respect the wishes of all project maintainers on how to handle them.

@rugk suggested a more holistic solution that supersedes the initial fix (thanks again for that) and it is now part of the 1.3.2 & 1.2.2 releases. A more in depth look at this issue can be found in the report on the vulnerability.

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

No branches or pull requests

2 participants