Skip to content
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 type checking vul in ctorName #31

Merged
merged 1 commit into from Jan 16, 2020
Merged

Conversation

xiaofen9
Copy link
Contributor

@xiaofen9 xiaofen9 commented Dec 30, 2019

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
Copy link

@stephengcox stephengcox commented Dec 30, 2019

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
Copy link
Contributor Author

@xiaofen9 xiaofen9 commented Dec 30, 2019

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.

index.js Show resolved Hide resolved
@jonschlinkert
Copy link
Owner

@jonschlinkert jonschlinkert commented Dec 30, 2019

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
Copy link

@stephengcox stephengcox commented Dec 31, 2019

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
Copy link
Owner

@jonschlinkert jonschlinkert commented Dec 31, 2019

@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
Copy link

@stephengcox stephengcox commented Jan 2, 2020

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
Copy link

@stephengcox stephengcox commented Jan 3, 2020

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
Copy link

@shusak shusak commented Jan 13, 2020

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

@stephengcox
Copy link

@stephengcox stephengcox commented Jan 15, 2020

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

@jonschlinkert
Copy link
Owner

@jonschlinkert jonschlinkert commented Jan 16, 2020

@shusak
Copy link

@shusak shusak commented Jan 16, 2020

Thanks @jonschlinkert !

@doowb doowb merged commit 1df992c into jonschlinkert:master Jan 16, 2020
1 check passed
@doowb
Copy link
Sponsor Collaborator

@doowb doowb commented Jan 16, 2020

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

@doowb doowb mentioned this pull request Jan 16, 2020
axelchalon added a commit to paritytech/parity-signer that referenced this issue Feb 8, 2020
This repo's dependencies currently use [older versions](https://github.com/paritytech/parity-signer/blob/97651dcc2f78027aa83c3dc7834f6941e084c8a7/yarn.lock#L5138) of kind-of which have a possibly exploitable vulnerability ([original issue](jonschlinkert/kind-of#30), [PR](jonschlinkert/kind-of#31)).
axelchalon added a commit to paritytech/operationsproxy that referenced this issue Feb 8, 2020
This repo's dependencies currently use [older versions](https://github.com/paritytech/operationsproxy/blob/1e42f36ab60f6368f673211d53e9b887227dc11d/yarn.lock#L3521) of kind-of which have a possibly exploitable vulnerability ([original issue](jonschlinkert/kind-of#30), [PR](jonschlinkert/kind-of#31)).
axelchalon added a commit to paritytech/substrate-ui that referenced this issue Feb 8, 2020
This repo's dependencies currently use [older versions](https://github.com/paritytech/substrate-ui/blob/b185aad6174ee6bf2f2a68eadd84801ec2efe39d/yarn.lock#L3362) of kind-of which have a possibly exploitable vulnerability ([original issue](jonschlinkert/kind-of#30), [PR](jonschlinkert/kind-of#31)).
axelchalon added a commit to openethereum/fether that referenced this issue Feb 8, 2020
This repo's dependencies currently use [older versions](https://github.com/paritytech/fether/blob/3c0363079e1148ae0dc0595523905f9bee7b9a84/yarn.lock#L10218) of kind-of which have a possibly exploitable vulnerability ([original issue](jonschlinkert/kind-of#30), [PR](jonschlinkert/kind-of#31)).
tomusdrw pushed a commit to paritytech/operationsproxy that referenced this issue Feb 8, 2020
* Use kind-of >=6.0.3 everywhere

This repo's dependencies currently use [older versions](https://github.com/paritytech/operationsproxy/blob/1e42f36ab60f6368f673211d53e9b887227dc11d/yarn.lock#L3521) of kind-of which have a possibly exploitable vulnerability ([original issue](jonschlinkert/kind-of#30), [PR](jonschlinkert/kind-of#31)).

* Update yarn.lock
amaurym pushed a commit to openethereum/fether that referenced this issue Feb 10, 2020
* Use kind-of >=6.0.3 everywhere

This repo's dependencies currently use [older versions](https://github.com/paritytech/fether/blob/3c0363079e1148ae0dc0595523905f9bee7b9a84/yarn.lock#L10218) of kind-of which have a possibly exploitable vulnerability ([original issue](jonschlinkert/kind-of#30), [PR](jonschlinkert/kind-of#31)).

* Update yarn.lock
hanwencheng pushed a commit to paritytech/parity-signer that referenced this issue Feb 10, 2020
This repo's dependencies currently use [older versions](https://github.com/paritytech/parity-signer/blob/97651dcc2f78027aa83c3dc7834f6941e084c8a7/yarn.lock#L5138) of kind-of which have a possibly exploitable vulnerability ([original issue](jonschlinkert/kind-of#30), [PR](jonschlinkert/kind-of#31)).
favna added a commit to RWS-NL/air-node-packages that referenced this issue Apr 1, 2020
@resynth1943
Copy link

@resynth1943 resynth1943 commented Apr 4, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants