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

client: stop accepting sigs on the truncated messages (sig fix stage 4) #1526

Merged
merged 1 commit into from
Sep 23, 2022

Conversation

chappjc
Copy link
Member

@chappjc chappjc commented Mar 17, 2022

Rebased on #1530. This PR is just the one final commit 70ec51b

This is stage 4 of the signature message truncation fix plan outlined below.

In these commits, client stops accepting sigs on the truncated messages. All servers must be sending the correct sigs i.e. have deployed the stage 3 of the fix.


Full signature fix plan

In some places we were signing and verifying unhashed messages, resulting in truncation of the data we intend to sign and verify. Because the truncation was performed on both sides, we didn't have errors, but the full payloads were not part of the verified message. This included the signatures created and verified in the main comms (i.e. between Core and AuthManager), as well as in DCR and BTC's backends where coin signatures are used to verify ownership of the coins.

Note that in the asset coin sign/verify bits, it is always the client signing coins with their wallet keys, and the server verifying. In other comms, both client and server are generating and verifying signatures.

Also note that all comms are end-to-end encrypted with TLS, and most truncated payloads included unique data anyway. The message payloads and client signatures are also not publicly available data, so it is not straightforward to exploit this bug.

This change includes all the commits that will occur in several stages:

  1. Both server and client start recognizing sigs created on both the correctly hashed message and the truncated messages. These commits should be deployed asap with 0.4.2 so that servers are ready when clients update and start sending correct signatures. Server will continue to sign the truncated data so buggy clients will work with updated servers. start recognizing signatures of the hashed messages (sig fix stage 1) #1528
  2. Client starts signing the hashed messages, making client incompatible with servers not running the fix in stage 1 (0.4.2). Client continues accepting sigs on the truncated messages. Server continues sending the truncated message sigs. client: sign and verify hashed messages (sig fix stage 2) #1529
  3. Server starts signing the hashed messages, making server incompatible with clients not running the fix in stage 1 (0.4.2). Server stops accepting sigs on the truncated messages. server: sign and verify hashed messages (sig fix stage 3) #1530
  4. Client stops accepting sigs on the truncated messages.

Note that stage 2 also fixes client/core.(*Core).Register failing to re-set their own pubkey in the msgjson.RegisterResponse from the server, where it is omitted from the JSON serialization but included in the binary serialization when the payload is signed.

In summary, the server commits should land in 0.4.2 (like today), while the client commits should land in master and later in 0.4.3+.
Lastly, a future commit for 0.5 and the "V0PURGE" should be drafted that makes server sign outgoing hashed messages to the clients and stop verifying signatures on truncated data from the client, breaking compatibility with older clients.

This approach is proposed to minimize disruption to older clients, delaying the breaking changes to 0.5/1.0 and the great v0 API purge.

Discuss. 👵

WARNING: I've not tested this interactively yet.

@chappjc
Copy link
Member Author

chappjc commented Mar 17, 2022

Split the first 3 commits into a separate branch, that way we can test server updated but not client as is in the plan.
https://github.com/chappjc/dcrdex/tree/sign-verify-hash-server-only

@buck54321
Copy link
Member

Plan sounds right. Looking forward to the purge.

@chappjc
Copy link
Member Author

chappjc commented Mar 17, 2022

Updated the plan a bit to be more explicit about the stages and which commits are in each. Also added the final commits for both sides to stop accepting the truncated msg sigs.

@chappjc chappjc changed the title multi: correctly sign and verify message hashes client: stops accepting sigs on the truncated messages (sig fix stage 4) Mar 17, 2022
@chappjc chappjc changed the title client: stops accepting sigs on the truncated messages (sig fix stage 4) client: stop accepting sigs on the truncated messages (sig fix stage 4) Mar 17, 2022
@chappjc chappjc added this to the 0.5.1 milestone May 19, 2022
@chappjc chappjc modified the milestones: 0.5.1, 0.5.2 Aug 29, 2022
@chappjc chappjc modified the milestones: 0.5.3, TBD Sep 19, 2022
@chappjc chappjc marked this pull request as ready for review September 22, 2022 15:09
@chappjc chappjc merged commit 3cd4f10 into decred:master Sep 23, 2022
@chappjc chappjc deleted the sign-verify-hash branch September 23, 2022 18:07
@chappjc chappjc modified the milestones: 0.5.3, TBD Sep 23, 2022
@chappjc chappjc removed this from the TBD milestone Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants