fix(crypto): strengthen signature length validation#6776
Open
Federico2014 wants to merge 5 commits into
Open
Conversation
Sunny6889
reviewed
May 18, 2026
3for
reviewed
May 18, 2026
3for
reviewed
May 18, 2026
6185c26 to
3fa9b53
Compare
Sunny6889
reviewed
May 18, 2026
Sunny6889
reviewed
May 18, 2026
Sunny6889
reviewed
May 18, 2026
Sunny6889
reviewed
May 18, 2026
Sunny6889
reviewed
May 18, 2026
| private boolean validPbftSign(Raw raw, List<ByteString> srSignList, | ||
| List<ByteString> currentSrList) { | ||
| //valid sr list | ||
| if (srSignList.size() < Param.getInstance().getAgreeNodeCount()) { |
Contributor
There was a problem hiding this comment.
Put this line above if (srSignList.size() != 0) {, when srSignList.size() == 0 old logic will return true, while new logic will return false.
Collaborator
Author
There was a problem hiding this comment.
This is intentional. An empty srSignList cannot meet the 2/3 + 1 quorum, so returning true in that path was a latent bug: the old code accepted a PBFT commit message with zero attached signatures as if it had passed consensus. The new guard rejects it consistently with the size-based quorum check.
Sunny6889
reviewed
May 18, 2026
Sunny6889
reviewed
May 18, 2026
Sunny6889
reviewed
May 18, 2026
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.
Summary
ChainConstant.MIN_SIGNATURE_SIZE(65) andChainConstant.MAX_SIGNATURE_SIZE(68), and centralize the predicate inSignUtils.isValidLength(int size, boolean checkMaxSignatureSize).DynamicPropertiesStore.signatureMaxSizeChecked(), which reflects whetherALLOW_TVM_OSAKAis enabled.Wallet.getTransactionApprovedListon a lower-bound-only check, so historical transactions with padded signatures remain inspectable.Design Notes
Why use
[65, 68]instead of strict65?After TVM Osaka is enabled, the accepted signature-length window is
[65, 68]bytes rather than strict== 65.This is a deliberate compatibility trade-off:
r || s || v).MAX_SIGNATURE_SIZE = 68closes the previously unbounded encoding window while remaining compatible with historical data already on chain.== 65once the compatibility window is no longer needed.Because the bounds are centralized in
ChainConstantand the predicate is centralized inSignUtils.isValidLength, tightening the rule later will be straightforward.Why gate the upper bound with
signatureMaxSizeChecked()?The upper bound is enforced through
DynamicPropertiesStore.signatureMaxSizeChecked(), which currently maps togetAllowTvmOsaka() == 1L.This keeps the policy decision in chain state while keeping the signature-length predicate itself small and reusable:
SignUtils.isValidLength(size, false)enforces only the lower bound.SignUtils.isValidLength(size, true)enforces both the lower and upper bounds.DynamicPropertiesStore.Wallet.getTransactionApprovedListintentionally passesfalseso historical padded signatures remain readable even after Osaka is enabled.PBFT quorum accounting
validPbftSignnow deduplicates accepted votes by recovered SR address rather than by raw signature bytes.This keeps quorum accounting robust even when the same signer submits signatures with different byte-level encodings. A regression test,
PbftDataSyncHandlerTest.testValidPbftSignDedupesByAddress, covers this behavior.Compatibility
This PR tightens validation behavior, but it does not apply the new upper bound uniformly to every path:
>= 65) is enforced.[65, 68]).Wallet.getTransactionApprovedListremains intentionally more permissive and continues to skip the upper bound so historical transactions can still be inspected.Breaking Changes
The following public method signatures changed:
TransactionCapsule.checkWeight(Permission, List<ByteString>, byte[], List<ByteString>)(Permission, List<ByteString>, byte[], List<ByteString>, boolean)TransactionCapsule.addSign(byte[], AccountStore)(byte[], AccountStore, DynamicPropertiesStore)All in-tree callers were updated.
External JVM consumers of the
chainbaseartifact, such as SDKs, signing services, and third-party tools, must update these call sites. Existing callers will fail at compile time rather than silently misbehave at runtime.Main Code Changes
common/.../config/Parameter.javaMIN_SIGNATURE_SIZEandMAX_SIGNATURE_SIZEtoChainConstantcrypto/.../SignUtils.javaisValidLength(int, boolean)chainbase/.../DynamicPropertiesStore.javasignatureMaxSizeChecked()chainbase/.../TransactionCapsule.javacheckWeightandaddSignchainbase/.../BlockCapsule.javavalidateSignatureframework/.../Wallet.javaSignUtils.isValidLength; keep the read-only approved-list path lower-bound-onlyframework/.../RelayService.javacheckHelloMessageframework/.../PbftDataSyncHandler.javaValidPbftSignTask; deduplicate quorum by recovered SR addressframework/.../PbftMsgHandler.javaDynamicPropertiesStoreinto PBFT signature analysisconsensus/.../PbftBaseMessage.javaanalyzeSignatureconsensus/.../PbftMessageHandle.javaDynamicPropertiesStoreinto PBFT signature analysisactuator/.../TransactionUtil.javacheckWeightcall siteTest Coverage Added or Updated
TransactionCapsuleTestBlockCapsuleTestTransactionExpireTestgetTransactionApprovedListafter TVM Osaka.TransactionUtilTestgetTransactionSignWeight.PbftDataSyncHandlerTestPbftMsgHandlerTestRelayServiceTestShieldedTransferActuatorTest,ShieldedReceiveTest, andframework/.../TransactionUtils.javaTransactionCapsule.addSign(..., DynamicPropertiesStore)signature.