Paranoia with Prototype Pollution

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:

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.

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

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

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

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

@ljharb

@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?

Seems like a good approach as well.

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)

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.

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

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)

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

Navigation Menu

Repository navigation

Paranoia with Prototype Pollution #32

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Paranoia with Prototype Pollution #32

Description

Activity

ljharb commented on Jan 9, 2022

bcoe commented on Jan 16, 2022

ljharb commented on Jan 16, 2022

bcoe commented on Jan 16, 2022

ljharb commented on Jan 16, 2022

bcoe commented on Jan 16, 2022

ljharb commented on Jan 16, 2022

shadowspawn commented on Jan 17, 2022

ljharb commented on Jan 17, 2022

shadowspawn commented on Jan 17, 2022

shadowspawn commented on Jan 17, 2022

shadowspawn commented on Mar 30, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions