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

Normative: Restrict the use of PrivateNames in class decorators #136

Closed
wants to merge 1 commit into from

Conversation

littledan
Copy link
Member

There's been some concern about the trust involved in decorating
private class elements. Decorating a private field or method is
specified to give access to the private name which is declared.
Although this notion of privacy may be defensible in theory, we
take a couple additional precautions to ensure that it's private
in practice:

Previously, the idea was that class decorators would be trusted,
enabling class decorators to be used to deliberately grant access to
private fields and methods to things outside of the class. For
example, a @testable decorator could expose all private class elements
to a testing framework.

The new idea is, class decorators are less trusted, and can only see
that private class elements are present, insert or delete them, but
not read or write them. This increases the level of privacy in
practice (e.g., if a decorator is used for wrapping purposes and not
expected to be able to trusted reading private fields), but removes
certain use cases.

This patch enables class decorators to add and remove private elements,
which may be considered to have integrity implications with respect
to usages of private class elements as a "brand". The trust level of
class decorators is intermediate: they are trusted with these branding
rights, but not with the ability to actually read and write everything.

In a follow-on proposal, we could permit class decorators to get
un-restricted private names through a @trusted: decorator syntax;
we could also decide to "relax" the restriction on all class
decorators.

The logic here could be used to implement "restricted" private fields
as proposed in #24 (comment)
with the following implementation:

function restricted(privateName) {
  let restrictedName;
  function classDecorator({[{key}]}) {
    restrictedName = key;
  }
  function elementDecorator(descriptor) {
    return {...descriptor, key: privateName};
  }
  @classDecorator class X {
    @elementDecorator x;
  }
  return restrictedName;
}

There's been some concern about the trust involved in decorating
private class elements. Decorating a private field or method is
specified to give access to the private name which is declared.
Although this notion of privacy may be defensible in theory, we
take a couple additional precautions to ensure that it's private
in practice:
- #134: PrivateName is a defensive class, which is deeply frozen and
  does not permit monkey patching.
- #133: Class decorators get "restricted" PrivateName instances,
  which cannot be used to read or write private class elements.

Previously, the idea was that class decorators would be trusted,
enabling class decorators to be used to deliberately grant access to
private fields and methods to things outside of the class. For
example, a @testable decorator could expose all private class elements
to a testing framework.

The new idea is, class decorators are less trusted, and can only see
that private class elements are present, insert or delete them, but
not read or write them. This increases the level of privacy in
practice (e.g., if a decorator is used for wrapping purposes and not
expected to be able to trusted reading private fields), but removes
certain use cases.

This patch enables class decorators to add and remove private elements,
which may be considered to have integrity implications with respect
to usages of private class elements as a "brand". The trust level of
class decorators is intermediate: they are trusted with these branding
rights, but not with the ability to actually read and write everything.

In a follow-on proposal, we could permit class decorators to get
un-restricted private names through a `@trusted: decorator` syntax;
we could also decide to "relax" the restriction on all class
decorators.

The logic here could be used to implement "restricted" private fields
as proposed in #24 (comment)
with the following implementation:

```js
function restricted(privateName) {
  let restrictedName;
  function classDecorator({[{key}]}) {
    restrictedName = key;
  }
  function elementDecorator(descriptor) {
    return {...descriptor, key: privateName};
  }
  @classDecorator class X {
    @elementDecorator x;
  }
  return restrictedName;
}
```
@@ -438,11 +438,12 @@ <h1>%PrivateName% ( )</h1>
</emu-clause>

<emu-clause id="sec-private-name-object" aoid=PrivateNameObject>
<h1>PrivateNameObject ( _name_ )</h1>
<h1>PrivateNameObject ( _name_, _restricted_ )</h1>
<p>When PrivateNameObject is called with Private Name _name_, the following steps are taken:</p>
Copy link
Member

Choose a reason for hiding this comment

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

"and Boolean _restricted_"? (and then should there be an Assert that restricted is a boolean, or is that implied?)

@@ -438,11 +438,12 @@ <h1>%PrivateName% ( )</h1>
</emu-clause>

<emu-clause id="sec-private-name-object" aoid=PrivateNameObject>
<h1>PrivateNameObject ( _name_ )</h1>
<h1>PrivateNameObject ( _name_, _restricted_ )</h1>
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it might be better to name this "privileged" instead of "restricted", so that behavior can default to the safe thing instead of to the unsafe thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you suggesting a change in runtime semantics, or just an editorial change?

