Scoping down the use of PrivateName

[littledan]

In the July 2018 TC39 meeting, @allenwb mentioned somehow restricting the use of PrivateName down somewhat, to avoid some kinds of leakage, but I didn't quite understand what he was suggesting. Allen, would you be able to comment here with more details?

[allenwb]

First the problem:

Key to the "hard private" protection model that TC39 has adopted is that access to private fields is lexically limited to code that statically occurs as part of a class definition that is within the scope of the private field definition. Key hard private invariant: It is possible to expose external access to private fields but only if the author of a class definition has included source code in the definition that exposes that accesses. For example:

let exposedAccessor;
class WithExposedField {
  #exposed;   
  constructor() {
     if (!exposedAccessor) {
       //a static initialization block would be a better way to do this
       exposedAccessor = {
          get: obj => obj.#exposed,
          set: (obj, value) => obj.#exposed = value
       }
     }
  }
}

let instance = new WithExposedField;
exposedAccessor.set(instance, "set from outside of class def");

Private fields, in the absence of the decorator proposal, provides no way for an attacker to gain access to a private field other than by changing source code before it is loaded.

Private Names objects, as defined in the current decorators proposal, breaks this invariant in three ways:

1. It reifies the key of private names in a manner that can be leaked from the class definition by a malicious decorator (eg, a trojan horse decorator).
2. A leaked private name is a capability that can be used by any code to read and write the associated private field of any object that has that field. This violates the Key hard private invariant.
3. A leaked private name when used in conjunction with a cooperating constructor provides a capability to covertly add a specific private field (for example a field used for branding) to instances of any class using that decorator.

It's fairly easy to remediate the 1st and 3rd item. A simple step is to scope the useful lifetime of each PrivateName object to the duration of the ClassDefinitionEvaluation that exposes the PrivateName object via an element or class descriptor. This could work by ClassDefinitionEvaluation maintaining a List of PrivateName objects it has exposed and invalidating each of those instances (by nulling out its [[PrivateNameDate]] internal slot before exiting (either normally or abruptly) from ClassDefinitionEvaluation. The 3rd item could be address by making ClassDefinitionEvaluation refuse to create any private fields using Private Name objects that are not in its List of exposed Private names.

The 2nd problem is more fundamental. Private Name objects expose a read/write capability to enable decorators to instantiate decorator generated functions that read/write private fields. Such functions are typically installed as the values of method properties or get/set functions of accessor properties. Using access to a lexically captured PrivateName object with read/write capability is a way to allow a decorator author to code such privileged functions. Because, they are evaluated long after ClassDefinitionEvaluation completes limiting the effective lifetime of PrivateName objects or revoking their read/write capability breaks that use case.

The Private Name object read/write capability is a fundamental violation of the Key Hard Private Invariant. It enables a third party (the author of a decorator) to access a private field that the author of a class definition has not explicit exposed. I don't believe that there is a way to support all of the decorator use cases that require Private Name objects with read/write capability without violating the Key Hard Private Invariant. To me that suggest we should abandon the goal of supporting those use cases.

In TC39 we routinely make trade-offs between expressibility and other goals such as security or usability. There are many attractive features that we have left out of the language because of such trade-offs. This is such a situation. All of the decorator use cases that require Private Name read/write capability are possible using Symbol keyed "soft private" fields. The only reason to have hard private fields in the language is to provide an ultimate level of secure integrity to objects that really require it. In all other situations soft private is more expressive.

It seems to me that if decorators expose persistent Private Name objects with read/write capabilities we will have eliminated the primary reasons for the existence of private fields.

cc/ @erights @zenparsing @ waldemarhorwat

[ljharb]

Do you think the invariant is still a problem if the private field itself is decorated, as well as a class containing them - or only when the class is decorated?

[allenwb]

@ljharb The field decorator might be something that sounds useful and not a threat to the privacy of a private field.

I'm primarily thinking about Trojan horse decorators. It would be easy enough for somebody to publish on NPM an attractive sounding decorator module that actually does useful things but under the covers leaks access to certain private fields. Also, there are various ways that attackers might gain access to mutable bindings that are used to access decorators and substitute a Trojan.

[jridgewell]

@allenwb: I think this misses a critical use case if we're not able to install new private names onto classes. For example, a common theme in frameworks it to cause a re-render when a property is changed:

class Comp extends Framework {
  // When count changes, re-render the component
  count = 1;

