Privatebin: HTML injection via unescaped attachment filename

[elrido]

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]

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]

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

[elrido]

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.