Discuss
  • Proxy target/handler access leak in Node

Proxy target/handler access leak in Node

# Darien Valentine (6 years ago)

A few weeks ago I’d commented on an open Node github issue regarding Proxies and inspection. While the bulk of the comment concerns an opinion that proxies should not be treated as a special case, I included an example of a mechanism by which the current implementation allows outside code to access the target and handler objects of a proxy that it does not own.

On reflection I realized this specific issue might be worth drawing more attention to.

const util = require('util');

const victim = new Proxy({}, {
  SECRET: 'Nothing outside can access this'
});

let secret;

const invariantViolator = {
  [util.inspect.custom](depth, options) {
    const { stylize } = options;

    options.showProxy = true;

    options.stylize = (value, color) => {
      secret = value;
      options.stylize = stylize;
      return stylize(value, color);
    };

    return victim;
  }
};

util.inspect(invariantViolator);

console.log(secret); // 'Nothing outside can access this'

The implication is that even if running Node with no C++ addons, it is presently possible for proxies to be violated using just the standard lib, which may be significant from a security perspective. I’m not sure if that’s the case in practice, but just in case, I figured I should try to get eyes on it.

Note that even if this particular hole is patched, the "analog hole" (so to speak) of just analyzing the string output remains.

# Mike Samuel (6 years ago)

Nicely done!

One more reason to prefer WeakMaps to properties when relating objects and secrets.

# Mark Miller (6 years ago)

This is indeed quite scary. I have never used of explored the Node util API. What is going on here? Can you explain?

The Realms shim and SES (which build on the Realms shim) create an environment in which only those globals defined by EcmaScript, not by the host, are present by default. Agoric/SES/tree/master/demo, rawgit.com/Agoric/SES/master/demo Does that mean this attack is impossible from code loaded into a new realm as made by the shim or SES?

# Darien Valentine (6 years ago)

What is going on here? Can you explain?

A C++/V8 API is used to obtain references to the target and handler from only the proxy object, even though those objects aren’t supposed to be available to this ES scope, in formatValue:

nodejs/node/blob/master/lib/util.js#L642-L647

The pair of objects is passed to another function, which in turn may pass values to the method ctx.stylize (via formatValue again), but that property may have been previously overwritten from user code:

nodejs/node/blob/master/lib/util.js#L566-L580

nodejs/node/blob/master/lib/util.js#L628

AFAICT, fixing the leak (beyond util.js) would be possible by making stylize unconfigurable/unwritable. However there may be other avenues here — I’m not sure.

Does that mean this attack is impossible from code loaded into a new realm as made by the shim or SES?

The exploit isn’t sensitive to realm-of-origin, since util is using v8 internals to get the references, not e.g. a patched version of Proxy. But it still depends on the proxy object in question being exposed to a context where util.inspect exists.

In the issue comment, I wrote this:

This exploit can be fixed pretty easily. But I think it’s symptomatic. My understanding was that V8’s C++ APIs are intended for making external functionality available to ECMAScript using ECMAScript values and ECMAScript semantics. Here though, it is being used to bypass ECMAScript semantics. The behavior seen in the above script isn’t possible in ECMAScript — but neither is distinguishing between objects which have been implemented as ordinary objects and objects implemented as proxy exotic objects.

# Isiah Meadows (6 years ago)

In the browser, I could see why not to allow it by default: it's a potential security hole, and GC concerns do exist (what happens if the symbol's data is no longer accessible without reflection?).

In embeddings, I'm not sure if there's any real problem, considering Node's vm API allows proper sandboxing to prevent scripts from getting ahold of a util object in the first place, and by extension, the private data assigned to it.

It might be worth accepting a level of non-determinism within the spec stating that if a symbol is deemed not accessible by the implementation, they aren't required to return it. Objects would have to hold those private symbols weakly, but the non-determinism would need to clarified with the weak ref proposal. (This is primarily a concern with private symbols, and an implementation might choose not to expose that in the first place, even to embedders.)

Isiah Meadows contact at isiahmeadows.com, www.isiahmeadows.com

# James M Snell (6 years ago)

Some background as I am the one who added the showProxy feature into Node.js...

util.inspect() is considered by Node.js to be a diagnostics API. The intent is to allow adequate debugging in a variety of scenarios. This was added to address a user issue where inspection of a Proxy object (that the user didn't know was a proxy object) would cause an infinite loop when invoking a certain getter that just happened to have a console.log statement (in Node.js, console.log uses util.inspect in some cases. There are cases where one does need to know when an object is a Proxy.

That said, the specific issue in this thread is a bit different. When implementing showProxy, I took the additional step of introspecting the component elements of the proxy because the goal of util.inspect is to provide as much insight as possible about the object being inspected. The ability to provide a custome stylize function for inspect is a long standing feature of Node.js long before showProxy was introduced.

One thing I would not classify this as is an attack, exploit, or vulnerability. The Node.js trust model assumes that all code is trusted. I can, however, bring this issue to the Node.js project and see how folks would feel about not exposing the internal objects when a Proxy is detected. That would reduce the diagnostic utility of util.inspect with Proxy objects, which is unfortunate, but it would resolve the issue here. Removing this would likely require a proper deprecation of the existing functionality which is a semver-major change.

The final thing I would say is: Node.js has a responsible disclosure process for potential security vulnerability reports. If one feels that some behavior implemented by node.js presents a vulnerability or attack vector, I would encourage them to please use our HackerOne site ( hackerone.com/nodejs) to report the issue before discussing in a public forum so that the potential risks can be evaluated before public disclosure.

# Darien Valentine (6 years ago)

Thanks for the context, James. Yes, this thread mainly concerns the issue of being able to obtain references to values within the handler/target from external code, though I did try to make a case for not having the showProxy option in the original issue thread.

I would also not have thought to call it an “attack” vector — agreed that from an “all code is trusted” POV, there is no security issue. But Mark would be able to say better for sure, and apologies if this should have been reported to HackerOne after all.

What it does is make an invariant of the language violable. It’s similar to exposing a function which, given only a function object, may return references to arbitrary values from that function’s scope.

# Mark Miller (6 years ago)

On Mon, Sep 17, 2018 at 8:32 AM Darien Valentine <valentinium at gmail.com>

wrote:

Thanks for the context, James. Yes, this thread mainly concerns the issue of being able to obtain references to values within the handler/target from external code, though I did try to make a case for not having the showProxy option in the original issue thread.

I would also not have thought to call it an “attack” vector. Mark would be able to say better for sure though. It does make an invariant of the language violable though. It’s similar to exposing a function which, given only a function object, may return references to arbitrary values from that function’s scope.

It’s similar to exposing a function which, given only a function object,

may return references to arbitrary values from that function’s scope.

This is an apt comparison. A debugger has access to such info. Likewise, in a secure OS, when one process is able to debug another, the first process can read any data from the address space of the second. There have even been language implementations that were otherwise supposed to be memory safe that had "peek" and "poke" operations for reading and writing arbitrary memory locations from programs in the language. Of course, memory allocators and garbage collectors typically need such access.

Whether these are "attacks" or "vulnerabilities" depends on how such permission for debug-level access or peek/poke access is controlled and provided.

From a bit of web searching, I found the following worrisome:

nodejs/node#20857, nodejs/node/commit/dadd6e16888baac8fd110432b81f3fd1237be3e1 seemingly in response to nodejs/node#20821, nodejs/node#22671

Making is a public symbol in this manner means it is almost impossible to deny. It is still true that "util" is deniable, so this isn't necessarily fatal. I am not yet oriented enough to understand what the consequences are of suppressing util; but I am worried.

# Mark Miller (6 years ago)

The Node.js trust model assumes that all code is trusted.

First I want to respond to this sentence out of context. I often hear such phrases. I know what people mean by this, but the phrase "trusted" here always leads to confusion and muddy thinking. I don't trust the code I wrote yesterday. Instead, what we mean by this is something like:

"The Node.js xxxx model assumes we are fully vulnerable to all code."

This phrasing helps us notice some of the questions made obscure by the earlier phrase. What is fully vulnerable to which code? What is meant in this case is presumably something more like

"...assumes the Node.js process is fully vulnerable to all code it is asked to run."

Under a traditional OS, a process executes as the account (or "user") executing that process, and has all the permissions of that user. So this becomes:

"...assumes the user is fully vulnerable to all code that any Node process executing as that user is asked to run."

(Which of course includes anything built on Electron, which makes the situation even worse in some ways.)

Given the way that this body of code is typically selected, by transitive alleged package dependencies, this is a ridiculously large attack surface. Fortunately, there is increasing appreciation that such pervasive vulnerability is problematic.

See www.youtube.com/watch?v=9Snbss_tawI&list=PLKr-mvz8uvUgybLg53lgXSeLOp4BiwvB2&index=23

Also relevant www.youtube.com/watch?v=wQHjITxQX0g&list=PLKr-mvz8uvUgybLg53lgXSeLOp4BiwvB2&index=18, www.youtube.com/watch?v=9WdbTucMaRo&list=PLKr-mvz8uvUgybLg53lgXSeLOp4BiwvB2&index=10, www.youtube.com/watch?v=pig-sIS8_Wc&list=PLKr-mvz8uvUgybLg53lgXSeLOp4BiwvB2&index=20

# Darien Valentine (6 years ago)

Making is a public symbol in this manner means it is almost impossible to deny. It is still true that "util" is deniable, so this isn't necessarily fatal. I am not yet oriented enough to understand what the consequences are of suppressing util; but I am worried.

I wasn’t under the impression that the util.inspect.custom symbol or the functionality it provides is itself problematic. It just happens to be possible to use it, currently, to get inside the proxy target/handler because its second argument is an object with a property whose value is a function that can be called with proxy target / handler values, yet that property may be overwritten by user code. This is very unlikely to have been an intentional facet of the API, so making opts.stylize unwritable/unconfigurable shouldn’t reduce the usefulness of custom inspect implementations, and afaict this would block off the avenue by which references to target/handler can escape util.js.

Is there another reason why the util.inspect.custom symbol contract would be an issue?