XSS Vulnerability

[jhelou96]

Hey,

someone pointed out an issue in another Angular markdown library about XSS vulnerability and it seems that this library presents the same issue as well.

Links are not being validated and as such, the following code could be used to execute javascript code:

[Click Me](javascript:alert('Injected!'%29)

[jfcere]

@jhelou96 thanks for reporting this, i'll look into it.

[jfcere]

Follow up

I made a PR to use Angular DomSanitizer but it currently breaks table alignments because marked parser uses style attribute style="align-text: ..." but this is considered XSS vulnerable and it is striped by the sanitization. They are changing the way to handle table alignment for align attribute align="...", which will solve the problem, but although the changes are in their master branch it has not been released yet.

Marked built-in sanitizer

Right now, there is an option when configuring the MarkdownModule that allows you to use marked built-in sanitizer. In the long run, they are planing to remove this feature as I saw in this PR so my PR is what should be done once they removed the sanitizer on their end and they release the fix for table alignment.

TL;DR

You can sanitize the markdown by setting the sanitize property to true when importing the MarkdownModule like this:

MarkdownModule.forRoot({
  provide: MarkedOptions,
  useValue: {
    gfm: true,
    tables: true,
    breaks: false,
    pedantic: false,
    sanitize: true, // enable marked built-in html sanitizer
    smartLists: true,
    smartypants: false,
  },
}),

You can report to the documentation for MarkdownModule configuration in the README.md file for more information.

[ganeshkbhat]

@jfcere is this closed or I yet have to sanitize my previous version v1.6? I saw this:

You can sanitize the markdown by setting the sanitize property to true when importing the MarkdownModule like this: Which version would this be?

[jfcere]

It is closed because you can already sanitize by setting property sanitize: true when providing MarkedOptions configuration with MarkdownModule.forRoot({ .... }). But the value is set to false by default which means you have to provide MarkedOptions to activate sanitize HTML.

This feature is available since v1.5.0.
See my previous #65 (comment) for an example.

[ganeshkbhat]

Ok cool. Thanks. Sorry for replying late. In what kind of cases do script tags or any javascript gets applied other than the link javascripts? Hopefully none right? Just making sure.

[jfcere]

I am not an expert on that matter but I know Javascript can be injected into different html elements such as script, link, body and some other tags and it can also be injected into CSS properties like background-url.

I would suggest you to read the following article: https://www.acunetix.com/websitesecurity/cross-site-scripting/ ... it's really interesting if you are not familiar with XSS vulnerability and it shows a couple of examples.

[ganeshkbhat]

ok. neither me. i will sanitise. :-)

[learnwell]

@jfcere You should let them know about the sanitize flag here as they've issued a security advisory:
https://nodesecurity.io/advisories/892

[KingDarBoja]

@jfcere You should let them know about the sanitize flag here as they've issued a security advisory:
https://nodesecurity.io/advisories/892

Same here, got the security warning on this package.

[jfcere]

@learnwell, @KingDarBoja thx for getting this to my attention.

I sent a support request to NPM to revoke the security advisory as sanitze option can be activated to use Angular's DOM sanitization.

As if it wasn't enough, the consumer has also the flexibility to provide his own sanitize function with sanitizer: function property through MarkedOptions when setting sanitize: true.

MarkdownModule.forRoot({
  provide: MarkedOptions,
  useValue: {
    gfm: true,
    tables: true,
    breaks: false,
    pedantic: false,
    sanitize: true, // will use Angular DOM sanitizer
    smartLists: true,
    smartypants: false,
  },
}),

[jfcere]

I've communicated with NPM Security Team and after providing mitigation steps they removed the vulnerability report.

They are suggesting me to add a warning to the README.md documentation in order to bring this issue/workaround to user's attention to avoid potential security risks.

A second option would be to switch the sanitize option to true by default and still add documentation on how to turn it off.

Action will be taken soon about the documentation and I'll think about the default value of sanitize option.

[pshields]

For what it's worth, my vote would be to turn sanitize on by default in the next major version release. As a consumer of this library, I'd rather Markdown-formatted content in my applications be sanitized by default. As I see it, the risks from accidentally failing to turn sanitization on outweigh the inconvenience of having to manually disable sanitization in cases where it truly is not needed.

[Razkaroth]

I also agree. The sanitization should be enabled by default, as people should only disable sanitization if they specifically need to do so and not the orher way around. El mié., 28 de agosto de 2019 9:40 p. m., Patrick Shields < [email protected]> escribió:

…

For what it's worth, my vote would be to turn sanitize on by default in the next major version release. As a consumer of this library, I'd rather Markdown-formatted content in my applications be sanitized by default. As I see it, the risks from accidentally failing to turn sanitization on outweigh the inconvenience of having to manually disable sanitization in cases where it truly is not needed. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#65?email_source=notifications&email_token=AHY4YWM4EVFDNSQFHWFFAD3QG4ZILA5CNFSM4E7R6FD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5NBI2A#issuecomment-525997160>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHY4YWIV2MOCGJPKERRNMX3QG4ZILANCNFSM4E7R6FDQ> .

[jfcere]

Thanks for your feedback gentlemen!

• Warning has been added on README.md
• Next major release will have sanitization enabled by default

[KingDarBoja]

Thanks for your feedback gentlemen!

• Warning has been added on README.md
• Next major release will have sanitization enabled by default

Awesome, glad to help you with these stuff 🍰

[jfcere]

⚠️ Breaking Change

Dependencies as been updated to support Marked 0.8.0 in latest ngx-markdown v9.0.0 release and brings some breaking changes regarding sanitization configuration.

Due to deprecation by Marked, the following properties has been removed from MarkedOptions when configuring the MarkdownModule:

• sanitize: boolean
• sanitizer: function

Instead, sanitization is now enabled by default when importing MarkdownModule.forRoot(). It uses Angular DomSanitizer with SecurityContext.HTML by default to avoid XSS vulnerabilities. The security level can be configured/turn-off by specifying the SecurityContext using sanitize option (outside markedOptions).

import { SecurityContext } from '@angular/core';

// enable default sanitization
MarkdownModule.forRoot()

// turn off sanitization
MarkdownModule.forRoot({
  sanitize: SecurityContext.NONE
})

Be sure to follow the sanitization section in the README file for instruction.

[Helveg]

@jfcere Even after reading through most of the DomSanitizer docs, and other relevant documentation, it remains unclear to me whether with the default settings any XSS vulnerabilities are present when using ngx-markdown with SecurityContext.HTML.

Can I, with SecurityContext.HTML rely on ngx-markdown to render entirely XSS-safe HTML output given an untrusted user string as data? It seems like with Markdown there is ample room for HTML-level trickery (script tags, malicious URLs, ...), so would that be sanitized by that SecurityContext?

[jfcere]

Hi @Helveg,

Can I, with SecurityContext.HTML rely on ngx-markdown to render entirely XSS-safe HTML output given an untrusted user string as data? It seems like with Markdown there is ample room for HTML-level trickery (script tags, malicious URLs, ...), so would that be sanitized by that SecurityContext?

The short answer is yes.

By default the library uses Angular DOM sanitizer with the strictest level available. So you are as much safe as Angular can be.