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

[crypto] Fast & Loose Key validation in FastNFT #21

Open
huitseeker opened this issue Dec 29, 2021 · 4 comments
Open

[crypto] Fast & Loose Key validation in FastNFT #21

huitseeker opened this issue Dec 29, 2021 · 4 comments
Assignees

Comments

@huitseeker
Copy link
Contributor

huitseeker commented Dec 29, 2021

The single error case of this TryFrom is just an invalid length error. There is a host of other problems completely ignored by the current implementation:

  • there are sequences of 32 bytes that will structurally not ever be usable as an Ed25519 Public Key in any way,
  • there are sequences of 32 bytes that will 100% be usable as an Ed25519 Public Key, but that demonstrably and unambiguously aim at tricking anybody who does so.

The above takes exactly none of that into account. Further, several of those checks will not be performed by check_internal's dalek::PublicKey::from_bytes (and the library has a nice warning to mention some of that).

I admit it's probably a completely orthogonal point to this PR, and worth tackling in a different issue (probably extracted from this comment), but I'd appreciate a spectacular comment on PublicKeyBytes making this clear. Here is an example of my personal minimum bar for the word "spectacular".

Originally posted by @huitseeker in MystenLabs/sui#94 (comment)

@gdanezis
Copy link

gdanezis commented Jan 5, 2022

This issue on ed25519 suggests that this is a doc bug, and no further validity check is needed:
dalek-cryptography/ed25519-dalek#150

This is also further evidenced by the docs on curve25519-dalek doc, that suggest that all edwards points are valid by construction:
https://github.com/dalek-cryptography/curve25519-dalek/blob/main/src/edwards.rs#L79-L86

@gdanezis
Copy link

gdanezis commented Jan 5, 2022

@huitseeker also says:

MystenLabs/sui#122 (comment)

@huitseeker
Copy link
Contributor Author

We are also dependent on verify_batch for performance, hence we must check for small subgroup points and more generally rogue key attacks. Checking public keys by validating they represent a point on curve is not enough.

On small subgroup components:
https://github.com/novifinancial/ed25519-speccheck
https://eprint.iacr.org/2020/1244

@kchalkias
Copy link
Collaborator

kchalkias commented May 9, 2022

Due to verify batch we have to be consistent here (there are 2 reasons for that, but it's too complex to explain here - i.e., if we're not careful and use randomized batch sig verification inside contracts, then one could even break determinism - and result to liveness issues).

The zebra lib specifically designed their protocol to support that, and Diem's implementation was stricter than zebra (based on our EdDSA security paper). This will be part of the generic crypto agility effort (all of the algorithms will need a key validation mechanism to be on the safe side).

@huitseeker huitseeker transferred this issue from MystenLabs/sui Aug 23, 2022
@kchalkias kchalkias assigned benr-ml and unassigned kchalkias Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants