DEV Community

Serguey Asael Shinder
Serguey Asael Shinder

Posted on

Your Merge Function Gave Every Object Admin Rights

The settings endpoint was simple.

Take the JSON from the request.

Deep merge it
into the user's saved preferences.

Save.

A few hours later
every account served by that process
could see the admin menu.

Nobody had been promoted.

The check was
if user.isAdmin.

For normal users
that property did not exist.

Until it did,
on every object at once.

The request body
had one odd key in it.

Underscore underscore proto.

JSON.parse
treats that as an ordinary name
and makes it a normal property
on the parsed object.

Harmless,
until a merge walks through it.

A hand written deep merge
loops over the keys,
and for each object value
it reaches into the target
and merges again.

So it asked the target
for its proto property.

And in JavaScript
that is not a field.

It is the door
to Object.prototype,
the object every plain object inherits from.

The merge stepped through
and wrote isAdmin, true,
onto the ancestor of everything.

From then on
any object that lacked its own isAdmin
inherited that one.

This is prototype pollution.

It has been found in widely used libraries,
including old versions of lodash's merge
and jQuery's extend.

Hand written versions
are usually worse,
because nobody is reporting bugs against them.

The fixes are small.

In any merge, clone or set by path,
skip the keys
proto, constructor and prototype.

Better,
validate the body against a schema
that only allows the fields you expect,
so the strange key never reaches the merge.

Use a Map,
or an object made with Object.create of null,
for anything keyed by user input.

Check permissions with Object.hasOwn,
so an inherited value
never counts as a grant.

In Node
you can also remove the proto accessor
with a startup flag.

Then try it.

Send the odd key
to every endpoint that merges.

Check a fresh empty object afterwards.

It should still be empty.

A missing property
is a decision too.

Make sure nobody else
can make it for you.

– Serguey Asael Shinder

Top comments (0)