Conversation
📝 WalkthroughWalkthroughThe PR adds input validation to ECIES key generation and secp256k1 curve operations to prevent processing of invalid public key coordinates. It introduces comprehensive test coverage for these security checks and updates the patch version number. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.16.3)crypto/secp256k1/curve.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m crypto/ecies/ecies.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m crypto/secp256k1/ext.h┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crypto/ecies/ecies.go (1)
121-127:⚠️ Potential issue | 🔴 CriticalGuard
pubbefore dereferencing it.
GenerateShared(nil, ...)still panics on Line 122 before the new validation runs. Since this method is now the public-key validation gate, please rejectpub == nil(andpub.Curve == nil) up front and returnErrInvalidPublicKeyinstead.🔧 Proposed fix
func (prv *PrivateKey) GenerateShared(pub *PublicKey, skLen, macLen int) (sk []byte, err error) { + if pub == nil || pub.Curve == nil || pub.X == nil || pub.Y == nil { + return nil, ErrInvalidPublicKey + } if prv.PublicKey.Curve != pub.Curve { return nil, ErrInvalidCurve } - if pub.X == nil || pub.Y == nil || !pub.Curve.IsOnCurve(pub.X, pub.Y) { + if !pub.Curve.IsOnCurve(pub.X, pub.Y) { return nil, ErrInvalidPublicKey }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypto/ecies/ecies.go` around lines 121 - 127, The method GenerateShared currently dereferences pub before validating it; add an up-front guard that returns ErrInvalidPublicKey if pub == nil or pub.Curve == nil, before any use of pub or pub.Curve (so this check happens before the existing comparison with prv.PublicKey.Curve and before pub.X/pub.Y checks); keep the later checks (curve equality, IsOnCurve, X/Y nil) as-is but ensure the new nil checks are the first lines in GenerateShared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crypto/secp256k1/curve.go`:
- Around line 95-97: The current check only rejects x or y >= BitCurve.P but
allows negative coordinates; update the point-validation logic (the function
that checks x and y against BitCurve.P — e.g., the IsOnCurve / point validation
routine where x and y are compared to BitCurve.P) to also reject negative values
by returning false when x.Sign() < 0 or y.Sign() < 0 (in addition to the
existing x.Cmp(BitCurve.P) >= 0 || y.Cmp(BitCurve.P) >= 0 check), so coordinates
are constrained to the affine field range [0, BitCurve.P).
In `@p2p/rlpx/rlpx_oracle_poc_test.go`:
- Around line 122-126: Add a modulo-equivalent negative test: construct xNeg =
new(big.Int).Sub(key.PublicKey.X, curve.Params().P) and yNeg =
new(big.Int).Sub(key.PublicKey.Y, curve.Params().P) and assert that
curve.IsOnCurve(xNeg, key.PublicKey.Y) and curve.IsOnCurve(key.PublicKey.X,
yNeg) both return false; this ensures IsOnCurve rejects coordinates equivalent
to the real point modulo P (use curve.Params().P, key.PublicKey.X and
key.PublicKey.Y to locate values).
---
Outside diff comments:
In `@crypto/ecies/ecies.go`:
- Around line 121-127: The method GenerateShared currently dereferences pub
before validating it; add an up-front guard that returns ErrInvalidPublicKey if
pub == nil or pub.Curve == nil, before any use of pub or pub.Curve (so this
check happens before the existing comparison with prv.PublicKey.Curve and before
pub.X/pub.Y checks); keep the later checks (curve equality, IsOnCurve, X/Y nil)
as-is but ensure the new nil checks are the first lines in GenerateShared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 111c9c82-af05-4ced-9b68-a32a3ae37bee
📒 Files selected for processing (5)
crypto/ecies/ecies.gocrypto/secp256k1/curve.gocrypto/secp256k1/ext.hp2p/rlpx/rlpx_oracle_poc_test.goparams/version.go
3 participants
1. Purpose or design rationale of this PR
Add additional checks.
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Bug Fixes
Tests
Chores