Node.js adds a property called domain to promises

[katelynsills]

Mark and I noticed that Node.js adds a property called domain to promises while running something from a REPL in SwingSet.

Mark says that domain leads to all sorts of objects whose semantics we haven't inspected for safety. This is an authority leak. If SES can't remove the domain property from all promises before the promise is exposed to user code, then this is a severe authority leak for SES as run on Node.js. Fortunately it seems to only be a problem when code is run from a REPL.

[edit 2020-09-15 @kriskowal]

SES will not be secure on Node.js until we have the means to disable domains. We must work with the Node.js project to reveal a command-line flag or the moral equivalent.

[erights]

See https://github.com/Agoric/ERTP/blob/master/util/makePromise.js
and https://github.com/Agoric/SwingSet/blob/master/src/makePromise.js
for some places where we are kludgily and unreliably removing some occurrences of .domain.

See Agoric/documentation#48

We should instead monkey patch the Promise constructor, and other elements of the std Promise API to remove these as much as possible. That will subsume the safety of anything like a naive makePromise built on the std Promise APIs. However, given Node's current behavior and the reachability of promises via async/await syntax, I see no way to remove .domain reliably. This is a serious security problem that probably prevents us from making strong security claims for SES running on an unmodified Node.

Attn @bmeck @katelynsills @DavidBruant

[DavidBruant]

From what i can read, domains are expected to go eventually

This eventually seems to be far away, though

In the short term, it might be possible to ask the Node project if one of these solutions is possible:

• add a --no-domains option which disables domains entirely
  • and SES can recommand running Node with this flag to guarantee security claims
• make domain a getter property on Promise.prototype (or Object.prototype since it seems to attached to more objects than promises)
  • SES can do delete Promise.prototype.domain and make security claims

[erights]

@bmeck , could Node do one of @DavidBruant 's suggestions? Or could Node plug this leak by any other means?

@DavidBruant , where else is it attached besides promises?

[bmeck]

@erights it is certainly possible to attempt both, though getter/setter is easier to land most likely.

[bmeck]

it looks like we do this to a variety of objects https://github.com/nodejs/node/blob/fce1a5198a89e8f62ea2d093e01971db46da9bf5/lib/domain.js#L62 , I need to think about this, but a flag might be simpler since some of those could be from userland

[bmeck]

Would making process.domain configurable be a suitable solution? It seems these always come from the underlying closure and internals pass through the underlying data by using this process.domain accessor.

[erights]

Would making process.domain configurable be a suitable solution? It seems these always come from the underlying closure and internals pass through the underlying data by using this process.domain accessor.

I am not oriented enough. How would this work? How might this make things safe? How might this fail to make things safe?

[bmeck]

@erights currently:

• all assignments to async resources' .domain (not just Promises) by node core are done via the member expression of process.domain with a direct reference to the primordial process object and dynamic lookup of "domain".
• process.domain is an accessor pair that is not configurable, so you cannot censor it by replacing the accessors.

By making a new accessor pair or replacing it with a data property, it could be forced to always be null or whatever the desired value is, e.g.

Object.defineProperty(process, 'domain', {
  get() {return null},
  set(v) {},
  enumerable: true,
  configurable: false
})

Would then result in promise.domain always being null.

Completely disabling domains would be a large effort and the assignment is not on Promise exclusively so this seems somewhat reasonable to me to put the censoring at the source of the value.

[erights]

Is there any reason to prefer this accessor approach to

Object.defineProperty(process, 'domain', {
  value: null,
  writable: false,
  enumerable: true,  // or false. I don't care
  configurable: false
});

Either seems fine to me. Other things being equal, I prefer the data property approach.
In either case, yes, let's do this! How do we get this fixed as fast as possible?

[bmeck]

@erights it wouldn't matter for the most part. i would think slightly diff assignment errors if the whole non-writable prototype thing hints is the only diff.

[bathos]

One reason to prefer the accessor might be to minimize difference — it’d normally be an enumerable accessor, so keeping it one reduces the change in this mode to just what’s really needed (that it return null).

[bmeck]

I opened a PR against node, though I'm sure something is likely to break if it expected the domain to be set and it suddenly was censored it should allow censoring at least.

[erights]

Thanks!

[bmeck]

We have some feedback trying to get together a test suite and concerns about breakage of the REPL itself since it uses this feature for error checking. Is there an example codebase that we can take a small look at that is trying to avoid this feature leaking info to complete the test suite against

[erights]

Hi @bmeck , well, SES. The existence of this feature causes leakage of stuff that should not be available. The uncontrolled, implicit, and syntactic creation of promises means SES cannot reliably intercept and remove these before user code can observe them. In addition to security leakage, observing them breaks host virtualizability, as these properties would not have been present on other hosts.

Whatever is currently doing this, can't it get the same effect with a private weakmap instead?

[bmeck]

Hi @bmeck , well, SES. The existence of this feature causes leakage of stuff that should not be available. The uncontrolled, implicit, and syntactic creation of promises means SES cannot reliably intercept and remove these before user code can observe them. In addition to security leakage, observing them breaks host virtualizability, as these properties would not have been present on other hosts.

This doesn't seem like a codebase we can use to check that disabling the feature allows code to work properly though. I was looking for some example code that had the problems being able to run tests against to show that allowing it to be disabled does indeed allow the node runtime and the SES code to work properly still.

[erights]

I don't think I yet understand what you're asking for. For any closed test of only functionality, the addition of an unexpected exploitable property won't cause the closed test to fail. However, it will cause security to fail because an attacker can know to exploit it. So what kind of test are you looking for?

What if we had a test that created a promise, checked the names of all its own properties, and fails if it has any own properties it doesn't expect, like domain?

