Private symbols can leak when using decorators

[rbuckton]

This issue was first discovered when discussing reified private names in the decorators repository. Here's the issue, in the context of private symbols:

A decorator wishes to attach private state to a class instance in such a way that another function in the same lexical scope as the decorator can access the private state (a common pattern in test frameworks, dependency injection, etc.):

// dec.js
const privateState = Symbol.private();

// decorator installs shared private state
export function decorator(privateData) {
  return function (memberDescriptor) {
    memberDesriptor.extras = [{
      kind: "field",
      placement: "instance",
      key: privateState,
      initializer: () => privateData
    }];
    return memberDescriptor;
  };
}

// function in same lexical scope can access shared private state
export function accessPrivateData(object) {
  const privateData = object[privateState];
  console.log(privateData);
}

// class.js
import { decorator } from "./dec";
export class C {
  @decorator("this is private data")
  foo() {}
}

// main.js
import { accessPrivateData } from "./dec";
import { C } from "./class";

const obj = new C();
accessPrivateData(obj); // prints: this is private data

A malicious package can access the privateState on the object by simply calling decorator:

// evil.js
import { decorator } from "./dec";
import { C } from "./class";

// demonstrate leak
const memberDescriptor = {};
decorator()(memberDescriptor);
const privateState = memberDescriptor.extras[0].key; 

// mutate private state
const obj = new C();
obj[privateState] = "this data is not so private after all";
accessPrivateData(obj); // prints: this data is not so private after all

The proposed solution is to be able to further restrict a private symbol by passing it through a runtime function that can restrict its privilege:

// dec.js (modified)
const safeSymbolRestricted= Symbol.restricted; // save copy to avoid a global mutation attack vector
const privateState = Symbol.private();

// decorator installs shared private state
export function decorator(privateData) {
  return function (memberDescriptor) {
    // copy of `privateState` that can only be used to define the property
    // but cannot be used to get/set values.
    const privateStateCopy = safeSymbolRestricted(privateState);
    memberDesriptor.extras = [{
      kind: "field",
      placement: "instance",
      key: privateStateCopy , 
      initializer: () => privateData
    }];
    return memberDescriptor;
  };
}

// function in same lexical scope can access shared private state
export function accessPrivateData(object) {
  const privateData = object[privateState];
  console.log(privateData);
}

There is still a vulnerability here, however, as evil.js still could create an object that "stores" the same private state and pass it around:

// evil.js (modified)
import { decorator, accessPrivateData } from "./dec";

// demonstrate leak
const memberDescriptor = {};
decorator()(memberDescriptor);
const privateStateDefineOnly = memberDescriptor.extras[0].key; // this is the restricted version

// masquerade as object
class M {
  @(memberDescriptor => {
    memberDescriptor.extras = [{ 
      kind: "field",
      placement: "instance",
      key: privateStateDefineOnly, 
      initializer: () => "not who you think I am" // <-- this is what we want to inject
    }];
    return memberDescriptor;
  })
  method() {}
}

const obj = new M();
accessPrivateData(obj); // prints: not who you think I am

A further proposed solution would be to have the runtime call decorators with an extra symbol argument that acts as a declaration time only access key for adding private state:

// dec.js (modified)
const safeSymbolRestricted= Symbol.restricted; // save copy to avoid a global mutation attack vector
const privateState = Symbol.private();

// decorator installs shared private state
export function decorator(privateData) {
  return function (memberDescriptor, accessToken) {
    // copy of `privateState` that can only be used to define the property on
    // but cannot be used to get/set values.
    // the copy is entangled with the class declaration's access token.
    const privateStateCopy = safeSymbolRestricted(privateState, accessToken);
    memberDesriptor.extras = [{
      kind: "field",
      placement: "instance",
      key: privateStateCopy, 
      initializer: () => privateData
    }];
    return memberDescriptor;
  };
}

// function in same lexical scope can access shared private state
export function accessPrivateData(object) {
  const privateData = object[privateState];
  console.log(privateData);
}

Now, evil.js has no mechanism to masquerade as C.

[zenparsing]

Can the decorator be written this way instead?

// dec.js
const throwawaySym = Symbol.private();
const privateState = Symbol.private();

// decorator installs shared private state
export function decorator(privateData) {
  return function (memberDescriptor) {
    memberDesriptor.extras = [{
      kind: "field",
      placement: "instance",
      key: throwawaySym, // This isn't used to store anything
      initializer: function() { this[privateState] = privateData; },
    }];
    return memberDescriptor;
  };
}

// function in same lexical scope can access shared private state
export function accessPrivateData(object) {
  const privateData = object[privateState];
  console.log(privateData);
}

It seems like there should be a way for the decorator to directly add "instance finishers" though...

[rbuckton]

This throws away the benefits of static analysis and hidden class optimizations that were one of the driving factors behind the changes to decorators in the current Stage 2 proposal from the version in the Stage 1 proposal. Your example also installs an unused property on the class.

[gsathya]

This throws away the benefits of static analysis and hidden class optimizations

Can you expand more on this? Current Stage 2 decorators will cause almost all of v8's static analysis optimizations to be thrown away so I'm curious what was saved.

[rbuckton]

I don't have specifics as to how v8 will implement support for decorators and what optimizations it will continue to support, as I am not involved in that project. My statement is purely based on information given to me as one of the reasons given in committee several years ago. The Stage 1 design evaluated decorators after the class definition was evaluated, which would result in numerous shape changes. The current Stage 2 design evaluates decorators during class definition evaluation, before the final set of members is defined for the class. This provides the opportunity for retaining some static analysis optimizations, at the very least.

[gsathya]

I don't have specifics as to how v8 will implement support for decorators and what optimizations it will continue to support, as I am not involved in that project.

I think you misunderstood. As a member of the v8 project, I was stating that the current stage2 decorators will kill most of our static analysis optimizations.

The Stage 1 design evaluated decorators after the class definition was evaluated, which would result in numerous shape changes. The current Stage 2 design evaluates decorators during class definition evaluation, before the final set of members is defined for the class

I see. Thanks for the background.

This provides the opportunity for retaining some static analysis optimizations, at the very least.

Eh, I don't think this helps all that much. There are tons of side effects to consider; this would have to be a very narrow fast path (that probably doesn't even occur much in practice).