Paranoia with Prototype Pollution

[shadowspawn]

Prototype pollution is a concern for situations where end-user input ends up on an object as a property name. (I am not an expert in this, so my questions may include misconceptions!)

Does it affect parseArgs? (I am guessing to some extent since we are taking end-user arguments as option names and storing values.)

Is it enough for our context to protect against using a property key named __proto__? (i.e. from arguments including --__proto__.)

Does using null objects help, or separate issue for our context?

• https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/create

__proto__ related links:

• Disable __proto__ nodejs/node#31951
• https://github.com/yargs/yargs/pull/1591/files
• https://github.com/substack/minimist/commit/38a4d1caead72ef99e824bb420a2528eec03d9ab
• https://github.com/substack/minimist/commit/63e7ed05aa4b1889ec2f3b196426db4500cbda94

[ljharb]

Yes, there’s been tons of CVEs about this on yargs and minimist and others. Not handling this will make node itself a CVE target.

[bcoe]

I think we should create all our objects with null prototypes, Object.create(null).

[ljharb]

That’s good, but isn’t sufficient to protect against prototype pollution because of the dunder proto setter on Object.prototype.

[bcoe]

In yargs-parser, we also have an additional sanitization step:

/ TODO(bcoe): in the next major version of yargs, switch to
// Object.create(null) for dot notation:
function sanitizeKey (key: string): string {
  if (key === '__proto__') return '___proto___'
  return key
}

The idea with the tripple underscore was thatm on the off chance that someone legit wants __proto__ to be passed into their application, perhaps they can atleast detect that someone passed in something resembling it.

A less silly approach now that I think about it, is perhaps we can instead do something like HTML encoding:

%5F%5Fproto%5F%5F

On the off chance a key that's being set on flags or values is __proto__.

[ljharb]

Yargs’ approach sounds fine; as does just outright rejecting an arg named that, because why support that use case.

[bcoe]

@ljharb @shadowspawn I'm wondering, part of why we went with the approach we went with in yargs-parser was to minimize refactoring, since it was a fairly large library with a lot of edge-cases, what if we just switched parseargs to using a Map:

> const values = new Map()
undefined
> values.set('__proto__', 'hello')
Map(1) { '__proto__' => 'hello' }
> values.set('apple', 99)
Map(2) { '__proto__' => 'hello', 'apple' => 99 }
> Object.fromEntries(values)
{ ['__proto__']: 'hello', apple: 99 }

And then return Object.fromEntries in the public interface?

[ljharb]

Seems like a good approach as well.

[shadowspawn]

With the idea proposed that we return something different than what we work with internally, map is a contender, and I like that Map is a container consistent with the intent to store any string key safely.

I am not clear on the risks to the client code though. After we write back the properties onto an object in the result, are we handing the client code a grenade if end-user has specified problematic option names? (like say toString)

[ljharb]

In JavaScript you always have to know to check for/iterate own properties anyways; toString isn’t an issue on the read side as long as the data structure is written properly.

[shadowspawn]

My current understanding is that we do not currently have ability to create prototype pollution (3) because we only ever overwrite properties directly by key. We do not do a deep copy, we do not support nested properties (like foo.bar, leading to __proto__.bar).

We do get some protection against future code changes introducing a CVE by taking the special keys out of play by internally using (say) map, and by protecting the very special __proto__.

[shadowspawn]

Closing #33 with the understanding that "one of the goals in the approach we land on to prevent prototype pollution should be that the public interface still feels natural".

#33 (comment)

[shadowspawn]

I am still interested in adding a unit test, but not going to hold this issue open.