Why is it bad pratice calling an array index with a variable?

Question

I'm currently developing a little game in Javascript and I'm using Codacy to review my code and help me cleaning it.

One of the most seen error is Generic Object Injection Sink (security/detect-object-injection).

It happens when I'm trying to access a value in an array using a variable. Like in this example :

function getValString(value)
{
    var values = ["Mis&eacuterable", "Acceptable", "Excellente", "Divine"];
    return values[value];
}

This function is used to display on screen the value's string of an item. It receives a "value" which can be 0, 1, 2 or 3 and returns the string of the value.

Now here's my problem :

Codacy is telling me that use of var[var] should be prohibited because it causes security issues and since I'm rather new to Javascript, I was wondering why and what are the good practices in that kind of situation.

Grosbeak

Code looks fine. However a switch or a lookup table would be more appropriate here... – Breastplate

No, just a map of value:koefficient. And a one liner... – Precessional

Both not answering OPs question - why is array index lookup being reported as bad security practice and is this valid? – Pansypant

Seems to me because the lookup relies on an external function parameter, which theoretically can be manipulated by clients which itself COULD cause any bad stuff like buffer overlfow or whatever to be exploited. Isn't there a documentation part of codacity, explaining this issue? – Slier

Yes you are right, the documentation states that it can causes a lot of exploits. I guess i'll have to correct it everywhere :s – Grosbeak

I think that Codacy should improve the checks, because this operation is a bad security practice only when the variable used as index is received from external (eg. user input or argument) and not when it is a local variable (eg. for/while loop counter). – Polemist

github.com/nodesecurity/eslint-plugin-security/issues/21, github.com/nodesecurity/eslint-plugin-security/blob/master/docs/… – Rhythm

The security warning comes when the index could be a string, and it is not a concern when the index is a number (it still could be an error, but not the same kind of security injection error). If you use +var, making it a number or NaN, I believe the security warning goes away. It would nice if were a little smarter; even when you do something like 'for (let i = 0; i < 3; i++)', you still need 'values[+i]' within the for. – Aguie

Answer 1

The security issue present here is that the stringified value of value may be accessing a property that is inherited from the Object's __proto__ hierarchical prototype, and not an actual property of the object itself.

For example, consider the scenario when value is a string literal of "constructor".

const property = "constructor";
const object = [];
const value = object[property];

The result of value in this context will resolve to the Array() function - which is inherited as part of the Object's prototype, not an actual property of the object variable. Furthermore, the object being accessed may have overridden any of the default inherited Object.prototype properties, potentially for malicious purposes.

---

This behavior can be partially prevented by doing a object.hasOwnProperty(property) conditional check to ensure the object actually has this property. For example:

const property = "constructor";
const object = [];
if (object.hasOwnProperty(property)) {
    const value = object[property];
}

Note that if we suspect the object being accessed might be malicious or overridden the hasOwnProperty method, it may be necessary to use the Object hasOwnProperty inherited from the prototype directly: Object.prototype.hasOwnProperty.call(object, property)
Of course, this assumes that our Object.prototype has not already been tampered with.

This is not necessarily the full picture, but it does demonstrate a point.

---

Check out the following resources which elaborates in more detail why this is an issue and some alternative solutions:

• https://github.com/nodesecurity/eslint-plugin-security/blob/master/docs/the-dangers-of-square-bracket-notation.md
• Securely set unknown property (mitigate square bracket object injection attacks) utility function

Titter

Answer 2

By itself this is not a bad practice, because you do want to develop a system and make it secure. It's difficult to imagine a higher security risk to a system than one which causes the nonexistence of that system.

Yet, not being allowed to use a variable to dynamically create/use/update an index practically reduces your options of hard-coding any indexes that you may use to refer items of an array or members of an object.

Not allowing indexes greatly reduces your options, so much that it threatens with the nonexistence of any system that you may want to create in Javascript. Let's see some of the use-cases:

Numbered loops:

for (let index = 0; index < arr.length; index++) {
    //do whatever with arr[index]
}

Of course, this is true to while loops as well.

in loops

for (let index in variable) {
    //do whatever with arr[index]
}

of loops

for (let item of variable) {
    // do whatever with item
}

see

finding dynamically a value

This is virtually used in quasi infinitely many ways, all the examples above are specific cases of this. Example:

function getItem(arr, index) {
    return arr[index];
}

summary

The fear of exploits because of dynamic indexing is the equivalent of the fear from a meteor hitting into the exact place and the exact time one is in. Of course, we cannot exclude it, but one can not live in constant fear of low-chanced catastrophes. Similarly, programming is also impossible with unreasonable, paranoid fears. So, instead of rejecting dynamic indexing altogether because of the possibility of the exploits, we must refer to the actual exploits that could be possible. If we are not allowed to use dynamic instances, then whatever system we are to develop, if it's not simple as pie, will not come to existence. So, whatever threats we are afraid of should be protected against otherwise.

Example: You retrieve values from a data-source and have a field for credit card IBAN. Yeah, if that's shown to a user who is not the owner, that's a high risk. But you should protect against this by making IBAN unavailable by the mere of a use of an index by external sources, such as POST requests sent by the browser of a user.

Condolence

Totally agree, I'm surprise how some people turn this rule on. It seems nonsense. It seems like the article to which everyone is refer is mislead. – Unrig