SCEditor - Cross-site Scripting (XSS) - Fix:

[huntr-helper]

https://github.com/mufeedvh fixed the vulnerability associated with Cross-site Scripting (XSS).
This fix is being submitted on behalf of https://github.com/mufeedvh - they have been awarded $25 for fixing the vulnerability through the huntr bug bounty program.
Think you could fix a vulnerability like this - get involved (https://huntr.dev).
Q | A
Version Affected | ALL
Bug Fix | YES
Further References | 418sec#2

[JamieSlome]

@brunoais @samclarke - any updates on this?

[brunoais]

I'm not the keeper of the code so I won't approve this big change.
@huntr-helper or @JamieSlome :
Please provide an example of XSS that SCE suffers.

[JamieSlome]

@brunoais - thank you for the swift response! ⚡️

Please see the comments by the fixer below:

⚙️ Fix:

As editor.wysiwygEditorInsertHtml() is defined to accept HTML input, we need to allow HTML input but prevent JavaScript code to execute/XSS attacks. So, I used DOMPurify to accept HTML but no JavaScript thus fixing the XSS vulnerability.

❓ How Does DOMPurify Work?

DOMPurify sanitizes HTML and prevents XSS attacks. You can feed DOMPurify with string full of dirty HTML and it will return a string (unless configured otherwise) with clean HTML. DOMPurify will strip out everything that contains dangerous HTML and thereby prevent XSS attacks and other nastiness. It's also damn bloody fast. We use the technologies the browser provides and turn them into an XSS filter. The faster your browser, the faster DOMPurify will be. [Source: DOMPurify GitHub Repository)

❓ How:

DOMPurify sanitizes HTML and prevents XSS attacks. You can feed DOMPurify with string full of dirty HTML and it will return a string (unless configured otherwise) with clean HTML. DOMPurify will strip out everything that contains dangerous HTML and thereby prevent XSS attacks and other nastiness. It's also damn bloody fast. We use the technologies the browser provides and turn them into an XSS filter. The faster your browser, the faster DOMPurify will be.

The editor.wysiwygEditorInsertHtml() function doesn't properly sanitize HTML input thus resulting in an XSS when given malicious JavaScript code to inputs: email, text, url,html. So excluding JavaScript events and functions but accepting only the HTML code can mitigate this vulnerability.

This fix has covered every possible occurrence of an XSS vulnerability via the editor.wysiwygEditorInsertHtml() function, a good reference is from the one who found this vulnerability (Edric Teo) itself: https://edricteo.com/sceditor-xss-vulnerability-in-version-2.1.3/.

🔥 Fix On Action:

$ npm install -g grunt-cli
$ npm install
$ grunt build
$ grunt test
$ grunt release
$ npm run dev

✌️ Fixed!

[brunoais]

@JamieSlome :
I checked that but you still didn't answer my request:

Please provide an example of XSS that SCE suffers.

[JamieSlome]

@brunoais - I have reached out to @mufeedvh - the original contributor. They will provide further details - thank you! ⚡️

[brunoais]

@JamieSlome I think this isn't going anywhere.
If a server suffers from XSS, it doesn't matter how much work you put in the web page, the server still suffers from XSS. If it spews javascript to the browser, it doesn't make a difference if it is to SCE or not.
For all I know, XSS can only happen in the server. Prove me otherwise and then I can start discussing about this kind of implementation.
Otherwise, no matter how efficient this library is, on the browser side, it is just security theater.
Only on the server side it is actually remarkable I want to use.

[mufeedvh]

Hello @brunoais,

If a server suffers from XSS, it doesn't matter how much work you put in the web page, the server still suffers from XSS. If it spews javascript to the browser, it doesn't make a difference if it is to SCE or not.
For all I know, XSS can only happen in the server. Prove me otherwise and then I can start discussing about this kind of implementation.

XSS aka Cross-site Scripting is a vulnerability occurring on the client-side (browser part) not the server-side of a web application. It occurs when user input is given a JavaScript code as value and the web application doesn't sanitize or escape the input which results in executing the particular JavaScript code on the front-end of the web application.

If a server suffers from XSS, it doesn't matter how much work you put in the web page, the server still suffers from XSS.

This commit is the fix for the XSS Vulnerability that SCEditor suffers.

---

📎 For Example:

A web application accepts a value from an input field for entering our name:

Entering a JavaScript code instead of our name as value: ("><script>alert("XSS Example");</script>")

How did it reflect on the client-side:

---

💀 Impact of an XSS Vulnerability:

The impact of an exploited XSS vulnerability on a web application varies a lot. It ranges from user's Cookie Hijacking, and if used in conjunction with a social engineering attack it can also lead to the disclosure of sensitive data, CSRF attacks, and other security vulnerabilities.

---

Please provide an example of XSS that SCE suffers.

Here you go, a Proof of Concept from the finder itself, it also has an assigned CVE: CVE-2019-19466: https://edricteo.com/sceditor-xss-vulnerability-in-version-2.1.3/

📚 For Reference:

• https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-19466
• https://snyk.io/vuln/SNYK-JS-SCEDITOR-536836
• https://edricteo.com/sceditor-xss-vulnerability-in-version-2.1.3/

Thanks & Regards,
Mufeed VH

[brunoais]

To show you how wrong you are:
That website of yours localhost:1337 suffers from XSS. You should fix the server code so the
echoing of the GET variable doesn't end up on the web page.

OK, jokes asside: SCE, by definition, cannot suffer from XSS because all its code is run on the client side. This library only brings extra processing that is exclusively security theatre.

About the CVE you set:

CWE-79 | Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting')

is wrong (my emphasis). There is no web page generation process done by SCE. At least not according to their own definition.
If there's a local script (userscript or similar) that can inject itself into SCE, it can inject itself anyway regardless of this proposed defense.

@mufeedvh

This commit is the fix for the XSS Vulnerability that SCEditor suffers.

If so, then provide a reproducible example of the vulnerability in action. I.e. something I can try myself to confirm. If I can reproduce, I can consider myself wrong. Until then, I will need user interaction steps to cause XSS that doesn't depend on an XSS nor stored XSS vulnerability in the server and isn't easily overturned by doing something similar.

All the sources you have shown are cross-referencing each other. Also all of them allow free entry. I.e. everybody can submit to them.

Here you go, a Proof of Concept from the finder itself
You mean this?

editor.wysiwygEditorInsertHtml(
	'<img' + attrs + ' src="' + url + '" />'
);

What about this?

document.querySelector('iframe').contentWindow.document.body.innerHTML += '<img' + attrs + ' src="' + url + '" />';

Or (I don't need the iframe at all):

document.body.innerHTML += '<img' + attrs + ' src="' + url + '" />';

Am I missing something here?

[mufeedvh]

Hi @brunoais,

You are not getting the point. And it seems like you don't know much about Web Application Security or XSS (Cross-site Scripting) in general. So I am gonna answer all your questions one-by-one. :)

---

To show you how wrong you are:

I am not.

---

That website of yours localhost:1337 suffers from XSS. You should fix the server code so the
echoing of the GET variable doesn't end up on the web page.

My friend, I was just showing you an example of what XSS is. It is not SCEditor nor is it associated with it. It is just an example. And again, XSS is not a server-side vulnerability, it's a client-side vulnerability that happens due to improper validation of user input (ie: sanitization or escaping).

---

OK, jokes asside: SCE, by definition, cannot suffer from XSS because all its code is run on the client side. This library only brings extra processing that is exclusively security theatre.

To achieve an XSS attack, you don't need any interaction with the server, it's all on the client-side. If you think I am wrong, read this.

And I don't know what is security theatre as you've stated. I assume you meant "security threat", I have stated about the impact of XSS in my previous comment.

---

About the CVE you set:

I am not the one who set the CVE, it's the CVE publishers who do. And also, only valid vulnerability submissions get a CVE ID assigned so do you know what that means? SCEditor suffers from XSS.

Learn about CVEs here

---

CWE-79 | Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting')

is wrong (my emphasis). There is no web page generation process done by SCE. At least not according to their own definition.

There is no need for a "web page generation process" to achieve an XSS attack. If a web application prints out a user input or even previews it without "reloading" or "web page generation", it is technically vulnerable to XSS except if the input is sanitized or escaped.

---

If there's a local script (userscript or similar) that can inject itself into SCE, it can inject itself anyway regardless of this proposed defense.

That's not supposed to happen. You can checkout CKEditor, a similar project which had suffered XSS vulnerabilities multiple times and they have fixed them too.

For reference: https://snyk.io/vuln/SNYK-JS-CKEDITOR-72618

---

If so, then provide a reproducible example of the vulnerability in action. I.e. something I can try myself to confirm. If I can reproduce, I can consider myself wrong.

This vulnerability exists and that's how it got a CVE assigned. You can reproduce it with the following steps from the original Proof of Concept.

---

editor.wysiwygEditorInsertHtml(
	'<img' + attrs + ' src="' + url + '" />'
);

What about this?

document.querySelector('iframe').contentWindow.document.body.innerHTML += '<img' + attrs + ' src="' + url + '" />';

Or (I don't need the iframe at all):

document.body.innerHTML += '<img' + attrs + ' src="' + url + '" />';

Am I missing something here?

Well yes, that Proof of Concept shows that the vulnerability exists on the editor.wysiwygEditorInsertHtml(). What you assumed were different ways to insert that in the web page which is not what this is all about. The editor.wysiwygEditorInsertHtml() blindly accepts user input without sanitizing or escaping them which results in an XSS (Cross-site Scripting) Vulnerability.

---

If you want to know how this issue has been fixed: checkout 418sec#2

And also this vulnerability is similar to #753 but not the same. :)

I have answered all your questions one-by-one, also I suggest you to check out the OWASP XSS Page where you can learn about XSS (Cross-site Scripting) attacks thoroughly. :)

Thanks & Regards,
Mufeed VH

[brunoais]

You are not getting the point. And it seems like you don't know much about Web Application Security or XSS (Cross-site Scripting) in general. So I am gonna answer all your questions one-by-one. :)

Yes please.

To show you how wrong you are:

I am not.

That website of yours localhost:1337 suffers from XSS. You should fix the server code so the
echoing of the GET variable doesn't end up on the web page.

My friend, I was just showing you an example of what XSS is. It is not SCEditor nor is it associated with it. It is just an example. And again, XSS is not a server-side vulnerability, it's a client-side vulnerability that happens due to improper validation of user input (ie: sanitization or escaping).

If you are showing me that the vulnerability is in the client, why are you showing me a vulnerability in the server?
If the server outputs <script>alert('ups!')</script> and it's supposed to be plain text, isn't it the server's fault? Or is it the browser's fault?

OK, jokes asside: SCE, by definition, cannot suffer from XSS because all its code is run on the client side. This library only brings extra processing that is exclusively security theatre.

To achieve an XSS attack, you don't need any interaction with the server, it's all on the client-side. If you think I am wrong, read this.

There is one detail I failed. If the javascript reads data directly from the URL or from cookies and pasts it using innerHTML or equivalent, then it can suffer from XSS. But SCE doesn't suffer from that.

And I don't know what is security theatre as you've stated. I assume you meant "security threat", I have stated about the impact of XSS in my previous comment.

I actually meant security theatre.

About the CVE you set:

I am not the one who set the CVE, it's the CVE publishers who do. And also, only valid vulnerability submissions get a CVE ID assigned so do you know what that means? SCEditor suffers from XSS.

I don't think they spent any time confirming the claims and I believe they just assumed the claims were correct. Only javascript calling functions were given here. If you can call javascript functions, you can already do anything beyond what SCE does. SCE can't stop an attacker that can already run whatever js he wants.

Learn about CVEs here

OK thanks. So there's some level of review... Given how this happened... I don't think they did any check whether the claims are correct. They only checked if the claim itself was sound.

CWE-79 | Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting')
is wrong (my emphasis). There is no web page generation process done by SCE. At least not according to their own definition.

There is no need for a "web page generation process" to achieve an XSS attack. If a web application prints out a user input or even previews it without "reloading" or "web page generation", it is technically vulnerable to XSS except if the input is sanitized or escaped.

If that's so, show me a couple of examples. If there's a vulnerability, first you prove it exists. Why all this discussion and no examples of the vulnerability?

If there's a local script (userscript or similar) that can inject itself into SCE, it can inject itself anyway regardless of this proposed defense.

That's not supposed to happen. You can checkout CKEditor, a similar project which had suffered XSS vulnerabilities multiple times and they have fixed them too.

For reference: https://snyk.io/vuln/SNYK-JS-CKEDITOR-72618

For reference: ckeditor/ckeditor4#1876 (comment)

Is the fist fix mentioned in 4.11.0 what you mean?

If so, then provide a reproducible example of the vulnerability in action. I.e. something I can try myself to confirm. If I can reproduce, I can consider myself wrong.

This vulnerability exists and that's how it got a CVE assigned. You can reproduce it with the following steps from the original Proof of Concept.

Where are the reproduction steps?

editor.wysiwygEditorInsertHtml(
	'<img' + attrs + ' src="' + url + '" />'
);

What about this?

document.querySelector('iframe').contentWindow.document.body.innerHTML += '<img' + attrs + ' src="' + url + '" />';

Or (I don't need the iframe at all):

document.body.innerHTML += '<img' + attrs + ' src="' + url + '" />';

Am I missing something here?

Well yes, that Proof of Concept shows that the vulnerability exists on the editor.wysiwygEditorInsertHtml(). What you assumed were different ways to insert that in the web page which is not what this is all about. The editor.wysiwygEditorInsertHtml() blindly accepts user input without sanitizing or escaping them which results in an XSS (Cross-site Scripting) Vulnerability.

In order for editor.wysiwygEditorInsertHtml() to work, it must not sanitize input. Whatever data is given to SCE needs to be sanitized already. Additionally, editor.wysiwygEditorInsertHtml() is not user API so no way editor.wysiwygEditorInsertHtml() can be a direct victim of XSS.

If you want to know how this issue has been fixed: checkout 418sec#2

I see... That does need to be addressed. Why didn't you give me an example in the first place?!?!
In this case, I believe it should be solved but properly, not by getting some sort of sledgehammer to handle something so straightforward.

For me to merge, solve each problem individually and at the source of the problem. Not after it has gone into the program.

Change the input types:

1. https://github.com/samclarke/SCEditor/blob/master/src/lib/templates.js#L68
2. https://github.com/samclarke/SCEditor/blob/master/src/lib/templates.js#L76
3. https://github.com/samclarke/SCEditor/blob/master/src/lib/templates.js#L58
4. https://github.com/samclarke/SCEditor/blob/master/src/lib/templates.js#L60
5. https://github.com/samclarke/SCEditor/blob/master/src/lib/templates.js#L62

That's the main idea.
Am I missing any or anything?

[mufeedvh]

Hi @brunoais,

Thanks for the quick response. :)

For me to merge, solve each problem individually and at the source of the problem. Not after it has gone into the program.

I used DOMPurify so that it would accept HTML content and strips out everything (JavaScript code) that could cause an XSS Vulnerability. I guess that's a good fix.

While fixing the issue, the only information I depended on was the original proof of concept and CVE notes from other resources, so I only focused on fixing the issue on the function itself.

Change the input types:

1. https://github.com/samclarke/SCEditor/blob/master/src/lib/templates.js#L68
2. https://github.com/samclarke/SCEditor/blob/master/src/lib/templates.js#L76
3. https://github.com/samclarke/SCEditor/blob/master/src/lib/templates.js#L58
4. https://github.com/samclarke/SCEditor/blob/master/src/lib/templates.js#L60
5. https://github.com/samclarke/SCEditor/blob/master/src/lib/templates.js#L62

Oh yes, so I guess you're saying that the input should be escaped before it is thrown into the editor.wysiwygEditorInsertHtml() inside template.js. Am I right?

[brunoais]

Oh yes, so I guess you're saying that the input should be escaped before it is thrown into the editor.wysiwygEditorInsertHtml() inside template.js. Am I right?

In the cases I found, such logic is not needed. Only the generated HTML needs to be changed. By changing the <input> tag's type, e.g. to number, the vulnerability should be handled at no cost.

Let me know if there's still gaps after such actions

[mufeedvh]

By changing the <input> tag's type, e.g. to number, the vulnerability should be handled at no cost.

How can an input tag's type attribute say changing to number mitigate this vulnerability? I don't completely understand, can you please explain more about the approach you're thinking of? :)

[brunoais]

How can an input tag's type attribute say changing to number mitigate this vulnerability? I don't completely understand, can you please explain more about the approach you're thinking of? :)

In an input that expects a number like weight/height of an image, you may just force the input to only accept numbers. There are many other input types. Here's two examples for url and number. If you try to input anything besides the valid for the given type, an error appears.
As for what options you can use, checkout mdn's entry on HTML's <input>.

[mufeedvh]

In an input that expects a number like weight/height of an image, you may just force the input to only accept numbers.

SCEditor is an A lightweight HTML and BBCode WYSIWYG editor so a user can literally add anything they want. The vulnerability is in the input handling of "anything" that goes into SCEditor not only the <input> tag. I hope you understood this issue properly.

At this point, I think we should wait for @samclarke thoughts on this issue. :)

Thanks

[brunoais]

OK. We wait for @samclarke . He has been quite unresponsive, unfortunately.
Since some changes some time ago, I've been unable to develop locally too. I am as collaborator but I barely did anything here.

[live627]

Couldn't this be fixed with the built in escaping methods as seen in #763?

[samclarke]

Thanks for the PR and sorry for taking so long to reply.

This PR would fix for the affected commands but if dompurify is going to be included it would probably make more sense to do the filtering inside range.insertHTML() and anywhere else HTML is set internally as that would make sure other commands can't forget to filter the HTML.

There is another possible solution which would be to make commands use insert() instead which accepts strings in whatever the editors current format is.

In the case of BBCode that should be completely safe as it takes BBCode and which is parsed and converted into HTML, escaping anything dangerous, before being inserted. For the XHTML format to be secure it would either need extra attribute filters (not ideal), to use a comprehensive whitelist (good and is supported but isn't currently done by default) or to use dompurify (also good).

Using insert() would make all commands safe but wouldn't make wysiwygEditorInsertHtml() safe so any code calling that would need to either use insert() or make sure the passed HTML is safe.

Using dompurify should make using wysiwygEditorInsertHtml() safe which would be better but would require including a largish dependency. I'm not sure which is the best option but using dompurify does sound like it might be the better choice.

[samclarke]

I've created a branch based off this PR to see what impact using dompurify internally would have. It adds ~7kb to the compressed size and adds a dependency but should make all methods of inserting HTML safe from XSS so might be the better choice.