Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cross-Site Scripting:DOM - Issue #847

Closed
karthikdav opened this issue May 27, 2019 · 29 comments
Closed

Cross-Site Scripting:DOM - Issue #847

karthikdav opened this issue May 27, 2019 · 29 comments
Assignees
Labels
Type: Bug / Error Something isn't working or is incorrect

Comments

@karthikdav
Copy link

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 knsv added the security label May 28, 2019
@knsv
Copy link
Collaborator

knsv commented May 28, 2019

Thx, we should look at this

@knsv
Copy link
Collaborator

knsv commented Jun 5, 2019

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
Copy link
Author

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
Copy link

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
Copy link

rogeralmeida commented Jun 26, 2019

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
Copy link

rogeralmeida commented Jun 26, 2019

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 &lt; and &gt; 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
Copy link
Collaborator

knsv commented Jun 30, 2019

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 &lt; and &gt; 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
Copy link
Collaborator

knsv commented Jun 30, 2019

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

@rogeralmeida
Copy link

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

@knsv
Copy link
Collaborator

knsv commented Jul 5, 2019

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
Copy link
Collaborator

knsv commented Jul 13, 2019

@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
Copy link

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
Copy link
Collaborator

knsv commented Jul 14, 2019

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

knsv added a commit that referenced this issue Jul 14, 2019
@mario-areias
Copy link

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
Copy link
Collaborator

knsv commented Jul 14, 2019

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
Copy link

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
Copy link

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

knsv added a commit that referenced this issue Jul 16, 2019
@knsv knsv closed this as completed Jul 21, 2019
@jackycute
Copy link

@knsv
Copy link
Collaborator

knsv commented Jul 21, 2019

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

@knsv knsv reopened this Jul 21, 2019
@jackycute
Copy link

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

螢幕快照 2019-07-21 下午11 10 01

@knsv
Copy link
Collaborator

knsv commented Jul 21, 2019

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
Copy link

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 added a commit that referenced this issue Jul 22, 2019
@knsv
Copy link
Collaborator

knsv commented Jul 23, 2019

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

@knsv knsv closed this as completed Jul 23, 2019
@mario-areias
Copy link

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
Copy link

MasterOdin commented Jul 31, 2019

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
Copy link

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

@mario-areias
Copy link

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
Copy link
Contributor

sagea commented Aug 5, 2019

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/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

No branches or pull requests

10 participants