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

Verify signatures after signing #718

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

quad
Copy link

@quad quad commented Aug 15, 2022

Description

Verify signatures after signing

As per BIP-340, footnote 14:

Verifying the signature before leaving the signer prevents random or
attacker provoked computation errors. This prevents publishing invalid
signatures which may leak information about the secret key. It is
recommended, but can be omitted if the computation cost is prohibitive.

Notes to the reviewers

How do we test this?

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@danielabrozzoni
Copy link
Member

Since the footnote says "prevents random or attacker provoked computation errors", maybe we should re-try if the verification fails, and after one retry, return an error instead of panicking?

@quad
Copy link
Author

quad commented Aug 17, 2022 via email

@quad quad requested a review from afilini August 17, 2022 16:05
@jmecom
Copy link

jmecom commented Aug 17, 2022

IMO, if a fault occurs, the caller should be made aware and the library should not silently retry until success occurs. The correct response to a fault detection may vary from system-to-system, and so the library should be flexible. I'll quote the Hardware Hacking Handbook on some possible fault responses:

A fault response is what to do when a fault is detected. The goal here is to reduce the chances of a successful attack to the point where an attacker will give up. On the one end of the spectrum, you can implement a program exit, OS reboot, or chip reset. These actions will delay an attacker but in principle still allow them infinite tries. Somewhere in the middle of the spectrum is signaling a backend system to flag this device as suspicious and perhaps disable the account. On the other end of the spectrum, you can implement permanent measures like wiping keys, accounts, or even burning fuses that disallow the chip from booting up.

@danielabrozzoni
Copy link
Member

Yeah, you're right, re-trying doesn't seem to be a good idea, sorry :)

Copy link
Member

@afilini afilini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 31eed5f

@afilini
Copy link
Member

afilini commented Aug 18, 2022

Wait, I can't merge this unless the commits are signed: @quad you can either sign them yourself, or if you prefer I can sign them with my own key, let me know.

@quad
Copy link
Author

quad commented Aug 18, 2022

Feel free to sign it with your key.

As per [BIP-340, footnote 14][fn]:
> Verifying the signature before leaving the signer prevents random or
> attacker provoked computation errors. This prevents publishing invalid
> signatures which may leak information about the secret key. It is
> recommended, but can be omitted if the computation cost is prohibitive.

[fn]: https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#cite_note-14
@quad quad force-pushed the ssr/20220815/verify-signatures branch from 31eed5f to 7b1ad1b Compare August 25, 2022 06:29
@quad
Copy link
Author

quad commented Aug 25, 2022

@afilini I've signed the commit. Would you please re-approve running workflows?

Copy link
Member

@afilini afilini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 7b1ad1b

@afilini afilini merged commit 0a3734e into bitcoindevkit:master Aug 25, 2022
@afilini afilini mentioned this pull request Sep 8, 2022
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants