slab / quill

Quill is a modern WYSIWYG editor built for compatibility and extensibility
https://quilljs.com
BSD 3-Clause "New" or "Revised" License
43.08k stars 3.35k forks source link

Security Issue CVE-2021-3163 #3364

Open hieroshima opened 3 years ago

hieroshima commented 3 years ago

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.

twocs commented 1 year ago

Perhaps this could be cleared up with better documentation.

If you use Quill instance's getText() method, you'll receive a WYSIWYG version of the contents. If you want completely santised text, you can use the Quill instance's root property to get the innerHTML, which is fully sanitised.

@snoopysecurity

I don't believe that documentation is sufficient. You could similarly restrict your page to no-inline or require nonces for inline scripts. It is not sufficient for there to be an option or other way to sanitise the input, because a module like quill is typically a third-party import that the typical developer is not an expert. It was only after several months that we realised that the library we use uses quill. To alleviate the security issue, the default must be to sanitise to prevent XSS; it is not sufficient that there is an option or other way to sanitise, that option must be the standard way to use the library.

The typical developer is using a library and will consider it to be reasonably secure. For example, OWASP suggests that:

Start with using your framework’s default output encoding protection when you wish to display data as the user typed it in. Automatic encoding and escaping functions are built into most frameworks. https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html

A similar library was marked as vulnerable by Snyk for a similar issue (now resolved). This article has some details on the initial patching that mitigated it.

Snyk flagged the "marked" package as vulnerable because although it had XSS protections, it was turned off by default. https://snyk.io/blog/marked-xss-vulnerability/

Finally we can see from the XSS CVE for Joomla (fixed since 1.5.20) that simply having default settings that allow an admin to perform XSS is problematic.

Inadequate input filtering leads to a multiple XSS vulnerabilities. Additionally, the default filtering settings could potentially allow users of the default Administrator user group to perform a XSS attack. https://nvd.nist.gov/vuln/detail/CVE-2018-11326

For these reasons, my opinion is that the way to fix this issue is for Quill to filter the input as the default, and I'd probably suggest it as the rule. It might be a breaking change but personally, I see no reason that Quill should be "enabling content that is executable by a web browser, such as JavaScript, HTML tags, HTML attributes, mouse events, Flash, ActiveX" (as defined for CWE 79 Cross Site Scripting https://cwe.mitre.org/data/definitions/79.html, which is this exact problem with Quill). If a developer wants something executable, they can do it explicitly, not by adding an inline script to a Quill editor config, that is then replayed by a Quill editor (which additionally has the issue that the inline script may unexpectedly be run twice in the two context, once on page load and the other when the Quill editor is loaded).

tjad commented 1 year ago

Simply put, santisation (XSS) is not the responsibility of an editor (unless the editor was responsible for a presentation layer).

Sanitisation is responsibility of the presentation layer.

In Quill's case, its presentation layer does not permit any XSS hacks

Saduff commented 1 year ago

To alleviate the security issue, the default must be to sanitise to prevent XSS; it is not sufficient that there is an option or other way to sanitise, that option must be the standard way to use the library.

That is the default behavior. What other way are you talking about?

For these reasons, my opinion is that the way to fix this issue is for Quill to filter the input as the default

Which it does, as my now 3 PoCs have shown, the last one with the <template> being the best way to demonstrate it. To be clear, these are just proof of concepts that are absolutely not the way to use Quill. Their only purpose was to prove a point.

SawkaDev commented 1 year ago

I am confused / new to using git-hub for production deployments.. Lets assume I have sanitized the inputs coming in from quill, how does that remove the dependency bot alert for this warning? do I just ignore it? I need to show that there are no vulnerabilities in current packages I am using. Or is that what is up to debate that the depencybot should not be flagging this issue?

And since this thread has not agreed any out of the box / proven solutions, would using something like https://github.com/cure53/DOMPurify suffice when everyone mentions "you should sanitize the inputs". Seems we should just leave sanitization as the solution and somehow get this dependencybot removed?

And I ran a test with DOMPurify and I see no difference before and after. Is quill changing the html input somehow? What is the proper way to test that input is getting sanitized

image

Saduff commented 1 year ago

Lets assume I have sanitized the inputs coming in from quill, how does that remove the dependency bot alert for this warning? do I just ignore it?

Yes, ignore.

I need to show that there are no vulnerabilities in current packages I am using. Or is that what is up to debate that the depencybot should not be flagging this issue?

Yes, it's a false positive.

What is the proper way to test that input is getting sanitized

This is the best way I found: https://codepen.io/Saduff/pen/rNvbWzv

You don't need DOMPurify or anything like that for Quill. However, depending on what else you do in your application, you may need it or something similar, but that depends on the specifics of the application and is out of scope of this discussion.

adamskwersky commented 1 year ago

Question about this. If I do not allow direct HTML editing, and the input is always stored and retrieved from the server as a Quill Delta, does this in effect prevent the XSS?

Saduff commented 1 year ago

Question about this. If I do not allow direct HTML editing, and the input is always stored and retrieved from the server as a Quill Delta, does this in effect prevent the XSS?

Yes, and that is how it should be done.

twocs commented 1 year ago

To alleviate the security issue, the default must be to sanitise to prevent XSS; it is not sufficient that there is an option or other way to sanitise, that option must be the standard way to use the library.

That is the default behavior. What other way are you talking about?

Quill does not sanitise inline javascript, such as onerror, by default.

Saduff commented 1 year ago

Quill does not sanitise inline javascript, such as onerror, by default.

Can you provide an example of that?

tjad commented 1 year ago

To alleviate the security issue, the default must be to sanitise to prevent XSS; it is not sufficient that there is an option or other way to sanitise, that option must be the standard way to use the library.

That is the default behavior. What other way are you talking about?

Quill does not sanitise inline javascript, such as onerror, by default.

@Saduff / @twocs This is true/confirmed.

On the 1 hand, I agree, when placing raw text into the editor which is unsanitised through its root element, it will result in the javascript executing. However, this is not caused by the editor not sanitising html. The method of input is explictly through route of raw HTML, hence the user should be cognisant to ensure this input is sanitised.

tjad commented 1 year ago

what I mean by "not caused by the editor not sanitising html" , is that the editor is not outputting unsafe HTML.

tjad commented 1 year ago

i.e if you take any element and place raw HTML within its innerHTML, XSS will happen if the user is not sanitising the html. The explicit API method to do that is Quill.clipboard.dangerouslyPasteHTML() , but again, this is explicitly pasting in potentially dangerous HTML (which is left as responsibility on the user to ensure it is sanitised - not the editor).

I would only consider this an XSS vulnerabiity if the output of the WYSIWYG editor is outputting non-safe HTML. If the user is putting in raw HTML that is unsafe through a mechanism purposefully not cleaning the input, you can't place that under responsibility of the editor not sanitising text. (Because the intent was to be raw).

Saduff commented 1 year ago

I would only consider this an XSS vulnerabiity if the output of the WYSIWYG editor is outputting non-safe HTML.

Yes, exactly. Which is what I have tried to show with my PoCs, i.e. the editor is outputting safe HTML.

The explicit API method to do that is Quill.clipboard.dangerouslyPasteHTML()

I was not aware of this method, but it doesn't change anything. Where is this documented? This seems like an internal undocumented method not designed to be used by users. It's not mentioned at https://quilljs.com/docs/api/

tjad commented 1 year ago

@twocs Moreover, if you utilise the Quill Delta API as it intends you to use. It will sanitise the input.

e.g:

        quill.setContents([
          { insert: {image: 'https://quilljs.com/images/cloud.png'}, attributes:{onerror:'console.log(1337)', width:45} },
        ]);
twocs commented 1 year ago

There are many variations on XSS exploits. The risk I'm seeing here is that quill allows malicious code to run in the trusted environment, not that the final output of Quill contains the malicious code. In other words, "A vulnerability in the HTML editor of Slab Quill allows an attacker to execute arbitrary JavaScript by storing an XSS payload (a crafted onloadstart attribute of an IMG element) in a text field". The issue is not that after Quill is finished its business, the output of the editor still contains the XSS payload, it's not there and it doesn't need to be there for the alert to popup.

It's possible to use setContents with properly crafted input to pop up an alert: https://codepen.io/twocs/pen/gOKOLGe, so the exploit simply needs quill to run quill.clipboard.convert on the malicious XSS payload.

var quill = new Quill('#editor')
var potentiallyMaliciousCode = '<img src=ffs onerror="alert(12345)">'
var contents = quill.clipboard.convert(potentiallyMaliciousCode);

Doesn't even require dangerouslyPasteHTML. Using this.clipboard.convert is literally the same pattern that is used in the Quill constructor, so I'd assume that's how a developer who is familiar with Quill will use it. Indeed, the typical pattern is that when Quill loads up, it runs the this.clipboard.convert(html) on whatever is in the #editor on the html page.

Why does the javascript alert run when you are only trying to convert html to a delta, not even intending for it to go anywhere?

Saduff commented 1 year ago

Thanks for that example. I can see why that would be a concern. However, I don't think this is an issue in this case.

I found that quill.clipboard.dangerouslyPasteHTML is actually documented here: https://quilljs.com/docs/modules/clipboard/

However, quill.clipboard.convert is not. So that is an internal undocumented method and can be considered equivalent to dangerouslyPasteHTML which is equivalent to setting the innerHTML of the editor.

Using this.clipboard.convert is literally the same pattern that is used in the Quill constructor, so I'd assume that's how a developer who is familiar with Quill will use it.

Based on what I wrote above, I don't think so. A developer would use documented methods, not look through the source code of the library and use internal undocumented methods. In my pentest case, the developers did not use a Quill method at all and simply set the innerHTML of the editor using jQuery.

Indeed, the typical pattern is that when Quill loads up, it runs the this.clipboard.convert(html) on whatever is in the #editor on the html page.

This is also not an issue. The reason why is already in your sentence: whatever is in the #editor on the html page.

What would be a vulnerability is if you can paste an XSS payload in the editor and it would execute. However, I have not seen evidence of that.

adamskwersky commented 1 year ago

It's possible to use setContents with properly crafted input to pop up an alert: https://codepen.io/twocs/pen/gOKOLGe, so the

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. Are you able to provide an example with setContents?

twocs commented 1 year ago

It's possible to use setContents with properly crafted input to pop up an alert: https://codepen.io/twocs/pen/gOKOLGe, so the

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. Are you able to provide an example with setContents?

Sorry, that's not how setContents would work. quill.clipboard.convert is used to convert arbitrary HTML to a delta, which you would then feed into one of the set functions. I stripped the example down to its minimal problem to avoid distraction.

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.

adamskwersky commented 1 year ago

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.

twocs commented 1 year ago

The problem seems to be caused by innerHTML here: https://github.com/quilljs/quill/blob/1.3.7/modules/clipboard.js#L77

Effectively, when converting potentially malicious html to a delta, this is being run: let potentiallyMaliciousCode = "<img src=q onerror=alert(1)>"; document.createElement('div').innerHTML = potentiallyMaliciousCode;

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.  This seems to be the right way to allow conversion of pasted html into a delta without rendering it. Fixed code:   const doc = new DOMParser().parseFromString(html, 'text/html'); https://github.com/quilljs/quill/blob/develop/modules/clipboard.ts#L117

Saduff commented 1 year ago

The problem seems to be caused by innerHTML here: https://github.com/quilljs/quill/blob/1.3.7/modules/clipboard.js#L77

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.

quill.clipboard.convert is not supposed to be called by users of the library. If that's followed, there is no vulnerability. However, I agree it would be better to avoid using innerHTML there. Quill shouldn't even expose this function to users.

Many projects store the output from Quill as html. They similarly load that output back into the editor for editing.

This could be fixed with better documentation. The proper way to serialize and deserialize Quill state is to use getContents and setContents.

twocs commented 1 year ago

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.

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.

Saduff commented 1 year ago

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 quill.clipboard.convert explicitly with user input, which shouldn't be done as it's an undocumented internal function. But it's good to see they're not using innerHTML in the next version of Quill.

tjad commented 1 year ago

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.

@adamskwersky As you are storing the Delta and using getContent(), `setContent(), your usage is 100% safe.

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

  1. As far as I'm aware browsers do not copy such JS content, ,and this is only an exploit during pasting (hopefully you are not trying to exploit yourself?).

  2. getContent() is still going to give you a sanitised Delta, and when inputting such Delta with setContent(), it will be resanitised.

tjad commented 1 year ago

The problem seems to be caused by innerHTML here: https://github.com/quilljs/quill/blob/1.3.7/modules/clipboard.js#L77

Effectively, when converting potentially malicious html to a delta, this is being run: let potentiallyMaliciousCode = "<img src=q onerror=alert(1)>"; document.createElement('div').innerHTML = potentiallyMaliciousCode;

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.  This seems to be the right way to allow conversion of pasted html into a delta without rendering it. Fixed code:   const doc = new DOMParser().parseFromString(html, 'text/html'); https://github.com/quilljs/quill/blob/develop/modules/clipboard.ts#L117

Found this interesting, thanks @twocs And yes, agree with @Saduff. Use the tools correctly(as intended as per documentation), otherwise it is simply a 10T error.

tjad commented 1 year ago

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.

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.

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

tjad commented 1 year ago

If this was such an exploit, why would so many large vendors back Quill ? Grammarly, LinkedIn, Microsoft, Salesforce, Slack ....

pauldraper commented 1 year ago

@tjad unfortunately, I don't think you're going to be able to countermand the CVE in this thread.

tjad commented 1 year ago

@pauldraper probably not, but I'm certain we have established a lot more clarity for any readers that this CVE is a false positive.

burninatorsec2 commented 1 year ago

If this was such an exploit, why would so many large vendors back Quill ? Grammarly, LinkedIn, Microsoft, Salesforce, Slack ....

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.

burninatorsec2 commented 1 year ago

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.

Saduff commented 1 year ago

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.

Tofandel commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 image

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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 commented 4 weeks ago

This issue appears to be fixed in https://github.com/slab/quill/pull/2226/commits/16b6a6cd842056f06c5ac2fc49ff9ac39dd17409 (via PR https://github.com/slab/quill/pull/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.