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

Fix EDDSA_LEGACY signing/verification with Ed448 keys #1675

Merged
merged 3 commits into from
Jun 3, 2024

Conversation

vanitasvitae
Copy link
Contributor

@vanitasvitae vanitasvitae commented May 23, 2024

When creating/verifying signatures made using PublicKeyAlgorithmTags.EDDSA_LEGACY, BC creates Signer object instances of Ed25519. While this is in line with the rfcs (EdDSALegacy is only specified for Ed25519), BC also supports key generation etc. using Ed448.
(Do we want to retain support for LEGACY_EDDSA + Ed448?)

(Update: LEGACY_EDDSA + Ed448 is defined in LibrePGP, see #1677 )

As a result, when generating/verifying signatures made using Ed448+EDDSA_LEGACY, BC tries to use Ed25519 signers, which fails.
Further, PGPSignature parses EDDSA_LEGACY signatures only as Ed25519, which means Ed448 signature result in OOB exceptions.

This PR changes the signer/verifier methods to detect, which curve is actually used when a EDDSA_LEGACY key is provided and to return an appropriate signer object accordingly.
The patch also fixes signature parsing for EDDSA_LEGACY Ed448 signatures by inspecting the length of the encoded signature values.

@vanitasvitae
Copy link
Contributor Author

vanitasvitae commented May 23, 2024

Hm, I'm currently adding more tests, including for Legacy X448 encryption and I wonder, if it's really worth supporting non-standard legacy X488 (with PublicKeyAlgorithmTags.ECDH) and legacy Ed448 (with PublicKeyAlgorithmTags.EDDSA_LEGACY).

Originally I thought support for these would be desirable, since the PGPKeyConverter classes had partial support already, but the more I think about it I guess supporting those key types would lead to people generating them, causing interop issues with other implementations.
What do you think?

Removing support for non-standard keys would involve reverting some changes from #1663 (simplifying EDDSA_LEGACY branches to always assume Ed25519, removing X448 from the ECDH branches), and removing the LegacyX448KeyPairTest, LegacyEd448KeyPairTest classes.

@vanitasvitae vanitasvitae marked this pull request as draft May 23, 2024 10:38
@vanitasvitae vanitasvitae force-pushed the fixEd448Signing branch 2 times, most recently from 5f455eb to 04104b0 Compare May 23, 2024 12:04
@vanitasvitae vanitasvitae marked this pull request as ready for review May 23, 2024 12:06
@dghgit
Copy link
Contributor

dghgit commented May 30, 2024

Usually the best way to deal with this is to support verification, but disable generation, so for now at least the best thing to do would be to revert the ability to use an Ed448 key in this context, but allow the signatures generated that way to be verified.

@vanitasvitae
Copy link
Contributor Author

Apparently, EDDSA_LEGACY+Ed448 is used by LibrePGP, so its debatable, whether it makes sense to deliberately revert signing support, now that its already implemented. I'd leave this decision up to you :)

@dghgit
Copy link
Contributor

dghgit commented May 30, 2024

Far be it from me to argue with the market on this one! Lets leave it as it be and hope for the best.

@hubot hubot merged commit 9aed89e into bcgit:main Jun 3, 2024
@vanitasvitae vanitasvitae deleted the fixEd448Signing branch June 3, 2024 10:55
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.

3 participants