Skip to content

Conversation

fredrik0x
Copy link
Owner

Makes it faster

Makes it faster
Copy link

Security Review

Confidence Score: 95%
Detected Security Issues: Yes

Summary

After verification: 1 confirmed vulnerabilities out of 1 initially found.

Detailed Findings

HIGH Severity Issue

  • Description: Removal of elliptic curve point validation in UnmarshalPubkey function at crypto/crypto.go line 192-194
  • Recommendation: Restore the S256().IsOnCurve(x, y) validation check before creating the public key
  • Confidence: 95%
📑 Detailed Explanation

What is the issue?

The code change removes the critical elliptic curve point validation that ensures the unmarshaled public key coordinates (x, y) actually lie on the secp256k1 curve. This validation is essential for cryptographic security as it prevents acceptance of invalid points that could lead to signature verification bypasses or other cryptographic attacks.

What can happen?

Without curve validation, an attacker could provide invalid public key coordinates that don't lie on the secp256k1 curve. This could lead to signature verification bypasses, allowing unauthorized transactions or message authentication failures. In blockchain contexts, this could enable transaction forgery or consensus manipulation.

How to fix it?

Restore the removed validation check by adding back the lines: 'if !S256().IsOnCurve(x, y) { return nil, errInvalidPubkey }' before creating the ecdsa.PublicKey struct. This ensures only valid curve points are accepted as public keys.

Code example:

<!-- PROBLEMATIC CODE -->
func UnmarshalPubkey(pub []byte) (*ecdsa.PublicKey, error) {
	// ... existing code ...
	if x == nil {
		return nil, errInvalidPubkey
	}
	<!-- REMOVED VALIDATION -->
	return &ecdsa.PublicKey{Curve: S256(), X: x, Y: y}, nil
}

<!-- RECOMMENDED FIX -->
func UnmarshalPubkey(pub []byte) (*ecdsa.PublicKey, error) {
	// ... existing code ...
	if x == nil {
		return nil, errInvalidPubkey
	}
	<!-- RESTORE THIS VALIDATION -->
	if !S256().IsOnCurve(x, y) {
		return nil, errInvalidPubkey
	}
	return &ecdsa.PublicKey{Curve: S256(), X: x, Y: y}, nil
}

Additional resources:

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.

1 participant