New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Security Issue CVE-2021-3163 #3364
Comments
|
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 |
|
I have not been able to reproduce this vulnerability either. The closest I've come is editing the contents of the 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? |
|
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. |
Feel free to give it a go and tag me for a review. |
|
@alexcodito Thank you for following up on that. |
So is this a valid vulnerability? If not then the a lot of people will be getting false potential security vulnerabilities alerts. |
|
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 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? |
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. |
|
Is anyone planning for a patch on this security Issue ?
|
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! |
|
I had a look at the source code and there aren't many places which allow XSS using 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:
To me this risk is acceptable and I can understand why no fix is available or will be available. |
|
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. |
|
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. |
|
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. |
|
fix would be appreciated. |
|
Still getting this as a vulnerability alert. What's the resolution here? |
|
The resolution is to scrub the data from quill before saving in a shared place. |
|
Still getting this as a vulnerability alert. How to solve it guys? |
|
Still getting this as a vulnerability alert |
|
still getting error any fix? |
|
No. You can tell by the way this issue is still open, and the alert doesn't include a fix version |
I looked at your codepen but it wasn't using setContents. I changed it to setContents and the alert no longer pops up. Also if I use setText, the alert doesn't pop up. |
Sorry, that's not how setContents would work. Many projects store the output from Quill as html. They similarly load that output back into the editor for editing. This indicates that if the hacker can get the malicious payload into the database, then the process of converting the HTML to a delta to display in the editor, Quill will run the code. I feel that Quill could convert HTML more safely. It should be possible to convert HTML into elements and attributes without rendering. |
Thanks for the explanation. Do you know if it is possible to copy from another web page and paste into Quill and have any of the JavaScript or onclick code included in the paste? I want to support copying and pasting from web pages (not the JS), just the static content, and my back-end stores the content as the JSON stringify of the Delta. |
|
The problem seems to be caused by innerHTML here: Effectively, when converting potentially malicious html to a delta, this is being run: Owasp article on mitigating XSS uses innerHTML as its example. The new version of quill.clipboard.convert (develop branch) no longer uses innerHTML, so would appear they will have addressed the issue in the upcoming major release version 2. |
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.
This could be fixed with better documentation. The proper way to serialize and deserialize Quill state is to use |
Whether or not you think it's problematic, if it can be even a small part of a complex hack, then it's a vulnerability. |
It can only be exploited if the developers (users of Quill) call |
@adamskwersky For 1.3.7, theoretically, if browsers copied the JS of attributes too, you'd be vulnerable to XSS if the alternative site that you select-copied from had XSS in it. However this is not a practical use-case for 2 reasons
|
Found this interesting, thanks @twocs |
@twocs Not quite, a vulnerability is an exposed attack surface. The surface here is not exposed in any practical sense. Unless a user exposes that surface through bad usage, how is it a vulnerability? Or otherwise every package is exploitable - through bad usage. |
|
If this was such an exploit, why would so many large vendors back Quill ? |
|
@tjad unfortunately, I don't think you're going to be able to countermand the CVE in this thread. |
|
@pauldraper probably not, but I'm certain we have established a lot more clarity for any readers that this CVE is a false positive. |
Big vendors do have security issues, they have them all the time. In the original blog post, this actually WAS reported to LinkedIn, who responded appropriately. |
|
At this point, I've completed the due diligence for demonstrating how the issue works and reporting it. For now I'll recommend that clients either write validation around it as best they can, or use a differently-designed wysiwyg editor, unless a patch or rewrite comes out. Thanks for the discussion everyone. |
No, you haven't demonstrated the issue, at all. You haven't produced a single PoC, not even in the original blog post. |
The purpose of setting the html string to container is to get the HTML elements. It is unsafe to do this via set innerHTML, which must render the HTML (including any inline javascript) to generate the HTML elements. In Quill 2, this problem is fixed by using DOMParser in a similar way as done here.
|
For a demonstration of the issue: 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 |
|
@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):
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 In addition, the vue-quill vulnerability is a separate one and has nothing to do with this. It's part of vue-quill code. |
|
@Saduff Fair enough, but vanilla quill has the same vulnerability in both 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? |
|
I agree that there is a vulnerability if you call
Can you point to where you found that in the docs? |
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 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 |
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).
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.
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. |
I mean, we are literally rehashing the conversation that's been going on since 2021. The 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. |
Yes, that is known, but it's irrelevant. Like I said last year:
Look at the source of the HTML which is later passed as input to the Line 160 in d2f689f
It's the container's
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 |
|
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. This compares with v1.3.7, notice direct assignment to innerHTML, this.container still referring to the content in the DOM: Line 77 in 0148738
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. |
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. |


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.
The text was updated successfully, but these errors were encountered: