A more relevant "detect-object-injection"

[benderTheCrime]

Is there any way that we can work towards a more helpful/relevant report of Object injection sinks?

I can't think of a relevant security use case where Object injection would be relevant outside of the scope of a function directly linked to a web service.

I can understand based on tree traversal that determining the difference in between functions that are used in response to direct network calls would be [near] impossible to determine, but if I use bracket notation at the top level of my module, likely this rule should not notify.

[evilpacket]

There are many instances in which Object Injection could be relevant. The pattern is RARE and shows up in random locations @jlamendo can probably talk more about that. Jon wrote a good post on it here https://blog.liftsecurity.io/2015/01/14/the-dangers-of-square-bracket-notation/

We know Object injection is pretty false positive prone and it would be nice to move towards having better accuracy on there. More than happy to accept a PR for improved accuracy if you have some ideas. I imagine there are a few things we can do to reduce false positives some but not sure if it can every be as reliable as we would like.

It should be noted that these rules are not to be meant to block in a CI, but more of rules to allow a human to audit a codebase. While you could use some that way some others like this one probably shouldn't be used in that manner (I'm just adding additional context for others that might be reading, not trying to assume thew ay you are using it)

[benderTheCrime]

Hey, thanks for getting back to me!

I read the article and that you linked. Yes, I know this was the case, but for the most part, if there is user defined input, it's likely coming from a function argument. If a user has the ability to define an environment variable or futz with primitives/the node module cache, you've got bigger issues!

If I were to create a PR that exposed an option to limit the scope of this rule to function bodies, would that be considered?

[AdrienLemaire]

@evilpacket can we get a new link for documenting detect-object-injection ? https://blog.liftsecurity.io/2015/01/14/the-dangers-of-square-bracket-notation/ is dead

[morgangraphics]

I am not sure if this is the same article as was originally written, however ... I've removed the link to the plagiarized article.

[jlamendo]

^liftsecurity was acquired by npm, Inc. earlier this year. As part of the transition, our blog content was taken down. We intend to re-post it at some point in the future, but for now, it's no longer accessible at the old URLs.

The privacysniffs.com link posted above is plagiarized from my blog post word for word, and attributed to another author.

The Internet Archive has a copy of my original post - you can refer to it until we can get our old blog content back up:

https://web.archive.org/web/20150430062816/https://blog.liftsecurity.io/2015/01/15/the-dangers-of-square-bracket-notation

[marcospgp]

This rule should at least detect whether a variable is coming from outside or if it has been declared as a literal. Take for example this code:

I'm gonna turn off this rule in the meantime because this is too much 😄

[marcospgp]

Actually I managed to circumvent the issue here with this:

But the point still stands 😃

[devinrhode2]

This rule has a lot of obvious false positives. A for loop with an int that is being incremented - no injection risk. For in loops should be converted to Object.keys(obj).forEach(prop => {...}).
But then each line inside the object.keys foreach loop has to be ignored, when it's clearly safe, because the keys are coming from Object.keys (which will ignore prototype/etc)

[devinrhode2]

In addition to converting loops to use Object.keys we can also use Object.values and Object.entries. Object.entries is a little ugly unless you use destructuring

Object.entries(obj).forEach(([key, val]) => {
  return key + val;
});

[devinrhode2]

Working on a bullet proof way to make all use of square brackets safe: https://stackoverflow.com/questions/57960770/securely-set-unknown-property-mitigate-square-bracket-object-injection-attacks/58204173#58204173

Please help!

[asyschikov]

So far I am seeing this rule it is exclusively false positives. Look at this code:

  const arr = [1, 2, 3];
  const i = 1;
  const x = arr[i];

Like all index access is getting flagged.

[devinrhode2]

yeah it's a pretty noisy rule. Probably there are better ways to spend time improving security: use CSP.. review security vulnerabilities.. try to hack your own application.. turn on "Enhanced" browsing protection in various browsers and see if everything still works correctly, etc.

[jlamendo]

I'm the original author of this rule - for a bit of context, it was originally written as an assistive tool for manual code reviews, to be used with the eslint plugin for VS Code. I would recommend disabling it for other use cases, as it's just going to be far too noisy.

[jlamendo]

Edit: Closed accidentally :)

[telion2]

If I could make a recommendation:
If this rule has many false positives, could the warning message say so directly?
e.g. instead of:
Generic Object Injection Sink eslint[security/detect-object-injection](https://github.com/eslint-community/eslint-plugin-security/blob/main/docs/rules/detect-object-injection.md)

=>
Generic Object Injection Sink. Check if the source of the new key can be set by the user, if not you can disable the rule by, adding this line above the potential Injection Sink: //eslint-disable-next-line security/detect-object-injection eslint[security/detect-object-injection](https://github.com/eslint-community/eslint-plugin-security/blob/main/docs/rules/detect-object-injection.md)

It does not have to be this exact message, but it would save a lot of time, to check if this is a real issue in your code, and to come up with solutions, just to find out the rule is over-enthusiastic.

[bestickley]

Would this plugin consider creating a "strict" configuration (in addition to current "recommended" configuration) that includes this rule and remove this rule from "recommended"?

[nzakas]

@bestickley it would help if you could explain your reasoning.

[bestickley]

@nzakas, sure. If most people's experience (not sure if this is true) with "detect-object-injection" is to have to disable the rule the majority of the time or altogether turn off the rule, it may make more sense to edit the recommended configuration, "plugin:security/recommended" to exclude this rule and create "plugin:security/strict" which includes this rule. Alternatively, an option is to keep "plugin:security/recommended" the same and offer a "plugin:security/loose" configuration which excludes this rule. Maybe it's not worth the effort but wanted to share the idea.

[nzakas]

Gotcha, thanks. I need to spend some time digging into that rule and figure out what to do. Some of these earlier rules are a bit questionable, for sure.