Sanitize the content of Topic/Message to avoid XSS

[RishiRajSahu]

Sanitize the content of message's topic and body to avoid possible XSS

Fyi @vikasrohit @acshields

[maxceem]

Overview

The message topic is safe.

The message text indeed allows rendering any arbitrary HTML, for example here is a red rectangular outside of the message, using position: fixed:

This issue happens only during viewing the message. Editing is safe, as during editing all HTML is shown as plain text:

This happens due to the fact, that we explicitly enable rendering html here https://github.com/appirio-tech/connect-app/blob/dev/src/helpers/markdownToState.js#L216.

If we disable this flag, then we would be totally safe, and all HTML would be shown as plain text, like this:

Though, I guess we cannot just disable this flag, as it looks like we might have old messages saved as HTML in the DB. See the commit message where this flag has been added 91b678d.

Solutions

There are 2 possible solutions:

Quick

• During rendering, we have to sanitize HTML tags, and only allow the small subset of tags without attributes.

  • advantage: We keep allowing rendering HTML to support old messages. And the fix is quick and easy.
  • disadvantage: We have to add one more library and executing sanitizer is quite a heavy operation, it could happen that when there are many messages in one topic we could start noticing a delay

Not-Quick

• Disable support for HTML at all, to be completely safe.
• But as we might have messages in DB which already using HTML markup which we need, we have to process all the messages in DB and replace good HTML tags with Markdown. Bad tags we can keep as it is and it would be rendered as text.

[maxceem]

@vikasrohit @RishiRajSahu created a hotfix base on the Quick solution described above:

• only the next tags are allowed 'blockquote', 'p', 'a', 'ul', 'ol', 'li', 'b', 'i', 'strong', 'em', 'strike', 'abbr', 'code', 'br', 'pre'

• all other tags would be displayed as text without attributes:

  

• also, updated the lib for rendering Markdown to avoid another known XSS issue https://snyk.io/vuln/SNYK-JS-REMARKABLE-174641

PRs:

• PROD [HOTFIX] [PROD] Sanitze message text HTML #4116
• DEV [HOTFIX] [DEV] Sanitze message text HTML #4117

[vikasrohit]

@maxceem @RishiRajSahu I think we should put sanitizing in tc-message-service as well. What do you suggest?

[maxceem]

@vikasrohit I would suggest sanitizing client-side only.

IMHO message service should allow storing any text message. Somebody might discuss attacks and include examples of XSS code like <img src=x onerror=alert("xss") /> so in some context such text would be absolutely legit.

The only thing all such code suppose to be rendered as text, and not to be rendered as real HTML.

The problem we have here is that we have to render some HTML code to support backward compatibility to the times when we stored HTML instead of Markdown in the message service. And this is forcing us to use selective sanitizing.

[RishiRajSahu]

@vikasrohit are we concerned here about someone directly accessing message-service endpoints with XSS contents ?

[lakshmiathreya]

• Verified that tags other than the list above are rendered as plain text.
• Verifying the entire sanitizing is beyond the scope of this Issue testing.
• Verified that old messages are rendered correctly in Prod.