Tui.editor: Consider using DOMPurify to sanitize HTML

[Joris-van-der-Wel]

I am trying out this library and I am really liking it so far. However I like to review the code that I use in my projects and I noticed a few dangerous things related to XSS vulnerabilities.

I would suggest using a library like DOMPurify which has seen a lot more scrutiny in this regard. I mention DOMPurify specifically because I am familiar with it.

Examples

Here are a few random things that I saw while reading over the code:

_replaceImgAttrToDataProp()

https://github.com/nhn/tui.editor/blob/b4cd04d53fac3e92188f9ef926dab6d021c1a672/src/js/convertor.js#L141-L149
I am not entirely sure what this is used for, but it can be bypassed by passing <img foo=">" src=x onerror="alert(1)">

[OverrideBuiltins]

https://github.com/nhn/tui.editor/blob/b4cd04d53fac3e92188f9ef926dab6d021c1a672/src/js/htmlSanitizer.js#L70-L88
Using DOM to sanitize is a good approach, however browsers have some quirks that could allow for a bypass.

For example:

A <form> element is marked with the [OverrideBuiltins] attribute in the HTML spec (see https://html.spec.whatwg.org/#htmlformelement and https://heycam.github.io/webidl/#OverrideBuiltins), which means that it is possible to replace DOM properties using just HTML.

Luckily in the current version of this library, all <form> elements are removed unconditionally:
https://github.com/nhn/tui.editor/blob/b4cd04d53fac3e92188f9ef926dab6d021c1a672/src/js/htmlSanitizer.js#L61-L63

However this might become a problem if https://github.com/nhn/tui.editor/issues/684 lands:

input = `<form onsubmit="alert(1)">
    <input type="submit">
    <input type="hidden" name="attributes">
</form>`;

console.log(htmlSanitizer(input, true, ['form']));

/* output:
<form onsubmit="alert(1)">
    <input type="submit">
    <input type="hidden" name="attributes">
</form>
*/

javascript: URL's

https://github.com/nhn/tui.editor/blob/b4cd04d53fac3e92188f9ef926dab6d021c1a672/src/js/htmlSanitizer.js#L31-L33
Sanitizing href's with a blacklist is tricky because there are many ways in which the browser will interpret an URL as a javascript URL. For example adding a space is enough to bypass this check and in firefox I will see an alert:

input = `123<a href=' javascript:alert(1)'>CLICK</a>`;
console.log(htmlSanitizer(input, true));
// output: 123<a href=" javascript:alert(1)">CLICK</a>

DOMPurify example

Here is how I integrated DOMPurify with this library. However note that I have not yet finished testing if this is completely safe.

import Editor from 'tui-editor';

// This is the config that I need for my use case, but it would have 
// to be expanded upon to support everything that tui supports.
const purifyOptions = {
    ALLOWED_TAGS: [
        'a', 'blockquote', 'br', 'code', 'em', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6',
        'hr', 'li', 'ol', 'p', 'pre', 'q', 's', 'span', 'strong', 'table', 'tbody',
        'td', 'th', 'thead', 'tr', 'ul',
    ],
    ALLOWED_ATTR: [
        'align', 'class', 'colspan', 'href', 'rowspan', 'style',
    ],

    // data-xxx
    ALLOW_DATA_ATTR: true,
};

const createMarkdownEditor = (options) => {
    const editor = Editor.factory({
        ...options,
        // do not allow initialValue because the unsafe value 
        // might be interpreted before the hook can be set.
        initialValue: '',
    });

    editor.addHook('convertorAfterMarkdownToHtmlConverted', (html) => {
        // important: never return something that is falsy from this 
        // function, otherwise the hook will be ignored
        return DOMPurify.sanitize(html, purifyOptions) || ' ';
    });

    return editor;
};

[seonim-ryu]

@Joris-van-der-Wel Thanks for the good comments. Detailed explanation was also good! 👍

I know that there are many things to improve on the sanitize feature, so I have plans to improve it. I think it's a good idea to use DOMPurify. But processing it using convertorAfterMarkdownToHtmlConverted, as in your example, doesn't seem to be a perfect solution. The editor needs to open the interface in a structure that allows to use libraries like DOMPurify.

I'll apply it when I improve it by reflecting your opinion. And if possible, you can contribute! :)

[Dassderdie]

Hi there :)

We are actually considering moving from tui.editor to another markdown-editor solely because of its XSS-vulnerabilities.

Our project uses angular und we would love it, if we could just pass angulars own sanitizer to this editor.

Thank you for this otherwise wonderful editor and have a nice day

[Joris-van-der-Wel]

I'll apply it when I improve it by reflecting your opinion. And if possible, you can contribute! :)

Sure, if we come up with a good plan I can help to fix this.

My thoughts are:

1. We introduce a new callback option that can be passed to Editor.factory. For example: Editor.factory({sanitize: (html) => ...})
2. This new option is called at the appropriate times by tui-editor whenever something has to be sanitized
3. We design a default config for DOMPurify which works correctly with all of tui-editor's features.
4. We default the new option to call DOMPurify with the config from step 3

After these steps, the library would be secure by default, but the user can override the callback at his own risk.

Alternatively, we could consider to always apply DOMPurify and providing an API for users to modify our default config. However this seems like a lot more work.

What I would need help is, is to figure out what the appropriate places are to call this new callback option. Calling it at the same time as the convertorAfterMarkdownToHtmlConverted hook might be enough, but I am not familiar enough with the source code of the library to state this with confidence.

And I also might need some help with determining what an appropriate default config for DOMPurify would be. The config in my opening post works for me, however I am not sure if all the tui-editor plugins still work.

[seonim-ryu]

@Joris-van-der-Wel Thanks for the good comments. 👍 I did a feature review based on your comments.

Currently, the sanitizer built into the TOAST UI Editor has many improvements. So first, I'm going to enhance the editor's built-in sanitizer to properly solve XSS vulnerabilities. There is a burden on capacity to embed a module that handles sanitizing like DOMPurify, so I'll consider a bit more with the help of other modules.

Then, in addition to using DOMPurify to receive an option value such as sanitaize as a callback function as you have given for example, I will process it so that the user can use a wanted module to sanitize.

This feature is important and I'd like to do it as soon as possible, so I'm going to put it in a release this month.

Thank you for your infinite interest in our editor and wait a little longer! :)

[seonim-ryu]

@Joris-van-der-Wel The customHTMLSanitizer option has been added to 2.1.0 release, and this option allows you to use an external sanitizer module such asDOMPurify.

And the built-in sanitizer in the Editor will be improved in the future. I'll open and manage this issue separately.

Then I'll close this issue. If you have any problems, please register again. :)

[msaKnowis]

Hi all,

is there a plan to release these changes within a bugfix release in the near future?

Thanks in advance,
Michaela

[crux153]

@seonim-ryu Can't #1010 be considered as fix for this? I see 2.2.0 is released with the fix for the default HTML sanitizer module, but npm audit still reports this module to have high vulnerability. https://www.npmjs.com/advisories/1521

Is there any way we can fix this?

EDIT: Just found out that #1148 already exists, sorry. Please contact npm team to close this.

[seonim-ryu]

@crux153 Thanks for the reporting. I will take steps to resolve this issue quickly.