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
XSS injection point in attr name binding for browser IE7 and older #1244
Comments
Good catch. Thanks for reporting this. |
We're required to either remove knockout, or update to a version where this is fixed on a few Microsoft sites right now. Can anyone who has time take a look at this vulnerability? Thanks. |
Thanks @Pangamma . I can't reproduce in any modern browsers I have access to. Is this an IE 6/7 thing? What's the crux here? I presume it can be sanitized. If you don't modify the element name via ko, you could make |
@brianmhunt Hang on I'll look into this more in a bit. I'm sure it could be sanitized. nowhere in our projects are we allowing end-users to provide any input for something that changes the name attribute of an element. Still, according to automated security warnings this has been declared to be an issue. |
The fix should just be to escape the name, I think. Anything else? |
See #2345 |
Nice! Impressive turn-around time, too. The security team commented on the speed as well. |
@Pangamma you're impressed by a turn around time of 4.1 years!? |
@jmaxxz I agree this should have been fixed a long time ago. I'm not sure how we missed it. |
@jmaxxz Maybe not the 4.1 years. Haha. The response time after bumping the issue and saying a fix was needed was good though. I've been able to file a security exception after patching the exploit in the project's local copy of knockout. Eventually, there will be a nice nuget package with the fix, and then we will be able to update to that version. |
I would also like to patch our project's local copy of knockout. We are using the minified version though. Can't find setElementName function. How do we patch the minified version? |
@farehar When I was fixing the minified version, I searched for something like "if(7>=", and that was around the section I needed to make changes to. Variable names and function names will be different for sure, but you can figure out what each variable means based on the context. If that method is difficult, another option is to just grab the unminified version, patch it, and then run it through a minifier. Attached: Partial image of the fix. (Some text got cut off in the screenshot) |
|
CVE-2019-14862 was assigned to this issue. |
The following: https://github.com/knockout/knockout/blob/511d0828d4e0c13644adcd0251dcdb957681c1dd/src/utils.js
Is called from here: https://github.com/knockout/knockout/blob/77de67ea2cfc1ace79b974765d0c7b9400443958/src/binding/defaultBindings/attr.js
Name is passed directly into the DOM through string concatenation. If a user can influence any part of the value bound to name they can inject in arbitrary javascript.
http://jsfiddle.net/u4bdD/ shows how mergeAttributes can be used to transfer more than just a name attribute.
The text was updated successfully, but these errors were encountered: