Skip to content

sign/{ed25519,ed448}: check if public key is identity.#617

Open
cjpatton wants to merge 1 commit into
mainfrom
cjpatton/eddsa-identity
Open

sign/{ed25519,ed448}: check if public key is identity.#617
cjpatton wants to merge 1 commit into
mainfrom
cjpatton/eddsa-identity

Conversation

@cjpatton

@cjpatton cjpatton commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

When parsing a public key or verifying a signature, check that the public key is not the identity element.


Open in Devin Review

When parsing a public key or verifying a signature, check that the
public key is not the identity element.
@bwesterb

bwesterb commented Jun 2, 2026

Copy link
Copy Markdown
Member

What's the reasoning here? We can't check for all ways a public key can be weak.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional findings.

Open in Devin Review

@drmikecrypto drmikecrypto left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice catch — accepting the identity point as a valid public key would undermine signature verification assumptions.

The IsIdentity() check in verify() is the right layer (after FromBytes succeeds but before scalar math). The test vectors covering Verify, VerifyPh, VerifyWithCtx, and UnmarshalBinaryPublicKey look thorough.

One minor note: worth confirming IsIdentity() matches RFC 8032's encoding of the identity (y=1, x=0) for all code paths that ingest public keys, not just verify — but the unmarshal test already covers the scheme entry point.

LGTM from a security perspective.

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