quill icon indicating copy to clipboard operation
quill copied to clipboard

Security Issue CVE-2021-3163

Open hieroshima opened this issue 3 years ago • 42 comments

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.

hieroshima avatar May 11 '21 14:05 hieroshima

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).

arktronic avatar May 13 '21 13:05 arktronic

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 avatar May 13 '21 20:05 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 avatar May 14 '21 18:05 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 avatar May 14 '21 18:05 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.

polyatail avatar May 14 '21 18:05 polyatail

@alexcodito Thank you for following up on that.

arktronic avatar May 16 '21 23:05 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).

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

HamzaAnis avatar Jun 04 '21 11:06 HamzaAnis

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?

samcambridge avatar Jun 18 '21 11:06 samcambridge

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.

pauldraper avatar Jul 02 '21 21:07 pauldraper

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.

HamzaAnis avatar Jul 04 '21 22:07 HamzaAnis

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!

jkneepkens avatar Aug 02 '21 12:08 jkneepkens

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.

image

ndtx avatar Aug 18 '21 18:08 ndtx

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.

einfallstoll avatar Aug 18 '21 19:08 einfallstoll

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

snoopysecurity avatar Aug 19 '21 10:08 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

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

AM1988 avatar Nov 18 '21 15:11 AM1988

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.

pauldraper avatar Nov 22 '21 17:11 pauldraper

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

Recurse-blip avatar Dec 14 '21 21:12 Recurse-blip

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) https://github.com/advisories/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

twocs avatar Mar 01 '22 00:03 twocs

fix would be appreciated.

EricDitp avatar Mar 09 '22 15:03 EricDitp

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

oalexdoda avatar Apr 08 '22 10:04 oalexdoda

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

pauldraper avatar Apr 10 '22 02:04 pauldraper

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

Nyamkhuub avatar Jun 04 '22 18:06 Nyamkhuub

Still getting this as a vulnerability alert

Sliffcak avatar Aug 23 '22 18:08 Sliffcak

still getting error any fix?

deepesh146 avatar Aug 27 '22 11:08 deepesh146

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

pauldraper avatar Aug 27 '22 20:08 pauldraper

please suggest any other text editor actually i have purchased a theme and quill is built in ,i m building a enterprise app security is most important for company

deepesh146 avatar Sep 02 '22 18:09 deepesh146

Hi, this is how to reproduce the issue - you need to edit the traffic with an interception proxy when storing the comment, in order to put this payload in:<div><image src=validateNonExistantImage.png onloadstart=alert(1337)> thepayload </div>

Then once it's stored, any other user to visit the page with that payload will get the XSS to pop. This vulnerability is very much still active, I've used it in multiple environments I've pentested or done bug bounties for since reporting it back in 2021 (here https://github.com/quilljs/quill/issues/3273, and here: https://github.com/quilljs/quill/issues/3558).

EDIT: spelling and wording

burninatorsec2 avatar Sep 06 '22 17:09 burninatorsec2

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

Yes, this is one way to remediate it, in lieu of Quill releasing a security patch.

burninatorsec2 avatar Sep 06 '22 17:09 burninatorsec2

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.

Correct, this same sanitation needs to be done server-side/API side, not just in the UI.

burninatorsec2 avatar Sep 06 '22 17:09 burninatorsec2

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.

That sounds reasonable. Any chance this can be implemented? Or if that's a public method users can call it on their own?

gustaff-weldon avatar Sep 12 '22 07:09 gustaff-weldon