Uses innerHTML - potentially allows injection attacks #255

cocoademon · 2022-06-24T01:27:16Z

In renderList, the list items are rendered using innerHTML with no escaping.
If the mentions list is sourced from unsafe (user-sourced) data, this might allow an injection attack when a Quill user hits @.

quill-mention/src/quill.mention.js

Line 391 in 0aa9847

li.innerHTML = this.options.renderItem(data[i], searchTerm);

A possible solution is to change renderItem to return a DOMelement, which uses textContent rather than innerHTML.

ALiangLiang · 2023-07-04T19:48:17Z

Thanks @cocoademon. It's really a XSS vulnerability. I'll stop using it in my product.

DEMO: https://codepen.io/ALiangLiang/pen/mdQMJXK

thexeos · 2023-09-05T05:48:48Z

Why stop using it if you can just call some generic escapeHTML on value before returning the list?

@cocoademon, there is another place where innerHTML is used and using DOMElement won't be a solution:

quill-mention/src/blots/mention.js

Line 21 in dc1a78c

node.innerHTML += data.value;

This is the code where the blot is rendered.

ALiangLiang · 2023-09-06T06:00:15Z

Why stop using it if you can just call some generic escapeHTML on value before returning the list?

@cocoademon, there is another place where innerHTML is used and using DOMElement won't be a solution:

quill-mention/src/blots/mention.js

Line 21 in dc1a78c

node.innerHTML += data.value;

This is the code where the blot is rendered.

I can't risk my bussiness product being hacked even if I use any escapeHTML packages. What if a new XSS method emerge? The best solution is to stop assigning user generated value to innerHTML.

csculley · 2023-09-06T20:55:48Z

Hello, I've attempted to partially address this issue with #341, which sanitizes renderItem with innerText since renderItem explicitly returns a string. Using innerText should be safe here since the list element isn't a script.

I think fully resolving this issue would most likely need to properly support returning HTML from renderItem somehow (maybe a wrapper?) , which could then be used also better sanitize renderLoading as well, but I mostly wanted to patch this issue in the meantime while the details for that could be discussed in this or another ticket. Thank you!

csculley · 2023-09-07T01:48:37Z

I'm thinking, maybe in the meantime we can provide {id: 1, value: 'text'} or {id: 2, html: '<html>'}, and for renderItem, returning {text: 'text'} or {html: '<html>'} that will use innerText or innerHTML respectively. Then, people will only need to modify their items if they need to use custom HTML and any custom renderItem callbacks, and the vast majority of the use cases that use user-provided plaintext will be protected. Does this sound good with everyone?

csculley · 2023-09-07T02:09:13Z

Doing even more thinking, we could check if the returned item is a string or object, that would remove the need to change the renderItem unless you are using custom HTML, so I would guess that most people wouldn't need to change anything if they're just using text 😄

thexeos · 2023-09-07T04:02:25Z

Doing even more thinking, we could check if the returned item is a string or object, that would remove the need to change the renderItem unless you are using custom HTML, so I would guess that most people wouldn't need to change anything if they're just using text 😄

I like this idea. It provides backward compatibility for anyone who never intended to output HTML and an easy upgrade path.

What if the returned object is actually a DocumentFragment or instanceof HTMLElement? Make the HTML truly explicit and give an option to whoever needs HTML to construct it with DOM API rather than String concatenation. And besides, setting innerHTML already uses DocumentFragment under the hood.

With explicit approach, the upgrade instructions would be something along the lines of:

Replace this

renderItem(item) {
  return '<span style="color: ' + item.color + '">' + item.value + '</span>'
}

With this

renderItem(item) {
  const renderer = document.createElement('template')
  renderer.innerHTML = '<span style="color: ' + item.color + '">' + item.value + '</span>'
  return renderer.content
}

Or even better

renderItem(item) {
  const element = document.createElement('span')
  element.innerText = item.value
  element.style.color = item.color
  return element
}

ALiangLiang · 2023-09-07T04:23:33Z

I thinks for backward compatibility. If we prodvide a new bool option named allowHTML which default true on mention module, and keeps item strucutre { id: '', value: '' }. And also comment the risk about the new option. So the package can bumps a minor version. And release a major version with default false allowHTML or even stops supporting HTML string.

const quill = new Quill("#editor", {
  modules: {
    mention: {
      allowedChars: /^[A-Za-z\sÅÄÖåäö]*$/,
      mentionDenotationChars: ["@", "#"],
      source: async function(searchTerm, renderList) {
        const matchedPeople = await suggestPeople(searchTerm);
        renderList(matchedPeople);
      },
      allowHTML: true // default for `true` in minor version bumping, `false` for major version bumping
    }
  }
})

Maybe supporting Node is better in above two version. Is's accepted by Element.append. Document, HTMLElement and DocumentFragment are all implementations of it.

Node.isPrototypeOf(Document) // true
Node.isPrototypeOf(Element) // true
Node.isPrototypeOf(DocumentFragment) // true

csculley · 2023-09-07T15:20:08Z

Hmm, i'm running into a roadblock with rendering the mention blots. There'll be no good way to tell the difference between blots that have been saved in the past that rely on blot.innerHTML = data.value. These will either be unsafe or could be unsupported when relying on something newer like Element.setHTML() .

Since this is already a potentially breaking change, we could introduce a breaking change into the blot structure by saving text under value and html that we consider sanitized in string form under html in the blot, but I'm not sure if there's a better solution.

csculley · 2023-09-07T16:28:16Z

I ended up saving the html in the mention blot under _html to avoid conflicting with any user-specified html fields. Please let me know if anyone has any thoughts on #341. Thank you!!

thexeos · 2023-09-07T18:59:12Z

Hmm, value being an object may cause more problems than I had initially anticipated. In essence, this changes resulting Delta in backwards incompatible way.

Before:

{ ops: [
  { insert: { mention: { id: '1', value: 'Regular Tag', denotationChar: '@'  } } }
  { insert: { mention: { id: '5', value: '<span style="color: red">Red Tag</span>', denotationChar: '@'  } } }
] }

After (unserialized):

{ ops: [
  { insert: { mention: { id: '1', value: 'Regular Tag', denotationChar: '@'  } } }
  { insert: { mention: { id: '5', value: HTMLSpanElement, _html: '<span style="color: red;">Red Tag</span>', denotationChar: '@'  } } }
] }

After (JSON serialized):

{ ops: [
  { insert: { mention: { id: '1', value: 'Regular Tag', denotationChar: '@'  } } }
  { insert: { mention: { id: '5', value: { text: 'Red Tag' }, _html: '<span style="color: red;">Red Tag</span>', denotationChar: '@'  } } }
] }

I see two consequences to this:

We are still left with stored XSS - malicious user may manipulate the contents of _html and have it execute on other people's devices when they load Delta into their editor.
Rolling back to quill-mention v3 is not possible, as content of value is an object, so we'd be rendering [object Object] in place of mention text.

For (1) it arguably is the website developer's responsibility to sanitize user-generated content, including Deltas from Quill, and the proposed implementation would already shield the original editor (whoever inserts these mentions in the editor) from incoming XSS via source list.

To address (2), we must refrain from storing objects under value field. So maybe HTML goes into value and we include the originally proposed html flag to trigger insertion with innerHTML instead of innerText. But (1) still stands.

The ultimate solution would of course be passing a render function to the blot itself, so the developer that needs a customized blot, based on some custom data values, can do so at runtime. The value would then remain a string and be rendered with innerText in absence of a custom renderer. Of course, that is not a realistic solution due to the nature of blot registration with Quill.

More realistic solution would be to allow developers to extend from MentionBlot class and then either implement their own create method or to include logic in create method that would invoke some custom render method. The plugin already supports overriding the blot name via options, so at most only the create method would have to be changed.

