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

simplify submission of attestations. no need for signed claimOfAttendance #215

Closed
pifragile opened this issue May 16, 2022 · 10 comments · Fixed by #231
Closed

simplify submission of attestations. no need for signed claimOfAttendance #215

pifragile opened this issue May 16, 2022 · 10 comments · Fixed by #231
Assignees
Labels

Comments

@pifragile
Copy link
Contributor

In my opinion, the design around signed claims of attendances is not very intuitive.
I don't see what benefit we get from the fact that the claims have a signature. Does it add any security?
Also the claims hold a lot of redundant information that needs to be processed on chain.

I would propose a new design, where meetup participants just send the following info after a meetup:

ceremony_index,
community_identifier,
meetup_index,
location,
timestamp,
list of public keys of meetup participants // instead of sigend attestations

Then, we would read the vote on the number of participants from the length of the list of public keys rather than from the claims.
This would simplify things and make it more intuitive. I cannot see any security issues with this new design. Am I missing something?

What are your thoughts on this @brenzi @clangenb?

@clangenb
Copy link
Member

Hmm, on the spot, I can't find a hard reason against that suggestion.

@pifragile
Copy link
Contributor Author

I think we should move the vote into a separate field, as is can be the case that 10 people are present but you are not able to scan one or two persons QR code for technical reasons.

@brenzi
Copy link
Member

brenzi commented May 27, 2022

Here's a possible attack if claimOfAttendance isn't signed by the claimant:

meetup assigned size: 10 participants
physically present at meetup: 3 participants, who all behave correctly.
Therefore, we'll have 3 votes for three participants

Because of

// is it a problem if this number isn't equal for all claims? Guess not.
// is it a problem that this gets inserted multiple times? Guess not.

an adversary could submit an attestation for any of the legit participants with a fabricated ClaimOfAttendance with a wrong vote

So, at least for the current implementation, I veto this change

@pifragile
Copy link
Contributor Author

an adversary could submit an attestation for any of the legit participants with a fabricated ClaimOfAttendance with a wrong vote

What do you mean exactly by this? The concept of ClaimOfAttendance would not exist anymore in this new design. The idea is that every participant calls an extrinsic with a signed origin and submits a vote and a list of addresses he wants to attest as present in the meetup. So an adversary could never forge a vote of another participant.

@brenzi
Copy link
Member

brenzi commented May 31, 2022

True. If this is really sound it would greatly simplify UX as there would be no need to scan qr codes anymore as we only need to know who out of the possible set is present. We could broadcast bluetooth beacons similar to the covid app or even just represent pubkeys with a couple of symbols (or a 4byte hash)

But the fact that participants do not need the private key for the account they are attending with concerns me. That appears too easy to be secure.

We should think about attack vectors if anyone can claim for everyone else. Basically this undermines ceremony randomization as people can swap locations to their liking

@pifragile
Copy link
Contributor Author

I understand the concern about the private key. But I have the feeling that all attacks concerning location swapping etc. would already possible now, just a little more complicated to implement technically. I think the question really is, if those really are attacks, ie. who would benefit from them.

Let's consider this:
A Person X shows up and claims to be another person Y. -> We cannot prevent that anyways, because persons are anonymous.
In the current system this would only be possible if persons X and Y know each other and can exchange a phone/private key/prepared claims before the meetup. With the new system, essentially anybody could show up and claim to be anybody else in the meetup because the pubkeys are known. The question here is, why would any person X do this, because X would not benefit from it and could also not harm the system with it.

What is still not possible is that I can go to any meetup and claim to be myself and get attested. So if I swap locations, I will need to trust the other person to show up in my location and claim to be me. So I think in the end people have a clear incentive to go to their location just to make sure that they really get their attestations without having to trust another person.

Are there any other scenarios that could go wrong?

@brenzi
Copy link
Member

brenzi commented Jun 3, 2022

The current claimOfAttendance design for sure is a legacy from a previous design where each participant submitted their own claims that were signed by the others.
I'm starting to see that your suggestion may actually be sound and we could massively simplify the meetup UX and we should prioritize this.
But we need a systematic way of testing this proposal against our threat model

@brenzi
Copy link
Member

brenzi commented Jun 3, 2022

looking at:

pub struct ClaimOfAttendance<Signature, AccountId, Moment> {
pub claimant_public: AccountId,
pub ceremony_index: CeremonyIndexType,
pub community_identifier: CommunityIdentifier,
pub meetup_index: MeetupIndexType,
pub location: Location,
pub timestamp: Moment,
pub number_of_participants_confirmed: u32,
pub claimant_signature: Option<Signature>,
}

attacks on your design proposal will always try to change at least one of these fields

  • claimant_public no attack possible as this is all covered by our threat model majority-honest assumption
  • ceremony_index no attack possible as index will be enforced by chain
  • community_identifierenforced by chain assignments
  • meetup_index enforced by chain assignments
  • location enforced by chain assignments
  • timestamp this would still be an argument of our extrinsic and will therefore be signed. q.e.d.
  • number_of_participants_confirmed this would still be an argument of our extrinsic and will therefore be signed. q.e.d.
  • claimant_signature well, that is enforced by verifying the signature now and not necessary afterwards.

So, yes @pifragile, as far as I can tell, your suggestion is sound 🚀 and is a massive simplification

I'll rename this issue to an actionable task. In order to be reverse compatible, I'd leave the current dispatchabe as-is:

pub fn attest_claims(
origin: OriginFor<T>,
claims: Vec<ClaimOfAttendance<T::Signature, T::AccountId, T::Moment>>,
) -> DispatchResultWithPostInfo {

but add a new one:

pub fn attest_observations(
   origin: OriginFor<T>,
   number_of_participants_confirmed: u32,
   timestamp: Moment,
   observations: Vec<AccountId>
) -> DispatchResultWithPostInfo {

I still believe it makes sense to vote on the number of participants, because collecting AccountId's is a process that can possibly go wrong due to human or technical errors

@clangenb
Copy link
Member

clangenb commented Jun 3, 2022

Yes, @pifragile and I also discussed this yesterday, and we could not come up with a potential attack either. I suggest we do this asap, it will make the meetups so much more easy!

@brenzi
Copy link
Member

brenzi commented Jun 3, 2022

yes. It will not be trivial to provide reverse compatibility with mixed versions of the app in the same meetup, so we need to think this through. We don't have a deprecation warning in the app itself so far. maybe we should introduce that first?

But from a pallet/runtime point of view, we can move ahead and include this in our upcoming runtime upgrade!

@brenzi brenzi changed the title Rethink design of claims of attendance simplify submission of attestations. no need for signed claimOfAttendance Jun 3, 2022
@brenzi brenzi added the security label Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants