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

Security: Include the sink AccountId in the signed message for ToPublic transactions #324

Merged
merged 12 commits into from Mar 7, 2023

Conversation

SupremoUGH
Copy link
Contributor

@SupremoUGH SupremoUGH commented Mar 3, 2023

Due to the bug where an attacker could perform a replay attack on someone's valid ToPublic post by changing the public AccountId, we needed to sign this last field together with the TransferPostBody.

Main changes:
-auth::sign and auth::verify now take BodyWithAccountsRef instead of TransferPostBody.

  • transfer:
  1. AccountId is now an associated type of the Configuration trait, instead of the Ledger one.
  2. The Transaction enum in the ToPublic case now includes an AccountId.
  3. TransferPost includes a sink_accounts field.
  4. IdentityVerification::verify now verifies against a public_account as well.
  • signer:
  1. The function identity_proof also takes (a vector of) C::AccountId as input.

Misc:

  • updated the clap dependency so it passes the lint after the latest rustup update.
  • more on the identity_proof clumsy function: Identity Proof abstraction #325

Related PRs:


Before we can merge this PR, please make sure that all the following items have been checked off:

  • Linked to an issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Added one line describing your change in CHANGELOG.md and added the appropriate changelog label to the PR.
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Checked that changes and commits conform to the standards outlined in CONTRIBUTING.md.

@SupremoUGH SupremoUGH changed the title first prototype Fix: Include the sink AccountId in the signed message for ToPublic transactions Mar 3, 2023
@SupremoUGH SupremoUGH changed the title Fix: Include the sink AccountId in the signed message for ToPublic transactions Security: Include the sink AccountId in the signed message for ToPublic transactions Mar 3, 2023
@SupremoUGH SupremoUGH self-assigned this Mar 3, 2023
@SupremoUGH SupremoUGH added A-security Area: Issues and PRs related to Security changelog:security Changelog: add these changes to the `security` section of the changelog labels Mar 3, 2023
@SupremoUGH SupremoUGH marked this pull request as ready for review March 3, 2023 20:19
@Apokalip
Copy link
Contributor

Apokalip commented Mar 5, 2023

@SupremoUGH I don't think we should keep the public polkadot key in the State, was talking with Charlie. The key is needed only for signing so the easiest way in the perspective for current signer is to be provided from the front end in its request for signing as it can already see and process all the public accounts from polkadotjs. Keeping the public key in signer just complicates how we save history and state as right now state is constructed based on a KeySecret from a mnemonic(signer account).

As for the signer-extension it would be easy to get and keep(cache) the keys as you have polkadotjs API directly or even the registed accounts in the extension itself.

@Garandor Garandor mentioned this pull request Mar 6, 2023
10 tasks
@SupremoUGH SupremoUGH merged commit 7a0eef3 into main Mar 7, 2023
@SupremoUGH SupremoUGH deleted the to_public_signature_fix branch March 7, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-security Area: Issues and PRs related to Security changelog:security Changelog: add these changes to the `security` section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants