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

feat: use constant-time equality checking for DHKE #232

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

AaronFeickert
Copy link
Contributor

This PR ensures that DiffieHellmanSharedSecret equality testing is done in constant time.

Previously, this equality testing was offloaded to the underlying PublicKey type. While this type supports the ConstantTimeEq trait, it is not guaranteed that equality testing will use this in all implementations.

@AaronFeickert AaronFeickert force-pushed the dhke-eq branch 2 times, most recently from 2dab023 to 9740715 Compare July 3, 2024 15:55
Copy link
Contributor

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

While the intention to use this type is good, I think this type mostly is unused. It adds very little apart from being a marker trait. I feel that now that we are adding code to make it less vulnerable means that it is more harm than good. I would suggest that we instead delete this type and it's usages and replace them with the standard public key

Copy link
Contributor

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

NACK

@stringhandler
Copy link
Contributor

sorry, this PR is fine, but I think the type should be deleted instead

@AaronFeickert
Copy link
Contributor Author

@stringhandler I get where you're coming from, but the intent isn't for this to simple be a PublicKey wrapper. The result of a DHKE is a (shared) secret value, and the type makes it more difficult to misuse: it's zeroized on drop (and can be zeroized manually), intentionally doesn't let you do scalar or group arithmetic, and only supports canonical byte slice visibility.

Removing the type and simply using a PublicKey means you lose these protections and need to rely on the implementer, which seems unnecessary.

As to this PR specifically, it was noted in #219 that while we can require constant-time equality support for PublicKey implementations, we can't enforce that all equality testing uses it. The addition of extra protections like forced constant-time equality in the DHKE type is meant to make up for this.

Copy link
Contributor

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

After discussing this offline, I'm happy to merge it

@SWvheerden SWvheerden added this pull request to the merge queue Jul 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jul 8, 2024
@SWvheerden SWvheerden merged commit 2a1715a into tari-project:main Jul 15, 2024
4 checks passed
@AaronFeickert AaronFeickert deleted the dhke-eq branch July 15, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants