lucianot54 commented 5 years ago

Expected Behavior

Don't execute html/javascript by default for Toasts, Tooltips and Autocomplete

Current Behavior

By default when you add dynamics contents like Toast, Tooltips and autocomplete you inject inputs data as HTML. I think all people who use MaterializeCss implement compontents like your examples.

It's a bad practice and by default, your input data could be sanitize or use jquery.text('untrustData') instead jquery.html('untrustData') or innerHTML = 'untrustData'.

Possible Solution

If you want allow HTML, why not but sanitize the html and don't allow javascript. If the end user want allow HTML and javascript add a new configuration with a parameter like options : { allowUnsafeData: true }

Steps to Reproduce (for bugs)

Toast

var userId = '<IFRAME SRC="javascript:alert(document.cookie);"></IFRAME>'
M.toast({html: `userId: ${userId} fail`})

Tooltips

<a class="btn tooltipped" data-position="bottom" data-tooltip="<IFRAME SRC='javascript:alert(document.cookie);'></IFRAME></script>">Hover me!</a>

Autocomplete

$('input.autocomplete').autocomplete({
    data: {
        "Apple": "\"><IFRAME SRC=\"javascript:alert(document.cookie);\"></IFRAME>",
        "Microsoft": null,
        "Google": 'https://placehold.it/250x250',
    }
});

Context

I'm agree about the developper need control his data before inject it in a third party library but sametime we forget to do it.

How i find this case :

(Client) Send javascript in a field
(Server) Server fail and return message with javascript
(Client) Reuse the message from the server and use the Toast
I have a reflected XSS

It's my fault, I didn't validate datas and I returned this script without sanitize. If by default your library don't allow html, I will not find this behavior.

Your Environment

Version used: 0.100.2 and 1.0.0

FINDarkside commented 5 years ago

If it needs to support custom content, it could take HTMLElement as argument. That's better than taking string as argument and then rendering it as HTML.

lucianot54 commented 5 years ago

@FinDarkside, I'm agree with you but in 80% of use case you will use text. If you present your example with html content by defaut, all developer will use it. Sanitize input parameters is an another subject.

If we had the choice between html or text, it can be well but with a toast/tooltips, we have just html option. So include a vulnerability by default.

DanielRuf commented 5 years ago

I see no XSS here.

If you have a found a real XSS with a PoC, please directly contact @Dogfalo in private (responsible disclosure).

lucianot54 commented 5 years ago

@DanielRuf to be honest, what is a XSS for you?

Reflected XSS: The application or API includes unvalidated and unescaped user input as part of HTML output. A successful attack can allow the attacker to execute arbitrary HTML and JavaScript in the victim’s browser. https://www.owasp.org/index.php/Top_10-2017_A7-Cross-Site_Scripting_(XSS)

On my first post you have a POC for each component. I can execute a JavaScript, it's a XSS.

MaterializeCss use by default HTML and for me it's a lack of security. You want use a content HTML, you must sanitizes datas.

Toasts

The problem for me about toasts, it's the documentation. all examples used

M.toast({html: 'message'}).

Why use html for all examples and after to have an exemple for the Custom HTML. I'm a developer, I copy paste this simple example with the html parameter and I added a vulnerability in my project. For a static value it isn't a problem, for a dynamic value... You can check bad usages: https://github.com/search?q=%22M.toast%28%7Bhtml%3A%22&type=Code

If you want allow html, sanitaze "inputs datas", if you want use a trust content, you can use this syntaxe :

M.toast({html: 'message', sanitize:false}).

https://www.npmjs.com/package/sanitize-html

Bootstrap do this: https://getbootstrap.com/docs/3.4/javascript/#js-sanitizer

Tooltips

It's explained in the documentation

HTML String : Can take regular text or HTML strings. https://materializecss.com/tooltips.html

How can I disable html content? For me it's HTML only. https://github.com/Dogfalo/materialize/blob/v1-dev/js/tooltip.js#L71 And the innerHTML ... https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/DOM_based_XSS_Prevention_Cheat_Sheet.md#example-dangerous-html-methods

Autocomplete

Same story, if you allow unsafe html, you can inject javascript... The problem isn't here https://github.com/Dogfalo/materialize/blob/v1-dev/js/autocomplete.js#L292 but here https://github.com/Dogfalo/materialize/blob/v1-dev/js/autocomplete.js#L384

