Conversation
| return ( | ||
| // A Signer object always contains these fields and no | ||
| // others. | ||
| obj.isFieldPresent(sfAccount) && | ||
| obj.isFieldPresent(sfSigningPubKey) && | ||
| obj.isFieldPresent(sfTxnSignature) && obj.getCount() == 3); | ||
| if (obj.getCount() != 4 || !obj.isFieldPresent(sfAccount)) | ||
| return false; |
There was a problem hiding this comment.
if (!obj.isFieldPresent(sfAccount))
return false;
// leaf signer
if (obj.isFieldPresent(sfSigningPubKey) &&
obj.isFieldPresent(sfTxnSignature) &&
!obj.isFieldPresent(sfSigners))
return obj.getCount() == 3;
// nested signer
if (!obj.isFieldPresent(sfSigningPubKey) &&
!obj.isFieldPresent(sfTxnSignature) &&
obj.isFieldPresent(sfSigners))
return obj.getCount == 2;
| if (lastAccountID == accountID) | ||
| return Unexpected("Duplicate Signers not allowed."); | ||
| checkSignersArray = [&](STArray const& signersArray, | ||
| AccountID const& parentAccountID, |
There was a problem hiding this comment.
unused parentAccuontID
|
First, thank you for doing this. |
Good question, wonder if there should be some kind of leaf specified limit ? |
| finishMultiSigningData(accountID, s); | ||
| auto const accountID = signer.getAccountID(sfAccount); | ||
|
|
||
| // The account owner may not multisign for themselves. |
There was a problem hiding this comment.
Nested signer matching parent account is not rejected in STTx::checkMultiSign
In the recursive checkSignersArray lambda in STTx::checkMultiSign, the check accountID == txnAccountID only compares against the top-level transaction account ID, not the parentAccountID at each recursion level. The parentAccountID parameter is passed into the lambda but never used.
There was a problem hiding this comment.
But parentAccountID (the account being signed for at the current nesting level) is never checked. This means if account B's signer list includes B itself, and B is a nested signer for account A, the recursive call would pass parentAccountID = B but never reject B appearing in its own nested signer array.
In the original non-recursive code, this wasn't an issue because there was only one level. Now with nesting, a signer at depth N could list themselves as a sub-signer at depth N+1, which should be rejected but isn't.
Note: Transactor::checkMultiSign has cycle detection via the ancestors set which would catch this at the preclaim stage, but STTx::checkMultiSign (the preflight signature verification) does not, allowing malformed transactions to pass signature verification and consume more processing resources than necessary.
| @@ -44,8 +44,9 @@ InnerObjectFormats::InnerObjectFormats() | |||
| sfSigner.getCode(), | |||
| { | |||
| {sfAccount, soeREQUIRED}, | |||
| {sfSigningPubKey, soeREQUIRED}, | |||
| {sfTxnSignature, soeREQUIRED}, | |||
| {sfSigningPubKey, soeOPTIONAL}, | |||
There was a problem hiding this comment.
Making sfSigningPubKey and sfTxnSignature optional changes validation for ALL signers, not just nested
The change in InnerObjectFormats.cpp:47-49 from soeREQUIRED to soeOPTIONAL for sfSigningPubKey and sfTxnSignature in the sfSigner inner object format is a global change that affects all signer deserialization, not just when featureNestedMultiSign is enabled. This means that even without the amendment enabled, a malformed signer object missing sfSigningPubKey or sfTxnSignature will now successfully deserialize (whereas before it would throw during applyTemplate). The runtime checks in STTx::checkMultiSign (STTx.cpp:445-449) and Transactor::checkMultiSign (Transactor.cpp:1101-1107) do validate that leaf signers have these fields, so this is caught later. However, the test in STTx_test.cpp:1797-1803 (Test case 2) had to be commented out because it tested that deserialization itself would reject a signer without sfSigningPubKey. This is a defense-in-depth regression—invalid data that was previously rejected at the serialization layer now passes through to application-level validation.
| // Attempt to match the SignerEntry with a Signer; | ||
| while (iter->account < txSignerAcctID) | ||
| // Check depth limit | ||
| if (depth > maxDepth) |
There was a problem hiding this comment.
No recursion depth limit on STObject deserialization for nested sfSigners
The sfSigner inner object format now includes sfSigners as an optional field (InnerObjectFormats.cpp:49), which means sfSigner objects can recursively contain sfSigners arrays. STObject::set at STObject.cpp:180 has a depth limit of 10, so deeply nested structures won't cause stack overflow during deserialization. However, the STTx checkMultiSign limits depth to 4, while the serializer allows up to 10 levels of nesting. This means an attacker could craft a transaction with 10 levels of nested signers that would successfully deserialize and consume resources during signature checking before being rejected at depth 5. The computational cost grows exponentially with nesting depth since each level can have up to 32 signers.
There was a problem hiding this comment.
Please see the latest code @
XRPLF/rippled#6368
And the XLS
XRPLF/XRPL-Standards#472
This PR is stale
Regards
…uashed # Conflicts: # include/xrpl/protocol/Feature.h # src/libxrpl/protocol/Feature.cpp # src/test/jtx/multisign.h # src/xrpld/app/tx/detail/Transactor.cpp
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #675 +/- ##
==========================================
- Coverage 66.55% 66.54% -0.01%
==========================================
Files 831 831
Lines 78166 78309 +143
Branches 44374 44465 +91
==========================================
+ Hits 52023 52113 +90
- Misses 17797 17830 +33
- Partials 8346 8366 +20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…uashed # Conflicts: # include/xrpl/protocol/detail/features.macro
…ltisign-xahaud-pr-squashed
# Conflicts: # include/xrpl/protocol/Feature.h # include/xrpl/protocol/detail/features.macro
NOTE
Please also see rippled side code @
XRPLF/rippled#6368
And the XLS
XRPLF/XRPL-Standards#472
NestedMultiSign Amendment
Overview
This amendment introduces nested multi-signature validation, allowing signer lists to delegate signing authority to other accounts that themselves have signer lists. This enables hierarchical organizational structures where, for example, a company account can require signatures from department accounts, which in turn require signatures from authorized individuals.
Key Features
Hierarchical Signing (up to 4 levels deep)
Company → Departments → Teams → IndividualsCycle Detection & Quorum Relaxation
effectiveQuorum == 0(all signers cyclic) correctly fail withtefBAD_QUORUMTransaction Structure
Nested signers are represented by including an
sfSignersarray instead ofsfSigningPubKey/sfTxnSignature:{ "Signers": [{ "Signer": { "Account": "DepartmentA", "Signers": [{ "Signer": { "Account": "Employee1", "SigningPubKey": "...", "TxnSignature": "..." } }] } }] }Validation Rules
SigningPubKeyandTxnSignatureError Codes
tefBAD_SIGNATUREtefBAD_QUORUMtefNOT_MULTI_SIGNINGtefMASTER_DISABLEDtefINTERNALBackward Compatibility
When
featureNestedMultiSignis disabled, nested signer arrays are rejected withtemMALFORMED, preserving existing behavior.