From 07d741cdd70c9301b094df5494d892d049044eda Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Mon, 2 Mar 2026 15:25:19 +0700 Subject: [PATCH] fix(export): harden collector duplicate and identity handling --- .../app/ExportSignatureCollector_test.cpp | 188 ++++++++++++++++++ .../misc/detail/ExportSignatureCollector.cpp | 121 ++++++++--- 2 files changed, 283 insertions(+), 26 deletions(-) create mode 100644 src/test/app/ExportSignatureCollector_test.cpp diff --git a/src/test/app/ExportSignatureCollector_test.cpp b/src/test/app/ExportSignatureCollector_test.cpp new file mode 100644 index 000000000..9eb3131d4 --- /dev/null +++ b/src/test/app/ExportSignatureCollector_test.cpp @@ -0,0 +1,188 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2026 XRPL Labs + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include + +namespace ripple { +namespace test { + +class ExportSignatureCollector_test : public beast::unit_test::suite +{ + static STTx + makeUnsignedTx() + { + auto const txKey = randomKeyPair(KeyType::secp256k1); + auto const txAccount = calcAccountID(txKey.first); + + return STTx(ttACCOUNT_SET, [&txAccount, &txKey](auto& obj) { + obj.setAccountID(sfAccount, txAccount); + obj.setFieldVL(sfMessageKey, txKey.first.slice()); + obj.setFieldVL(sfSigningPubKey, Slice{}); + }); + } + + static STObject + makeSigner(PublicKey const& pk, AccountID const& acc, Blob const& sig) + { + STObject signer(sfSigner); + signer.setAccountID(sfAccount, acc); + signer.setFieldVL(sfSigningPubKey, pk.slice()); + signer.setFieldVL(sfTxnSignature, sig); + return signer; + } + + void + testDuplicateCanReplaceUnverified() + { + testcase("duplicate replaces unverified"); + + beast::Journal journal{beast::Journal::getNullSink()}; + ExportSignatureCollector collector{journal}; + + auto const validator = randomKeyPair(KeyType::secp256k1); + auto const validatorAcc = calcAccountID(validator.first); + + auto tx = makeUnsignedTx(); + auto const txnHash = tx.getTransactionID(); + + Serializer txData; + tx.add(txData); + + Serializer sigData = buildMultiSigningData(tx, validatorAcc); + auto const goodSigBuf = + sign(validator.first, validator.second, sigData.slice()); + + Blob goodSig(goodSigBuf.begin(), goodSigBuf.end()); + Blob badSig = goodSig; + badSig.back() ^= 0x01; + + auto badSigner = makeSigner(validator.first, validatorAcc, badSig); + auto goodSigner = makeSigner(validator.first, validatorAcc, goodSig); + + BEAST_EXPECT(collector.verifyAndAddSignature( + txnHash, validator.first, badSigner, 100)); + BEAST_EXPECT(!collector.isSignatureVerified(txnHash, validator.first)); + + collector.stashTxnData(txnHash, txData); + + BEAST_EXPECT(collector.verifyAndAddSignature( + txnHash, validator.first, goodSigner, 101)); + BEAST_EXPECT(collector.isSignatureVerified(txnHash, validator.first)); + BEAST_EXPECT(collector.verifySignature(txnHash, validator.first)); + + auto const stored = + collector.getSignatureFrom(txnHash, validator.first); + BEAST_EXPECT(stored); + if (stored) + BEAST_EXPECT(stored->getFieldVL(sfTxnSignature) == goodSig); + } + + void + testVerifiedIsNotReplaced() + { + testcase("verified is stable"); + + beast::Journal journal{beast::Journal::getNullSink()}; + ExportSignatureCollector collector{journal}; + + auto const validator = randomKeyPair(KeyType::secp256k1); + auto const validatorAcc = calcAccountID(validator.first); + + auto tx = makeUnsignedTx(); + auto const txnHash = tx.getTransactionID(); + + Serializer txData; + tx.add(txData); + collector.stashTxnData(txnHash, txData); + + Serializer sigData = buildMultiSigningData(tx, validatorAcc); + auto const goodSigBuf = + sign(validator.first, validator.second, sigData.slice()); + + Blob goodSig(goodSigBuf.begin(), goodSigBuf.end()); + Blob badSig = goodSig; + badSig.back() ^= 0x01; + + auto goodSigner = makeSigner(validator.first, validatorAcc, goodSig); + auto badSigner = makeSigner(validator.first, validatorAcc, badSig); + + BEAST_EXPECT(collector.verifyAndAddSignature( + txnHash, validator.first, goodSigner, 200)); + BEAST_EXPECT(collector.isSignatureVerified(txnHash, validator.first)); + + BEAST_EXPECT(collector.verifyAndAddSignature( + txnHash, validator.first, badSigner, 201)); + BEAST_EXPECT(collector.verifySignature(txnHash, validator.first)); + + auto const stored = + collector.getSignatureFrom(txnHash, validator.first); + BEAST_EXPECT(stored); + if (stored) + BEAST_EXPECT(stored->getFieldVL(sfTxnSignature) == goodSig); + } + + void + testRejectsSignerIdentityMismatch() + { + testcase("reject signer identity mismatch"); + + beast::Journal journal{beast::Journal::getNullSink()}; + ExportSignatureCollector collector{journal}; + + auto const validatorA = randomKeyPair(KeyType::secp256k1); + auto const validatorB = randomKeyPair(KeyType::secp256k1); + auto const validatorBAcc = calcAccountID(validatorB.first); + + auto tx = makeUnsignedTx(); + auto const txnHash = tx.getTransactionID(); + + Serializer txData; + tx.add(txData); + collector.stashTxnData(txnHash, txData); + + Serializer sigData = buildMultiSigningData(tx, validatorBAcc); + auto const sigBuf = + sign(validatorB.first, validatorB.second, sigData.slice()); + Blob sig(sigBuf.begin(), sigBuf.end()); + + auto mismatchedSigner = + makeSigner(validatorB.first, validatorBAcc, sig); + + BEAST_EXPECT(!collector.verifyAndAddSignature( + txnHash, validatorA.first, mismatchedSigner, 300)); + BEAST_EXPECT(!collector.hasSignatureFrom(txnHash, validatorA.first)); + BEAST_EXPECT(!collector.hasSignatureFrom(txnHash, validatorB.first)); + } + +public: + void + run() override + { + testDuplicateCanReplaceUnverified(); + testVerifiedIsNotReplaced(); + testRejectsSignerIdentityMismatch(); + } +}; + +BEAST_DEFINE_TESTSUITE(ExportSignatureCollector, app, ripple); + +} // namespace test +} // namespace ripple diff --git a/src/xrpld/app/misc/detail/ExportSignatureCollector.cpp b/src/xrpld/app/misc/detail/ExportSignatureCollector.cpp index d73cf0635..f7e35f1dd 100644 --- a/src/xrpld/app/misc/detail/ExportSignatureCollector.cpp +++ b/src/xrpld/app/misc/detail/ExportSignatureCollector.cpp @@ -335,35 +335,60 @@ ExportSignatureCollector::verifyAndAddSignature( << " at ledger " << currentSeq; } - // Check if we already have this signature - auto& signerMap = signatures_[txnHash]; - if (signerMap.find(validator) != signerMap.end()) + // The signer payload must bind to the validator who carried it in + // TMValidation. Otherwise a peer can misattribute signatures. + if (!signer.isFieldPresent(sfSigningPubKey) || + !signer.isFieldPresent(sfAccount) || + !signer.isFieldPresent(sfTxnSignature)) { - JLOG(j_.trace()) << "Export: already have signature from " - << toBase58(TokenType::NodePublic, validator) - << " for " << txnHash; - return true; // Already have it + JLOG(j_.warn()) << "Export: malformed signer payload for " << txnHash + << " from " + << toBase58(TokenType::NodePublic, validator); + return false; } - // Try to verify if we have the txn data - bool verified = false; - auto txnIt = exportedTxnData_.find(txnHash); - if (txnIt != exportedTxnData_.end()) + auto const sigPubKey = signer.getFieldVL(sfSigningPubKey); + if (sigPubKey.empty() || !publicKeyType(makeSlice(sigPubKey))) { + JLOG(j_.warn()) << "Export: invalid signer pubkey payload for " + << txnHash << " from " + << toBase58(TokenType::NodePublic, validator); + return false; + } + + auto const signingAcc = signer.getAccountID(sfAccount); + PublicKey const signerPubKey{makeSlice(sigPubKey)}; + if (signerPubKey != validator || signingAcc != calcAccountID(validator)) + { + JLOG(j_.warn()) << "Export: signer identity mismatch for " << txnHash + << " from " + << toBase58(TokenType::NodePublic, validator); + return false; + } + + auto& signerMap = signatures_[txnHash]; + + // Verify if we have stashed tx data. Returns: + // true -> verified + // false -> verification failed + // nullopt -> cannot verify yet (no tx data) + auto verifyWithStashedData = + [&](STObject const& candidate) -> std::optional { + auto txnIt = exportedTxnData_.find(txnHash); + if (txnIt == exportedTxnData_.end()) + return std::nullopt; + try { - // Parse the stashed transaction SerialIter sit(txnIt->second.slice()); auto stpTrans = std::make_shared(std::ref(sit)); - // Get signer account from the signer object - auto signingAcc = signer.getAccountID(sfAccount); - auto sigPubKey = signer.getFieldVL(sfSigningPubKey); - auto signature = signer.getFieldVL(sfTxnSignature); + auto signingAcc = candidate.getAccountID(sfAccount); + auto sigPubKey = candidate.getFieldVL(sfSigningPubKey); + auto signature = candidate.getFieldVL(sfTxnSignature); - // Build the multisig data and verify Serializer sigData = buildMultiSigningData(*stpTrans, signingAcc); - verified = ripple::verify( + bool const verified = ripple::verify( PublicKey(makeSlice(sigPubKey)), sigData.slice(), makeSlice(signature), @@ -374,39 +399,83 @@ ExportSignatureCollector::verifyAndAddSignature( JLOG(j_.warn()) << "Export: signature verification FAILED for " << txnHash << " from " << toBase58(TokenType::NodePublic, validator); - return false; // Don't add invalid signature + return false; } JLOG(j_.trace()) << "Export: signature verified for " << txnHash << " from " << toBase58(TokenType::NodePublic, validator); + return true; } catch (std::exception const& e) { JLOG(j_.warn()) << "Export: signature verification exception for " << txnHash << ": " << e.what(); - return false; // Don't add if we can't verify + return false; } + }; + + auto verIt = verified_.find(txnHash); + bool const alreadyVerified = verIt != verified_.end() && + verIt->second.find(validator) != verIt->second.end(); + + // Duplicate from same validator: + // - keep existing once cryptographically verified + // - allow replacement while unverified so a stale/bad early copy can heal + if (auto existing = signerMap.find(validator); existing != signerMap.end()) + { + if (alreadyVerified) + { + JLOG(j_.trace()) + << "Export: ignoring duplicate verified signature " + << "from " << toBase58(TokenType::NodePublic, validator) + << " for " << txnHash; + return true; + } + + auto const verified = verifyWithStashedData(signer); + if (verified && !*verified) + return false; // Reject replacement with invalid signature + + if (!verified) + { + JLOG(j_.trace()) + << "Export: replacing unverified signature for " << txnHash + << " from " << toBase58(TokenType::NodePublic, validator) + << " (no txn data yet)"; + } + + existing->second = std::move(signer); + if (verified && *verified) + verified_[txnHash].insert(validator); + + JLOG(j_.trace()) << "Export: replaced signature from " + << toBase58(TokenType::NodePublic, validator) + << " for " << txnHash + << " (verified=" << (verified && *verified) << ")"; + return true; } - else + + auto const verified = verifyWithStashedData(signer); + if (verified && !*verified) + return false; // Don't add invalid signature + + if (!verified) { // No txn data yet - add unverified (will verify later or in Transactor) JLOG(j_.trace()) << "Export: adding unverified signature for " << txnHash << " (no txn data yet)"; } - // Add the signature signerMap.emplace(validator, std::move(signer)); - if (verified) - { + if (verified && *verified) verified_[txnHash].insert(validator); - } JLOG(j_.trace()) << "Export: added signature from " << toBase58(TokenType::NodePublic, validator) << " for " << txnHash << " (total: " << signerMap.size() - << ", verified=" << verified << ")"; + << ", verified=" << (verified && *verified) << ")"; return true; }