-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Fix for new security vulnerabilities? CVE-2019-11002, CVE-2019-11003, CVE-2019-11004 #6331
Comments
See: #6286 |
v0.x is not developed anymore. Only 1.x will get updates. There are no patches (yet) as this would break many things. In general the whole situation was badly handled - no responsible disclosure or coordination, a missing understanding that all of them are no real XSS vulns (it's possible but only if the user / developer uses user input without sanitization). So far you can ignore the security warning if you do not directly output and use user input in tooltip, toast and autocomplete. |
I can ignore it, but my clients who see these in their Github repo are NOT happy about it. 😳 Not sure about the 0.x comment since we use 1.0.0. |
Also these 3 are low severity. See https://www.npmjs.com/advisories for example. |
Also to make this right. These are no real XSS. Only if you use this the wrong way. It is improper input validation in MaterializeCSS (CWE 20), not XSS (CWE 79). Only if you directly handle any user controllable data in there. This should not be mixed. |
I'm not sure why you're still going with this narrative. Autocomplete vulnerability is almost textbook example of XSS, see DOM based XSS for reference. Autocomplete tries to strip HTML out before rendering, but fails to do it properly. It never supported having html as input, so it's very big stretch to call it a "feature". It's also a bit alarming how you think that it's ok or normal to intentionally have open vulnerabilities. Sure, there are lots of packages with unpatched vulnerabilities, but pretty much all of those packages are packages that no one uses and nobody probably maintains anymore. I agree that changing tooltip behavior would be breaking change. Fixing autocomplete, however, wouldn't be breaking change as it didn't really support HTML in the first place as explained here. And as also explained there, to "sanitize" your input, you must double escape your user input, which will prevent XSS, but will break autocompletion. It's in no way reasonable to expect for developers to know that you're supposed to double escape your input. I'm also willing to bet this isn't intended feature like you claim it is. Even in the very unlikely case that it was intended, it would still be a vulnerability, as it's completely undocumented. With libraries as popular as this, security should be taken very seriously. |
@kiere, you need just be careful if you use Tooltip, Autocomplete and Toast. If you didn't use this JavaScript feature, you can ignore warnings. The problem is MaterializeCss does not sanitize your HTML content and it's possible to inject a XSS.
0.x is vulnerable too, I hope to see a v0.100.3. |
I've read everything in both issues now. And it is really annoying to read everytime the same argument from @FINDarkside. DOM based XSS example is about Anyway: the website writes you should not output url input (an attacker could send this link to a victim). When using materializecss autocomplete the code (not manipulated; you have to write it on your website): $('input.autocomplete').autocomplete({
data: {
document.location.href.substring(document.location.href.indexOf("default=")+8): null,
'English': null,
}
}); I agree to @DanielRuf that this is not a vulnerability coming from materializecss. The vulnerability comes from your code when it allows such input to be unsanitized hand over to materializecss.. But on the other hand: it is really supposed to only contain text (html is possible but will be searched and the highlighting is broken) - so it should just be escaped in the output. Sounds like a bug not a vulnerability. |
Of my point of view, MaterializeCSS is a popular framework and it can offer more securities by default. I found many vulnerables developments who can to be avoid with a better security policy about the render. https://infomedialtd.github.io/angular2-materialize/#/ModelBindings
With this usage, you break Angular security just because a developper make a mistake. It's just one example. Developpers are responsables but MaterializeCSS can protect this bad usage. Allow HTML it's a good idea but you can do many things with it, like inject javascript. I'll explain again Angular example. It manage HTML and all datas are sanitazed by default. https://getbootstrap.com/docs/4.3/getting-started/javascript/#sanitizer HTML is a risk, as a popular framework, MaterializeCSS can reduce this risk and use with the initial usage and more security. I reviewed the code source and security problems are datas as HTML when the basic funtionality is a simple text. I found again more "bugs" because the code use innerHtml (example: i18n) or images allow a string/url/html. |
@lucianot54 You are absolutely right. It can protect you from writing bad code. But it is not a security vulnerability but a feature. And now we have a CVE because of a feature request and that is sad because the people not having a security issue on their page get a warning from their package management system. And maybe they step back from using materializecss because they are afraid the library is unsecure. Some developers make a great library and others destroy the reputation because they can't distinguish between a vulnerability they created and a vulnerability of the library itself. That makes me sad, sorry. |
Dogfalo already confirmed autocomplete rendering html is not a feature, so no point in continuing the argument over this.
I'm not going to waste my time on this anymore. I'll just point out that Snyk has classified this as XSS vulnerability. Having a vulnerability does not "destroy the reputation", not taking them seriously does. The point of this is not to shit on materialize or someone else, it's to keep people safe. You seem to think this is some kind of personal attack. Having vulnerabilities is nothing out of ordinary, even the most popular packages have vulnerabilities at some point. What comes to this being "just a bug", if a bug exposes users to security threat, it is by definition a vulnerability. |
That's wrong. Of course your installation rate drops when you have a vulnerability.
And the code example shows a code that I have to execute in developer tools to be a victim. Can you send me a link to a page not under your control that executes javascript in my browser? That is the definition of a xss as you can read in the link you sent. But I have to admit it is wasting of time to discuss if it is xss or not. Now there is an open ticket and it has to be fixed. |
So you're really going the route that Snyk doesn't know what a vulnerability and xss is? They've literally made a business of knowing these things.
Technically, of course, but that'd be unethical and possibly illegal. We can't use random people's sites to demonstrate security vulnerabilities.
I'll try one more time as other PoCs are already public. I'll hope you'll read this with open mind, forget your initial opinion on this topic and just read trough this. Let's assume there's a website which lets you search other users with autocomplete. Kinda like github when you @ them. Now you argue that users should sanitize their inputs. There's nothing inherently wrong with the username, so it should be stored as it is. As Dogfalo said, autocomplete is not supposed to render HTML, so there's also no reason to sanitize it before passing it to materialize, right? Materialize is expected to handle the input as text instead of html as shown in the examples. If materialize was handling those usernames as text, sanitizing them would not make sense. If you'd sanitize But fine, there's another site which actually sanitizes the input. You can confirm that it's now safe to type "typethis". But actually, by sanitizing your input now you've turned the previously safe user How about double escaping all html characters? That would probably solve the issue, but it'd make autocompletion pretty unusable. So yes, it is completely user preventable, by double sanitizing the input. That however does not mean that this isn't a vulnerability. Autocompletion is not working as it's documented, and if it's used in a way it's documented you'll expose yourself to xss if you autocomplete any user given content. It's absolutely not reasonable to expect developers to somehow know to double escape their inputs, when even the project maintainers didn't know it was necessary. Now if you still don't agree that this is a vulnerability, that's fine, I doubt there's much that will change you mind. But how many people do you think know that they need to double sanitize their inputs? Probably not many, as this issue was brought up just now. So I think we can agree that majority of people using this do not know it, which means their site is possibly vulnerable. And you're saying that you feel sad because @lucianot54 requested a couple CVE ids? You should instead be happy he found this issue and that people were alerted, so they can fix their site if needed. |
i have been assigned a task to fix these 3 XSS attacks. But, i'm having a hardtime in understanding these XSS attacks. yes there are many articles online to read about it but i'm able to reproduce only one of the XSS attacks. this is my code for textField an user who visits my website enters below value into my textField Usually we do this to handle the form information in the front end
in both cases, if we consider the data entered by the bad user and If this validation is not done, then while the user via the same website tries to view the stored i'm not able to reproduce any XSS attacks other than this. kindly help me out it appears to me that XSS attacks can be fixed only by handling the response data carefully in the server side code. i'm confused on why XSS attack vulnerability is reported on this |
Text field is not one of the vulnerable components. If you render user input as html on server side, it's obviously your fault and doesn't really have much to do with materialize. The thing is, that some components in materialize do client side rendering, autocomplete for example. Have you read the comment above your comment, it has 2 PoCs for the autocomplete vulnerability. Here's one of them, type Also, toast issue isn't really a vulnerability, so there's only 2 vulnerabilities. Both Snyk and npm have released public advisories of the vulnerabilities with autocomplete and tooltip. |
@FINDarkside Thank you for your response, i understand it now. But, there is still some confusion. when it comes to i'm still confused on, when this In my opinion, if |
No, you're not safe. It's not wrong for your api to return
Look at both of the PoCs. The first one is probably how it'd usually go, as there's really no point in escaping html entities on server side if you're just returning text. The second one is a case when you do escape html entities client side but you'll still be vulnerable to xss attack. You can think these PoCs as if |
@FINDarkside i understand your point. i was trying to simulate the xss attack say, i recieve data via ajax and i'm trying to render it in browser. Incase of both
|
Now you're on right tracks. innerText is indeed safe, you won't be able to execute any scripts using it, but autocomplete uses innerHTML equivalent. If you do
It should run immediately. If you want to run it trough autocomplete, it needs to be
|
To understand the problem with autocomplete, you can check here
I created an exemple to bugfix this point, I hope it will help you understand. Without XSS With XSS After if you want allow HTML, with my exemple isn't possible. I proposed to add news parameters like that
If you need more helps for others |
@FINDarkside i have a new perspective on certain things now because of you :). Please read my below comment @lucianot54 i have to fix these 3 cve's. in my project we download the
i tried below
|
Good to hear @Divine1. About your solution to fix it, I don't think it's enough, as the element is then passed to _highlight function which will rewrite the element innerHTML. In the end, line 6718 is the problem. Lines 6713 and below are also problem if you want autocomplete to work correctly (not a security problem though), as currently it's doing |
@FINDarkside i agree with you. Personally , i don't prefer css library for simple features like Since i have below 2 questions about i think But |
@FINDarkside @lucianot54 Among i tried to reproduce the vulnerability for i have added the code snippet to check the vulnerability for |
If you want mores XSS examples: If you want use 1, I recommend you this one:
And MaterializeCSS doesn't use Jquery but cash |
Only via iframe XSS attack is possible for tooltip and toast ? |
No, anything works there. It should be quite obvious though, since you're passing a field named
because it's not obvious that |
@lucianot54 @FINDarkside Initially i was planning to fix in this library code itself, but its not a proper approach. i'm available to contribute to this activity if additional people are required. |
Well, I wanted to know why Github tells me there are vulnerabilities on this package, I decided to see the "reproducers" and I found they were "not that harmful" with the usage I have, but was still hoping to find a fix, and I see this big discussion without any fix coming 😕 Is there anyone that plans to put an end to the warning, or at least some bits of a fix, at least for people that have a CI that blocks whenever there's a vulnerability? |
You can't as there is no patched version. So your package manager will warn until npm inc gets a patched version and marks this as resolved. |
I noticed there's no patch version. As @kiere said, some clients don't really like the fact that we use a library with vulnerabilities, and even if I may not care (for a personal project for example), and I don't like enterprises' protocols, well, a project that's blocked because some QA says that "a package is vulnerable" is an issue anyway. That's also a message of worry about the fact that the maintainers are "taking a break" but did not delegate the maintenance to anyone, therefore "abandoning the project" for a while. This has been the case a long time ago for some other projects and most of them delegated the maintenance to one trustworthy person at least "for a while", and eventually came back (this was the case for LeafletJS for instance) |
Could this issue be resolved, and could a patch version be created? Thank you! |
Note that this project has been abandoned. A fork has been created at https://github.com/materializecss/materialize which is being actively maintained. |
I've seen it, I am working on implementing that into my project, thanks! |
Github is alerting us that materialize-css 1.0.0 has vulnerabilities. Are you aware of this and do you know when a fix will be released?
The text was updated successfully, but these errors were encountered: