fix(core): guard keypair sign/verify against short and multibyte key inputs - #341
Open
e-desouza wants to merge 3 commits into
Open
fix(core): guard keypair sign/verify against short and multibyte key inputs#341e-desouza wants to merge 3 commits into
e-desouza wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #341 +/- ##
==========================================
+ Coverage 87.01% 87.03% +0.02%
==========================================
Files 255 255
Lines 33668 33710 +42
==========================================
+ Hits 29295 29341 +46
+ Misses 4373 4369 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the core keypair signing and verification APIs against panics caused by short key strings and invalid UTF-8 slicing boundaries, addressing a remote DoS vector described in #282.
Changes:
- Replaced unconditional
&key[..2]prefix slicing with safe.get(..2)and error propagation in keypair dispatch. - Hardened Ed25519
sign/is_valid_messageagainst invalid slicing and fragiletry_into().expect/unwrapassumptions. - Added regression tests for empty/short key scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/core/keypairs/mod.rs |
Makes dispatching by key prefix safe (no panics on short/invalid boundaries), propagates errors in sign, returns false safely in is_valid_message, and adds regression tests. |
src/core/keypairs/algorithms.rs |
Makes Ed25519 signing/verifying resilient to invalid slices and conversion failures, returning Err/false instead of panicking, and adds a direct-trait guard test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+53
to
60
| fn _get_algorithm_engine_from_key(key: &str) -> XRPLCoreResult<Box<dyn CryptoImplementation>> { | ||
| let prefix = key.get(..2).ok_or(XRPLKeypairsException::InvalidSecret)?; | ||
| if prefix == ED25519_PREFIX { | ||
| Ok(_get_algorithm_engine(CryptoAlgorithm::ED25519)) | ||
| } else { | ||
| Ok(_get_algorithm_engine(CryptoAlgorithm::SECP256K1)) | ||
| } | ||
| } |
Comment on lines
+371
to
375
| #[test] | ||
| fn test_is_valid_message_short_pubkey_returns_false() { | ||
| assert!(!is_valid_message(&[], "", "ab")); | ||
| } | ||
| } |
Address two review comments:
- `_get_algorithm_engine_from_key` now delegates through `_get_algorithm_from_key`
and `_get_algorithm_engine` so prefix parsing exists in exactly one place and
the two helpers can no longer drift apart.
- `test_is_valid_message_short_pubkey_returns_false` now uses a 1-byte key ("a")
to exercise the actual <2-byte panic guard. A new
`test_is_valid_message_multibyte_pubkey_returns_false` uses "E£" (3 bytes, non-
char-boundary at offset 2) to cover the multibyte-slice regression path.
e-desouza
force-pushed
the
fix/issue-282-keypairs-panic
branch
from
July 15, 2026 16:41
c82acfb to
fb4fa44
Compare
The try_into() match at line 478 — which previously panicked via .unwrap()
and now correctly returns false — had no negative-path test coverage.
Add test_ed25519_is_valid_message_trait_wrong_length_key exercising:
- 1-byte decoded key (hex suffix "AB" → 1 byte ≠ 32)
- single-byte public key (too short for "ED" prefix check)
- non-char-boundary key ("E£") that would panic on a byte-indexed slice
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
_get_algorithm_from_keyand_get_algorithm_engine_from_keyperformed&key[..2]unconditionally. A key shorter than two bytes panicked immediately.Ed25519::signandEd25519::is_valid_messagesliced&key[ED25519_PREFIX.len()..]after a byte-length guard, but byte-length ≥ 2 does not guarantee a valid UTF-8 char boundary at offset 2. A key beginning with a multibyte character (e.g."E£") passed the guard yet panicked at the slice.is_valid_messagealso had adsig.try_into().expect(...)call that assumed the priordsig.len()guard would always fire first — a fragile invariant that a future refactor could break silently.Fixes #282.
What changed
_get_algorithm_from_key/_get_algorithm_engine_from_key: replaced&key[..2]withkey.get(..2).ok_or(InvalidSecret)?._get_algorithm_engine_from_key: now composes with_get_algorithm_from_keyto avoid duplicated logic.Ed25519::sign/Ed25519::is_valid_message: replaced bare[ED25519_PREFIX.len()..]slice with.get(ED25519_PREFIX.len()..), which returnsNoneon non-char-boundary.is_valid_message: replaced.expect("is_valid_message")ondsig.try_into()with amatchthat returnsfalseon conversion failure.How to validate
New tests:
test_sign_empty_key_returns_error,test_sign_short_key_returns_error,test_is_valid_message_empty_pubkey_returns_false,test_is_valid_message_short_pubkey_returns_false,test_ed25519_sign_trait_short_key_guard.