fix type checking vul in ctorName

[xiaofen9]

Thanks for the response. Here is the PR that fixes the issue.

Discussion about how this might be used maliciously: Kind-of is widely used for type checking. If developers use kind-of to check whether user-input object is in string format, the attacker can cheat kind-of into recognizing an arbitrary plain object into str type. As a result, all the built-in attributes (e.g., length) of str type can be manipulated. For example, if the manipulated user_obj.length is inserted into the DB, SQL Injection can be introduced.

[stephengcox]

I think this check could still be defeated by doing:

input = { “foo”: “bar”, “constructor”: function Symbol() { return “Symbol” } }; kindOf(input)

The problem is the reliance on the constructor.name variable. It's not trusted.

[xiaofen9]

Thanks for the comments. In most cases, user-input is passed into nodejs module in the form of plain object (e.g., via json). As a result, function cannot be passed into module by the attacker. This fix aims at achieving both high defense coverage and efficiency.

[jonschlinkert]

The problem is the reliance on the constructor.name variable. It's not trusted.

@stephengcox, I'm open to recommendations and pull requests. I also appreciate the thinking and look forward to hearing your solution.

However, I'm having a hard time seeing how this qualifies as a "vulnerability", and I think you're taking some creative license with the word "trusted". This seems hyperbolic. No user-defined value should ever be trusted. Potential "bug waiting to happen" seems more appropriate. Could someone cause an error? Sure, but that's a far stretch from allowing malicious code to be executed, and there are much easier and more malicious ways to cause errors.

Perhaps you can clear things up for me. What is the specific scenario in which an end-user would be able to:

1. Pass an object with a custom constructor property directly to a function in your code that does a type check with kind-of, before any other validation?
2. You aren't sure whether or not the user will pass an object to your server, thus necessitating the need of a type-checking library?
3. Bypass more robust validation. It seems obvious that you would have other validation and stricter to prevent the throw an error if the user tried to override the constructor, prototype, or __proto__ properties.
4. Etc. etc....

I am fully prepared to learn. I suppose I could just merge the PR and say "thanks", but given that this library is downloaded 200 million times every month, I tend to overanalyze every minor change, and I want to understand the types of scenarios you're describing.

[stephengcox]

Hey Jon, thanks for the note.

Perhaps "trusted" isn't the right word. What I meant was is basically what you are saying: it's a user-definable value and not a "trusted" part of the language that prevents user control. The mozilla docs on javascript mention that very point, it's not always safe to rely on it. Probably because OOP was an afterthought for Javascript much like it was in PHP and Python, but that's another discussion. 😊

I'm not an expert in javascript internals so I don't have a better suggestion. Perhaps you could develop signatures of the object types you want to match rather than relying on constructor.name but that may be brittle (although perhaps manageable with regular regression testing). Also somewhat more of an invasive change.

I agree with the assessment that this could be used to bypass type checks or in information hiding style attacks where the attacker is trying to hide a payload in order to execute something like SQL injection.

Also agree that @xiaofen9's fix addresses the 90% case (and it looks like your competing libraries have this issue). But given the "low level" nature of this library and it's wide usage, can't rule out cases where it could be abused in the way I described.

[jonschlinkert]

@stephengcox thanks for the additional detail and for discussing with me.

Ideally (for performance) we would just directly check the constructor (not constructor.name, the actual function), to see if it's the same function. However, this will fail in some situations, like when the constructors are from different realms (so things like foo instanceof Array might even fail if foo was created on a different realm than the Array constructor).

Perhaps we would be better off just moving up the toString.call() to this line, and get rid of the constructor check. There would be a tiny performance cost, but it might be worth it in this case.

Thoughts?

[stephengcox]

Could you override toString() in a similar way as my example, so that a malicious input object can provide whatever it wants?

@xiaofen9's commit is probably fine, honestly. It covers the 90% case without a large rethink of the object matching logic.

[stephengcox]

A lot of this is limitations of the language and not of this library, if you think about it. Should make a change that is safe and covers the vast majority of use cases.

[shusak]

@jonschlinkert @stephengcox - do you have an idea when or even if, you will be able to resolve this PR for the CVE finding?

[stephengcox]

I suggest that @jonschlinkert merge this PR and consider additional improvements on a future major release.

[jonschlinkert]

Agreed, I’ll merge ASAP. Or @doowb can merge if he gets to it before me.

Sent from my iPhone

On Jan 15, 2020, at 8:57 AM, Stephen Cox notifications@github.com wrote:

I suggest that @jonschlinkert merge this PR and consider additional improvements on a future major release.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[shusak]

Thanks @jonschlinkert !

[doowb]

Merged... I'll get this published shortly.

[resynth1943]

Hmm. Interesting. GitHub just put out a warning for this package. This seems more like a bug than a vulnerability though from my point of view...