Jquery + append()/html() = XSS

A simple POC, go to the page : https://materializecss.com/autocomplete.html

use this code in your console

$('input.autocomplete').autocomplete({
data: {
    "Apple": `x" onerror="javascript:alert(document.cookie)`,
    "Microsoft": null,
    "Google": 'https://placehold.it/250x250',
}
});

And search Apple :)

If you check XSS for bootstrap, you have the same problem and it doesn't allow an unsafe html https://github.com/twbs/bootstrap/issues?utf8=%E2%9C%93&q=xss https://github.com/twbs/bootstrap/pull/28236 https://github.com/twbs/bootstrap/issues/28290 https://github.com/twbs/bootstrap/issues/26625

DanielRuf commented 5 years ago

Sorry but this is not how XSS works. Show me a codepen with a default setup of our components which parse user input from a default field or the URL.

So far devs have to ensure what the data is allowed to contain and sanitize it.

DanielRuf commented 5 years ago

User input can not create a malicious data object like you have shown.

Using the console does not show that there is a real XSS vuln as this is not user controlled input (form fields, addressbar, ...).

DanielRuf commented 5 years ago

Reflected XSS is for example this:

Enter HTML and JS into search form. Submit form. Search result page ouputs it unsanitized.

FINDarkside commented 5 years ago

This is exactly how XSS works. If you, for example do auto completion on user search, you can do XSS by naming your user properly. But as you've said, might not be the best idea to talk about this here, I've made a PoC and reported it to npm. They'll contact the package owner if they deem it to be XSS.

You don't have to form any objects, the malicious input in the example by @lucianot54 is a string.

DanielRuf commented 5 years ago

No, there is no real direct input. entry.data is from the data object / response so you have to manipulate this part.

Still, no real XSS when a dev allows user input to be unsanitized in the output.

lucianot54 commented 5 years ago

What is a Real XSS? You are confuse with a vulnerability and an exploitability. Materialize doesn't control input parameters, so you are an injection. It's the first line from OWASP TOP 10 A1-injection

User-supplied data is not validated, filtered, or sanitized by the application. https://www.owasp.org/index.php/Top_10-2017_A1-Injection

Also, Do you want a use case to exploit this vulnerability? I store my autocompletion in a database, with name and image. I'm a user who can update this list I store the name (XSS) or my image (XSS). URL or javascript it's just a string All people who use this autocomplete will be infected, It's a XSS stored.

FINDarkside commented 5 years ago

Still, no real XSS when a dev allows user input to be unsanitized in the output.

Of course it is, because it's completely clear that the searchable input should be rendered as text instead of html. And Materializecss is what handles the output, so by your definition this is a vulnerability. There's nothing wrong with the input, there's lot of wrong on how autocomplete handles the input. I'm not going to argue about this anymore as it doesn't really matter, I've already made the report with PoC. What comes to sanitizing the input, it's actually not possible to do without breaking the auto completion.

DanielRuf commented 5 years ago

To be clear, it is intended that we support HTML there. We do not explicitely filter or sanitize it.

If you use any user input in there you should ensure to sanitize any user input in general. As this is not the normal use case the sanitizing part is out of the projects scope imo.

Sure you can trigger this manually with malicious snippets like you demonstrated. But when such code can be executed through UI interactions you have a much bigger issue (no X-XSS header, no WAF and sanitization rules and so on).

FINDarkside commented 5 years ago

To be clear, it is intended that we support HTML there.

It's actually very clear that it's not supposed to support HTML, as it indeed does not (XSS is possible though), HTML tags will break the auto completion. And it also makes no sense, as the search is performed on your whole input, which makes no sense if it's meant to be HTML. WAF nor X-XSS also have nothing to do with this and can't prevent this. And as I already said, the input can't be sanitized in a way that auto completion works properly.

The right side of input is supposed to be url. Are you really claiming that it's intended to be rendered as html, inside a img source attribute? The only use case for rendering string as html inside img tag source attribute is to do XSS attack. There's no reason for developers to expect that this library handles input that's supposed to be url as html. Would you say that if materialize ran the input trough eval it would also be users fault for not sanitizing the input?

As this is not the normal use case

Doubt this is the "official" opinion. And if it were, the readme will need a big warning about it.

DanielRuf commented 5 years ago

It's actually very clear that it's not supposed to support HTML, as it indeed does not (XSS is possible though), HTML tags will break the auto

userId = '<IFRAME SRC=

x" onerror="javascript:alert

Well, this is valid HTML an there are many attributes and things to sanitize.

WAF nor X-XSS

These do prevent reflected XSS. Reflected (not stored) XSS is about user controllable input (and its output). Not output in general.

Reflected XSS: The application or API includes unvalidated and unescaped user input as part of HTML output.

DanielRuf commented 5 years ago

Would you say that if materialize ran the input trough eval it would also be users fault for not sanitizing the input?

We do not know and can not know how you process and use user input.

0.x will not get any updates anymore and 1.0 dev is a bit paused at the moment.

FINDarkside commented 5 years ago

Well, this is valid HTML an there are many attributes and things to sanitize.

This is how you render text (with jquery as done in autocomplete) elem.text(s) and this is how you render it as html elem.html(s). Or without Jquery, textContent, vs innerText instead of innerHTML

We do not know and can not know how you process and use user input.

Again, this is not about how "we" handle input, it's about how materialize handles input. If something is supposed to be url, it's reasonable to expect that it'll be handled as text instead of html. If something is supposed to be string to be auto completed, it's reasonable to expect it'll be handled as text instead of html.

Reflected XSS: The application or API includes unvalidated and unescaped user input as part of HTML output.

Yes, but you're the only one who's talking about Reflected XSS. Besides you're not supposed to rely on X-XSS, it's just a safe guard. If it saves you from something, you have done something seriously wrong.

DanielRuf commented 5 years ago

Well, I did not, see https://github.com/Dogfalo/materialize/issues/6286#issuecomment-480297078

Anyway, this will not be fixed in 0.x, and probably also not in 1.x as far as I can say.

FINDarkside commented 5 years ago

Okay, thanks for the input. Also, I was only talking about the case with autocomplete, where it's not expected that the input is handled as html. With toasts it's obviously completely clear as you have to supply an object with html field.

DanielRuf commented 5 years ago

And to make this clear again: html: and other options are meant to work like this and always worked like this.

https://materializecss.com/toasts.html

To rephrase you:

I'm agree about the developper need control his data before inject it in a third party library but sametime we forget to do it.

It's my fault, I didn't validate datas and I returned this script without sanitize. If by default your library don't allow html, I will not find this behavior.

https://github.com/OWASP/CheatSheetSeries/blob/master/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.md

lucianot54 commented 5 years ago

Well, this is valid HTML an there are many attributes and things to sanitize.

If you use blacklist yes it's impossible, if you use a white list,it's possible. Example: https://getbootstrap.com/docs/3.4/javascript/#js-sanitizer

OR sanitaze by default and if you want use a safe HTML. You can allow it with a parameter. Angular(not angularjs) is 100% safe because it escape all by default.

WAF nor X-XSS prevent XSS

YES but that's a reason to keep a vulnerability? It's a library not your custom website. Everybody doesn't know what is a XSS. So HTTP Headers (x-xss-protection/Content-Security-Policy) or a WAF...

These do prevent reflected XSS. Reflected (not stored) XSS is about user controllable input (and its output). Not output in general.

An XSS is always output, you change the dom and update the html,if you have a script, you execute a XSS. And to protect output, you need control input...

We do not know and can not know how you process and use user input.

You can...

By default convert all in text
If HTML added in the config, allow with a whitelist/sanitize
Allow all like now with an additional parameter {html: true, sanitaze: false}

Just 3 options with flexibility and security

To be clear, it is intended that we support HTML there. We do not explicitely filter or sanitize it.

If you allow XSS, it's ok for me, but MaterializeCSS need a CVE to explain that publicly

lucianot54 commented 5 years ago

And to make this clear again: html: and other options are meant to work like this and always worked like this. https://materializecss.com/toasts.html

There is 0 example without html and the result of the community: https://github.com/search?q=%22M.toast%28%7Bhtml%3A%22&type=Code

Use HTML is a feature, with risk but a feature. Why present him like a default usage?

for autocompletion, MaterializeCSS have an XSS on a image source. Explain me, why allow a html in a SRC from the tag <img src="HTML?">?

FINDarkside commented 5 years ago

You need sanitizer only if you want to allow subset of html. It's way easier you intend to just render it as text, since you just use $.text or textContent or any other API meant to do that. In the autocomplete case, html simply makes no sense. If you give it html it will match when user writes html tags, and additionally it won't render the html tags properly because it inserts span tags when highlighting the match.

lucianot54 commented 5 years ago

I requested 3 CVE : https://nvd.nist.gov/vuln/detail/CVE-2019-11002 https://nvd.nist.gov/vuln/detail/CVE-2019-11003 https://nvd.nist.gov/vuln/detail/CVE-2019-11004

lirantal commented 5 years ago

Hi folks,

Liran from Snyk and The Node.js Security WG chiming in here 👋

Appreciate the detailed report @lucianot54 and @FINDarkside's elaborate reasoning as well. There is definitely merit here, and I wanted to share my own as well to see if we can get to an alignment.

As for the Toast related vulnerability

As I see it, this is not vulnerable per-say, as the API expects an HTML (M.toast({html: 'message'}).) so I agree with @DanielRuf that this is “by design”. Would you say that jQuery's $.append() method is vulnerable to XSS because it allows you to add HTML elements? (see reference here: http://api.jquery.com/append/). It seems to me that this ability of adding HTML is as the library owner had intended, and at least it is somewhat made verbose by the use of the html property.
On the other hand, that design is not very security-minded and that puts traps for uneducated developers which may mistakenly put whatever input they get there and its game over on the browser.

I believe that there’s room to create a more secure API by default with either encoding by default (with an opt out) as was suggested, or creating another property altogether M.toast({text: 'message'}). which will indeed render as text and not HTML.

Regardless of whether the APIs will change or not, which is at the discretion of the maintainer and their time to handle this, at the very least the maintainer should add a very clear and obvious security disclosure in the docs and README to make users aware of the dangers and pitfalls of using the insecure html mode.

For autocomplete and tooltip related vulnerability

I believe there is indeed a case for valid vulnerabilities in these as it is completely not obvious that HTML is allowed there, plus not sure it makes too much sense either, and I don't think the maintainers have intended for users to put HTML there. Would require @DanielRuf's assessment on that.

--

EDIT: also want to invite @MarcinHoppe my colleague from the WG to see if he has similar or different opinions on this.

FINDarkside commented 5 years ago

Thanks for your input! The main thing which points that autocomplete rendering html is not intentional, is that you have to actually escape all html characters to render them as html. All the html tags will be stripped in _hihlight function, so <div>Hello</div> will become just Hello. That however does not prevent xss, as the results are then appended as html. This means that <div>Hello</div> will be rendered as <div>Hello</div>. The original matching is also done on the whole string, while highlight function will strip out html tags. Option <div>Hello</div> and input < will actually render HellHello.

lirantal commented 5 years ago

@FINDarkside completely agree.

Dogfalo commented 5 years ago

Hi @FINDarkside, Thanks for bringing this to our attention. In the tooltip and toast, it was intended to allow html as we wanted to allow developers to customize the content. We should have made warnings clear that the input was not sanitized.

However, we will make a change to sanitize html input for all 3 of the components to help protect developers who haven't sanitized their inputs.

OlivierJM commented 5 years ago

This is still reported here https://www.npmjs.com/advisories/818 on npm as moderate vulnerability, that means Github and npm will keep popping up the security issue.

@Dogfalo any idea when we can expect the fix for this ?

methodbox commented 5 years ago

Does this really apply if we aren't using the autocomplete component?

Exa.

import { updateTextFields } from 'materialize-css';

Also, it's been a month. Any fix coming some time soon, or would someone want to work with me on a PR to do so?

DanielRuf commented 5 years ago

Does this really apply if we aren't using the autocomplete component?

Probably not.

Any fix coming some time soon, or would someone want to work with me on a PR to do so?

A PR is very welcome but as mentioned before this would be a breaking change.

Also only v1.x is further developed.

Developers can already filter user input in their code using available solutions.

Dogfalo / materialize

Security question: Use HTML by default for Toasts/Tooltips/Autocomplete and expose an XSS #6286

Expected Behavior

Current Behavior

Possible Solution

Steps to Reproduce (for bugs)

Toast

Tooltips

Autocomplete

Context

Your Environment

As for the Toast related vulnerability

For autocomplete and tooltip related vulnerability