imp(staking): detect the length of the ed25519 pubkey in CreateValidator to prevent panic #18506
Conversation
…tor to prevent panic
WalkthroughWalkthroughThe Cosmos SDK has implemented a crucial update to enhance the robustness of the staking module. A new check has been introduced to prevent potential panics by validating the length of the ed25519 public key during the creation of a validator. This update includes additional test cases to ensure the functionality works as expected and introduces a specific error for invalid public key lengths. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- x/staking/keeper/msg_server.go (3 hunks)
- x/staking/keeper/msg_server_test.go (4 hunks)
- x/staking/types/errors.go (1 hunks)
Additional comments: 8
CHANGELOG.md (1)
- 66-66: The changelog entry correctly summarizes the change and provides a link to the pull request for more details. It's important that the entry is clear and concise, as it is here, to ensure users understand the nature of the fix and the potential impact on their systems.
x/staking/types/errors.go (1)
- 51-54: The addition of the
ErrConsensusPubKeyLenInvaliderror is correctly implemented and follows the existing pattern for error registration in the Cosmos SDK. The error message is clear and the error code 48 is in sequence following the last error code 47, which is good for maintainability and consistency.x/staking/keeper/msg_server.go (3)
18-21: The import of
ed25519andcryptotypesis necessary for the new validation logic that checks the size of the ed25519 public key. This is a critical security update to prevent system panic due to an invalid public key size.85-89: This is the newly introduced check for the ed25519 public key length. It is important to ensure that the length of the public key matches the expected size for ed25519 keys, which is 32 bytes. This prevents potential security issues that could arise from using keys of incorrect sizes.
93-110: Reordering of validation checks for coin denomination and description length is a non-functional change and should not affect the logic of the
CreateValidatorfunction. However, it's important to ensure that these changes do not inadvertently affect the flow of validation in scenarios that were not considered.x/staking/keeper/msg_server_test.go (3)
- 4-10: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [4-21]
The import statements are well-organized and grouped according to the Go conventions: standard library, third-party packages, and then local packages. However, it's important to ensure that all imported packages are used within the file to avoid unnecessary imports which can lead to compilation errors or binary bloat.
42-53: The setup for the consensus parameters and the creation of an invalid public key length are correctly implemented. The test case for an invalid public key length is a critical addition to ensure the new validation logic is properly tested. It's important to ensure that the
ed25519pkvariable simulates an invalid public key accurately, which in this case is done by providing a byte slice of incorrect length.116-141: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [55-138]
The test cases are well-structured and cover a variety of scenarios, including both expected failures and a success case. It's good practice to cover edge cases in unit tests to ensure the robustness of the code. However, it's important to verify that the error messages match the actual error messages returned by the
CreateValidatorfunction to ensure the tests are accurate.
x/staking/keeper/msg_server.go
Outdated
| "got: %s, expected: %s", pk.Type(), cp.Validator.PubKeyTypes, | ||
| ) | ||
| } | ||
|
|
||
| if pkType == "ed25519" && len(pk.Bytes()) != ed25519.PubKeySize { | ||
| return nil, errorsmod.Wrapf( | ||
| types.ErrConsensusPubKeyLenInvalid, | ||
| "got: %d, expected: %d", len(pk.Bytes()), ed25519.PubKeySize, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| if _, err := k.GetValidatorByConsAddr(ctx, sdk.GetConsAddress(pk)); err == nil { | ||
| return nil, types.ErrValidatorPubKeyExists | ||
| } | ||
|
|
||
| bondDenom, err := k.BondDenom(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if msg.Value.Denom != bondDenom { | ||
| return nil, errorsmod.Wrapf( | ||
| sdkerrors.ErrInvalidRequest, "invalid coin denomination: got %s, expected %s", msg.Value.Denom, bondDenom, | ||
| ) | ||
| } | ||
|
|
||
| if _, err := msg.Description.EnsureLength(); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| validator, err := types.NewValidator(msg.ValidatorAddress, pk, msg.Description) |
There was a problem hiding this comment.
Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [64-90]
The addition of the public key size check for ed25519 keys is a crucial security measure. It ensures that only keys of the correct size are used to create validators, which is essential for the integrity of the consensus mechanism. The error handling with ErrConsensusPubKeyLenInvalid provides clear feedback for incorrect key sizes.
|
Btw, you can send transaction using the command line on main. Just not when the keyring is not at the default directory. make install
make init-simapp
simd startsimd tx bank send alice bob 100stake |
julienrbrt
added
the
backport/v0.50.x
Thanks for your guidance, I just retested using your method and it works as expected. use cli command {
"pubkey": {
"@type": "/cosmos.crypto.ed25519.PubKey",
"key": "bHVrZQ=="
},
"amount": "1000000stake",
"moniker": "myvalidator",
"identity": "optional identity signature (ex. UPort or Keybase)",
"website": "validator's (optional) website",
"security": "validator's (optional) security contact email",
"details": "validator's (optional) details",
"commission-rate": "0.1",
"commission-max-rate": "0.2",
"commission-max-change-rate": "0.01",
"min-self-delegation": "1"
}The transaction receipt is as follows, no panic will occur {
"height": "8",
"txhash": "1A256088AA0D337762FB71110F0FDE3D9577BD09DB4DF691CBCA2543274E4298",
"codespace": "staking",
"code": 48,
"data": "",
"raw_log": "failed to execute message; message index: 0: got: 4, expected: 32: consensus pubkey len is invalid",
"logs": [],
"info": "",
"gas_wanted": "200000",
"gas_used": "47238",
"tx": {
"@type": "/cosmos.tx.v1beta1.Tx",
"body": {
"messages": [
{
"@type": "/cosmos.staking.v1beta1.MsgCreateValidator",
"description": {
"moniker": "myvalidator",
"identity": "optional identity signature (ex. UPort or Keybase)",
"website": "validator's (optional) website",
"security_contact": "validator's (optional) security contact email",
"details": "validator's (optional) details"
},
"commission": {
"rate": "0.100000000000000000",
"max_rate": "0.200000000000000000",
"max_change_rate": "0.010000000000000000"
},
"min_self_delegation": "1",
"delegator_address": "",
"validator_address": "cosmosvaloper1shlek07kh379w6hye8lxgy3nx58spnkhnxtj9j",
"pubkey": {
"@type": "/cosmos.crypto.ed25519.PubKey",
"key": "bHVrZQ=="
},
"value": {
"denom": "stake",
"amount": "1000000"
}
}
],
"memo": "",
"timeout_height": "0",
"extension_options": [],
"non_critical_extension_options": []
},
"auth_info": {
"signer_infos": [
{
"public_key": {
"@type": "/cosmos.crypto.secp256k1.PubKey",
"key": "AqsPL6jYMTN/mIqaw5uxU8XKytS3LfuHy8dSgVWllSP6"
},
"mode_info": {
"single": {
"mode": "SIGN_MODE_DIRECT"
}
},
"sequence": "0"
}
],
"fee": {
"amount": [],
"gas_limit": "200000",
"payer": "",
"granter": ""
},
"tip": null
},
"signatures": [
"CrEPOsAi+NSMc8lC2EDU0QswfOSQf/3tU1K4OEJyP4ILsZR/7Cq2QHQiYODo8Ez82hVysWdLXazPNQCPdIUCig=="
]
},
"timestamp": "2023-11-19T11:48:36Z",
"events": [
{
"type": "tx",
"attributes": [
{
"key": "fee",
"value": "",
"index": true
},
{
"key": "fee_payer",
"value": "cosmos1shlek07kh379w6hye8lxgy3nx58spnkhkjl8fp",
"index": true
}
]
},
{
"type": "tx",
"attributes": [
{
"key": "acc_seq",
"value": "cosmos1shlek07kh379w6hye8lxgy3nx58spnkhkjl8fp/0",
"index": true
}
]
},
{
"type": "tx",
"attributes": [
{
"key": "signature",
"value": "CrEPOsAi+NSMc8lC2EDU0QswfOSQf/3tU1K4OEJyP4ILsZR/7Cq2QHQiYODo8Ez82hVysWdLXazPNQCPdIUCig==",
"index": true
}
]
}
]
} |
|
By the way, I want to use |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/staking/keeper/msg_server_test.go (13 hunks)
Additional comments: 6
x/staking/keeper/msg_server_test.go (6)
4-10: The import of
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"is added, but there is no context provided to understand if this import is used in the test cases. Verify that this import is necessary and used within the test suite to avoid unnecessary dependencies.15-21: The import of
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"is added, which is used in the new test case for invalid consensus pubkey length. This is a necessary addition to support the new validation logic.42-53: The setup for the new test case "validator pubkey len is invalid" is correct. It creates an invalid public key with a length that is not 32 bytes, which is the expected length for an ed25519 public key. This will help ensure that the new validation logic correctly rejects invalid public keys.
255-258: The denomination for coins is updated from hardcoded "stake" to
sdk.DefaultBondDenom. This change aligns with the SDK's denomination standards and ensures consistency across the test suite.120-137: This new test case checks the behavior of the
CreateValidatorfunction when provided with a public key of invalid length. It is a critical test to ensure that the new validation logic is functioning as expected. The test case is well-structured and covers the scenario outlined in the pull request summary.459-465: The test cases for
TestMsgEditValidatorandTestMsgDelegateare updated to usesdk.DefaultBondDenominstead of a hardcoded "stake" string. This change is consistent with the updates made in other test cases and ensures that the tests are using the correct denomination.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- types/staking.go (1 hunks)
- x/staking/keeper/msg_server.go (3 hunks)
- x/staking/keeper/msg_server_test.go (13 hunks)
Files skipped from review due to trivial changes (1)
- types/staking.go
Additional comments: 14
x/staking/keeper/msg_server.go (6)
15-21: The import of the
ed25519package is correctly added to support the new validation logic for the public key length. This change is necessary for the implementation of the new feature.85-90: The check for the public key length is correctly implemented to ensure that the
ed25519public key is exactly 32 bytes long. This is a critical update for preventing runtime panics due to incorrect key sizes.93-95: The check to ensure that the validator's consensus public key does not already exist is important for preventing duplicate validators. This logic is correctly placed after the new public key length validation.
102-106: The validation for the coin denomination is correctly moved to a different location within the function. This change is part of the restructuring of the existing validation logic and is consistent with the summary provided.
108-110: The validation for the description length is also correctly moved within the function. This is in line with the restructuring of the validation logic and ensures that the description meets the required length constraints.
112-112: The creation of a new validator object is correctly placed after all the necessary validation checks. This ensures that the validator is only created if all the preconditions are met, which is a good practice for robustness.
x/staking/keeper/msg_server_test.go (8)
4-10: The addition of new import statements for
github.com/cometbft/cometbft/proto/tendermint/typesandcosmossdk.io/collectionsis noted. Ensure that these packages are used in the test cases and that there are no unused imports.15-21: The addition of new import statements for
github.com/cosmos/cosmos-sdk/crypto/typesand related packages is noted. Ensure that these packages are used in the test cases and that there are no unused imports.42-53: The test setup for the consensus parameters is correct and ensures that the
PubKeyTypesincludes thesdk.PubKeyEd25519Type. This is important for the new test case that validates the behavior when an invalid consensus public key length is provided.45-47: The creation of an invalid public key with a length that is not 32 bytes is correct and will be used to test the new validation logic in the
CreateValidatorfunction. This is a good test case to ensure that the new error handling is functioning as expected.111-141: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [55-137]
The new test cases added to validate various scenarios, including "empty description", "invalid validator address", "empty validator pubkey", "validator pubkey len is invalid", "empty delegation amount", and others, are comprehensive and cover a wide range of potential issues. This ensures that the
CreateValidatorfunction behaves correctly under different conditions.
120-137: The new test case "validator pubkey len is invalid" is particularly important as it directly tests the critical update made to the
CreateValidatorfunction to check for the correct length of the ed25519 public key. This test case should trigger the newErrConsensusPubKeyLenInvaliderror when the provided public key length is incorrect.285-291: The test setup for creating a validator within the
TestMsgEditValidatorfunction is correct. It is important to ensure that the validator creation is successful before testing the edit functionality.459-465: Similar to the
TestMsgEditValidatorfunction, the test setup for creating a validator within theTestMsgDelegatefunction is correct. This setup is necessary to test the delegation functionality with a valid validator.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/staking/keeper/msg_server.go (4 hunks)
Additional comments: 3
x/staking/keeper/msg_server.go (3)
3-9: The import of the "slices" package is not used in the provided code. If it's not used elsewhere in the file, it should be removed to keep the code clean and avoid unnecessary package dependencies.
63-84: The validation logic for the public key type and length is correctly implemented. It checks if the public key is of the expected type and if the length matches the required size for an ed25519 public key. This should prevent runtime panics due to invalid public key lengths.
103-108: The error handling for the
EnsureLengthmethod ofmsg.Descriptionhas been moved to a more appropriate location within theCreateValidatorfunction. This change improves the readability and maintainability of the code by keeping the validation logic together.
julienrbrt
removed
the
backport/v0.50.x
Description
Using the command
simd tx staking create-validator --pubkey='{@type:/cosmos.crypto.ed25519.PubKey,key:bHVrZQ==}'(the latest main branch has been read validator info from the file. I am just doing a demonstration here.) to create a validator will cause a panic. In my actual test, the receipt of the transaction looks like this:
Because the length of the key is incorrect, it should be 32 bytes. So i added a check for the length of the pubkey in the
CreateValidatorfunction to prevent a panic.Note: Since it is currently not possible to send transactions using the CLI command, as mentioned in bug #18122 , I actually tested this using evmos. I have only completed the unit tests and meet the expectations. Please test it yourself using the CLI command.A transaction receipt sent for testing after modification on evmos is as follows:
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Error Handling