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

Add ECDH primitives #125

Merged
merged 6 commits into from
Sep 17, 2024
Merged

Add ECDH primitives #125

merged 6 commits into from
Sep 17, 2024

Conversation

marsella
Copy link
Contributor

@marsella marsella commented Aug 27, 2024

Closes #123.

This adds the ECC_CDH primitive (probably what most people would call ECDH) from NIST SP 800-56A rev 3. The specification is very long and includes a lot of things that are hard to instantiate in cryptol, like key validation routines that involve trusted third parties and key generation based on sampling randomness.

This PR adds the most absolute barebones primitive needed to compute a shared secret. On its own this is totally inadequate for any actual key generation: it's missing parameter validation, key derivation, key confirmation, and any notion of (input) key management. I added some docs to that effect, but I would like input on whether it's clear that this is not secure as-is and needs a bunch of additional routines to approach security.

There are also a fair number of changes to the EC interface. The most complex one is the xCoord method, since that required propagating some changes to ECDSA. The rest are all straightforward additions.

I added test vectors for P-256 to make sure it works. I will write up a separate issue to add other instantiations and their corresponding test vectors.

Crypto review request (looking for one crypto review from either @b13decker or @Ra1issa):

  • Did I miss any insecurities of this standalone primitive?
  • Are there any properties I should write up for the key validation methods?

General review request:

  • Are you alarmed by the documentation?
  • Are there other basic primitives that would be useful to include in this first pass?
  • Are there other instantiations for specific curves that you would like to see?

- Changes method that previously retrieved & converted the x coordinate
  to a representation suitable for ECDSA to only retrieve
- Add field modulus to the EC interface because I'm now confident that
  that's a reasonable structure to assume.
- Updates ECDSA to use the new interface
- Implements the ECC_CDH primitive
- Adds documentation and warnings about use
- Adds CAVP test vectors
Copy link
Contributor

@mccleeary-galois mccleeary-galois left a comment

Choose a reason for hiding this comment

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

LGTM overall except for some clarity questions. I was able to follow along with the spec cited and it seemed all kosher to me.

Answered questions posed below.

Are you alarmed by the documentation?

  • I think it is important that we call out the current issues like you have done so.

Are there other basic primitives that would be useful to include in this first pass?

  • None that I could think of, I think more will come to light as we add more curves and instantiations.

Are there other instantiations for specific curves that you would like to see?

  • I don't think anything should block getting in this PR but lets chat about prioritization here soon.

Copy link
Collaborator

@Ra1issa Ra1issa left a comment

Choose a reason for hiding this comment

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

Here is a first batch of comments. I will scrutinize the changes against the spec in a bit. Overall, the changes you've made vastly improve readability and clearly outlines the limitations of this work

@Ra1issa
Copy link
Collaborator

Ra1issa commented Sep 3, 2024

Can someone clarify to me why this exists and what its intended use is if "On its own this is totally inadequate for any actual key generation" ? Its not just a couple of minor things that are missing for this to be a valid and verified key establishment, its core properties that @marsella outlined both in her comments and the docs.

@Ra1issa
Copy link
Collaborator

Ra1issa commented Sep 3, 2024

It would also be super helpful to have a single place that highlights all the parts of NIST SP 800-56A rev 3 that are validated and those that are not. Maybe this already exists and I just missed it ?

@marsella
Copy link
Contributor Author

marsella commented Sep 5, 2024

Thanks @Ra1issa! I updated the docs in a few ways:

  • Added a more obvious warning at the top that this isn't key establishment!
  • Added a list at the top of all the things we don't include here (hopefully this addresses your last comment to a certain degree) and a short explanation of why each one is a necessary part of key establishment
  • Rephrased the warnings in the per-function docs and made sure they were all labeled appropriately.

Hopefully with all of these things combined it will be very hard for someone to accidentally misuse this. There is significantly more warning text than code in the file now.

@marsella marsella merged commit 2227e5b into master Sep 17, 2024
2 checks passed
@marsella marsella deleted the 123-add-ecdh branch September 17, 2024 14:38
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.

Add ECDH primitive
4 participants