Security concern regarding class definition

[benweet]

Just looked at the code in utils.js and I can see that CSS classes are added in stylesheet elements using innerHTML attribute. There might be a security risk (XSS attack, see http://stackoverflow.com/questions/476276/using-javascript-in-css#answer-482088) by doing this with user entered data (my plan is to integrate mermaid in a markdown editor where files can be shared between different users).
Is there any sanitization of the included CSS, or more generally, any check of the input data? If not, is there a way to disable CSS classes for the whole library?
Thanks.

[knsv]

Good point!

I think the basic idea to start with was that the styles being copied are the ones already in-play on the page. If those styles have been compromised then the hostile script has already executed on the page. Is this different in your context? Do you have users entering styles or similar?

Currently css files tagged with: title="mermaid-svg-internal-css" are ignored.

Some styles are required for the diagrams and charts to be rendered properly so it would not work well to ignore all css files.

@bjowes, do you have further comments to this one?

[benweet]

Thanks for fast response.

StackEdit (https://stackedit.io) works with js-sequence-diagrams (http://bramp.github.io/js-sequence-diagrams/) at the moment. Users can type diagrams between triple backticks:

```sequence
Alice->Bob: Hello Bob, how are you?
Note right of Bob: Bob thinks
Bob-->Alice: I am good thanks!
```

And the live preview shows the result in realtime. I didn't make any security audit personally on js-sequence-diagrams but the library looks straight forward. Mermaid has a couple of functionalities like styling/css classes that are very powerful but also quite scary. If css is loaded from user input (ie by typing classDef className fill:#f9f,stroke:#333,stroke-width:4px; in the editor), users may end up running scripts from other users when opening a shared file.

The same issue applies to other libraries like MathJax but it's already used in big web sites like stack exchange which is quite comforting.

[bjowes]

Interesting!

As Knut points out, this was originally intended to copy styles within the existing page. The purpose is to make the generated SVG independent, such that it could be saved as a separate file and still display in the same manner as on the web page.

I haven't looked into how your project is set up, but from what I could see in the page you referred on Stackoverflow, scripts are loaded in CSS through a XML file. This file must reside somewhere - if you are running your project over https, modern browsers should reject loading of content from other domains, which I presume should block this possibility unless you allow users to upload files to your domain.

A general solution with sanitation usually proves to be quite difficult - even if there are few known cases to check for now they tend to grow over time.

If it really proves to be a security concern, I would opt for adding an option to skip copying of CSS from the page and stick only to the default styling of mermaid.

[benweet]

My point is that users would be able to inject XSS attacks in a mermaid diagram definition inside their markdown files. For example, a markdown file could look like this (styling and class definition could contain XSS code):

# Flowchart below

```
graph LR
id1(Start)-->id2(Stop)
style id1 fill:#f9f,stroke:#333,stroke-width:4px;
classDef className fill:#f9f,stroke:#333,stroke-width:4px;
class id2 className;
```

A quick and dirty workaround is to make pre-sanitization of the diagram definition by removing styling from it before generating the preview.
A better solution would be to have an option in mermaid startup to disable flowcharts styling.
Another idea (I don't know if it's feasible) would be to have mermaid converter that generates HTML text (SVG+CSS) from diagram definition and let the integrator do the innerHTML injection in the page, just like a markdown converter. This would allow the integrator to pass through a post-sanitizer before injecting the HTML in the page. I guess I'm thinking of a mermaid CLI without the phantomJS dependency...

[knsv]

I see your point and I have been thinking a bit on this!

Maybe the responsibility for that check should be in the interface where the code is entered. Many times such capabilities are present already in web frameworks like for instance angularjs. If we add this into mermaid there is a risk of unnecessary overlap.

I see where you are going with the HTML text converter. d3 uses the DOM but maybe there is a fake DOM that could be used. Seems to be some amount of work involved though...

Knut Sveidqvist
Sent with Sparrow (http://www.sparrowmailapp.com/?sig)

On Thursday 9 April 2015 at 18:54, Benoit Schweblin wrote:

My point is that users would be able to inject XSS attacks in a mermaid diagram definition inside their markdown files. For example, a markdown file could look like this (styling and class definition could contain XSS code):

Flowchart below graph LR id1(Start)-->id2(Stop) style id1 fill:#f9f,stroke:#333,stroke-width:4px; classDef className fill:#f9f,stroke:#333,stroke-width:4px; class id2 className;

A quick and dirty workaround is to make pre-sanitization of the diagram definition by removing styling from it.
A better solution would be to have an option in mermaid startup to disable flowcharts styling.
Another idea (I don't know if it's feasible) would be to have mermaid converter that generates HTML text (SVG+CSS) from diagram definition and let the integrator do the innerHTML injection in the page, just like a markdown converter. This would allow the integrator to pass through a post-sanitizer before injecting the HTML in the page. I guess I'm thinking of a mermaid CLI without the phantomJS dependency...

—
Reply to this email directly or view it on GitHub (#148 (comment)).

[knsv]

I am going through issues am closing this one. Let me know you disagree.