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

fix: prepare for far classes #1251

Merged
merged 5 commits into from
Aug 23, 2022
Merged

fix: prepare for far classes #1251

merged 5 commits into from
Aug 23, 2022

Conversation

erights
Copy link
Contributor

@erights erights commented Aug 14, 2022

In order to support Agoric/agoric-sdk#5970 , we need to allow at least one level of remotable inheritance. Currently supported in agoric-sdk as a patch. TODO after this is merged and agoric-sdk updated to depend on it, remove the patch.

@erights erights self-assigned this Aug 14, 2022
@erights erights changed the base branch from master to 4333-markm-stricter-passStyleOf August 14, 2022 08:55
@erights erights changed the title WIP prepare for far classes fix: prepare for far classes Aug 14, 2022
@erights erights marked this pull request as ready for review August 14, 2022 09:01
@erights erights requested review from gibson042 and mhofman August 14, 2022 09:01
Base automatically changed from 4333-markm-stricter-passStyleOf to master August 17, 2022 01:27
@erights erights force-pushed the markm-prepare-for-far-classes branch 2 times, most recently from 0b06470 to f04036b Compare August 18, 2022 19:45
@erights
Copy link
Contributor Author

erights commented Aug 18, 2022

ping

Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm requesting confirmation on the recursion depth question.

Also, this review uncovered behavior that smells like a bug: nothing checks if a prototype is hardened, so it can change underneath passStyleOf.

import "@endo/init";
import { Far, passStyleOf, PASS_STYLE } from "@endo/marshal";

const nonEnumerable = { configurable: true, writable: true, enumerable: false };
const mercurialProto = new Proxy(
  Object.defineProperties({}, {
    [PASS_STYLE]: { ...nonEnumerable, value: "remotable" },
    [Symbol.toStringTag]: { ...nonEnumerable, value: "Remotable" },
  }),
  {
    getPrototypeOf(obj) {
      // Self-mutate after returning the original prototype one time
      // (to checkRemotableProtoOf).
      if (obj[PASS_STYLE] !== "error") {
        obj[PASS_STYLE] = "error";
        return Object.prototype;
      }
      return Error.prototype;
    },
  }
);

const makeInput = () => Object.freeze({ __proto__: mercurialProto });
const input1 = makeInput();
const input2 = makeInput();

console.log("# passStyleOf(input1)");
console.log(passStyleOf(input1)); // => "remotable"

/* same because of passStyleMemo WeakMap */
console.log(`# passStyleOf(input1) again (cached "Purely for performance")`);
console.log(passStyleOf(input1)); // => "remotable"

/* different because of changes in the prototype */
console.log("# passStyleOf(input2)");
console.log(passStyleOf(input2)); // => Error (Errors must inherit from an error class .prototype)

packages/marshal/test/test-passStyleOf.js Show resolved Hide resolved
packages/marshal/test/test-passStyleOf.js Show resolved Hide resolved
packages/marshal/src/helpers/remotable.js Outdated Show resolved Hide resolved
packages/marshal/src/helpers/remotable.js Show resolved Hide resolved
@erights
Copy link
Contributor Author

erights commented Aug 21, 2022

Also, this review uncovered behavior that smells like a bug: nothing checks if a prototype is hardened, so it can change underneath passStyleOf.

Hi @gibson042 , when you tried this experiment, was that on a system that was pre or post #1250 ? Your attack seems much like Agoric/agoric-sdk#4333 , which I believe I fixed in #1250, and which this PR relies on. Note that I fixed 4333 only in #1250 in endo. I did not fix the patch in agoric-sdk, so it you're testing that, your attack should still work.

In any case, worried that #1250 didn't really fix it, I tried your test case, which I will add to this PR. I'm pleased to report that your first call to passStyleOf threw 'A tagRecord must be frozen: "[undefined: undefined]"'.

Nevertheless, I will carefully review the code and see if any variation of this attack seems possible. PTAL and see if you can still find any way to attack the system.

Further note:

Even though your attack as written fails, looking again, I'm not yet sure I understand what's going on. More news as I figure it out.

More news:

I read it more carefully and traced it through with the debugger. I think the code is indeed doing the right thing for the right reason, and is not open to this attack. Can you still find any way to attack it?

@erights erights force-pushed the markm-prepare-for-far-classes branch from f04036b to 3a6eca2 Compare August 21, 2022 05:47
@erights erights requested a review from gibson042 August 22, 2022 03:55
packages/marshal/test/test-passStyleOf.js Show resolved Hide resolved
packages/marshal/test/test-passStyleOf.js Outdated Show resolved Hide resolved
@erights erights merged commit c22476f into master Aug 23, 2022
@erights erights deleted the markm-prepare-for-far-classes branch August 23, 2022 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants