feat: extra checks on signatures/pubkeys + check the signature first in antehandle #18194
Conversation
WalkthroughThe changes in this update focus on improving the security and functionality of the Cosmos SDK. Key updates include the introduction of a function to verify if a public key is on the curve, the addition of new keypair generation functions, and modifications to test functions. Import statements have been updated and reordered, unused imports and comments have been removed, and function parameters have been modified in some files. The overall goal is to enhance the codebase's security, maintainability, and test coverage. Changes
TipsChat with CodeRabbit Bot (
|
… into feature/anteKeysChecks
github-prbot
requested review from
a team,
facundomedica and
julienrbrt
and removed request for
a team
… into feature/anteKeysChecks
![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: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- crypto/keys/secp256k1/secp256k1.go (4 hunks)
- testutil/testdata/tx.go (3 hunks)
- x/auth/ante/sigverify.go (6 hunks)
- x/auth/ante/sigverify_test.go (3 hunks)
Files skipped from review due to trivial changes (1)
- testutil/testdata/tx.go
Additional comments: 13
crypto/keys/secp256k1/secp256k1.go (4)
12-12: The import statement
github.com/decred/dcrd/dcrec/secp256k1/v4is renamed tosecp256k1dcrd. Ensure that all references to the old import namesecp256k1have been updated to the new namesecp256k1dcrdthroughout the codebase.42-42: The function call
secp256k1.PrivKeyFromBytes(privKey.Key).PubKey()is updated tosecp256k1dcrd.PrivKeyFromBytes(privKey.Key).PubKey(). This change is consistent with the updated import statement.104-104: The function call
secp256k1.S256().Nis updated tosecp256k1dcrd.S256().N. This change is consistent with the updated import statement.132-132: The function call
secp256k1.S256().Nis updated tosecp256k1dcrd.S256().N. This change is consistent with the updated import statement.x/auth/ante/sigverify_test.go (3)
7-7: The import statement
github.com/decred/dcrd/dcrec/secp256k1/v4is renamed tosecp256k1dcrd. Ensure that this change does not affect the rest of the codebase and that the new import name is used consistently throughout the code.345-347: The
SetPubKeyDecoratorandIncrementSequenceDecoratorare now chained together in theantehandler. This is a change from the previous version where only theIncrementSequenceDecoratorwas used. Ensure that this change is intentional and that it does not introduce any unexpected behavior.370-468: A new test function
TestAnteHandlerChecksis added. This function tests various scenarios with different key types, including keys that are not on the curve. This is a good addition as it improves the test coverage and ensures that the code behaves as expected in different scenarios.x/auth/ante/sigverify.go (6)
8-10: The import statement
github.com/decred/dcrd/dcrec/secp256k1/v4is renamed tosecp256k1dcrd. Ensure that this change does not affect the rest of the codebase and that the new import name is used consistently throughout the code.99-101: The function
verifyIsOnCurveis called to check if the public key is on the curve. This is a new security measure added to the code.195-200: The function
verifyIsOnCurveis called to check if the public key is on the curve. This is a new security measure added to the code.264-302: The function
verifyIsOnCurveis defined. It checks if the public key is on the curve. It supports different types of keys: ed25519, secp256k1, secp256r1, and multisig. For ed25519 keys, it returns an error as they are not supported. For secp256k1 and secp256r1 keys, it checks if they are on the curve. For multisig keys, it checks if all the keys are on the curve. If the key type is not recognized, it returns an error.339-341: The function
verifyIsOnCurveis called to check if the public key is on the curve. This is a new security measure added to the code.433-439: The function
verifyIsOnCurveis called to check if the public key is on the curve. This is a new security measure added to the code.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- testutil/testdata/animal.go (1 hunks)
- testutil/testdata/grpc_query.go (1 hunks)
Files skipped from review due to trivial changes (2)
- testutil/testdata/animal.go
- testutil/testdata/grpc_query.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- tests/integration/bank/keeper/deterministic_test.go (17 hunks)
- tests/integration/staking/keeper/deterministic_test.go (26 hunks)
- testutil/testdata/grpc_query.go (2 hunks)
- x/auth/keeper/deterministic_test.go (13 hunks)
Files skipped from review due to trivial changes (3)
- tests/integration/bank/keeper/deterministic_test.go
- tests/integration/staking/keeper/deterministic_test.go
- testutil/testdata/grpc_query.go
Additional comments: 14
x/auth/keeper/deterministic_test.go (14)
123-123: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.135-135: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.152-152: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.177-177: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.184-184: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.195-195: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.211-211: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.221-221: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.230-230: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.241-241: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.304-304: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.316-316: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.351-351: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.363-363: The order of arguments in the
DeterministicIterationsfunction call has been changed. Ensure that this change is reflected across all calls to this function in the codebase.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/auth/ante/sigverify_test.go (3 hunks)
Additional comments: 2
x/auth/ante/sigverify_test.go (2)
7-7: The import statement
github.com/decred/dcrd/dcrec/secp256k1/v4is renamed tosecp256k1dcrd. Ensure that this change does not affect the rest of the codebase and that the new import path is correct.345-347: The
pubKeyDecoratorandIncrementSequenceDecoratorare now chained together in theantehandler. This is a change from the previous version where only theIncrementSequenceDecoratorwas used. Ensure that this change is intentional and that it does not introduce any unexpected behavior.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- tests/integration/bank/keeper/deterministic_test.go (17 hunks)
- tests/integration/staking/keeper/deterministic_test.go (26 hunks)
Files skipped from review due to trivial changes (2)
- tests/integration/bank/keeper/deterministic_test.go
- tests/integration/staking/keeper/deterministic_test.go
|
Done with the comments. Thx for the review @alexanderbez |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- x/auth/ante/sigverify.go (6 hunks)
- x/auth/ante/sigverify_test.go (3 hunks)
Additional comments: 10
x/auth/ante/sigverify_test.go (4)
4-10: The new import
secp256k1dcrdis used in the testTestAnteHandlerChecksto generate a private key that is not on the curve. This is a good test case to ensure that the code correctly handles invalid keys.342-348: The
IncrementSequenceDecoratoris added to theantehandlerchain. This decorator is responsible for incrementing the sequence of the account after the transaction is processed. This is a necessary step to prevent replay attacks.359-368: The test cases for
TestIncrementSequenceDecoratorare well defined. They test the decorator in different contexts (recheck, check, simulate) and ensure that the sequence is correctly incremented.370-471: The
TestAnteHandlerChecksfunction tests different key types and their behavior. It includes a test case for asecp256k1key that is not on the curve, which is a good test case to ensure that the code correctly handles invalid keys.x/auth/ante/sigverify.go (6)
4-11: The new imports for
secp256k1dcrdanderrorsare added correctly.95-100: The
verifyIsOnCurvefunction is correctly called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.191-199: The
verifyIsOnCurvefunction is correctly called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.263-300: The
verifyIsOnCurvefunction is implemented correctly. It checks if the public key is on the curve for supported key types. This is a good security practice to prevent invalid keys from being used.335-340: The
verifyIsOnCurvefunction is correctly called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.429-438: The
verifyIsOnCurvefunction is correctly called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/auth/ante/sigverify.go (6 hunks)
Additional comments: 6
x/auth/ante/sigverify.go (6)
4-13: New import added for
secp256k1dcrdpackage. Ensure that the package is compatible with the current project and its dependencies.96-101: The
verifyIsOnCurvefunction is called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.192-200: The
verifyIsOnCurvefunction is called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.261-302: The
verifyIsOnCurvefunction is added to verify if a public key is on the curve. It supportssecp256k1,secp256r1, and multisig public keys. This is a good security practice to prevent invalid keys from being used.336-345: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [336-439]
The
verifyIsOnCurvefunction is called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.
- 430-439: The
verifyIsOnCurvefunction is called to check if the public key is on the curve. This is a good security practice to prevent invalid keys from being used.
… into feature/anteKeysChecks
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/auth/ante/sigverify_test.go (3 hunks)
Additional comments: 3
x/auth/ante/sigverify_test.go (3)
359-367: The test cases are well structured and cover a variety of scenarios. However, it would be beneficial to add comments explaining what each test case is testing for better readability and maintainability.
396-400: The creation of an invalid secp256k1 key is done correctly. This is a good test case to ensure that the system correctly handles invalid keys.
434-470: The test cases are well structured and cover a variety of scenarios. However, it would be beneficial to add comments explaining what each test case is testing for better readability and maintainability.
| @@ -69,7 +69,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
| * [#17733](https://github.com/cosmos/cosmos-sdk/pull/17733) Ensure `buf export` exports all proto dependencies | |||
| * (version) [#18063](https://github.com/cosmos/cosmos-sdk/pull/18063) Include additional information in the Info struct. This change enhances the Info struct by adding support for additional information through the ExtraInfo field | |||
| * [#18204](https://github.com/cosmos/cosmos-sdk/pull/18204) Use streaming json parser to parse chain-id from genesis file. | |||
|
|
|||
| * (crypto | x/auth) [#14372](https://github.com/cosmos/cosmos-sdk/pull/18194) Key checks on signatures antehandle | |||
There was a problem hiding this comment.
could we get more information on this, this is vague
There was a problem hiding this comment.
Do you want me to change the changelog or just leave a comment in this PR?
| @@ -250,6 +261,46 @@ func OnlyLegacyAminoSigners(sigData signing.SignatureData) bool { | |||
| } | |||
| } | |||
|
|
|||
| func verifyIsOnCurve(pubKey cryptotypes.PubKey) (err error) { | |||
There was a problem hiding this comment.
is there a performance overhead observed here?
There was a problem hiding this comment.
Didn't observe any noticable changes, do you want me to do some benchmark testing?
|
Answered them @tac0turtle how ∂o you want to proceed? |
Description
Closes: #14372
This PR add checks on antehanler where pubkeys are being used. It check if secp keys are on curve, but not ed since these are not supported yet. It also add tests for cases where keys are or not in the respective curve.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
Infostruct to support additional information through theExtraInfofield.Bug Fixes
buf exportcommand exports all proto dependencies.Security Improvements
Tests