Security Issue CVE-2021-3163

[hieroshima]

Hi.

I would like to raise a security issue which is described in CVE-2021-3163. Is there any fix for that or do someone know an ETA when that security issue will be fixed?

Thanks in advance.

[arktronic]

Has anyone been able to reproduce this vulnerability? I tried using various setups without luck. The originally referenced issue (#3273) has been deleted. The only way I've been able to see the XSS in action is on the CodePen Playground on the bottom of https://quilljs.com/docs/delta/ but that's because, for the right side of the playground, they're assigning directly to .innerHTML without escaping (thus, that particular instance is not itself a QuillJS issue).

[polyatail]

I have not been able to reproduce this vulnerability either. The closest I've come is editing the contents of the Quill <div> as HTML and forcing something like <div><img src="foobar.jpg" onclick="alert(1234)">foobar</div> to exist on the page. Even then, copying and pasting that image within the editor, the pasted image box is stripped of the onclick property.

[polyatail]

1. Save/persist contents of Quill

So to clarify, this issue affects only cases where un-sanitized contents are loaded into a Quill editor via e.g., an API call to the server that reads the contents from a database. It is not possible to directly input this attack into the editor. Correct?

[polyatail]

Perhaps a solution here is to pipe the initial input of the editor through the same mechanism that sanitizes clipboard pasting, since that seems to be effectively removing these problematic element attributes.

[polyatail]

That sounds sensible to me. Will you be taking a look at this? If not, I may do a pull-request in the near future.

Feel free to give it a go and tag me for a review.

[arktronic]

@alexcodito Thank you for following up on that.

[HamzaAnis]

Has anyone been able to reproduce this vulnerability? I tried using various setups without luck. The originally referenced issue (#3273) has been deleted. The only way I've been able to see the XSS in action is on the CodePen Playground on the bottom of https://quilljs.com/docs/delta/ but that's because, for the right side of the playground, they're assigning directly to .innerHTML without escaping (thus, that particular instance is not itself a QuillJS issue).

So is this a valid vulnerability? If not then the a lot of people will be getting false potential security vulnerabilities alerts.

[samcambridge]

I noticed that the blog post revealing this "exploit" (https://burninatorsec.blogspot.com/2021/04/cve-2021-3163-xss-slab-quill-js.html) mentioned a html editor. Please correct me if i'm wrong but Quill doesn't provide this functionality out of the box.

The only way I've managed to replicate this is using a https://github.com/benwinding/quill-html-edit-button, which describes itself as a Module which allows you to quickly view/edit the HTML in the editor.

Pasting the HTML from the blog post into the HTML editor available in this demo https://benwinding.github.io/quill-html-edit-button/src/demos/javascript/demo.html reproduces the issue. Still, that does not make it a Quill issue.

If anyone has had any success reproducing it without custom modules can they let me know how?

[pauldraper]

So is this a valid vulnerability?

I have been able to reproduce this by loading the unsanitized payload into Quill.

Yes, this is a valid vulnerability if untrusted content is loaded.

For example, Alice submits content with XSS attack on a website (say, via its API). Bob opens a Quill editor on that website with Alice's content and is attacked.

[HamzaAnis]

Is anyone planning for a patch on this security Issue ?

So is this a valid vulnerability?

I have been able to reproduce this by loading the unsanitized payload into Quill.

Yes, this is a valid vulnerability if untrusted content is loaded.

For example, Alice submits content with XSS attack via a website's API. Bob opens a Quill editor with Alice's content and is attacked.

[jkneepkens]

So is this a valid vulnerability?

I have been able to reproduce this by loading the unsanitized payload into Quill.

Yes, this is a valid vulnerability if untrusted content is loaded.

For example, Alice submits content with XSS attack on a website (say, via its API). Bob opens a Quill editor on that website with Alice's content and is attacked.

But shouldn't this be handled by the backend/API which is serving this XSS content to the Quill editor to remove already this malicious content? Otherwise it would be really nice if it gets fixed in a patch!

[ndtx]

Some feedback regarding plans to address this would be appreciated. quill package being flagged as vulnerable we have to document why this is not an issue if it is not being fixed or replace if it is not maintained.

[einfallstoll]

I had a look at the source code and there aren't many places which allow XSS using .innerHTML. From the blog post my best guess is the reporter just included an XSS payload in the editor div (just like documented in the Quickstart) and then the XSS will execute:

<!-- Include Quill stylesheet -->
<link href="dist/quill.snow.css" rel="stylesheet">

<!-- Create the toolbar container -->
<div id="toolbar">
  <button class="ql-bold">Bold</button>
  <button class="ql-italic">Italic</button>
</div>

<!-- Create the editor container -->
<div id="editor">
  <image src=validateNonExistantImage.png onerror=alert(1337)> hey girl hey
</div>

<!-- Include the Quill library -->
<script src="dist/quill.js"></script>

<!-- Initialize Quill editor -->
<script>
  var editor = new Quill('#editor', {
    modules: { toolbar: '#toolbar' },
    theme: 'snow'
  });
</script>

If this is the actual XSS, the risk is fairly low, because that's not a vulnerability in Quill but intended behavior in ... a browser. To mitigate this:

• don't include any user-input without HTML encoding it first (which you basically always must do)
• use a Content-Security-Policy to prevent unsafe-inline scripts

To me this risk is acceptable and I can understand why no fix is available or will be available.

[snoopysecurity]

Hi, I've contacted the CVE database to revoke this CVE with the evidence provided. I will also get this removed from the Snyk Security Database. https://snyk.io/vuln/SNYK-JS-QUILL-1245047. Hopefully that will reduce the noise

[AM1988]

Hi, I've contacted the CVE database to revoke this CVE with the evidence provided. I will also get this removed from the Snyk Security Database. https://snyk.io/vuln/SNYK-JS-QUILL-1245047. Hopefully that will reduce the noise

Hi. Any updates on this? Seems still treated as a vulnerable package.

[pauldraper]

To clarify for all the visitors not understanding if this applies to them:

If you save and load shared Quill content without server-side filtering, you have XSS.

For example:

Alice submits malicious Quill-ish content to a website via direct HTTP request. The website saves this content verbatim.
When Bob edits/views that content on the website with Quill editor, Bob is attacked.

[Recurse-blip]

As I understand this is a non-issue and the CVE will be revoked. Is there any progress on this matter?

[twocs]

Hi. Any updates on this? Seems still treated as a vulnerable package.
and
As I understand this is a non-issue and the CVE will be revoked. Is there any progress on this matter?

It is still indicated as vulnerable.
Github advisory (powers npm audit) GHSA-4943-9vgg-gr5r
The CVE advisory is marked as disputed https://nvd.nist.gov/vuln/detail/CVE-2021-3163
However, the SNYK advisory was revoked https://security.snyk.io/vuln/SNYK-JS-QUILL-1245047

[EricDitp]

fix would be appreciated.

[oalexdoda]

Still getting this as a vulnerability alert. What's the resolution here?

[pauldraper]

The resolution is to scrub the data from quill before saving in a shared place.

[Nyamkhuub]

Still getting this as a vulnerability alert. How to solve it guys?

[SawkaDev]

Still getting this as a vulnerability alert

[deepesh146]

still getting error any fix?

[pauldraper]

No. You can tell by the way this issue is still open, and the alert doesn't include a fix version

[Tofandel]

For a demonstration of the issue:
https://codesandbox.io/s/cool-scooby-77pb5e?file=/src/App.vue

In short, the content going through quill is not sanitized, so if you are just sanitizing the output to a web page as you should always do and is not a CVE, but are using this same content as is in the quill editor without sanitization (which is quite possible) then you are affected by this CVE

But it should be a low enough CVE because it can be mitigated quite easily by using the same sanitization for the input of quill as for what you would output as raw html

[Saduff]

@Tofandel, that does not demonstrate a vulnerability in Quill. However, it does demonstrate a very real vulnerability in https://github.com/vueup/vue-quill.

From the v-model:content documentation of vue-quill (emphasis mine):

Two-way binding editor content, can be Delta object, plain text, or html string

Quill does not support setting editor content via a HTML string. So in order to add that support, vue-quill has introduced a vulnerability at https://github.com/vueup/vue-quill/blob/dc589e1f1a1d1f76e717abf7f031dc43dcfcadeb/packages/vue-quill/src/components/QuillEditor.ts#L323-L325

---

@twocs, care to explain the thumbs down? I've seen your patch similar to Quill v2 and what that does is it prevents execution of the XSS a second time, which is a good thing. However, to call that a vulnerability is a bit of a stretch as the only way it can be triggered is if users of Quill explicitly call convert or pasteHTML, which are internal functions I haven't found any documentation about. The only other way is to set innerHTML of some element and then initialize Quill on top of that. In that case, the vulnerability exists already before Quill and the XSS has already executed before Quill will execute it a second time.

In addition, the vue-quill vulnerability is a separate one and has nothing to do with this. It's part of vue-quill code.

[Tofandel]

@Saduff Fair enough, but vanilla quill has the same vulnerability in both pasteHTML and clipboard.convert which the doc implicitely advertised as the safe way to do that as opposed to dangerouslyPasteHTML and is now placarded on stackoverlow as the first 3 answers you'll find, now that's a vulnerability in quill which itself uses innerHTML
https://codepen.io/Tofandel/pen/zYJYmOL
https://github.com/quilljs/quill/blob/1.3.7/modules/clipboard.js#L77

This is a bit disappointing given the resulting HTML in the editor has been stripped out, so the logic seems present, but the script has still been executed. If there was a way to apply it before the use of innerHTML that would be nice or using iframe sandboxing with script disallowed if supported to mitigate the risk

On the bright side, quill 2.0 does fixes this issue as it uses the native DomParser instead https://github.com/quilljs/quill/blob/develop/modules/clipboard.ts#L117, so now the question is when is it coming out?

[Saduff]

I agree that there is a vulnerability if you call clipboard.convert, pasteHTML or dangerouslyPasteHTML. What I don't understand is, why would anyone think that's an acceptable thing to do (i.e. I have not found any documentation about these functions aside from dangerouslyPasteHTML which has 'dangerous' in its name and a disclaimer about XSS in the docs). Frameworks like React have similar functions for introducing XSS vulnerabilities, but I don't see anyone calling that a vulnerability in React.

both pasteHTML and clipboard.convert which the doc implicitely advertised as the safe way to do that as opposed to dangerouslyPasteHTML

Can you point to where you found that in the docs?

[Tofandel]

why would anyone think that's an acceptable thing to do

Because it's the only provided way to work with HTML both ways in quill, which is y'know kinda what you need to output on the frontend, not everybody can work with delta as an input, especially if you've migrated your content from some older html

But on what we agree on is that it's not acceptable to not sanitize server-side the content you're gonna send to these functions, but I can think of a few cases where that might not be possible, like collaborative editing over websocket where that content doesn't directly go through a server (though using delta is probably a better format there)

I meant implicitely as not documented, but given one function is named dangerouslyPasteHTML one can mistakenly assume that pasteHTML is the safe version, though it's just an alias

Non documented code necessary for basic functionnality has a way to write itself in forums, blog posts, stackoverflow etc, and in this case it's exactly what happened as that documentation has been made up using either .innerHTML, pasteHTML, clipboard.dangerouslyPasteHTML or clipboard.convert

[Saduff]

Because it's the only provided way to work with HTML both ways in quill, which is y'know kinda what you need to output on the frontend, not everybody can work with delta as an input, especially if you've migrated your content from some older html

That's a good point. I'm not as familiar with the use cases as I'm not a Quill user myself. However, this seems to me more like a misuse of Quill for something it was not designed for, so another rich text editor might be a better choice for such a use case (or perhaps a newer version of Quill, if such a version exists).

But on what we agree on is that it's not acceptable to not sanitize server-side the content you're gonna send to these functions, but I can think of a few cases where that might not be possible, like collaborative editing over websocket where that content doesn't directly go through a server (though using delta is probably a better format there)

Contrary to popular belief, it is perfectly acceptable to sanitize client-side. Client-side is actually preferable for client-side libraries. Server-side is more for when HTML is rendered on the server, which is not so common these days, as frontends are commonly made using a framework such as Vue, Angular or React.

Non documented code necessary for basic functionnality has a way to write itself in forums, blog posts, stackoverflow etc

I agree that one would expect a documented API for inputting HTML to a rich text editor, but instead of abusing one that doesn't, I'd look elsewhere for one that does. But that's just me, there may be good reasons for sticking with Quill.

[twocs]

I agree that there is a vulnerability if you call clipboard.convert, pasteHTML or dangerouslyPasteHTML... ~ @Saduff

I mean, we are literally rehashing the conversation that's been going on since 2021. The clipboard.convert does not imply you're pasting into the field. It's used in the core Quill js for the setup.
https://github.com/quilljs/quill/blob/d2f689fb4744cdada96c632a8bccf6d476932d7b/core/quill.ts#:~:text=%3D%20this.-,clipboard.convert,-(%7B

The new version of Quill works and does not have the problem, but it seems like it's never going to be released, despite in 2021 the docs heading being updated to say "Note: This branch and README covers the upcoming 2.0 release. View 1.x docs here." Quill core in 1.x is using the clipboard.convert function in the setup.

[Saduff]

It's used in the core Quill js for the setup.

Yes, that is known, but it's irrelevant. Like I said last year:

Yes, I found this gets called when I was creating my first PoC. However, I did not think of it as problematic, because it's only called in the constructor here: https://github.com/quilljs/quill/blob/1.3.7/core/quill.js#L100. The constructor doesn't take HTML input.

Look at the source of the HTML which is later passed as input to the convert function:
https://github.com/quilljs/quill/blob/d2f689fb4744cdada96c632a8bccf6d476932d7b/core/quill.ts#L160

It's the container's innerHTML. What this means is:

• If all you do is switch to Quill v2 where the issue is fixed, your app is still vulnerable
• If you fix the issue in your app, the Quill version no longer matters

In short, the issue is not within Quill, the source of the vulnerability is outside of Quill. If you don't fix the vulnerability in your app, switching to Quill v2 will only prevent the second execution of the XSS, which doesn't matter from an attacker's perspective.

It would be a different case if the Quill constructor took a HTML string as input. In that case, Quill will be the only one who will execute the XSS. So, like I've said before, there is no reason for Quill to set the innerHTML of the container in the convert function, so v2 is an improvement, but it doesn't magically fix the vulnerability in your app.

[twocs]

The "container" html in v2 is the same as the container in v1. However, the clipboard.convert function no longer uses the DOM to get the content, instead using DOMParser.
https://github.com/quilljs/quill/blob/d2f689fb4744cdada96c632a8bccf6d476932d7b/modules/clipboard.ts#L93-131

This compares with v1.3.7, notice direct assignment to innerHTML, this.container still referring to the content in the DOM:
https://github.com/quilljs/quill/blob/0148738cb22d52808f35873adb620ca56b1ae061/modules/clipboard.js#L77

DOMParser alone will not keep you safe, because the xss still exists in the html. But from there, Quill v2 then converts the html into a Quill Delta before it is inserted into the DOM. From my experimentation, it doesn't perform the XSS replay that Quill v1 is prone to doing, so you will only see one alert message, not two. As a result, you can have a higher level of trust to saving the user input, since you can control whether inline scripting is acceptable in the Quill Delta, and by default it's untrusted.

[Saduff]

Non documented code necessary for basic functionnality has a way to write itself in forums, blog posts, stackoverflow etc, and in this case it's exactly what happened as that documentation has been made up using either .innerHTML, pasteHTML, clipboard.dangerouslyPasteHTML or clipboard.convert

This is very true. Just recently during a pentest, I found the developers doing the following in their Angular application:

quill.setContents(quill.clipboard.convert(this.value));

Which as we know is vulnerable, but should be fixed in Quill v2.

[tysonclugg]

This issue appears to be fixed in 16b6a6c (via PR #2226) as part of v2.0.0.

I'm no expert, but does this mean a the original CVE should be re-issued with a new number?

I'm suggesting the security advisory be re-issued because:

1. You've fixed the issue, well done!
2. It's good practice to issue security advisories for your own software.
3. There doesn't seem to be any way to retract the DISPUTED status of a CVE.
4. Users of tools like Synk may have ignored this issue based on the DISPUTED status of the original CVE. Their security would be best served by having a new advisory with a new number that isn't in their ignore list, so they receive notification to upgrade to a patched version.