Cross-Site Scripting:DOM - Issue

[karthikdav]

Sending unvalidated data to a web browser can result in the browser executing malicious code. This was reported by Fortify scanner since I'm using minified version I'm not able to get the exact line.

[knsv]

Thx, we should look at this

[knsv]

Hi again. In what context are you getting this error.

Mermaid itself takes elements on the wepage compiles them to svg element and inserts the svg in the DOM. Generally when talking about cross site scripting there is a input field or similar in which an external user could add javascript code that the unprepared web site then puts in its DOM where it starts to execute.

I am certain mermaid would not allow javascript in the diagram code (even if would be a cool but unsafe feature).

[karthikdav]

Even I'm not sure how that'll happen, I even cross checked the security scan report. Even I'm little skeptical about the code where it's happening

[darrenmothersele]

You can confirm this by going to the editor and entering the code given on the NPM advisory https://www.npmjs.com/advisories/751

[rogeralmeida]

Hey, I was checking the code and I believe that a possible solution to this problem would be:

/**
* MermaidAPI.js line 239
*/
   function parse (text) {
  //SANITIZE INPUT!
  const graphType = utils.detectType(text)
  let parser
  ...

The testing for this would be interesting though. If you guys believe this would be an elegant solution I can put sometime to try to fix it in the next 48 hours and open a PR

[rogeralmeida]

I spoke to a senior security specialist yesterday about this, thanks @mario-areias. After understanding better the situation, I don't think my above suggestion is valid anymore. It is unfair to tag mermaid as unsafe for Cross Site Scripting. The attack would happen if the attacker gain access to the input method. AFAIK mermaid has two input methods: content in a html page and via API.

Content in an HTML page

If the attacker gain privileged access to the HTML page, than the attack is already executing the XSS. The attacker doesn't need to use mermaid to gain access as he/she would already have the access.

Via API

Because mermaid offers an API, that can be used to receive the input from anywhere, then yes, this can be a compromise of the security. In this case have to implement a counter measure. I tried to simulate the attack via API and the for the simple case that I implemented the API accepted the input as valid. See below:

it('should prevent XSS injection', function () {
      expect(() => mermaidAPI.parse('graph TD;A["<img src=invalid onerror=alert(\'XSS\')></img>"]')).toThrow()
    })

The result is:

 FAIL  src/mermaidAPI.spec.js
  ● when using mermaidAPI and  › checking validity of input  › should prevent XSS injection

    expect(function).toThrow(undefined)

    Expected the function to throw an error.
    But it didn't throw anything.

      43 |     })
      44 |     it('should prevent XSS injection', function () {
    > 45 |       expect(() => mermaidAPI.parse('graph TD;A["<img src=invalid onerror=alert(\'XSS\')></img>"]')).toThrow()
         |                                                                                                      ^
      46 |     })
      47 |   })
      48 | })

      at Object.toThrow (src/mermaidAPI.spec.js:45:102)

Test Suites: 1 failed, 9 passed, 10 total
Tests:       1 failed, 267 passed, 268 total
Snapshots:   0 total
Time:        4.278s

In this case, one suggestion from @mario-areias is to scape the output, e. g., replace all < and > with < and > respectively.
I'm not familiar with the code yet to know if this is easy or hard to implement.

Conclusion

If you came here because you want to remove that annoying critical vulnerability from you npm audit report:

• If your input is the HTML page you are safe for now.
• if you use the API, we still have to prove that mermaid is safe.

[knsv]

Hi!

Thanks for the anlysis, @rogeralmeida a PR for this would be great.

One thing to consider is that in som cases tags are valid. <br/> for instance is allowed. Another thing to consider is that if the characters are replaced where you suggest this will affect the parsing of the code and subsequencly the grammars (jison files) would need to be updated to look for < and > instead.

