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

Attestation secret is shared with DevCon #266

Open
jot2re opened this issue Jun 17, 2022 · 7 comments
Open

Attestation secret is shared with DevCon #266

jot2re opened this issue Jun 17, 2022 · 7 comments
Assignees
Labels
enhancement New feature or request Security A security issue that should be fixed, since it might allow for attacks.

Comments

@jot2re
Copy link
Collaborator

jot2re commented Jun 17, 2022

See section 2.1.1 in the Token-negotiator report.
We note that while not pointed out in the Token-negotiator report, we believe that the problem can be fixed having attestation.id produce a special zero-knowledge proof, which can be validated by DevCon and combined with a zero-knowledge proof the ticket secret by DevCon. The combined zero-knowledge proof can then be verified by Hotel Bogota.

See this Jira issue.

@jot2re jot2re added enhancement New feature or request Security A security issue that should be fixed, since it might allow for attacks. labels Jun 17, 2022
@jot2re jot2re self-assigned this Jun 17, 2022
@oleggrib
Copy link
Collaborator

Looks like its same level of security when Attestation secret or specialZPK shared with DevCon and DevCon and use this value to create email ZPK.
so "secret level of security" = "specialZPK level of security", isn't it?

@jot2re
Copy link
Collaborator Author

jot2re commented Jun 20, 2022

Yes, issues is that DevCon front end learns both attestation secret and ticket secret. This is not a huge issue. It is an issue we previously accepted as being needed a needed evil with our design. However I realised there is a way to prevent it.
This issue is about implementing the fix, if @weiwu-zhang thinks it is worth putting in the effort to handle.
(The expected time is not much, but the change must be done both in front end and back end)

@oleggrib
Copy link
Collaborator

So we afraid that DevCon_front has secrets , which it use to create ZKP.
what the difference if DevCon_front get some other values and can use it to create ZKP?

are those new items limited in time? @jot2re

@jot2re
Copy link
Collaborator Author

jot2re commented Jun 28, 2022

The issue is that if DevCon front learns the attestation secret, which means it could use the attestation in other settings (if they exist).
My thought about this suggestion was that the ZKP would be linked to a specific context such that the partial ZKP DevCon front received from the Attestation iframe cannot be used in other settings than the current proof. Hence protecting against misuse by a malicious DevCon front.

@oleggrib
Copy link
Collaborator

@jot2re , If DevCon front page will be hacked then it can send any context request to the attestation.id and create any proof.

My suggestion is:

  1. Add confirmation request to the attestation.id to share emailAttestation and secret with specific page, so user have to obviously confirm attestation share.
  2. share some kind of secret-ZKP, limited in time for 1 day or few hours

@jot2re
Copy link
Collaborator Author

jot2re commented Jun 30, 2022

@oleggrib

  1. Yes, I think that would be great as a minimal fix.
  2. Yes, that makes sense too. Although I think that making that work might be more complex than adapting the ZKP procedure as suggested should also handle this.

I think we should have a talk with Sunil or Weiwu about prioritising this issue and related issues of UN's and context in the ZKPs.

@oleggrib
Copy link
Collaborator

oleggrib commented Jun 30, 2022

@AW-STJ , @weiwu-zhang what do you think about 2 things to make emailAttestation more secure:

  1. Add attestation.id confirmation popup to share attestation+secret (currently any page in user browser can request and receive user attestation , if attestation generated and saved )
  2. update crypto-flow to return emailAttestationSecret with short TTL, 6-24 hours (user still have to issue and sign attestation with wallet once a month only ) , emailAttestationSecret can be based on some [UN_signed_by_attestation_id(include TTL), original_ emailAttestationSecret], so we have TTL, smart contract can verify signature and compare with attestation.id address and validate proof.

worth to do it?
I prefer option 1

cc @foxgem , @nicktaras

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Security A security issue that should be fixed, since it might allow for attacks.
Projects
None yet
Development

No branches or pull requests

2 participants