Knockout: XSS injection point in attr name binding for browser IE7 and older

[jmaxxz]

The following: https://github.com/knockout/knockout/blob/511d0828d4e0c13644adcd0251dcdb957681c1dd/src/utils.js

setElementName: function(element, name) {
            element.name = name;

            // Workaround IE 6/7 issue
            // - https://github.com/SteveSanderson/knockout/issues/197
            // - http://www.matts411.com/post/setting_the_name_attribute_in_ie_dom/
            if (ieVersion <= 7) {
                try {
                    element.mergeAttributes(document.createElement("<input name='" + element.name + "'/>"), false);
                }
                catch(e) {} // For IE9 with doc mode "IE9 Standards" and browser mode "IE9 Compatibility View"
            }
        }

Is called from here: https://github.com/knockout/knockout/blob/77de67ea2cfc1ace79b974765d0c7b9400443958/src/binding/defaultBindings/attr.js

            // Treat "name" specially - although you can think of it as an attribute, it also needs
            // special handling on older versions of IE (https://github.com/SteveSanderson/knockout/pull/333)
            // Deliberately being case-sensitive here because XHTML would regard "Name" as a different thing
            // entirely, and there's no strong reason to allow for such casing in HTML.
            if (attrName === "name") {
                ko.utils.setElementName(element, toRemove ? "" : attrValue.toString());
            }
        });

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.

[mbest]

Good catch. Thanks for reporting this.

[Pangamma]

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.

[brianmhunt]

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 ko.utils.setElementName a no-op.

[Pangamma]

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

[mbest]

The fix should just be to escape the name, I think. Anything else?

[mbest]

See #2345

[Pangamma]

Nice! Impressive turn-around time, too. The security team commented on the speed as well.

[jmaxxz]

@Pangamma you're impressed by a turn around time of 4.1 years!?

[mbest]

@jmaxxz I agree this should have been fixed a long time ago. I'm not sure how we missed it.

[Pangamma]

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

[farehar]

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?

[Pangamma]

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

[farehar]

Thanks @Pangamma. Which minifier does knockout use for their releases @mbest ?

[mbest]

Which minifier does knockout use for their releases @mbest ?

It's https://developers.google.com/closure/compiler/

[pedrohc]

CVE-2019-14862 was assigned to this issue.