It seems to me like this is the only sure way to avoid stored XSS during hydration of an editor while still allowing custom design for the blot.

The changes to blot code may look like this:

class MentionBlot extends Embed {
  // ...

  static create(data) {
    const node = super.create();
    const denotationChar = document.createElement("span");
    denotationChar.className = "ql-mention-denotation-char";
    denotationChar.innerText = data.denotationChar;
    node.appendChild(denotationChar);

    if (typeof this.render === 'function') {
      node.appendChild(this.render(data))
    } else {
      node.innerText += data.value;
    }

    return MentionBlot.setDataValues(node, data);
  }

  // ...
}

User instructions may look like this:

import { MentionBlot } from 'quill-mention'

class StyledMentionBlot extends MentionBlot {
  constructor(scroll, node) {
    super(scroll, node);
  }

  static render(data) {
    const element = document.createElement('span');
    element.innerText = data.value;
    element.style.color = data.color;
    return element;
  }
}
StyledMentionBlot.blotName = "styled-mention";

Quill.register(StyledMentionBlot);

// ...

const quill = new Quill('#editor', {
  modules: {
    mention: {
      // ...
      dataAttributes: ['id', 'value', 'denotationChar', 'link', 'target', 'disabled', 'color'],
      blotName: 'styled-mention',
    }
  }
});

And we'd have to export MentionBlot.

csculley · 2023-09-08T14:17:49Z

This seems like a good path forward! I think it mirrors the way other custom blots are added to Quill. I'm still a bit confused by

Of course, that is not a realistic solution due to the nature of blot registration with Quill.

but that might just be due to my inexperience with Quill.

Currently (if i'm understanding correctly) there's only one blotName that can be set at a time, so when I think of the user defining custom mention blots, I think that they would also want to be able to embed multiple types simultaneously and have their respective custom blots created. However, instead of going down that rabbithole, could we just soup up the current MentionBlot and provide a single overrideable serializeBlot and renderBlot function that is sourced from the options?

To me it seems like this would be easier to implement initially (if they just have one custom type they want to style, just throwing in a custom serializeBlot and renderBlot and calling it a day) but would be less suitable, say if you want to wrangle with 15 different blot types all at once. I'm not sure if that's even really a use case we need to consider, and if not then we could wait until someone opens an issue with that feature request, and just add overrideable callbacks to the current mention blot in the meantime.

Please let me know if this sounds good or if i'm on the right track at least 😄

csculley · 2023-09-08T14:51:27Z

I think I'm understanding a bit more after reading into custom blots. It seems like there's been some interest in rendering blots based on editor instance options (which is what we'd need for my proposed solution) which has been discussed here: quilljs/quill#1162

But I'm thinking it might be better to jump through all the hoops of defining custom blots off of Mention to avoid hacky ways of getting the editor configuration. I'll get to work on getting this working 😄

thexeos · 2023-09-09T00:19:47Z

However, instead of going down that rabbithole, could we just soup up the current MentionBlot and provide a single overrideable serializeBlot and renderBlot function that is sourced from the options?

You've already discovered the present Quill limitations in blot rendering, but for anyone looking for more context and how it all relates to this issue, here is the short explanation.

TL;DR blot is a class with create function that is passed a primitive data object extracted from Delta. Since Delta needs to be serialized, there is no reliable way to pass (custom) functions into the create function of a blot.

The nature of the blot is to act as a Node factory with Delta OPs as inputs. Blot allows Quill to interface with your custom content, describing how it interacts with the editor and other content around it. To render a blot a create function is called, and it is passed a data object extracted from the Delta OP that contained this blot's custom name (e.g., given { insert: { mention: { value: 'John Doe' } } } OP, the data = { value: 'John Doe' }). Since Delta is usually serialized to JSON for storage, the contents of data object are limited to supported JS types (string, number, boolean, null, [serializable] object, array). While you can always pass a custom function via a property in data, this function will lose its purpose during serialization.

What's also important is that Quill runs a global registry for blots. So if you register a blot named mention, then all editors will run its create(data) method to produce a Node. Thus, if you want one editor to render mentions as plain text and another editor to prepend an with a profile photo next to the name, you will need to either: (a) find a way to pass a custom render function in data (won't work after serialization), or (b) define a custom blot, which would implement the corresponding create(data) method.

The current approach that quill-mention takes - storing the rendered HTML in data - doesn't actually cover the above use case. Once the HTML is added as value to data - all Quill editors on the page would use that HTML in their mention Nodes.

Furthermore, my proposed custom blot name is also an incomplete solution, as it would only be "renderable" by the original custom blot and not by other implementations of the MentionBlot.

So I'd say the current list of requirements is:

Maintain data.value type as String and always load it through innerText to avoid a built-in XSS
Maintain blotName the same for both backwards and lateral compatibility (across different mention plugin renderer implementations) unless the user requires it to be different for other purposes
Since the first requirement makes rendering of HTML from any stored data.* impossible, a new way to render HTML is needed

Now, since no editor context is passed to blot's create(data), we can only ever define at most one custom renderer via this plugin - the same custom function that would be called for all mention blots on the page. Because of this, it doesn't make sense to accept a render function into plugin options, if only one function could ever operate at once.

So to recap:

The HTML inside quill-mention's blot should only ever be set inside MentionBlot's create(data) method
That method is static and it only ever receives data object, which lacks pointer to the editor in question
While a custom function can be called from MentionBlot's create method, and such function can be stored by quill-mention in some internal registry, there is no universal way to select between multiple registered functions

I don't currently have any non-hackish ideas to solve this.

csculley mentioned this issue Sep 6, 2023

Fix innerHTML XSS vulnerability #341

Merged

csculley closed this as completed in #341 Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uses innerHTML - potentially allows injection attacks #255

Uses innerHTML - potentially allows injection attacks #255

cocoademon commented Jun 24, 2022

ALiangLiang commented Jul 4, 2023 •

edited

thexeos commented Sep 5, 2023

ALiangLiang commented Sep 6, 2023

csculley commented Sep 6, 2023 •

edited

csculley commented Sep 7, 2023 •

edited

csculley commented Sep 7, 2023

thexeos commented Sep 7, 2023 •

edited

ALiangLiang commented Sep 7, 2023 •

edited

csculley commented Sep 7, 2023 •

edited

csculley commented Sep 7, 2023

thexeos commented Sep 7, 2023 •

edited

csculley commented Sep 8, 2023

csculley commented Sep 8, 2023

thexeos commented Sep 9, 2023

Uses innerHTML - potentially allows injection attacks #255

Uses innerHTML - potentially allows injection attacks #255

Comments

cocoademon commented Jun 24, 2022

ALiangLiang commented Jul 4, 2023 • edited

thexeos commented Sep 5, 2023

ALiangLiang commented Sep 6, 2023

csculley commented Sep 6, 2023 • edited

csculley commented Sep 7, 2023 • edited

csculley commented Sep 7, 2023

thexeos commented Sep 7, 2023 • edited

ALiangLiang commented Sep 7, 2023 • edited

csculley commented Sep 7, 2023 • edited

csculley commented Sep 7, 2023

thexeos commented Sep 7, 2023 • edited

csculley commented Sep 8, 2023

csculley commented Sep 8, 2023

thexeos commented Sep 9, 2023

ALiangLiang commented Jul 4, 2023 •

edited

csculley commented Sep 6, 2023 •

edited

csculley commented Sep 7, 2023 •

edited

thexeos commented Sep 7, 2023 •

edited

ALiangLiang commented Sep 7, 2023 •

edited

csculley commented Sep 7, 2023 •

edited

thexeos commented Sep 7, 2023 •

edited