PrivateName leak in decorators #24
Comments
|
I don't quite understand. PrivateName in decorators is designed in order to leak through decorators that you chose to call. This is the basis for decorators implementing friends/protected. Are you worried about running untrusted decorators? |
|
@littledan, if I understand the example correctly, the desire is to be able to write a decorator which you will pass to untrusted code such that the decorator can install a particular shared private field (and possibly also methods referring to that field) on classes but which does not give the untrusted code the ability to access that field. As currently designed, if a decorator installs a shared private field, granting someone the ability to call that decorator also grants them arbitrary access to that private field on any object which has it. In principle, these could be separated, I believe. I haven't thought about whether it's worth doing, but I think that's the problem statement. |
|
Should there be two types of decorators, one of which is untrusted and one of which is trusted, with different syntax? Or should we do protected/friends some other way? |
|
I don't think the solution is different kinds of decorators, as that will only complicate matters. I'm proposing separating the identity of the private name from the ability to read and write to it. This doesn't prevent a malicious user from installing the private name on another object using the opaque |
|
I need to look that over tomorrow, but I think it's still feasible. |
|
Thanks for taking a look. |
|
I investigated the "friend class" support in your example and here's what I came up with: https://gist.github.com/rbuckton/52e2078566fe4d8915f0f35c22a2dd10 I also took a look at "protected" state: https://gist.github.com/rbuckton/41bb58736874217880d0894f2a969a14 I don't think |
You're completely right that this decorator doesn't solve the issue of how to distribute the friend key. I think it's not as bad as "they have to be in the same file", though--I think plenty of user module systems could distribute the key to some places but not every place. It's true that ESM doesn't have any concept of package private, or any other ways to restrict who you export things to, but friends is not the first place this comes up. I'm a little wary of the imperative mechanism you use in your gist. It could lend itself to strange states where multiple packages are competing to be the first one to get the friendship, and module loading order is a bit complicated, so the top level would somehow indirectly get to choose between then. How do you know the good guy comes first? Any other ideas, besides "who gets there first", to lock down friend access?
Oops, I didn't think about protected super calls! I bet it could be worked out, but I'd have to think about it more.
I'd be really interested in hearing your proposal in more detail here. I'm a little embarrassed--I thought we could solve this problem with decorators, and this was part of how the committee decided to move forward with just private. I definitely think we should work this out before moving too fast on decorators--the current decorator design is based on friends and protected as motivating use cases for certain aspects of their design. |
In C++, it is the responsibility of a class to declare the friends that are allowed to see its private state, e.g.: class A {
friend class B;
private:
int x;
};
class B {
public:
B(A a) {
a.x = 0;
}
};To expand on my example, if you want I wrote up my early thoughts for |
Is the use case here "base class defines a protected method and and a public method which refers to it, and derived classes can override the protected method so that the inherited public method behaves differently"? If so, I too am confident you can make that work using the "pair of decorators" approach, although you still wouldn't be able to use To allow referring to a superclass's protected methods in a subclass which overrides them, one straightforward (and also otherwise useful) solution is to make a version of class Base {
@protected m(){
return 0;
}
}
class Derived extends Base {
@inheritFrom('m') #superM;
constructor() {
super();
this.#superM() === 0; // true
}
}Then an overriding method can just refer to the aliased method, rather than |
|
@rbuckton A difference in C++ is that it lets you forward-declare classes, which isn't available in JS. This allows for mutual friendship. |
|
This is an concern that I noticed a few weeks ago and mentioned to @erights when we were both at the splash conference. Note with the current design I can easily imagine somebody writing a trojan decorator that was specifically designed to expose access to some private fields. I'm keeping this short right now. But I don't think it is necessary (and actually very undesirable) to make PrivateName a new kind to primitive value. A safer approach, is for private fields that be exposed to decorators as thunks (or unique objects) that are only usable while a specific class definition (or perhaps invocation of a decorator) is being process. Once that definition is finished those values become useless. I can fill in more details. But the current approach really is not good from a hard private perspective. |
|
Looking forward to the details! It is a little hard for me to picture. |
|
To get back to the original question, about leaking the capability to read/write fields: @rbuckton and I discussed this issue further offline, where he patiently re-explained the problem to me again which @bakkot had immediately understood above (thanks!). As explained above, it's a deliberate design decision to expose this capability to other decorators. However, I can see the logic in doing the split in capabilities that you're suggesting. The patch #18 does not do this sort of capabilities split--the get/set functions that are exposed allow reading or writing for any private name. But we could do this in some other sorts of follow-on proposal which would look something like this: There could be another function which takes a PrivateName and outputs a PrivateName with fewer capabilities. This sort of "poor PrivateName" would be able to be used in element descriptors in decorators, but would throw a TypeError when passed into the I think we could add "poor PrivateNames" in a follow-on proposal, since
Any thoughts? |
|
I do like the change proposed, but I think this is fine personally since I expect to have to guard decorators from leaking to untrusted code. |
|
@bmeck I'm not sure what you mean by leaking, and what is possible to do to guard against it in the current proposal. Are you referring to #24 (comment) ? |
|
@littledan the decorator's reference can be used to expose the PrivateName instances it would add like @rbuckton showed by invoking it. I personally think it is fine to need to prevent people from getting a hold of my decorator reference. I don't think it can be guarded against fully in terms of key exposure, the rewrite in #24 (comment) does at least guard the values it stores on Objects. |
I don't understand how we could prevent people from getting a hold of the decorator reference without preventing them from using the decorator in their class declaration. |
|
@littledan You would not give people a direct reference to my decorator, you would need to use some form of a mixin or other behavior to apply it: function mixinAllocationTrace(parent = Object) {
@trackAllocation
class Child extends parent {}
return Child;
}class Tracked extends mixinAllocationTrace() {
} |
|
I'm not sure why you'd bother using a decorator there, rather than manually defining the private field and constructor in that mixin. |
|
@littledan I probably wouldn't, just explaining how to do it. Decorators would probably only be rarely exposed to untrusted sources for me right now since it does grant ways to get private fields. |
|
@bmeck Are you talking about the class definition not trusting the decorator, or the decorator not trusting the class definition? I believe the leak @rbuckton is talking about is more the decorator not trusting the class definition, but the fact that decorators grant access to private fields has to do more with a class definition that might not trust its decorator. |
|
@littledan I'm not really talking about |
|
@bmeck OK, I see, you're talking about the original issue and it was a misunderstanding on my part. It sounds like you're saying, until this v2 feature is added, you'll be hesitant to use decorators for very much. Is that right? |
|
@littledan I think there might be some confusion. I plan on using decorators within trusted code quite a bit; if there is no private data I plan on leaving exposing decorators to untrusted code probably; however, when there is private data I don't plan on exposing them to untrusted code. There are ways to work around things with initializers causing WeakMap entries to be created, but I probably would not do that. So... I still plan on using them quite a bit, just not for private data. |
|
And when you say "when there is private data", I assume you mean not all of the times when you'd have a decorator that uses a private name, just when there's a decorator that uses a single shared private name across multiple things? (Presumably, in that case, you could still use a decorator, you would just back the data in a WeakMap rather than a private field, right?) |
|
@littledan unclear to me at this point, but I would probably just use WeakMaps instead of decorators for private data (using the term "data" to avoid talking about any specific mechanism such as closures/PrivateName/WeakMap) for now at least. |
|
Note that it is a very particular case where WeakMaps would be needed--only when there is a family of decorators which are working together to store metadata related to the same class. If it's a single decorator which works by itself, you can probably avoid leaks by using a new private name each time it is called (e.g. the @observed decorator in metaprogramming.md) |
|
I'm going to ignore the Bradley-Daniel exchanges above, because I'm not sure it's on topic for this issue. @rbuckton's revised sample code is just one example of Level 2 leakage ("Privilege of the descriptor's key"). @nicolo-ribaudo brought up another case of this in #130. His illustrates that it's not just me writing a privacy-minded decorator that has to be careful of the APIs I use, but any decorator. Take these two well-intentioned uses: @counter
class X {
#prop = 1;
}
class X {
@counter
#method() {};
}Here, The way to fix it, as @rbuckton pointed out, is to not give the // Psuedocode
class PrivateName {
key = new %PrivateNameKey%(this);
get() {}
set() {}
}
class PrivateNameKey {
#privateName;
constructor(priv) {
this.#privateName = key;
}
}
function privateDecorator(descriptor, privateNameGetter) {
const { key } = descriptor;
// privateNameGetter can only be used to access syntatic privates
// that are needed for this decorator invocation.
const priv = privateNameGetter(key);
}
class X {
// privateDecorator needs read/write access
// but only to #prop, not #other.
@privateDecorator
#prop = 1;
// privateNameGetter should not be able to access #other's read/write.
#other = 2;
} |
|
@jridgewell One solution we came up with was this: Assume private name looks something like this: class PrivateName {
[[PrivateNameKey]] = ...
get(obj) { … }
set(obj, value) { … }
}Any private names on a class provided to a decorator will be an instance of class PrivateName {
[[PrivateNameKey]] = ...
[[PrivateNamePrivilege]] = "full"
get(obj) {
// spec-ish pseudo-code
if (this.[[PrivateNamePrivilege]] !== "full") throw new TypeError();
...
}
set(obj, value) {
// spec-ish pseudo-code
if (this.[[PrivateNamePrivilege]] !== "full") throw new TypeError();
...
}
static restricted(key) {
// spec-ish pseudo-code
if (!([[PrivateNameKey]] in key) throw new TypeError();
let restrictedKey = ObjectCreate(%PrivateNamePrototype%, « [[PrivateNameKey]], [[PrivateNamePrivilege]] »);
restrictedKey.[[PrivateNameKey]] = key.[[PrivateNameKey]];
restrictedKey.[[PrivateNamePrivilege]] = "restricted";
return restrictedKey;
}
}Then your decorator can ensure there is no leak by doing this: const fullPrivilegePrivateName = new PrivateName();
export function myDecorator(descriptor) {
const reducedPrivilegePrivateName = PrivateName.restricted(fullPrivilegePrivateName);
descriptor.extras = [{
key: reducedPrivilegePrivateName, // define using the reduced privilege private name
...
}];
}
export function myPrivateNameReader(obj) {
// local access to the full privilege
const value = fullPrivilegePrivateName.get(obj);
} |
|
That's a good step, but doesn't address the good actors who don't need to use private fields. For the function counter(classDescriptor) {
// Need to be careful about _anything_ I call with classDescriptor.
// It contains a privileged private name, even though my decorator
// has nothing to do with privacy.
}
@counter
class X {
#leaked = 1;
} |
|
I thought we already made the determination that any time you put a decorator on a class, you are explicitly opting into allowing privileged access access to all declared privates. You can work around this by creating a pass-through decorator that redacts private state: function redact(decorator) {
return function (descriptor) {
const privates = descriptor.elements.filter(...)
descriptor.elements = descriptor.elements.filter(...);
descriptor = decorator(descriptor) || descriptor;
descriptor.elements = descriptor.elements.concat(privates);
return descriptor;
}
}
@redact(counter)
class X {
#notLeaked = 1;
}You currently can't work around the issue in the description above. |
But the redactor leaks in 4 separates spots, and it's damn near impossible to write one that doesn't. function redact(decorator) {
return function (descriptor) {
const privates = descriptor.elements.filter(...); // Leak
descriptor.elements = descriptor.elements.filter(...); // Leak
descriptor = decorator(descriptor) || descriptor; // Leak
descriptor.elements = descriptor.elements.concat(privates); // Leak
return descriptor;
}
}Fully privileged by default means I have to very carefully restrict access beforehand: function redact(decorator) {
return function (descriptor) {
const { elements } = descriptor;
// Can't use an array here, has to be a prototype-less object.
// Hope Object.create wasn't monkied to return a proxy.
const privates = Object.create(null);
// Can't use for of, cause that'll use iterator API
for (let i = 0; i < elements.length; i++) {
const element = elements[i];
const { key } = elements[i];
if (typeof key === "object") { // Don't use instanceof
// Store the privileged key
privates[i] = key;
// restrict access
elements.key = restricted(key);
}
}
// Hope return value isn't a Proxy
const ret = decorator(descriptor);
if (ret) {
// Need to deep clone ret to make sure nothing's a proxy.
descriptor = ret;
}
for (const index in privates) { // Don't use Object.keys
elements[index].key = privates[index];
}
return descriptor;
}
}Compare with this a unprivileged by default and a getter function: function redact(decorator) {
return function (descriptor, privateGetter) {
// Just don't pass privateGetter.
// Real invocations of the other decorators will receive it.
return decorator(descriptor);
}
} |
|
@jridgewell I don't think the kind of protection you're getting at is compatible with supporting the features that this proposal attempts to support. Ultimately, if we do give decorators the power to provide access to private class elements outside the class (which has many use cases documented in this repository), decorator authors will have to take some steps to not give everyone that access. @rbuckton This is my understanding as well. |
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; } ```
|
I was thinking about this more in depth, and there are actually two attack vectors we need to guard against:
The mitigation mentioned above only solves (1), but does not solve (2). Here's an example of the attack vector: // dec.js
const fullPrivilegePrivateName = new PrivateName();
export function myDecorator(descriptor) {
const reducedPrivilegePrivateName = PrivateName.restricted(fullPrivilegePrivateName);
descriptor.extras = [{
key: reducedPrivilegePrivateName, // define using the reduced privilege private name
...
}];
}
export function myPrivateNameReader(obj) {
// local access to the full privilege
const value = fullPrivilegePrivateName.get(obj);
}
// evil.js
import { myDecorator, myPrivateNameReader } from "./dec"
// leak "restricted" private name
const memberDescriptor = {};
decorator()(memberDescriptor);
const privateStateDefineOnly = memberDescriptor.extras[0].key; // this is the restricted version
// Masquerade as the expected object with the data we want to inject.
class M {
@(descriptor => {
descriptor.extras = [{
kind: "field",
placement: "instance",
key: privateStateDefineOnly,
initializer: () => "whatever data we want to inject"
}];
})
x;
}
const obj = new M();
myPrivateNameReader(obj);The problem is that we need to restrict the private name not only for get/set but also for define. We can mitigate this by having the runtime provide a unique "access token" Symbol to the decorator that we entangle with the restricted private name. This ensures that the private name can only be used on that declaration: // dec.js
const fullPrivilegePrivateName = new PrivateName();
export function myDecorator(descriptor, accessToken) {
const reducedPrivilegePrivateName = PrivateName.restricted(fullPrivilegePrivateName, accessToken);
descriptor.extras = [{
key: reducedPrivilegePrivateName, // define using the reduced privilege private name
...
}];
}
export function myPrivateNameReader(obj) {
// local access to the full privilege
const value = fullPrivilegePrivateName.get(obj);
} |
|
@rbuckton: The reifier that I discussed above and in #133 (comment) also solve both issues. |
|
@jridgewell: Can you elaborate? I can see how it can help in the case where your decorator receives the private members of a class, but not in the case where your decorator needs to install its own private state on a class and share it with another function in the same scope (which is the crux of this issue). |
|
The extraction method doesn't make sense with the reifier: const memberDescriptor = {};
decorator()(memberDescriptor);You would have to provide a reifier function to the extraction here, and any userland function you provide it won't be able to extract a local variable in some other module. You can't spoof a real reifier. |
|
How is a new private name created in this scenario? This also doesn't seem to solve the masquerade issue, if they can just do this: const memberDescriptor = {};
let defineOnlyPrivateName;
try {
decorator()(memberDescriptor, (name) => {
defineOnlyPrivateName = name;
});
} catch (e) {}
// now I can masquerade as an object with this decorator, and install a field
// initializer to inject data |
|
Ha, you're right. Never mind. |
|
The only downside of a const restrictPrivateName = PrivateName.restricted;
...However, this is a known downside of developing secure code in JavaScript today anyways. |
|
Decorators for private fields and methods can still access that field or method. However, there is no private name concept any more. I think this removes the need for any kind of "restricted" private name. |
The current semantics for the
PrivateNameObject can result in decorators that leak shared customPrivateNamevalues, breaking "hard privacy". For example:One possible approach to avoid this is to break
PrivateNameinto two parts: a property "mutator" and an opaque "key". For example:Then the decorator above could be rewritten:
While this still results in a key that could be added to any object, it prevents an untrusted third party from reading or writing to that state.
The text was updated successfully, but these errors were encountered: