endojs / endo

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

WIP test(pass-style): demonstrate safe promise loophole for @FUDCo #1781

Closed erights closed 1 year ago

erights commented 1 year ago

In https://github.com/endojs/endo/pull/1275 I loosened the safe-promise.js checks used by passStyleOf in order to accommodate our workaround for the extra symbol-named own properties that Node's current async_hooks might add to promise instances. This checks these possible extra own properties against symbol-named own properties that would otherwise be inherited from Promise.prototype.

However, in doing so I seem to have forgotten that, even without async_hooks, Promise.prototype always has an own property named Symbol.toStringTag. Thus, the safe-promise rules currently allow that, but only if the property's "value" passes further checks. However, apparently by design, this "value" is obtained by a normal property GET (pr[key]), which might invoke a getter, allowing these overriding properties to be accessors. The easiest way for the value to pass the subsequent checks is to be a number.

The getter can be a per-instance function that, when called with a truthy argument, returns something else, like the string @FUDCo needs. The normal builtin toString use of pr[Symbol.toStringTag] expects a string value, which would not pass the test, causing a conflict. However, when pr[Symbol.toStringTag] is instead a number, the builtin toString behavior falls back to printing [object Object] rather than [object Promise], which is harmless enough for the very specialized case in which @FUDCo would use this loophole.

erights commented 1 year ago

@FUDCo because this PR simply demonstrates that endo already has this odd property, you could utilize it for now without waiting for another endo release cycle. See the TODOs added by the PR.

erights commented 1 year ago

What's the name of the law that all observable behavior, whether documented or not, might get relied on, and thus no longer changeable? If we use this loophole, it'll become one of those, which would be bad. Let's discuss.

erights commented 1 year ago

For debugging-only purposes,

export DEBUG=label-instances`

provokes heap exos and well as virtual and durable objects to have a vref-revealing own @@toStringTag property. For virtual and durable objects, this is set by

https://github.com/Agoric/agoric-sdk/blob/3f47cf03423b46b69851b021a18707b6ce42f056/packages/swingset-liveslots/src/virtualObjectManager.js#L231-L249

  if (LABEL_INSTANCES) {
    // This exposes the vref to userspace, which is a confidentiality hazard
    // as noted in the CONFIDENTIALITY HAZARD NOTE above.
    //
    // Aside from that hazard, the frozen string-valued data property is
    // safe to expose to userspace without enabling a GC sensor.
    // Strings lack identity and cannot be used as keys in WeakMaps.
    // If the property were a accessor property, we'd need to
    // ```js
    //   linkToCohort.set(self, getterFunc);
    //    unweakable.add(getterFunc);
    // ```
    defineProperty(self, Symbol.toStringTag, {
      value: `${proto[Symbol.toStringTag]}#${baseRef}`,
      writable: false,
      enumerable: false,
      configurable: false,
    });
  }

In order for this to work, the passStyleOf remotable checking rules had to explicitly allow this @@toStringTag own data property with a string value, which it does at https://github.com/endojs/endo/blob/162a916137b80c59d1f91ce76ef926f57e014657/packages/pass-style/src/remotable.js#L217

It seems attractive to similarly have the passStyleOf safe-promise checking rules to explicitly allow promises to carry an @@toStringTag own data property with a string value. Then, it could just be a data property and we could avoid all the nonsense with it being an accessor in order to have a number value and a hidden string value.

Since @FUDCo 's need is also debug-only, it further seems attractive to extend the purpose of

export DEBUG=label-instances`

to label some promises with vrefs and krefs refs, using a similar @@toStringTag own data property.

FUDCo commented 1 year ago

What's the name of the law that all observable behavior, whether documented or not, might get relied on, and thus no longer changeable? If we use this loophole, it'll become one of those, which would be bad. Let's discuss.

Hyrum's Law. https://medium.com/se-101-software-engineering/what-is-the-hyrums-law-and-why-should-software-engineers-care-9bd98fbe74da

I'm really not sure how I feel about this. This particular loophole would actually work just fine for my use case, because the kref for a promise always has the form kpNN. On the other hand, it would bake this pattern in in a weird place and if we should ever decide to change it it will be a royal pain. On the other other hand, what distinguishes one promise kref from another probably wants to be number; what we'd probably change if we ever did is the kp part. On the other other other hand, if we found we needed to encode some additional metadata into the kref (like a type-ish indicator), that would once again put us in a world of hurt. (We do, in fact, have such a piece of metadata in vrefs, which encode whether the reference is to something durable or not).

Another consideration is that our token object for remotables also has a second method that provides the iface string. I'm not sure that's important for promises (presumably they're all, well, promises) but maybe we might use the iface string to indicate what type of thing the promise is allegedly a promise for. But if we wanted that, we'd need a way to add a second method of this sort to the promise token as well.

erights commented 1 year ago

what distinguishes one promise kref from another probably wants to be number;

Look again. The number is just to pass the safe-promise check as it is. The getter parameter is so we can pass through whatever string we'd like.

This would just be until we rev endo. If we rev endo to do https://github.com/endojs/endo/pull/1781#issuecomment-1727080735 , then the own @@toStringTag property could be a string-values data property.

FUDCo commented 1 year ago

OK, I definitely misunderstood this the first time through.

Now that I read it through again much more carefully, my weirdness meter has redlined.

Let me try again to make sure I understand. I may be being overly detailed in this explication because the behavior in question is too bizarre for me to quite believe this is reality:

I can't even. But, hey, it looks like it works. I think the Hyrum's Law queasiness is entirely justified. But, hey, it looks like it works.

FUDCo commented 1 year ago

Is iface irrelevant in the case of a Promise?

mhofman commented 1 year ago

I am weirded out by this @@toStringTag hackery.

I guess we don't have pseudo-promises yet we could use for this purpose? AFAIK, @FUDCo's constraint is to pass-through values but they don't have to be functional, right?

erights commented 1 year ago

Is iface irrelevant in the case of a Promise?

Yes

erights commented 1 year ago

I am weirded out by this @@toStringTag hackery.

FWIW, me too!

I guess we don't have pseudo-promises yet we could use for this purpose? AFAIK, @FUDCo's constraint is to pass-through values but they don't have to be functional, right?

That was my first thought as well, and where I think the proper fix lies. But @FUDCo explained that he needs something soon, so I came up with this as a first step that happens to work without even revving endo.

erights commented 1 year ago

Now that I read it through again much more carefully, my weirdness meter has redlined.

FWIW, me too!

... too bizarre for me to quite believe this is reality:

But this is JavaScript!

  • Something, somewhere, for reasons of its own that defy explanation, expects the [Symbol.toStringTag] property of a promise to be a ...

The check was unaware of Symbol.toStringTag. It was checking all symbol-named properties on Promise.prototype under the WRONG assumption that these would only be added by NodeJS's async_hooks. So my trick is to override Promise.prototype[Symbol.toStringTag] with a property whose value passes the tests that were written only to validate that the expected async_hooks properties didn't contain anything unexpected and dangerous.

All the rest of your comments are essentially correct.

erights commented 1 year ago

See https://github.com/endojs/endo/pull/1785

erights commented 1 year ago

Closing in favor of https://github.com/endojs/endo/pull/1785