The other way to do this is to sanitize after the parsing. The parsing results in a an object containing the data of the graph later used for rendering like an array of vertices etc. To sanitizing after the parsing would perhaps be easier in some sense but not as elegant.

You wound not need to bother with the grammars but the sanitizing would be needed for each diagram type. I think I would go for your suggested route.

[knsv]

@rogeralmeida I added this to the 8.2.0 project. Let me know if you want me to assign it to you.

[rogeralmeida]

OK, I will give it a go and get back to you soon.

[knsv]

Relevant examples fro duplicate issue in #869.

Hi, I found XSS issues in mermaid. This affects all the projects that use mermaid.

There are three different ways to trigger.

The first one:

graph TD
B --> C{<script src=https://www.google-analytics.com/gtm/js?id=GTM-TQ6RV7G ></script>}

The second one:

graph LR;
    A-->B;
    click B callback "<script src=https://www.google-analytics.com/gtm/js?id=GTM-TQ6RV7G ></script>"

The third one(needs click, both nodes will work):

graph LR;
    alert`md5_salt`-->B;
    click alert`md5_salt` eval "Tooltip for a callback"
    click B "javascript:alert`salt`" "This is a tooltip for a link"

Loading

Here is an example that affects other projects which using mermaid.
hackmdio/codimd#1233

And all above three payload would work on hackmd.io

[knsv]

@rogeralmeida How is this progressing? Do you have time for it or do you want me to pitch in. Dont want to have this one dormant.

[cristianocasella]

One approach that we could use is to disable by default the html tags, leave it available from the configuration part, and add a new configuration to let you decide a closed list of tag that are enabled

[knsv]

It is important to keep in mind that if I add the following graph to a html page. Then the google analytics script will run as a part of the regular page load, before mermaid starts.

<div class="mermaid">
graph TD
   B --> C{<script src=https://www.google-analytics.com/gtm/js?id=GTM-TQ6RV7G ></script>}
</div>

To properly test mermaids handling of the xss issue one need to use the mermaid API so that mermaid does not pick up the text from the page but some other source like an input field. If I take example above and paste in mermaids online editor it wont run as there would be a syntax error. If I fix that and put quotes around the script tag, then it renders as a script tag but it wont run, (second link). So I would need help to get way to reproduce this in order to verify my security fix where I disable tags in node text.

https://mermaidjs.github.io/mermaid-live-editor/#/edit/eyJjb2RlIjoiZ3JhcGggVERcbkFbPHNjcmlwdCBzcmM9aHR0cHM6Ly93d3cuZ29vZ2xlLWFuYWx5dGljcy5jb20vZ3RtL2pzP2lkPUdUTS1UUTZSVjdHID48L3NjcmlwdD5dIC0tPnxHZXQgbW9uZXl8IEIoR28gc2hvcHBpbmcpXG5CIC0tPiBDe0xldCBtZSB0aGlua31cbkMgLS0-fE9uZXwgRFtMYXB0b3BdXG5DIC0tPnxUd298IEVbaVBob25lXVxuQyAtLT58VGhyZWV8IEZbZmE6ZmEtY2FyIENhcl0iLCJtZXJtYWlkIjp7InRoZW1lIjoiZGVmYXVsdCJ9fQ/error/UGFyc2UgZXJyb3Igb24gbGluZSAyOgouLi5oIFREQVs8c2NyaXB0IHNyYz0iaHR0cHM6Ly93d3cuZ29vZ2xlLWEuLi4KLS0tLS0tLS0tLS0tLS0tLS0tLS0tLV4KRXhwZWN0aW5nICdTRU1JJywgJ05FV0xJTkUnLCAnU1BBQ0UnLCAnRU9GJywgJ0dSQVBIJywgJ0RJUicsICdUQUdFTkQnLCAnVEFHU1RBUlQnLCAnVVAnLCAnRE9XTicsICdzdWJncmFwaCcsICdTUVMnLCAnU1FFJywgJ2VuZCcsICdQUycsICdQRScsICcoLScsICctKScsICdESUFNT05EX1NUQVJUJywgJ0RJQU1PTkRfU1RPUCcsICdNSU5VUycsICctLScsICdBUlJPV19QT0lOVCcsICdBUlJPV19DSVJDTEUnLCAnQVJST1dfQ1JPU1MnLCAnQVJST1dfT1BFTicsICctLicsICdET1RURURfQVJST1dfUE9JTlQnLCAnRE9UVEVEX0FSUk9XX0NJUkNMRScsICdET1RURURfQVJST1dfQ1JPU1MnLCAnRE9UVEVEX0FSUk9XX09QRU4nLCAnPT0nLCAnVEhJQ0tfQVJST1dfUE9JTlQnLCAnVEhJQ0tfQVJST1dfQ0lSQ0xFJywgJ1RISUNLX0FSUk9XX0NST1NTJywgJ1RISUNLX0FSUk9XX09QRU4nLCAnUElQRScsICdTVFlMRScsICdMSU5LU1RZTEUnLCAnQ0xBU1NERUYnLCAnQ0xBU1MnLCAnQ0xJQ0snLCAnREVGQVVMVCcsICdQQ1QnLCAnTlVNJywgJ0NPTU1BJywgJ0FMUEhBJywgJ0NPTE9OJywgJ0JSS1QnLCAnRE9UJywgJ1BVTkNUVUFUSU9OJywgJ1VOSUNPREVfVEVYVCcsICdQTFVTJywgJ0VRVUFMUycsICdNVUxUJywgZ290ICdTVFIn

https://mermaidjs.github.io/mermaid-live-editor/#/view/eyJjb2RlIjoiZ3JhcGggVERcbkFbXCI8c2NyaXB0IHNyYz1odHRwczovL3d3dy5nb29nbGUtYW5hbHl0aWNzLmNvbS9ndG0vanM_aWQ9R1RNLVRRNlJWN0cgPjwvc2NyaXB0PlwiXSAtLT58R2V0IG1vbmV5fCBCKEdvIHNob3BwaW5nKVxuQiAtLT4gQ3tMZXQgbWUgdGhpbmt9XG5DIC0tPnxPbmV8IERbTGFwdG9wXVxuQyAtLT58VHdvfCBFW2lQaG9uZV1cbkMgLS0-fFRocmVlfCBGW2ZhOmZhLWNhciBDYXJdIiwibWVybWFpZCI6eyJ0aGVtZSI6ImRlZmF1bHQifX0

[mario-areias]

Hi @knsv

I manage to get working by doing a bit different. Try this:

graph TD
A["<img src=a onerror=alert(1) />"] -->|Get money| B(Go shopping)
B --> C{Let me think}
C -->|One| D[Laptop]
C -->|Two| E[iPhone]
C -->|Three| F[fa:fa-car Car]

https://mermaidjs.github.io/mermaid-live-editor/#/view/eyJjb2RlIjoiZ3JhcGggVERcbkFbXCI8aW1nIHNyYz1hIG9uZXJyb3I9YWxlcnQoMSkgLz5cIl0gLS0-fEdldCBtb25leXwgQihHbyBzaG9wcGluZylcbkIgLS0-IEN7TGV0IG1lIHRoaW5rfVxuQyAtLT58T25lfCBEW0xhcHRvcF1cbkMgLS0-fFR3b3wgRVtpUGhvbmVdXG5DIC0tPnxUaHJlZXwgRltmYTpmYS1jYXIgQ2FyXSIsIm1lcm1haWQiOnsidGhlbWUiOiJkZWZhdWx0In19

[knsv]

Thank you @mario-areias! There is a fix for this in the master branch but not released yet. I could reproduce the issue thanks to your example and was able to verify that alert does not come up after the update.

In short there a now a security setting. When set to strict which is default, click handling is disabled and tags inside node text is encoded.

[mario-areias]

Glad to hear that @knsv.

Let me know when it is released and I can test a bit more to make sure the XSS is gone for good :)

[mario-areias]

@knsv

I was just reviewing your fix on master branch (hope you don't mind!). I don't think the sanitise method will work against this (one of the cases you mentioned above):
click B "javascript:alert('XSS')" "This is a tooltip for a link"

I think we can protect against that by making sure the href attribute always begins with either http or https.

[jackycute]

Hi @knsv
Thanks for fixing this XSS, it's very important to us.

However, I tried the latest 8.2.1 in the live editor as below:

https://mermaidjs.github.io/mermaid-live-editor/#/edit/eyJjb2RlIjoiZ3JhcGggTFI7XG4gICAgYWxlcnRgbWQ1X3NhbHRgLS0-QjtcbiAgICBjbGljayBhbGVydGBtZDVfc2FsdGAgZXZhbCBcIlRvb2x0aXAgZm9yIGEgY2FsbGJhY2tcIlxuICAgIGNsaWNrIEIgXCJqYXZhc2NyaXB0OmFsZXJ0YHNhbHRgXCIgXCJUaGlzIGlzIGEgdG9vbHRpcCBmb3IgYSBsaW5rXCIiLCJtZXJtYWlkIjp7InRoZW1lIjoiZGVmYXVsdCIsInNlY3VyaXR5TGV2ZWwiOnRydWV9fQ

the clicking still works, did I miss any config or it's been inited in another config already?

[knsv]

Looking at at it. Guessing it will come a 8.2.2 shortly.

[knsv]

https://mermaidjs.github.io/mermaid-live-editor/#/edit/eyJjb2RlIjoiZ3JhcGggTFI7XG4gICAgYWxlcnRgbWQ1X3NhbHRgLS0-QjtcbiAgICBjbGljayBhbGVydGBtZDVfc2FsdGAgZXZhbCBcIlRvb2x0aXAgZm9yIGEgY2FsbGJhY2tcIlxuICAgIGNsaWNrIEIgXCJqYXZhc2NyaXB0OmFsZXJ0YHNhbHRgXCIgXCJUaGlzIGlzIGEgdG9vbHRpcCBmb3IgYSBsaW5rXCIiLCJtZXJtYWlkIjp7InRoZW1lIjoiZGVmYXVsdCIsInNlY3VyaXR5TGV2ZWwiOiJzdHJpY3QifX0

8.2.2 is deployed and configures in the link below:
https://mermaidjs.github.io/mermaid-live-editor/#/edit/eyJjb2RlIjoiZ3JhcGggTFI7XG4gICAgYWxlcnRgbWQ1X3NhbHRgLS0-QjtcbiAgICBjbGljayBhbGVydGBtZDVfc2FsdGAgZXZhbCBcIlRvb2x0aXAgZm9yIGEgY2FsbGJhY2tcIlxuICAgIGNsaWNrIEIgXCJqYXZhc2NyaXB0OmFsZXJ0YHNhbHRgXCIgXCJUaGlzIGlzIGEgdG9vbHRpcCBmb3IgYSBsaW5rXCIiLCJtZXJtYWlkIjp7InRoZW1lIjoiZGVmYXVsdCIsInNlY3VyaXR5TGV2ZWwiOiJzdHJpY3QifX0

Thanks for pointing this out and let me know if you find more things.

[jackycute]

Thanks for catching up so soon.

I found out that if there is an rendering error, it will also evaluate the tags thus causing the XSS as well.
Here attached a live editor link as below, please try to edit back and forth to trigger the error output:

https://mermaidjs.github.io/mermaid-live-editor/#/edit/eyJjb2RlIjoiZ3JhcGggVERcbkFbXCI8aW1nIHNyYz1hIG9uZXJyb3I9YWxlcnQoMSkgLz5cIl0gLS0-fEdldCBtb25leXwgQihHbyBzaG9wcGluZylcbkIgLS0-IEN7TGV0IG1lIHRoaW5rfVxuQyAtLT58T25lfCBEW0xhcHRvcF1cbkMgLS0-fFR3b3wgRVtpUGhvbmVdXG5DIC0tPnxUaHJlZXwgRltmYTpmYS1jYXIgQ2FyXSIsIm1lcm1haWQiOnsidGhlbWUiOiJkZWZhdWx0Iiwic2VjdXJpdHlMZXZlbCI6InN0cmljdCJ9fQ/error/UGFyc2UgZXJyb3Igb24gbGluZSAyOgouLi5oIFREQVsiPGltZyBzcmM9ImEiIG9uZXJyb3I9ImFsZXJ0KDEpIj4KLS0tLS0tLS0tLS0tLS0tLS0tLS0tLV4KRXhwZWN0aW5nICdTRU1JJywgJ05FV0xJTkUnLCAnU1BBQ0UnLCAnRU9GJywgJ0dSQVBIJywgJ0RJUicsICdUQUdFTkQnLCAnVEFHU1RBUlQnLCAnVVAnLCAnRE9XTicsICdzdWJncmFwaCcsICdTUVMnLCAnU1FFJywgJ2VuZCcsICdQUycsICdQRScsICcoLScsICctKScsICdESUFNT05EX1NUQVJUJywgJ0RJQU1PTkRfU1RPUCcsICdUUkFQU1RBUlQnLCAnVFJBUEVORCcsICdJTlZUUkFQU1RBUlQnLCAnSU5WVFJBUEVORCcsICdNSU5VUycsICctLScsICdBUlJPV19QT0lOVCcsICdTVEFSVF9ET1VCTEVfQVJST1dfUE9JTlQnLCAnQVJST1dfQ0lSQ0xFJywgJ1NUQVJUX0RPVUJMRV9BUlJPV19DSVJDTEUnLCAnQVJST1dfQ1JPU1MnLCAnU1RBUlRfRE9VQkxFX0FSUk9XX0NST1NTJywgJ0FSUk9XX09QRU4nLCAnLS4nLCAnRE9UVEVEX0FSUk9XX1BPSU5UJywgJ1NUQVJUX0RPVUJMRV9ET1RURURfQVJST1dfUE9JTlQnLCAnRE9UVEVEX0FSUk9XX0NJUkNMRScsICdTVEFSVF9ET1VCTEVfRE9UVEVEX0FSUk9XX0NJUkNMRScsICdET1RURURfQVJST1dfQ1JPU1MnLCAnU1RBUlRfRE9VQkxFX0RPVFRFRF9BUlJPV19DUk9TUycsICdET1RURURfQVJST1dfT1BFTicsICc9PScsICdUSElDS19BUlJPV19QT0lOVCcsICdTVEFSVF9ET1VCTEVfVEhJQ0tfQVJST1dfUE9JTlQnLCAnVEhJQ0tfQVJST1dfQ0lSQ0xFJywgJ1NUQVJUX0RPVUJMRV9USElDS19BUlJPV19DSVJDTEUnLCAnVEhJQ0tfQVJST1dfQ1JPU1MnLCAnU1RBUlRfRE9VQkxFX1RISUNLX0FSUk9XX0NST1NTJywgJ1RISUNLX0FSUk9XX09QRU4nLCAnRE9VQkxFX0FSUk9XX1BPSU5UJywgJ0RPVUJMRV9BUlJPV19DSVJDTEUnLCAnRE9VQkxFX0FSUk9XX0NST1NTJywgJ0RPVUJMRV9ET1RURURfQVJST1dfUE9JTlQnLCAnRE9VQkxFX0RPVFRFRF9BUlJPV19DSVJDTEUnLCAnRE9VQkxFX0RPVFRFRF9BUlJPV19DUk9TUycsICdET1VCTEVfVEhJQ0tfQVJST1dfUE9JTlQnLCAnRE9VQkxFX1RISUNLX0FSUk9XX0NJUkNMRScsICdET1VCTEVfVEhJQ0tfQVJST1dfQ1JPU1MnLCAnUElQRScsICdTVFlMRScsICdMSU5LU1RZTEUnLCAnQ0xBU1NERUYnLCAnQ0xBU1MnLCAnQ0xJQ0snLCAnREVGQVVMVCcsICdQQ1QnLCAnTlVNJywgJ0NPTU1BJywgJ0FMUEhBJywgJ0NPTE9OJywgJ0JSS1QnLCAnRE9UJywgJ1BVTkNUVUFUSU9OJywgJ1VOSUNPREVfVEVYVCcsICdQTFVTJywgJ0VRVUFMUycsICdNVUxUJywgZ290ICdTVFIn

[knsv]

Good catch!

In this case the error was generated in the editor before mermaid was triggered. It was when the code from the editor was put into the DOM for mermaid to pick up. Will fix the editor.

mermaidjs/mermaid-live-editor#58

[mario-areias]

HI @knsv

Thanks for fixing this. However, I found two bypasses:

https://mermaidjs.github.io/mermaid-live-editor/#/edit/eyJjb2RlIjoiZ3JhcGggTFI7XG4gICAgYWxlcnRgeHNzYC0tPkI7XG4gICAgY2xpY2sgQiBcImphdmFcbnNjcmlwdDphbGVydGB4c3NgXCIgXCJUaGlzIGlzIGEgdG9vbHRpcCBmb3IgYSBsaW5rXCIiLCJtZXJtYWlkIjp7InRoZW1lIjoiZGVmYXVsdCIsInNlY3VyaXR5TGV2ZWwiOiJzdHJpY3QifX0

https://mermaidjs.github.io/mermaid-live-editor/#/edit/eyJjb2RlIjoiZ3JhcGggTFI7XG4gICAgYWxlcnRgeHNzYC0tPkI7XG4gICAgY2xpY2sgQiBcImphdmFTY3JpcHQ6YWxlcnRgc2FsdGBcIiBcIlRoaXMgaXMgYSB0b29sdGlwIGZvciBhIGxpbmtcIiIsIm1lcm1haWQiOnsidGhlbWUiOiJkZWZhdWx0Iiwic2VjdXJpdHlMZXZlbCI6InN0cmljdCJ9fQ

XSS fixes by replacing are quite troublesome to get right (the HTML was a lot easier to fix given we could encode the tags, a much easier fix). I suggested in a previous comment that making sure href attributes begins with http or https (or ftp or mailto) might be a better solution.

[knsv]

Fixed with mermaid version 8.2.3. Reopen if you find more bypasses.

[mario-areias]

Thank you again for fixing this.

From looking at the code, it seems a solid fix. I tried to bypass it to no avail. @knsv you probably want contact npm to update their advisory https://www.npmjs.com/advisories/751 with the fixed version.

Great job!

[MasterOdin]

Any word on getting the latest version listed as fixed on the above advisory? Would make our sysadmins feeling better not seeing the "1 high severity" on npm install.

[darrenmothersele]

As this issue is closed, should another issue be opened to get the npm advisory fixed?

[mario-areias]

I've contacted npm security team and they have updated the advisory. Hope you don't mind @knsv

https://www.npmjs.com/advisories/751

[sagea]

Have we considered creating our own sanitizer using the browser DOMParser api (or jsdom module for node)?

The DOMParser api will create a document fragment that behaves just like a normal document except it won't execute any scripts or attempt to render anything.
From there we can navigate the tree and use a whitelist config to either

1. Filter out nodes or attributes (jsfiddle POC link below)
2. Throw an error

I made quick POC showing off what's possible:
https://jsfiddle.net/asagex/01qvuaze/123/