[bmeck]

this issue arose from

a REPL in SwingSet.

I'm looking for a reproduction of the REPL and/or environment we are trying to ensure works. We cannot simply use a WeakMap as ecosystem APIs and the deprecated Node API are using that for public accessible interactions. Any change we make to the API needs to ensure things like the REPL in question continue to work. Node's repl builtin module for example uses Domains and would potentially break in odd ways. We are looking to see how this problem was first seen so we can replicate it and prevent regressing via any fix we introduce.

[erights]

We cannot simply use a WeakMap [...] Node's repl builtin module for example uses Domains and would potentially break in odd ways.

I am still not understanding. Are you saying that you cannot avoid adding the domain property to promises? If so, then whatever test we write, you will almost certainly fail. What kind of test do you expect we could write that would both check that you're not breaking security while simultaneously allowing you to add a visible and uncensorable domain property that would break security?

At this point I think I remain totally confused.

My suspicion is that the only way forward is indeed to write a test that would fail if it sees a domain property, and for the Node repl builtin and anything else that currently depends on a publicly visible domain property to be fixed to rely on a weakmap instead.

[bmeck]

@erights we can avoid it, but we cannot do so without someone censoring the API. you have to opt-in to us not adding that property. We would have to make any WeakMap publicly accessible and for backwards compat would need to keep the original behavior unless you opt-in to the breaking change somehow.

[erights]

Good, thanks. That we can do. As we discussed, were it just on promises, we could refactor to make it an accessor on Promise.prototype that SES could censor at initialization time by removing it. However, as you pointed out, it might be added to any object. I think you already suggested we could make process.domain deletable, and then SES initialization could delete it. This would be a special case in SES initialization, but that's tolerable. Does that do what we all need?

[erights]

What old code would break that

• anyone cares about, and
• couldn't be rewritten to not break

?

[bmeck]

anyone cares about

This is largely irrelevant to our being able to state it is safe to fix/demeans existing codebases.

The node default REPL depends on domains and it is unclear the impact this would have on it in all the widespread usage it has.

couldn't be rewritten to not break

This is part of the question, domain does not have a direct replacement possible due to its interaction with Node's runtime itself. If people had to stop using all modules relying on that for SES, would it be fine to ask them to rewrite their module so that it works when using SES?

[bmeck]

we can talk about this tomorrow, @misterdjules would it be possible for you to attend?

[michaelfig]

Mark won't be at the meeting tomorrow due to a prior engagement.

…

-- Michael FIG Software Engineer, Agoric

On Wed., Dec. 11, 2019, 8:55 p.m. Bradley Farias, ***@***.***> wrote: we can talk about this tomorrow, @misterdjules <https://github.com/misterdjules> would it be possible for you to attend? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <https://github.com/Agoric/SES/issues/113?email_source=notifications&email_token=AADPUHDW37HDDXEHN7IBB2TQYGRYTA5CNFSM4HFW2POKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGVJKJA#issuecomment-564827428>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADPUHGQ5DJYG2SE722C7VTQYGRYTANCNFSM4HFW2POA> .

[misterdjules]

@bmeck What date and time is the meeting?

[bmeck]

@misterdjules Weekly Thursday at 1PM Pacific, but it appears that this week might not be ideal.

[misterdjules]

@bmeck I would be available today, but not next week.

[kriskowal]

A tip from @dominictarr suggests it is possible to disable domains by defining domain as a non-configurable property of the process global, as that would prevent require("domain") from initializing. In some circumstances, we can also check whether domain initialized before we put our trap in place.

if (typeof process === 'object' && process !== null) {
  // Prevents domains from initializing:
  Object.defineProperty(process, 'domain', {value: null, configurable: false, writable: false, enumerable: false});
  if (typeof require === 'function') {
    let domainsInitializedAlready = false;
    try {
      require('domain')
      domainsInitializedAlready = true;
    } catch {
      // We expect an initialization error.
    }
    if (domainsInitializedAlready) {
      throw new Error(`SES cannot guarantee containment if Node.js domains have already been initialized`);
    }
  }
}

This covers CommonJS only. We might be able to do the same with dynamic import, but only asynchronously, and we can’t feature-detect ESM with typeof.

[kriskowal]

Now without caveats:

if (typeof process === 'object' && process !== null) {
  // Check whether domains were initialized:
  const domainDescriptor = Object.getOwnPropertyDescriptor(process, 'domain');
  if (domainDescriptor !== undefined && domainDescriptor.get !== undefined) {
    throw new Error(`SES cannot guarantee containment if Node.js domains have been installed`);
  }
  // Prevents domains from initializing:
  Object.defineProperty(process, 'domain', {value: null, configurable: false, writable: false, enumerable: false});
}

[mhofman]

An alternative in case we want t allow import 'domain', but prevent domain usage:

• To prevent domains from being created: Add a Domain.prototype.on function which throws (normally inherited from EventEmitter.prototype), and harden Domain
• For good measure to make sure domains can't be switched if one was created: Object.freeze(domain._stack)
• To do all this while preventing modification to the process global: Replace globalThis.process with a proxy which traps a few things (like define domain, set setUncaughtExceptionCaptureCallback, add on which ignores new listeners ?), import 'domain', restore globalThis.process and eventually restore EventEmitter.prototype.emit for good measure, event though it should be inert at that point.

[kriskowal]

One of the implicit albeit soft requirements is to avoid coupling SES to any particular module system. Currently, the same source works in a browser script, CJS, and ESM, since it’s just presenting its API as mutations to the global scope. This keeps the build system small and avoids initializing anything it doesn’t need.

I’m going to present a PR with the snippet above.