  render() {
    return html`<span>${this.count}</span>`;
  }
}

The full-proof way to do this is to make a getter-setter pair, but it's cumbersome. You also have to think about where does the getter-setter store it's state? Usually it's a _counter "private" property. Or, with PrivateNames, we could use a #counter when we write the class:

class Comp extends Framework {
  #count = 1;
  get count() {
    return this.#count;
  }
  set count(v) {
    this.#count = v;
    Framework.render(this.render());
  }

  render() {
    return html`<span>${this.count}</span>`;
  }
}

It's a lot of boilerplate for a simple re-render. We had imagined an @tracked decorator would be able to make this much easier on the programmers:

class Comp extends Framework {
  @Framework.tracked
  count = 1;

  render() {
    return html`<span>${this.count}</span>`;
  }
}

But, if @tracked can't create and install a reified PrivateName onto the Comp class, we're back to using a _count (or a new WeakMap instance, but that won't be hard encapsulated).

---

If I can recategorize you're three points, they come down to two different issues:

1. Leaked privates expose read and write, exposing private state
2. Leaked privates can be installed on classes, allow their instances to masquerade past brand checks

From my code examples above, I don't believe we can solve 2 without forbidding useful code patterns. But we might be able to fix 1:

There's another issue (https://github.com/tc39/proposal-decorators/issues/24) that essentially boils down to "keys are read/write privileged by default", which allows the kind of Trojan horse decorators you described. This is because the descriptor pass to the decorator call contains a fully privileged PrivateName key:

function dec(descriptor) {
  // descriptor looks like:
  // {
  //   key: privateNameInstance,
  //   initializer: () => 1
  // }

  const {key} = descriptor;
  // assert(key instanceof PrivateName);
  // assert(typeof privateName.get === 'function');
  // assert(typeof privateName.set === 'function');
}

class Ex {
  @dec
  #count = 1;
}

My current thinking is that the descriptor key for private fields should not be read/write privileged, it should just be able to install the internal slot onto class instances (let's call it a PrivateNameKey). To get a fully privileged read/write PrivateName, you pass the PrivateNameKey instance to some helper function that's alive for the decorator call:

function dec(descriptor, privateNameReifier) {
  const { key } = descriptor;
  // assert(key instanceof PrivateNameKey);
  // assert(key.get === undefined);
  // assert(key.set === undefined);

  const privateName = privateNameReifier(key);
  // assert(privateName instanceof PrivateName);
  // assert(typeof privateName.get === 'function');
  // assert(typeof privateName.set === 'function');
}

class Ex {
  @dec
  #count = 1;
}

So common operations on the descriptor pair might leak the PrivateNameKey. I think that's unfortunate, but necessary for the meta programming example I had above. But that leaked key doesn't grant access to read/write access, preserving hard encapsulation. Would this be secure enough against Trojan decorators?

[littledan]

It enables a third party (the author of a decorator) to access a private field that the author of a class definition has not explicit exposed.

I agree with your analysis that decorating private class elements requires trusting decorators. This proposal takes "hard private" with respect to decorators to mean that decorators are part of the class. That is, if you decorate a private class element or a class with private elements, you are deciding to trust the decorator with access to those elements.

In TC39 we routinely make trade-offs between expressibility and other goals such as security or usability.

I like your phrasing of how it's our trade-off to make. An alternative would be to not expose private class elements to decorators. This is a legitimate alternative that we discussed at the July 2018 TC39 meeting.

In my view, a critical factor is that decorators on private class elements make privacy more usable in practice, by giving the flexibility to meet critical use cases. In the end, adding the ability to decorate private class elements means that the ecosystem will be better able to adopt private fields. I expect that, over time, limited, trusted and well-maintained decorator libraries will emerge which maintain good properties, the way we have limited, trusted libraries for membranes.

At the July 2018 meeting, my understanding of the committee's response was widespread support for decorating private class elements, even understanding that this means giving the decorator these abilities. Most of the discussion was focused on how to make sure the private key does not accidentally leak to other code outside of the decorator; work is ongoing to make PrivateName a defensible class to achieve that goal.

Sounds like, for Stage 3, we should reaffirm that the committee wants to take this trade-off.

[allenwb]

@jridgewell I agree that a Private Name object that lacked read/write capability and that was scoped to the dynamic extend of a ClassDefinitionEvauation would be of less concern.

But I don't see how a design that is limited in that way could support your use cases. For example, a decorator that generator an accessor property for a private name would have to be able to return an element descriptor that contained get and set functions that access the private names. They would look something like:

const { key } = descriptor;
const privateName = privateNameReifier(key);
    ...
return {
   ...
    descriptor: {
      get() { return privateName.get(this); },
      set(value) {
        privateName.set(this, value);
      },
  ...
}

The read/write capability of privateName couldn't be limited to the dynamic extent of a ClassDefinitionEvauation because the get/set accessor need to run on instantiated instances of the class. I haven't been able to think of a scheme that would let you write such accessor functions that could not also be used to leak the privateName read/write capability in an arbitrary manner.

But, if @tracked can't create and install a reified PrivateName onto the Comp class, we're back to using a _count (or a new WeakMap instance, but that won't be hard encapsulated).

I believe, read/write capability is the actual issue, create and install may be doable.

...or a Symbol based soft private field. And you're correct that isn't hard encapsulated. But neither are the current private fields in the presences of unverified decorators of the sort allowed by the current proposal. You need to either audit all third party decorators that you intend to use or simply never use a third party decorator on a class where you need hard encapsulation. I can imagine a linter rule: decorator used in a class that defines private fields. Soft encapsulation using Symbol keyed properties is a fine alternative if you don't actually need extreme hard encapsulation.

@littledan

At the July 2018 meeting, my understanding of the committee's response was widespread support for decorating private class elements, even understanding that this means giving the decorator these abilities.

If you are talking about the presentation where both @waldemarhorwat and I raised our concerns then I don't think you can characterize that as TC39 making an informed decision. I wasn't clear to me that the majority of the attendees understood the issues or trade-offs involved.

At the very least it seems like you need to have a serious security review of this feature that includes developing threat models for things like Trojan horse decorators. Then make sure TC39 actually understands the results of that review and has consensus on accepting any threats it identifies

[littledan]

Maybe I misunderstood--i though @waldemarhorwat was advocating for a defensible class design. I've attempted to take that feedback into account in https://github.com/tc39/proposal-decorators/pull/134 .

Could you tell me more about what you'd like to see in a security review? I tried to raise these issues in the July 2018 TC39 meeting; it would be helpful to understand what was missing from that presentation.

[littledan]

@jridgewell What do you think of @rbuckton's proposal in this area? It seems largely similar to yours, mostly differing in what is the default. Do you see your version as providing more protection?

[jridgewell]

The read/write capability of privateName couldn't be limited to the dynamic extent of a ClassDefinitionEvauation because the get/set accessor need to run on instantiated instances of the class.

In my example, the reifier function is only lives for the ClassDefinitionEvauation. Ie, you may only reify a PrivateNameKey to a PrivateName while your decorator is running. But anything that's reified lives on afterwards, so privateName instance can be called later.

let key;
let reifier;
let privateName;
function dec(descriptor, privateNameReifier) {
  // Let's extract these to demonstrate lifetimes
  key = descriptor.key;
  reifier = privateNameReifier;
  privateName = privateNameReifier(key);

  return {
    descriptor: {
      get() { return privateName.get(this); },
      set(value) {
        privateName.set(this, value);
      },
  };
}

class Ex {
  @dec
  count = 1;
}

// And after `ClassDefinitionEvauation`:

assert(privateName.get(new Ex) === 1);
assert.throws(() => {
  reifier(key) // throws TypeError
});
// Key could be installed onto another class,
// but its a lot of code to demonstrate

But neither are the current private fields in the presences of unverified decorators of the sort allowed by the current proposal. You need to either audit all third party decorators that you intend to use

That's my intention with the reifier, to make any accesses to PrivateName very explicit. If I see a call to the second param of a decorator, I know its return value is a privileged privateName. It also means I only have to track what uses that binding, not the entire descriptor.

It seems largely similar to yours, mostly differing in what is the default. Do you see your version as providing more protection?

There's a 4 comment back-and-forth that explains my position:

1. First, @rbuckton's original comment (privileged descriptor key by default, restricted method to make a key)
2. My response that mens every decorator will need to be defensive, even if it doesn't touch private state.
3. @rbuckton's response that a redact function could be used when defining the decorator call (@redact(counter) class X { #x = 1 })
4. My response that it's damn near impossible to securely write redact.

I think privileged-by-default makes things much less secure than unprivileged. Although I do really like the idea of a redact helper function in my class source text, making it explicit that I don't really trust the decorator I'm passing it.

[littledan]

@jridgewell I don't understand the proposal in a couple ways:

• It sounds like this idea, by design, would only protect against accidental leaks, and deliberate leaks would still be permitted. So decorators will still need to be considered trusted in some sense. Is that right?
• Would this reifier function only be able to reify the PrivateName(s) which are exposed to that particular decorator, or would it be able to reify any private name at all in the time period when it is valid? In the latter case, a leak is still possible.

All together, this seems like a big change and erognomics regression to prevent certain kinds of accidental leaks. I wonder, what if we mitigated the issue by publishing a correct version of the redact decorator in npm, and considered adding it to a future decorator standard library?

[littledan]

I have a feeling that if we try to use a language-based mechanism to restrict the use of PrivateNames, beyond using defensible classes and solving for certain use cases like @rbuckton's proposal, it will be a never-ending battle as more cases of passing around PrivateNames are discussed.

I'm not totally convinced by the idea here that adding a field isn't a meaningful operation in terms of privacy. It may be less significant, but private names may also be used for "secure" brand checks, so it's not nothing to be able to falsify these.

By including decorators on private names, we are deciding to give developers the power and control over whether they expose these names to others, one way or another. As @allenwb says above, it is important to think carefully about whether we want to provide this power.

[jridgewell]

It sounds like this idea, by design, would only protect against accidental leaks, and deliberate leaks would still be permitted. So decorators will still need to be considered trusted in some sense. Is that right?

Correct. From the @tracked use case above, I think deliberate "leaks" (really no more of a leak than the explicit exposure in https://github.com/tc39/proposal-decorators/issues/133#issuecomment-408626263) are necessary for common use cases.

Would this reifier function only be able to reify the PrivateName(s) which are exposed to that particular decorator, or would it be able to reify any private name at all in the time period when it is valid?

The think the reifier would be valid for any syntatic private fields in the class body. We could decide instead to be scoped down to only the private field if you're just decorating that. But definitely reifiers are not the same function for every class decoration, eg. I can't extract a private name using a reifier from a previous class:

let reifier;
function extractReifier(_, privateNameReifier) {
  reifier = privateNameReifier;
}
@extractReifier class Extractor { #x = 1 };

// reifier should not have access to later class' privates:
function trojan(descriptor) {
  // notice using old reifier
  assert.throws(() => {
    const extracted = reifier(descriptor.key);
  });
}
@trojan class Ex { #x = 2 }

I wonder, what if we mitigated the issue by publishing a correct version of the redact decorator in npm, and considered adding it to a future decorator standard library?

Potentially. I am a fan of its explicit "I don't trust this decorator" source text, but I feel like it's a really weird solution to force every end user to use in their code. It's kind of like, if the decorator isn't explicitly dealing with a private field, it should always include this redact. My reifier makes that the default (and also makes the redact function simpler -- at the end of the comment), so nothing has to worry about securing private fields unless it explicitly deals with them.

it will be a never-ending battle as more cases of passing around PrivateNames are discussed.

Likely. From my original 3-level security comment, I'm only going to fight over L1 leaks (I think it's absolute critical that we secure at least that). It seems @allenwb and @waldemarhorwat have a little apprehension on allowing these L2 leaks.

[rbuckton]

Perhaps, rather than an @redacted decorator we might consider a contextual keyword or scoping operator to grant explicit trust to a specific decorator, like an @trusted: someDecorator, where only trusted decorators can see existing private members on a class:

@decoratorThatCannotSeeX
@trusted: decoratorThatCanSeeX
class C {
  #x
} 

[littledan]

@jridgewell

My reifier makes that the default (and also makes the redact function simpler -- at the end of the comment), so nothing has to worry about securing private fields unless it explicitly deals with them.

This is the part that I don't understand. I think a @redacted decorator would still be useful with @jridgewell 's proposal, as the reify proposal just protects against accidental leaks, whereas @redacted makes all private names omitted.

@rbuckton This is an interesting idea. We could say, if you decorate a private method or private field, you're clearly syntactically giving it trust, but if you decorate a class, it could go either way. So omitting the trusted: prefix on a class decorator filters out private class elements, but on element decorators the prefix has no effect.

If we want to go down this route, I'd say that we could leave trusted: class decorators for a v2 proposal, and our v1 proposal would be, just omit private class elements from class decorators, though allow them to be added. How would that sound? A bit disappointing--we'd have worse ergonomics both in the short and long term--but it's the more conservative option to respond to the security concern.

To make it concrete, rather than a @testable decorator for the whole class, you'd have to annotate each individual field or method as @testable. I'm not sure how bad this would be.

[littledan]

@allenwb @waldemarhorwat @jridgewell It would be great to hear from you if you feel that #136 addresses the problem, or if more changes are needed.

[littledan]

We ended up merging https://github.com/tc39/proposal-decorators/pull/151 to address this issue. However, issues later appeared, and I'd like to reconsider this decision. Apologies for the wall of text that follows--this is a letter I sent to @erights' SES mailing list to explain the rationale. I would be interested in your thoughts here as well.

---

Hi SES folks,

tl;dr: Should we go back and let class decorators access private fields and methods again?

In the SES review of the Decorators proposal, the big actionable recommendation we got out of the group was to ensure that class decorators don't see private fields and private methods, as some class decorators will not make use of private fields and methods, so the grant of authority could be surprising to developers who first encounter decorators which don't use that visiblity, and later switch to decorators which do. Discussion: https://github.com/tc39/proposal-decorators/issues/133 PR: https://github.com/tc39/proposal-decorators/pull/151

Thinking it through a bit more, we realized that it's not really possible to separate out public and private class elements in the elements array: If you filter the private class elements out, apply the class decorator, and then tack the private class elements back on the end, you'll be changing the evaluation order for private field initializers. Issue: https://github.com/tc39/proposal-decorators/issues/183 . A few different solutions have been drafted out, but the only one that seems really viable to me is saying that class decorators don't get any class elements; they only have the ability to append extras. (See the PR https://github.com/tc39/proposal-decorators/pull/184 for our best attempt at a midway solution; this doesn't feel right at all to me.)

It turns out, though, that there are very important use cases for both public and private class elements to be visible, and this topic has been a big source of feedback from the community of people who are experimenting with the draft decorators proposal in transpilers:

• Some thoughts on the use of public elements in class decorators for a custom elements system: https://github.com/tc39/proposal-decorators/issues/192
• Concern about, more or less, the fatally bad ergonomics of having to annotate private each element in order to get some behavior. https://github.com/tc39/proposal-decorators/issues/191
• As we discussed in the call, it could be a real improvement to ergonomics to allow a class decorator to make all private elements available to debugging and testing; such a usability improvement could increase the usability of private fields and methods.

Note that other programming languages with strong encapsulation guarantees and memory safety permit access to private from their decorator-like constructs, for example Rust's custom derive macros https://doc.rust-lang.org/book/ch19-06-macros.html#how-to-write-a-custom-derive-macro (and the macros here are for exactly the same kinds of things we want to do in JS, e.g., deposing debug information). Maybe we could ask Rust folks if they have ever seen this feature lead to misplaced expectations.

It seems to me that, after a thorough investigation, we have determined that seeing the entire array of class elements, including private elements, actually is an inseparable part of the authority that needs to be granted to decorators for their core use cases, and therefore should be included in a proposal that tries to grant minimal authority.

I think we can make granting this additional power acceptable through strong preparation of educational materials that make this grant of authority clear--making sure that, in both introductory and advanced materials. We've been strengthening our collaboration between TC39 and authors of educational material through the educators outreach group and the direct participation of developer relations experts in TC39, so I think this is achievable.

Thinking a bit broader, I'd like to encourage the mental model of decorators to be, the decorator has the right to mess around with all of the code which is lexically contained inside the element following the decorator. I've been thinking about how decorator-like syntax (which avoids the grammar mess of contextual keywords) could be used for more different kinds of needs for syntax that modifies things (explainer to come). One example, instead of a string-based pragma for Function.prototype.toString censorship https://github.com/domenic/proposal-function-prototype-tostring-censorship/issues/13 . So, I'd like us to consider whether we can think of decorators as something which potentially has deep, not shallow, impact; from this perspective, it's completely unsurprising that class decorators can see private fields and methods.

Thoughts? Dan

[littledan]

At this point, I see two feasible alternatives:

• 197 : Include access to private names in class decorators, and do not restrict their scope

• 203 : Exclude all elements in class decorators (including public), and only permit class decorators to add extras

I prefer that we include elements, as we have a number of interesting use cases presented, and it makes sense for the class decorator to have a somewhat "deep" view into the class.

[littledan]

See related issue https://github.com/tc39/proposal-decorators/issues/191 .

[littledan]

Private name values have been removed from the current draft, so I'm closing this issue.