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

Is confused deputy attacks are really harmful? #12

Open
Igmat opened this issue Jan 10, 2019 · 4 comments
Open

Is confused deputy attacks are really harmful? #12

Igmat opened this issue Jan 10, 2019 · 4 comments

Comments

@Igmat
Copy link

Igmat commented Jan 10, 2019

I do. In the long thread at tc39/proposal-class-fields#183 (comment) I finally noticed that all of the private symbol proposals still suffer from fatal confused deputy attacks. I don't think this direction can work at all.
Originally posted by @erights in #11 (comment)

@Igmat
Copy link
Author

Igmat commented Jan 10, 2019

I seen this quite a few time mentioned as problem of Symbol.private approach. @erights could you, please, clarify why do you insist that ability to install privates to another object is problem and not additional possibility?

Because, I see this as replacement for a lot of WeakMap usages rather then language design problem. Consider this:

const someRelations = new WeakMap();

class A {
    set relation(obj) {
        someRelations.set(this, obj);
        someRelations.set(obj, this);
    }
    get relation(obj) {
        return someRelations.get(this);
    }
}

Replaced with:

const someRelation = Symbol.private();

class A {
    set relation(obj) {
        this[someRelation] = obj;
        obj[someRelation] = this;
    }
    get relation(obj) {
        return this[someRelation];
    }
}

And lets imagine shorthand syntax for Symbols:

class A {
    private #someRelation; // creates `Symbol.private` referenced by `#someRelation` constant
                           // and defines `#someRelation` property on `A` instances
    set relation(obj) {
        this.#someRelation = obj;
        obj.#someRelation = this;
    }
    get relation(obj) {
        return this.#someRelation;
    }
}

Isn't it looks nice? There are a lot of other use-cases (especially in game development), where we want to install some privates to other objects. And it's Proxy safe out of the box.

@bathos-wistia
Copy link

problem and not additional possibility?

I think it can be both. Sometimes you want to be able to initialize late, sometimes you don’t. It’s only when you don’t that it becomes “confused deputy.”

In the PNLSOWNSF take, late* initialization appears to be served. My interpretation of the objection is that private symbols are considered to make unintended late initialization into a footgun. Personally, I do continue to prefer private symbols, but I thought PNLSOWNSF presented an interesting compromise: minor concession in ergonomics for the private symbol camp, but with their needs still served; no concession for the current private fields proposal.

* In practice, late initialization usually doesn’t mean “arbitrary objects at a later time” (in my experience anyway), so the phrase might be misleading. Rather it’s usually meant things like bootstrapping instances of publicly unconstructable classes and sharing common private contracts across objects without common inheritance.

@littledan
Copy link

littledan commented Jan 10, 2019

I think the term "confused deputy attack" is a bit... confusing. Another way to describe these sorts of issues is, "Some code looks like it does something, but actually it does something else, as a result of interaction with a third thing."

Private fields and methods are designed to be reliable, and have semantics that you can predict by looking at a relatively small, local amount of code. The amount of code you have to examine to figure out what's going on is larger for private symbols than with the WeakMap semantics (but, in both cases, it's way smaller than using ordinary properties).

In general, interactions and reliability are is a serious thing to consider, but we won't land at the conclusion that we should prohibit that interaction every time. I understand and respect @erights' impulse to start from a default of discouraging or disallowing these interactions--something you could use the fancy acronym "Principle of Least Autority" (POLA) for. Maybe we do want to support the interactions, but we should understand why.

@Igmat
Copy link
Author

Igmat commented Jan 10, 2019

Maybe we do want to support the interactions, but we should understand why.

We, already talked about reasons behind allowing such interactions dozens of time, bu I'll shortly summarize main points:

  1. Expanding Proxy issue from relatively small amount of built-ins almost infinite amount of user's code, which will use encapsulation
  2. Such interactions make them property-like, so all issues (like private static, unwritable private methods and etc.) caused by WeakMap-like semantic goes away
  3. Dozens of use-cases for Symbols itself that may replace some existing WeakMap usages

@littledan, I know that you probably tired of discussion like this, but this statement kinda misleading

In general, interactions and reliability are is a serious thing to consider, but we won't land at the conclusion that we should prohibit that interaction every time.

I would love this to be true. And if it could be so, I would be ok with reliability by default. But in current state - it doesn't postpone interactions to future proposal or language design evolution, it restricts them fully.
By providing WeakMap-like semantic as default way of encapsulation, you just cut-off any future interactions. To elaborate this statement, lets imagine that someday in future we'll have some opportunity to provide such interactions:

  1. We don't want to make incompatible changes in language - so all existing code with privates works the same way as it worked before.
  2. To provide interaction we add new API (syntax, some built-in objects/functions or special decorator), that could be used by developer, to show that some particular private is allowed to interact
  3. I'm as library author that relies on such interactions go to the community and say

    You are able to use my cool library at last!

    But you have to rewrite WHOLE your project replacing/decorating all your privates with this new syntax/API

  4. Users NEVER adopt my library widely, because it is too verbose and there are a lot of places, where they just can't make privates not throw at TRANSPARENT proxies, because those privates exist at some another library they don't control, even though author's intent was encapsulation and not defensive programming.

So this is trade-off. From my perspective it looks like this:
Defense against:

class Foo {
  #state;
  ...
  setState(newState) {
    this.#state = newState;
  }
}

const f = new Foo();
const notAFoo = {};
f.setState.call(notAFoo);

By dropping:

const f = new Foo();
const notAFoo = new Proxy(f, {
    // some pretty complex relation tracking Proxy implementation, like this (very simplified) example
    // https://github.com/Igmat/metaf/blob/develop/packages/core/src/atom/index.ts
});
f.setState.call(notAFoo);

I don't understand why the first is so important, because:

  1. notAFoo can't catch/trap reified #state key, unless this key was shared BY INTENT
  2. code outside of Foo can't interact with #state, unless key was shared BY INTENT
  3. it guarantees only private shape, while public shape isn't less important
    • f.methodThatDoesntInteractWithPrivates.call(notAFoo); will need EXPLICIT brand-check anyway
  4. brand-check is easy
    class Foo {
        #brand = this;
        #checkBrand(obj = this) {
            if (obj.#brand !== this) throw 'Brand check failed';
        }
        #state;
    
        setState(newState) {
            this.#checkBrand(); // if you really need it
            this.#state = newState;
        }
    }
    • 3 additional rows for adding brand-check implementation and 1 call in methods that require it
    • if it's too much and repetitive, we may create some mechanism that adds brand-check everywhere (decorator probably, or some base built-in class, or even syntax if it is so required feature)

@erights, as far as I understand you're the main proponent of providing IMPLICIT brand-check by default (and @littledan was referring to you few times). Could you please clarify and/or answer to previous question? Is it possible for you to provide some really important case when brand-check is required (ideally from common developer perspective)?

P.S.

@erights, don't you see that your last example in your documentation behaves EXACTLY in same way as Symbol.private + syntax sugar (in your case it is ::, and in mine it is #).

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

No branches or pull requests

3 participants