Copy link
Member

Choose a reason for hiding this comment

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

An editorial change, to invert the default from "privileged" to "restricted".

Copy link
Member

Choose a reason for hiding this comment

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

The design guideline used in ES5 when designing property descriptors:

Names should say what is allowed rather than what is denied. APIs should follow the security best practice of "deny by default".

Applying that guideline to this API the second parameter might be called readWriteAllowed and should default to false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note, there is no public API proposed in this patch; we are talking about names for what goes on when decorators expose private fields.

How about getSetAllowed? Read/write would be sort of new terms.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that it isn't a public API, but it is still an API used by spec. writers and readers.

I don't really care what name you chose as long as it describes what setting it to true enables.

@allenwb
Copy link
Member

allenwb commented Aug 4, 2018

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

I'm afraid it really doesn't. The main problem I see is that a spoofed or Trojan horse decorator can be used as an attack vector to penetrate hard private encapsulation in some situations. If I understand this change, what it has done is narrowed but not eliminated that attack vector. Basically, it means that an attacker would now focus on identifying and attacking situations where high security private state is associated with an private field that has an element decorator. Oh, also branding use cases which I suspect will be a very common use of private fields.

A smaller security hole is still a security hole.

As a cautionary tale, consider that for much of the ES2015 development cycle we thought that Symbols would be a mechanism would support strong encapsulation use cases. We even had syntactic support (@NAMEs) in the spec. to enhance the usability of Symbol keyed properties. But then we realized Proxy provided a way to penetrate symbol based encapsulation and we couldn't come up with a way to close that hole while preserving the full functionality of Proxy that was needed for some of its use cases. So we backed away from the idea of Symbols proving strong encapsulation and deferred solving that problem to post ES2015. We didn't really change anything WRT Symbols (other than removing the @name connivance syntax) but stopped talking about them as solution for strong encapsulation.

Lexically scoped private names were then developed as a strong encapsulation (ie, "hard private") solution. And they work for that purpose until decorators (or some subset of decorators) are introduced that can expose (some) private fields to access that is not lexically constrained. In the presence of the possibility of such decorators, private fields do not provide "hard private" strong encapsulation. They are just "less soft" than Symbol based fields. If we continue on the current path, even with this revision, what is the solution for the use cases that actually needs truly "hard private"? In the next round do we add truly private fields (##name??) that are not exposed to decorators?

It seems to me we have only two alternatives ahead: 1) restrict the functionality of decorators to preserve the strong lexical encapsulation of private fields; or 2) stop taking about private fields as being "hard private" and start working on a new solution for that use case.

But here's the thing, we could be there today if we follow the first alternative. Soft private via Symbol keyed fields is good enough for encapsulation use cases that don't require impenetrable strong encapsulation. If a developer understands that decorating a private field potentially destroys its strong encapsulation then they should never decorate one. But if decorating them is allowed then sometimes private fields will be intentionally decorated when the author knows that only soft private is required. In that case, if you are reading code in a class definition and see an undecorated private field, how are you going to determine whether that field requires "hard private" and hence should never be decorated or really only needs to be "soft private" hence is ok to decorate.

I suggest that you stop trying to fix what is probably an unfixable conflict between strong encapsulation and the use of decorations, even as refined in this patch. Instead remove reification of private names from the decorator proposal and make it clear that decorators don't work with private fields. Start marketing symbol based fields as the encapsulation solution in all cases expect where impenetrable strong encapsulation is a requirement.

Regardless, do that security review focusing on ways that a decorator might be spoofed. Don't forget to consider the things that bundlers do with ES modules and how that may expanded the attack surface for spoofing decorators.

@jsg2021
Copy link

jsg2021 commented Aug 4, 2018

I think I would prefer educating that decorators would break hard private if used on a private field. But, I’d be okay with not allowing on private fields too.

@littledan
Copy link
Member Author

I wonder if the issue here is that we haven't adequately defined and documented the encapsulation guarantees. This patch makes a big change in the contract, but when I looked through the documents in this repository to see what to update, I couldn't find any. We've just been discussing the invariants in various issue threads. We should clearly address this; thanks for raising the point.

The design in this proposal achieves different goals from if we went with a public symbol-based design. In particular, class authors have to specifically opt into exposing private class elements, and then they are only exposed to particular decorators. I believe this will be significant in practice, that decorated classes will still be able to achieve strong encapsulation in more cases than they would be able to achieve with public symbols.

Maybe we need to figure out the right terminology to adopt to describe this particular invariant. "Hard private" might lead people to inappropriate conclusions, given that we are delegating trust to decorators.

@allenwb
Copy link
Member

allenwb commented Aug 4, 2018

I wonder if the issue here is that we haven't adequately defined and documented the encapsulation guarantees.

yes, it would be good to do so.

In particular, class authors have to specifically opt into exposing private class elements, and then they are only exposed to particular decorators.

Two concerns about that:

  1. are class authors really thinking about breaking strict encapsulation when they use a decorator that has some utility (or perhaps is required by a framework)
  2. the decorator spoofing/trojan issues

Here is an alternative design:

Change nothing about #name element declarations except that the lexically scoped name binding is to a Symbol rather than a private name. Such # element names are fully exposed to decorators and proxies. This is "soft private" it supports all of the soft private uses cases including meta level access.

Element declarations of the form ##name (or pick your syntax) use private names. Such element declarations can not be decorated and are not exposed to class declarators or proxies. This is "hard private" but it might be better to call a ##name a "secure name/secure element".

A class/framework designer gets to choose their poison either use a "private element" that may be decorated but may not be secure or use a "secure element" that can't be decorated but is guaranteed by the language to be inaccessible from outside of the lexical scope of the class definition.

@littledan
Copy link
Member Author

@allenwb See the past discussion about "soft private" at in GitHub and at TC39 meetings. Previously, we settled on strong encapsulation, with a principle that it would be analogous to WeakMap semantics. WeakMaps can be accessed by more than just one class body--they can be passed around flexibly, and decorators on private class elements behave analogously.

We've talked at a high level about including multiple sigils for various purposes, such as protected, hard vs soft private, etc. My intuition here is that multiple sigils will be pretty confusing. We don't want JS to devolve into punctuation soup. It's unfortunate that we need sigils at all. I think we should just choose a semantic and a single sigil, and further modes of functioning in objects could be accomplished through decorators or contextual keywords.

@rbuckton
Copy link
Collaborator

rbuckton commented Aug 4, 2018

Even if decorators don't have access to private state, private state still has security issues due to the often unrestricted nature of script execution. We have already discussed at length that hard privacy can already be overcome if you are able to mutate builtins (i.e. Array, Map, Set, etc.), unless the developer goes out of their way to save off builtins in advance. Using a decorator in any way implies the same trust as if the code was written directly as part of the class, especially since any decorator can replace the constructor (and replace it with one that can just capture anything you wanted to make "private" at a non-private boundary). The kind of security we need really need requires a trust model that goes beyond just decorators.

@rbuckton
Copy link
Collaborator

rbuckton commented Aug 4, 2018

To be honest, I see private state more of an encapsulation feature that's stronger than just _ prefixing a field, rather than a security feature given the lack of any kind of code access security or trust model, at least until we have something like frozen realms or module-scoped membranes that could get us to that point.

Developers that require secure private state will need to be highly defensive. Saving builtins is already common, so rolling their own @redacted decorator or wrapping untrusted code in a privilege-reducing membrane wouldn't be any different.

@littledan
Copy link
Member Author

@rbuckton The practical benefit of private state will likely be that for many programmers, agreed. At the same time, I think private state and its interaction with decorators stands up under an ocap-style analysis as a security feature from that perspective (with or without this patch).

littledan added a commit that referenced this pull request Aug 7, 2018
As discussed in #136, this is an important missing piece of documentation. cc @allenwb @erights
@littledan littledan mentioned this pull request Aug 7, 2018
@littledan
Copy link
Member Author

I don't think this approach will work to provide any reasonable guarantees. @rbuckton pointed out offline, this patch permits a class decorator to replace a private field with a private getter/setter pair, "leaking" the data. It seems like our options are actually, either permit full use of private from class decorators, or entirely filter them out.

@littledan littledan closed this Aug 8, 2018
littledan added a commit that referenced this pull request Aug 8, 2018
As discussed in #136, this is an important missing piece of documentation. cc @allenwb @erights
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.

None yet

5 participants