endojs / endo

Endo is a distributed secure JavaScript sandbox, based on SES
Apache License 2.0
829 stars 72 forks source link

scope terminator proxies are configured with an object literal instead of a null-prototype object #1851

Closed naugtur closed 8 months ago

naugtur commented 1 year ago

While it's not risky after lockdown, it's theoretically open to prototype pollution.

mhofman commented 1 year ago

Could you clarify which objects are literal and would be susceptible to prototype pollution? I see the scopeProxyHandlerProperties which is only used for its own properties.

naugtur commented 12 months ago

Yes, I mean the objects specifying handlers for proxies.

Object.prototype.get=console.log.bind(console,'doh')
a=new Proxy({},{})
a.z 

prints doh {} z {}

mhofman commented 12 months ago

As I mentioned this is not an issue in the implementation of scope terminators.

export const strictScopeTerminatorHandler = freeze(
  create(
    alwaysThrowHandler,
    getOwnPropertyDescriptors(scopeProxyHandlerProperties),
  ),
);

Creates a handler object from the own props of scopeProxyHandlerProperties, which means using an object literal for that object is fine.

Furthermore, its prototype alwaysThrowHandler is defined as follow:

export const alwaysThrowHandler = new Proxy(
  immutableObject,
  freeze({
    get(_shadow, prop) {
      Fail`Please report unexpected scope handler trap: ${q(String(prop))}`;
    },
  }),
);

Which means if the proxy logic ever tried to get a trap not defined on the handler, it would throw.

kriskowal commented 10 months ago

@naugtur Are you convinced? Should this be closed?

erights commented 8 months ago

@naugtur Are you convinced? Should this be closed?

naugtur commented 8 months ago

Apologies for the delay. Working on my notifications config.

If only own properties are used for defining traps, I don't see a way to exploit it.

Ok to close.