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

Zoe is gullible to issuer's allegation of brand #1378

Closed
erights opened this issue Aug 5, 2020 · 5 comments · Fixed by #1414
Closed

Zoe is gullible to issuer's allegation of brand #1378

erights opened this issue Aug 5, 2020 · 5 comments · Fixed by #1414
Labels
bug Something isn't working ERTP package: ERTP security Zoe package: Zoe

Comments

@erights
Copy link
Member

erights commented Aug 5, 2020

const brandP = E(issuer).getBrand();

together with
table.create(issuerRecord, brand);
issuerToBrand.init(issuer, brand);
issuersInProgress.delete(issuer);

enters a binding from the brand that the issuer alleges to an issuer record which includes the issuer. But a malicious issuer could report someone else's brand, disrupting Zoe's possible later attempt to register the true issuer of that brand. We just need to also check brand.isMyIssuer(issuer) to ensure they are mutually acceptable to each other. The pair may still misbehave, but they can't interfere with a later attempt to register the true owner of that brand.

There may be similar vulnerabilities elsewhere we ask an issuer for its brand without asking the brand's opinion.

@erights erights added bug Something isn't working ERTP package: ERTP security Zoe package: Zoe zoe-alpha-release labels Aug 5, 2020
@katelynsills
Copy link
Contributor

Yup, this is indeed a vulnerability. We use the issuerTable in a couple of places: the Zoe Service, the Contract Facet, and the wallet. We should check each of these places. I think the Zoe redesign PR is an appropriate place for this change.

@katelynsills
Copy link
Contributor

@erights does this address the vulnerability for at least the Zoe package? 4ec0070

@erights
Copy link
Member Author

erights commented Aug 5, 2020

Yes, that looks good.

@katelynsills
Copy link
Contributor

Fixed in agoric-sdk in #1414 but we should remain aware of this issue.

@katelynsills
Copy link
Contributor

Closed in #1443

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ERTP package: ERTP security Zoe package: Zoe
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants