vanillaes/absurdum

Security Vulnerabilities?

evanplaice opened this issue · 5 comments

evanplaice commented

Just found this article about a security vilnerability in jQuery. It relies on exploiting object merge operations to manipulate the Object prototype.

https://snyk.io/vuln/SNYK-JS-JQUERY-174006

Since this lib provides similar functionality, we should probably audit the code for any potential vulnerabilities.

Notes

References

jkvyff commented

Yep, we do have that structure. Though, it would appear to be less damaging as both objects are being merged onto a new object so it couldn't effect the input objects upstream. I will test what is possible this Friday.

jkvyff commented

After a review, it would seem we are secure. Because when we set properties using previous objects, we have used Object.keys to grab any property names. and we never grab the redefined proto property to set on the new object.

We would have to be careful of this depending on the implementation of functions using inherited properties to create new objects, but unless I missed something, the current implementation should not have this issue.

We effectively use the prevention of using an object with the default prototype and not copying or characteristics of the source objects prototypes.

jkvyff commented

Also just made this to visualize part of the problem

const obj1 = { left: 22 };
const func1 = () => 'Howdy';
Object.defineProperty(obj1, 'toString', {
  value: func1,
  enumerable: true
});
const obj2 = { __proto__: obj1, up: 57 }; /// Don't allow __proto__ to be merged to new object

 // ***Possible Issue To avoid***
console.log(Object.prototype.toString())
// [object Object]
console.log(obj2.toString())
// 'Howdy'

// ***Property key we use*** 
console.log(Object.keys(obj1))
// [ 'left', 'toString' ]


console.log([...Object.keys(obj2), ...Object.getOwnPropertyNames(Object.getPrototypeOf(obj2))])
// ***All Defined***
// [ 'up', 'left', 'toString' ]

console.log([...Object.keys(obj2), ...Object.keys(obj1), ...Object.getOwnPropertyNames(Object.getPrototypeOf(obj1))])
// ***All runnable*** 
/* [ 'up', 'left', 'toString',
  'constructor',
  '__defineGetter__',
  '__defineSetter__',
  'hasOwnProperty',
  '__lookupGetter__',
  '__lookupSetter__',
  'isPrototypeOf',
  'propertyIsEnumerable',
  'toString',
  'valueOf',
  '__proto__',
  'toLocaleString' ]
*/
evanplaice commented

I was thinking of something more like this. Although, it doesn't mutate the proto.

If objects.merge worked recursively to merge both own properties and proto properties, that would be a concern. _.set was another operator mentioned in the report but we decided to skip that one pre-1.0 b/c using reduce to set one value isn't very efficient.

On second thought, none of our operators mutate the input. Isn't that kind of the point?

Assuming there is some leak that we haven't considered, we could always add an input check to verify that none of the inputs are the Object prototype

if (input.isPrototypeOf(Object)) { throw Error('The Object prototype is not a valid input'); }
evanplaice commented

Well, this lib doesn't suffer from any security vulnerabilities but some deeply nested dependencies did. The reason the bot patch didn't fix it was due to one specific deeply nested dependency of ESLint. I almost had to patch ESLint to fix it but fortunately the dep with the issue published a fix.

Gonna close this for now unless you can think of an other potential security vulnerability vectors.

  • 👍1