Node: util.inspect can be used to leak references to values from external scopes

[bathos]

_@BridgeAR suggested this should be described in a distinct issue from #10731, where I’d mentioned it as part of the case for not expanding (and ideally limiting or removing) the exceptions already made for observing Proxy status/internals in util.inspect._

Currently, util.inspect provides avenues for violating ECMAScript invariants of Proxy objects, leaking (a) their status as Proxies and (b) actual references to the target and handler objects. The latter is more accurately violating an invariant of scopes, not Proxies.

This is possible because of the use of V8 APIs to obtain references to out-of-scope ES values, followed by passing those values into functions which may be externally provisioned or replaced (including its own context object and its children, but also global intrinsics like Array.isArray).

Here is an example. There are several ways to do this, and the results will vary depending on the avenues taken. (In this particular case, early exits for empty objects would not be captured.)

const { inspect } = require('util');

function obtainIllegalAccess(proxy) {
  const seen = new class extends Array { pop() {} };
  inspect(proxy, { seen, showProxy: true });
  return seen;
}

const t = { foo: 1 };
const h = { bar: 2 };
const p = new Proxy(t, h);

obtainIllegalAccess(p); // [ { foo: 1 }, { bar: 2 } ]

My opinion is that this can be seen as symptomatic, where the underlying problem is the attempt to pivot on Proxy status at the ES layer at all; by design, this isn’t supposed to be possible. However even if the showProxy functionality is retained, it’d probably be possible to fix this leak by tightening how ctx works and closing over references to intrinsics at module evaluation time.

[devsnek]

accepting a seen array option is a bit strange and we can probably remove that.

at that point, the only leak would be modifying Array.prototype globally, which we can get around by adding a SafeArray to internal/safe_globals (cc @joyeecheung)

@nodejs/util

[bathos]

@devsnek the seen array is one avenue here, provided as an example. this is one of the more reliable ones, but there are quite a few others, some of which would only work depending on the specific contents of the proxy target/handler objects.

[devsnek]

@bathos at a certain point, util is something that just exposes a lot of information. it violates the spec invariants of proxies, promises, weak sets, weak maps, all iterators, etc by exposing their internal properties/state to js code

[bathos]

Yep. I think this is a bit different though from revealing status or even say promise resolution value. If the caller has access to a promise, they also already (can) have access to its resolution value. It’s a violation, but a ... softer one. In this case, it doesn’t follow that if the caller has access to a proxy it also has or may obtain access to its handler and target objects — in fact it implies that it specifically was meant not to.

Re: Weak collections, that also seems pretty bad to me, if the same loopholes apply (calling out to unknown user code with the keys/values). However in practice one would rarely be exposing weakmaps meant to hold private state to anything external to begin with, whereas with Proxies, that’s a primary use case.

[bnoordhuis]

The change was made in response to #16483. Summary: misbehaving proxies produce unusable output; and since util.inspect() is for humans, not machines, it's okay to bend the rules a little. Object inspectors in browsers also treat proxies specially.

We can fix bugs but the current behavior is useful, more useful than the alternative.

[bathos]

@bnoordhuis would you agree that it’s a bug that the references to the target/handler of an arbitrary proxy object can be obtained? That seems solvable without removing the proxy-internals-viewing behavior, but would entail breaking API changes related to the inspect context object.

[bnoordhuis]

You mean the seen array? As Gus mentioned, we could remove that (undocumented implementation detail, no real reason it should be exposed) but being the conservative creatures we are, it'd be a semver-major change; it could go into v12 but not <= v10.

[bnoordhuis]

@bathos You want to follow up? I'll close out this issue in a few days otherwise.

[bathos]

@bnoordhuis Follow up as in open a PR for this or as in clarify?

Clarifying would be: yes, making the seen array inaccessible or otherwise immutable from outside, but also stashing references to intrinsics like isArray at module evaluation time instead of relying on dynamic property lookup. I believe this strategy is used in some other places already in node internals(?).

If you’d like, I could have a go at it myself. I’ve never contributed to node source before so it’d be new territory for me.

[bnoordhuis]

I was asking for clarification but a PR would be even better. The relevant code is in lib/internal/util/inspect.js; tests are in test/parallel/test-util-inspect*.js.

I believe this strategy is used in some other places already in node internals

Correct, but to manage expectations: it's as a resilience thing for monkey-patches gone wrong, it's not an arms race to stop malicious tampering.