Harden account key validation#3791
Conversation
b227e60 to
2c1f633
Compare
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
2c1f633 to
646d817
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3791 +/- ##
==========================================
- Coverage 60.19% 59.23% -0.97%
==========================================
Files 2328 2236 -92
Lines 194502 184036 -10466
==========================================
- Hits 117088 109011 -8077
+ Misses 66862 65322 -1540
+ Partials 10552 9703 -849
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
646d817 to
ad38c3a
Compare
PR SummaryMedium Risk Overview New helpers ( Regression tests in Reviewed by Cursor Bugbot for commit b62b16f. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A focused, well-tested hardening change that validates account pubkey metadata (including nested multisig keys) in the SetPubKeyDecorator ante handler before persistence, with panic recovery correctly guarding against wrong-length key panics. No correctness or security blockers found; only minor observations.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor second-opinion review (./cursor-review.md) was empty — that pass produced no output. Codex (./codex-review.md) reported no material issues, consistent with this review.
- The PR is labeled
app-hash-breaking: the new validation now rejects transactions that previously could persist malformed key metadata (e.g. a multisig with an invalid nested key), which is the intended hardening. This is not gated behind an upgrade constant, so it relies on release coordination to roll out uniformly. Worth confirming the rollout plan treats this as a consensus-affecting change, but per repo review guidelines Sei's version-by-version sync model makes this acceptable and not a correctness bug. - Minor: in
validatePubKeyNoPanic, thedefaultbranch'slen(pubKey.Address()) == 0check is largely defensive/redundant for well-known key types (Address() returns a fixed-length hash or panics on bad input, which is caught by the recover in validatePubKey). It's harmless, but the effective guard is the panic recovery rather than the length check.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any correctness issues, but this changes the SetPubKeyDecorator ante handler — code that runs on every transaction and is marked app-hash-breaking — so it's worth a human sign-off before merging.
What was reviewed: the new validatePubKey/validateMultisigPubKey gating (missing keys, zero/over threshold, recursive multisig members, empty Address()), the simulate-mode placeholder normalization, and the panic-to-error recovery. Also checked the ruled-out candidates (guard-ordering, dead-code Address() check, multisig nesting CPU cost) against the current diff.
Extended reasoning...
Overview
The PR adds pre-persistence validation of signer public keys in SetPubKeyDecorator.AnteHandle (sei-cosmos/x/auth/ante/sigverify.go), rejecting malformed keys (missing, empty Address(), multisig threshold of 0 or exceeding key count, malformed nested multisig members) before SetPubKey/SetAccount are called, and normalizes empty secp256k1 placeholder keys during simulation. It adds two new regression tests covering the rejection path and the simulation placeholder path.
Security risks
This is squarely auth/signature-verification code executed in the ante handler for every transaction. A logic error here could either (a) allow malformed key metadata to be persisted into account state, or (b) incorrectly reject valid transactions/simulations chain-wide. The PR is marked app-hash-breaking, meaning it can change consensus-visible behavior across nodes — exactly the kind of change where a subtle mismatch between validation logic and existing key types would be very costly to catch after the fact.
Level of scrutiny
The bug hunting system found no issues and its candidate findings (guard-ordering around the pre-existing pubkey check, the 'dead code' Address()==0 check, multisig nesting cost) were investigated and refuted. Independently, I confirmed the panic-recovery wrapper is sound, the multisig threshold/address checks look correct against the existing LegacyAminoPubKey/secp256k1/secp256r1/ed25519 Address() implementations, and the simulate-mode normalization only affects empty-key secp256k1 placeholders. Despite the clean result, this class of change (consensus-affecting, security-sensitive, touches every tx) warrants a human reviewer's sign-off per the repo's own risk profile, not an automatic approval.
Other factors
Test coverage for the new behavior looks reasonable (nested-multisig rejection, simulation placeholder acceptance), and CI (Buf, Codecov) is green. No outstanding reviewer comments need addressing.
| return validatePubKeyNoPanic(pubKey) | ||
| } | ||
|
|
||
| func validatePubKeyNoPanic(pubKey cryptotypes.PubKey) error { |
There was a problem hiding this comment.
We call this function exactly once; I would avoid over refactoring it and would inline it into validatePubKey.
| } | ||
| } | ||
|
|
||
| if len(pubKey.Address()) == 0 { |
There was a problem hiding this comment.
This would accept any byte slice of length 33. Can we harden the check? Address() just wraps the bytes into type.
ad38c3a to
b62b16f
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b62b16f. Configure here.
| return sdkerrors.Wrapf(sdkerrors.ErrInvalidPubKey, "invalid public key type: %T", pubKey) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Weak pubkey byte validation
Medium Severity
validatePubKeyNoPanic treats a non-empty Address() as proof the key is valid. For secp256k1, Address() only checks length 33 and hashes the bytes, so any 33-byte slice passes—including keys that are not valid curve points. That lets invalid nested key metadata reach account state when other multisig signers satisfy the threshold, which undercuts the goal of keeping invalid account key metadata out of state. The same file already uses btcec.ParsePubKey elsewhere for real secp256k1 checks.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b62b16f. Configure here.
There was a problem hiding this comment.
A small, well-targeted defensive fix that validates account key metadata (including nested multisig subkeys) before it is written to account state on the live cosmos ante path, with focused regression tests. No blocking issues; one consistency/coverage note carried over from Codex.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Consistency/coverage (from Codex, downgraded to non-blocking): the same
validatePubKeyguard is only added to the customlegacyabciante path (CosmosStatelessChecks/CheckPubKeys), not to the stockante.SetPubKeyDecoratorused by the BaseApp-registeredNewAnteHandler. That decorator still callspk.Address()/SetPubKeywithout the new validation. Impact is limited: the live CheckTx/DeliverTx consensus path runs throughlegacyabci(now covered), and the BaseApp handler is only reached viaSimulate, wherebaseapp.runTxrecovers panics (baseapp.go:904) into an error rather than halting. Still, consider sharing one validation helper across both paths, or adding a test that exercises the malformed nested key through the BaseApp ante/simulate path, to prevent future drift. - The PR is labeled
app-hash-breakingbut the new rejection is unconditional (no in-code upgrade-height gate). This is consistent with a coordinated release rollout and the repo review guidelines, so not flagged as a bug — just confirm the change ships as part of a coordinated upgrade so all validators begin rejecting these keys at the same height. cursor-review.mdis empty — the Cursor second-opinion pass produced no output.- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| return validatePubKeyNoPanic(pubKey) | ||
| } | ||
|
|
||
| func validatePubKeyNoPanic(pubKey cryptotypes.PubKey) error { |
There was a problem hiding this comment.
[nit] Minor: the case nil arm only matches an untyped nil interface, not a typed nil (e.g. (*secp256k1.PubKey)(nil)), which would fall through to default and panic in pubKey.Address(). That's fine here because validatePubKey's recover() converts it into an ErrInvalidPubKey, but worth being aware of. No change required.
| func validatePubKeyNoPanic(pubKey cryptotypes.PubKey) error { | ||
| switch pk := pubKey.(type) { | ||
| case nil: | ||
| return sdkerrors.Wrap(sdkerrors.ErrInvalidPubKey, "missing public key") | ||
| case *kmultisig.LegacyAminoPubKey: | ||
| return validateMultisigPubKey(pk) | ||
| default: | ||
| if len(pubKey.Address()) == 0 { | ||
| return sdkerrors.Wrapf(sdkerrors.ErrInvalidPubKey, "invalid public key type: %T", pubKey) | ||
| } | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 validatePubKeyNoPanic's default case only checks that pubKey.Address() is non-empty, but for secp256k1.PubKey, Address() just hashes the raw bytes and only panics on wrong length — it never checks the bytes are a valid point on the curve. So any 33-byte garbage slice (e.g. as a multisig child key) passes validation and can be persisted into account state via CheckPubKeys' SetAccount before signature verification runs, which is exactly the gap maintainer masih flagged inline ("This would accept any byte slice of length 33... Can we harden the check?") and the PR's own test only covers a wrong-length (34-byte) key, not this correctly-sized case.
Extended reasoning...
The bug: validatePubKeyNoPanic's default branch (app/ante/cosmos_checktx.go:257-269) is meant to be the hardened validity check for all non-multisig pubkey types, but it only asserts len(pubKey.Address()) == 0. For secp256k1.PubKey, Address() (sei-cosmos/crypto/keys/secp256k1/secp256k1.go:149-158) is:
func (pubKey *PubKey) Address() crypto.Address {
if len(pubKey.Key) != PubKeySize {
panic("length of pubkey is incorrect")
}
sha := sha256.Sum256(pubKey.Key)
hasherRIPEMD160 := ripemd160.New()
hasherRIPEMD160.Write(sha[:])
return crypto.Address(hasherRIPEMD160.Sum(nil))
}It panics only when len(Key) != 33, and otherwise blindly hashes the raw bytes — it never calls anything like btcec.ParsePubKey to confirm the bytes decode to an actual point on the secp256k1 curve. RIPEMD160(SHA256(x)) is never empty for any non-empty input, so for any 33-byte slice, len(pubKey.Address()) == 0 is false and validatePubKeyNoPanic returns nil (valid).
Trigger path: This matters most for the nested-multisig path this PR explicitly targets. validateMultisigPubKey recurses into each child key via validatePubKey → validatePubKeyNoPanic. A LegacyAminoPubKey with threshold 1 and children [validKey, garbage33ByteKey] will have its garbage child pass the default branch, satisfy the threshold/count checks, and the outer multisig's own Address() is also non-empty, so the whole multisig object validates. An attacker who signs only with the valid child key satisfies the threshold, so CheckSignatures passes, and CheckPubKeys (cosmos_checktx.go:419-440) then calls acc.SetPubKey(pk) and accountKeeper.SetAccount(ctx, acc), persisting the multisig pubkey — including the garbage, off-curve child — into account state before/regardless of any check on that child ever succeeding cryptographically.
Why the PR's own regression test misses it: buildNestedMultisigTx in app/cosmos_checktx_test.go constructs malformedPubKey := &secp256k1.PubKey{Key: bytes.Repeat([]byte{1}, secp256k1.PubKeySize+1)} — i.e. 34 bytes, one byte too long. That trips the len(Key) != PubKeySize panic inside Address(), which validatePubKey's recover() wrapper turns into an ErrInvalidPubKey. The test therefore only proves the wrong-length case is caught; it does not exercise a correctly-sized (33-byte) but off-curve key, which is the case the maintainer's inline comment was explicitly about ("This would accept any byte slice of length 33... Address() just wraps the bytes into type. Can we harden the check?").
Step-by-step proof:
- Take any 33 arbitrary bytes, e.g.
key := bytes.Repeat([]byte{0x02}, 33)— note0x02as the first byte mimics a compressed-point prefix but the remaining bytes are not guaranteed to correspond to a valid x-coordinate on the curve; even fully random 33-byte inputs work, since curve validity is never checked. pk := &secp256k1.PubKey{Key: key}.pk.Address()does not panic (len(key) == 33) and returnsRIPEMD160(SHA256(key)), a non-empty 20-byte value.validatePubKeyNoPanic(pk)hits thedefaultcase, checkslen(pk.Address()) == 0→ false → returnsnil. The garbage key is "valid."- Embed
pkas a child of a threshold-1LegacyAminoPubKeyalongside one real key.validateMultisigPubKeyvalidates each child viavalidatePubKey; the garbage child returnsnilas shown above, threshold (1) ≤ len(pubKeys) (2), and the multisig's ownAddress()is non-empty → the whole multisig passesvalidatePubKeyNoPanic. - Sign the tx with only the real child key (satisfies threshold 1).
CosmosStatelessChecksandCheckSignaturesboth pass. CheckPubKeyscallsacc.SetPubKey(multisigPubKey)(which stores theAny-packed multisig, garbage child included) andaccountKeeper.SetAccount(ctx, acc), persisting it into state — the exact "invalid account key metadata written into account state" this PR says it exists to prevent.
Impact: The garbage child key itself cannot be used to produce a valid signature (it isn't a real keypair), so this is not a full auth bypass — the specific tx used to plant it must still be validly signed by the threshold of legitimate children. But the PR is billed as a hardening/defensive change (app-hash-breaking label, consensus-adjacent ante logic) whose stated purpose is exactly to stop malformed key metadata from reaching account state, and a maintainer explicitly asked for this to be hardened before merge. As written, secp256k1 (and similarly ed25519, whose Address() also just hashes fixed-length bytes without curve validation) garbage keys of the correct byte length still get through and get persisted.
Fix: For the secp256k1.PubKey case specifically, validate curve membership before accepting the key, e.g. by attempting btcec.ParsePubKey(pk.Key) (already imported and used elsewhere in this file, in UpdateSigners) and rejecting on error, rather than relying solely on Address() producing a non-empty result.
2 participants
Summary
Validation
go test ./app -run 'Test(CosmosStatelessChecksRejectsInvalidNestedMultisigKey|CheckPubKeysRejectsInvalidNestedMultisigKey)' -count=1go test ./app/ante -count=1gofmt -s -l .goimports -l app/ante/cosmos_checktx.go app/cosmos_checktx_test.goNote:
goimports -l .reports existing generated files and nested local worktree files unrelated to this branch.