Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util.inspect can be used to leak references to values from external scopes #24765

Closed
bathos opened this issue Dec 1, 2018 · 10 comments
Closed
Labels
util Issues and PRs related to the built-in util module.

Comments

@bathos
Copy link

bathos commented Dec 1, 2018

@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 devsnek added the util Issues and PRs related to the built-in util module. label Dec 1, 2018
@devsnek
Copy link
Member

devsnek commented Dec 1, 2018

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
Copy link
Author

bathos commented Dec 1, 2018

@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
Copy link
Member

devsnek commented Dec 1, 2018

@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
Copy link
Author

bathos commented Dec 1, 2018

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
Copy link
Member

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
Copy link
Author

bathos commented Dec 2, 2018

@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
Copy link
Member

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
Copy link
Member

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

@bathos
Copy link
Author

bathos commented Dec 7, 2018

@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
Copy link
Member

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.

BridgeAR added a commit to BridgeAR/node that referenced this issue Feb 28, 2019
This makes sure the internal `stylize` function is not used to render
anything and instead just uses the regular inspect function in case
of reaching the maximum depth level.

PR-URL: nodejs#24971
Refs: nodejs#24765
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this issue Feb 28, 2019
This prevents leaking of the internal `inspect()` properties when
using a custom inspect function.

It also aligns the indentation to the way it was in v8.0.0 since
that changed unintentionally. All strings returned by the custom
inspect function will now be indented appropriately to the current
depth.

PR-URL: nodejs#24971
Refs: nodejs#24765
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this issue Feb 28, 2019
This prevents any proxy traps from being called while inspecting
proxy objects. That guarantees a side-effect free way of inspecting
proxies.

Refs: nodejs#25212
Refs: nodejs#24765
Fixes: nodejs#10731
Fixes: nodejs#26231
BridgeAR added a commit to BridgeAR/node that referenced this issue Feb 28, 2019
This prevents any proxy traps from being called while inspecting
proxy objects. That guarantees a side-effect free way of inspecting
proxies.

PR-URL: nodejs#26241
Fixes: nodejs#10731
Fixes: nodejs#26231
Refs: nodejs#25212
Refs: nodejs#24765
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants