Privatebin: Preview sourcecode in 1.3.3 is broken

Created on 25 Feb 2020 · 18Comments · Source: PrivateBin/PrivateBin

After the update to 1.3.3 it seems that the source code preview is broken.

Steps to reproduce

Add some example PHP code
Change format to Source code
Press preview

What happens

It's not correct. A lot of strings are removed/stripped.

What should happen

Same behaviorus as in 1.3.2. Properly formatted code.

Additional information

After the downgrade (1.3.2) everything works again.

Basic information

Server OS:
Linux

Webserver:
NGINX

Browser:
Firefox / Chromium

PrivateBin version:
1.3.3

I can reproduce this issue on https://privatebin.net: Yes

bug

Source

rodehoed

👍1

All 18 comments

I tried to reproduce this with some privatebin PHP code. I noticed that the <?php on the first line gets removed (I assume by the DOMpurify library), but every thing else remained intact. When using one of our template files, it seems that anything resembling HTML tags is now very aggressively removed. Since version 1.3.2 we did upgrade Dompurify from 2.0.7 to 2.0.8, so maybe it got more strict on what tags it filters? Have to investigate some more.

elrido on 25 Feb 2020

👍1

it seems that anything resembling HTML tags is now very aggressively removed. Since version 1.3.2 we did upgrade Dompurify from 2.0.7 to 2.0.8, so maybe it got more strict on what tags it filters?

To be a bit more specific, it does filter (some) things such as the PHP start syntax. But slightly worse, it actually parses some tags too. Try this, for example:

<strong>Some bold text</strong>

Not sure if this creates a potential security risk, basic XSS/JS examples don't seem to work as far as I can tell. Can't say anything about more complex code though. Nevertheless, parsing (some) tags doesn't seem like a good thing. :)

LH-Eelko on 25 Feb 2020

Ugh yeah, it should just assign it as .textContent, then we don't need DomPurify. There is definitively something wrong and too aggressive. :thinking:

rugk on 25 Feb 2020

👍1

Unfortunately we can't just side step HTML entities in that case:

https://github.com/PrivateBin/PrivateBin/blob/b0800060c271aab29abe7f294f66f4d5b6cfcc65/js/privatebin.js#L2425-L2472

In case of the source code view, the prettyPrint() function is expected to return HTML, that we have to trust (but wanted to purify, just in case). The same is true for markdown and even in plain text we do the Helper.urls2links(), which will add links into the text.

elrido on 25 Feb 2020

The question would be, does the source code ever need HTML links??
Given https://github.com/PrivateBin/PrivateBin/issues/579 people seem to want it, but this is only about the non-source-code format, I guess "text"?

And in the Markdown version you'd obviously never post source code without code blocks, and can expect it to be interpreted as HTML.

So I guess _for source code format_ we can disable url2links at least.

Now, I also just see we need to prettify. This is this lib AFAIK. As I am not sure what they even recommend to do against XSS, I've asked.

Question would be: Could not we do a trick and put the source code into a Markdown ``` tag internally, then use it? Okay no, showdown does not even support syntax highlighting out-of-the-box.

Or, no, what would actually work with DomPurify. Can we put a <code> or <pre> tag around the source code, then DomPurify it and that's it?
Actually, we can just insert the code element into the DOM then directly after processing. 🤔

rugk on 26 Feb 2020

Keep up the deep thinking - I'm doing some notes of my own and will compare yours with mine over the weekend, providing any change ideas via PR for review.

The problem occurs in plain text as well, where we already use a pre tag. My gut feeling at the moment is that we will have to encode the HTML entities, before passing it to dompurify (only in the plain text and source code cases, not the markdown) and adjust the urls2links to recognize the encoded links, so that there should be no more "bad" HTML tags that dompurify considers removing.

elrido on 26 Feb 2020

If you try some variations of how HTML code is escaped or not:

<pre><code>
<p>dff</p>
</code>
</pre>

You will notice the p tag is rendered, which is bad, because then DomPurify may mangle with it.
So pre is not enough, code neigher.
See this comparison, which gives some interesting results:

we could use a textarea, there nothing is rendered
some deprecated stuff I ignore
or as this doc states, it is enough to encode '<' character as '<' , which interestingly does work:
<pre><code> <p>dff</p> </code> </pre>
That means we don't have to encode too much. (and reverse or adjust things then)

rugk on 26 Feb 2020

this also breaks my C++ code when pasting it:

#include <foo>
#include <bar>
#include <stdint.h>
#include <cstdint>

#include "my_own_header.hpp"

the first 4 lines will be missing any characters in the preview after the <.
viewing paste as raw reveals the code fine

angelfor3v3r on 2 Mar 2020

Dropping a comparison matrix in here to have them all in one place, discussion on the PRs back in the PRs, of course:

Back in 1.0, we didn't yet use DOMpurify, we just encoded all of the HTML entities by inserting them as text. The urls2links got applied on top of the already encoded text.

elrido on 5 Mar 2020

inserting them as text. The urls2links got applied on top of the already encoded text.

Wow, well, nice solution, is not it? Why did we drop that? Why not go back to that? (Stupid I did not consider/thought about that...)
Okay DomPurify last always may be more secure, but mhh.

rugk on 6 Mar 2020

Ok, let me know if we want to go back to that plus DOMpurify. Do you do a new PR or should I?

Also, we both seemed to have felt changes with the switch() statement to be necessary. Would you want to include d2e9e47b673f272772a8c2c0ca6736eca083050c or f13a5d0a553357082503c3ba308391b506e59367 or leave it as is for now?

elrido on 6 Mar 2020

Well... it is a possibility, but if #591 works, and I see no problem in your example, then why not use #591?

rugk on 6 Mar 2020

I am sorry, but there are clear problems with #591, as it only fixes the display of <, but doesn't correctly display any of the other entities:

| #589 | #591 |
|-----|-----|
| entities-589 | entities-591 |
| xss-589 | xss-591 |