From 514e60b71c6c5757ebc69f4794d733ebd830d8d8 Mon Sep 17 00:00:00 2001 From: Nicholas Dudfield Date: Mon, 2 Mar 2026 15:54:53 +0700 Subject: [PATCH] fix(export): age and validate stashed tx data for signature checks --- .../app/ExportSignatureCollector_test.cpp | 81 +++++++- src/xrpld/app/misc/ExportSignatureCollector.h | 6 +- .../misc/detail/ExportSignatureCollector.cpp | 191 +++++++++++------- 3 files changed, 193 insertions(+), 85 deletions(-) diff --git a/src/test/app/ExportSignatureCollector_test.cpp b/src/test/app/ExportSignatureCollector_test.cpp index 86775bbfe..2fe3504ac 100644 --- a/src/test/app/ExportSignatureCollector_test.cpp +++ b/src/test/app/ExportSignatureCollector_test.cpp @@ -81,7 +81,7 @@ class ExportSignatureCollector_test : public beast::unit_test::suite txnHash, validator.first, badSigner, 100)); BEAST_EXPECT(!collector.isSignatureVerified(txnHash, validator.first)); - collector.stashTxnData(txnHash, txData); + collector.stashTxnData(txnHash, txData, 100); BEAST_EXPECT(collector.verifyAndAddSignature( txnHash, validator.first, goodSigner, 101)); @@ -111,7 +111,7 @@ class ExportSignatureCollector_test : public beast::unit_test::suite Serializer txData; tx.add(txData); - collector.stashTxnData(txnHash, txData); + collector.stashTxnData(txnHash, txData, 200); Serializer sigData = buildMultiSigningData(tx, validatorAcc); auto const goodSigBuf = @@ -156,7 +156,7 @@ class ExportSignatureCollector_test : public beast::unit_test::suite Serializer txData; tx.add(txData); - collector.stashTxnData(txnHash, txData); + collector.stashTxnData(txnHash, txData, 300); Serializer sigData = buildMultiSigningData(tx, validatorBAcc); auto const sigBuf = @@ -202,7 +202,7 @@ class ExportSignatureCollector_test : public beast::unit_test::suite txnHash, validator.first, badSigner, 400)); BEAST_EXPECT(collector.signatureCount(txnHash) == 1); - collector.stashTxnData(txnHash, txData); + collector.stashTxnData(txnHash, txData, 400); BEAST_EXPECT(collector.signatureCount(txnHash) == 0); BEAST_EXPECT(!collector.hasSignatureFrom(txnHash, validator.first)); @@ -296,7 +296,7 @@ class ExportSignatureCollector_test : public beast::unit_test::suite Serializer txData; tx.add(txData); - collector.stashTxnData(txnHash, txData); + collector.stashTxnData(txnHash, txData, 500); Serializer sigData = buildMultiSigningData(tx, validatorAcc); auto const goodSigBuf = @@ -352,7 +352,7 @@ class ExportSignatureCollector_test : public beast::unit_test::suite Serializer txData; tx.add(txData); - collector.stashTxnData(txnHash, txData); + collector.stashTxnData(txnHash, txData, 601); Serializer sigData = buildMultiSigningData(tx, validatorAcc); auto const goodSigBuf = @@ -382,7 +382,7 @@ class ExportSignatureCollector_test : public beast::unit_test::suite Serializer txData; tx.add(txData); - collector.stashTxnData(txnHash, txData); + collector.stashTxnData(txnHash, txData, 602); Serializer sigData = buildMultiSigningData(tx, validatorAcc); auto const goodSigBuf = @@ -404,6 +404,71 @@ class ExportSignatureCollector_test : public beast::unit_test::suite BEAST_EXPECT(collector.isSignatureVerified(txnHash, validator.first)); } + void + testStaleCleanupRemovesTxnDataOnlyEntries() + { + testcase("stale cleanup removes txn-data-only entries"); + + 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, 10); + + collector.cleanupStale(300, 256); + + Serializer sigData = buildMultiSigningData(tx, validatorAcc); + auto const goodSigBuf = + sign(validator.first, validator.second, sigData.slice()); + Blob badSig(goodSigBuf.begin(), goodSigBuf.end()); + badSig.back() ^= 0x01; + + auto badSigner = makeSigner(validator.first, validatorAcc, badSig); + BEAST_EXPECT(collector.verifyAndAddSignature( + txnHash, validator.first, badSigner, 301)); + BEAST_EXPECT(!collector.isSignatureVerified(txnHash, validator.first)); + } + + void + testStashTxnDataRejectsPoisonedCache() + { + testcase("stashTxnData rejects poisoned cache"); + + 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 badData; + badData.add8(0x00); + collector.stashTxnData(txnHash, badData, 1); + + Serializer goodData; + tx.add(goodData); + collector.stashTxnData(txnHash, goodData, 2); + + Serializer sigData = buildMultiSigningData(tx, validatorAcc); + auto const goodSigBuf = + sign(validator.first, validator.second, sigData.slice()); + Blob goodSig(goodSigBuf.begin(), goodSigBuf.end()); + auto goodSigner = makeSigner(validator.first, validatorAcc, goodSig); + + BEAST_EXPECT(collector.verifyAndAddSignature( + txnHash, validator.first, goodSigner, 3)); + BEAST_EXPECT(collector.isSignatureVerified(txnHash, validator.first)); + } + public: void run() override @@ -418,6 +483,8 @@ public: testAddSignatureRejectsIdentityMismatch(); testAddSignatureVerifiesWhenTxnDataPresent(); testAddSignatureRejectsInvalidReplacementWhenVerified(); + testStaleCleanupRemovesTxnDataOnlyEntries(); + testStashTxnDataRejectsPoisonedCache(); } }; diff --git a/src/xrpld/app/misc/ExportSignatureCollector.h b/src/xrpld/app/misc/ExportSignatureCollector.h index 6bd042a21..3e9d7346f 100644 --- a/src/xrpld/app/misc/ExportSignatureCollector.h +++ b/src/xrpld/app/misc/ExportSignatureCollector.h @@ -201,9 +201,13 @@ public: @param txnHash The hash of the exported transaction @param txnData Serialized STTx for building verification data + @param currentSeq Current ledger sequence (for stale cleanup aging) */ void - stashTxnData(uint256 const& txnHash, Serializer txnData); + stashTxnData( + uint256 const& txnHash, + Serializer txnData, + LedgerIndex currentSeq); /** Verify and add a signature. diff --git a/src/xrpld/app/misc/detail/ExportSignatureCollector.cpp b/src/xrpld/app/misc/detail/ExportSignatureCollector.cpp index 3fd1743fc..bc93ff0d4 100644 --- a/src/xrpld/app/misc/detail/ExportSignatureCollector.cpp +++ b/src/xrpld/app/misc/detail/ExportSignatureCollector.cpp @@ -412,103 +412,140 @@ ExportSignatureCollector::cleanupStale( void ExportSignatureCollector::stashTxnData( uint256 const& txnHash, - Serializer txnData) + Serializer txnData, + LedgerIndex currentSeq) { std::lock_guard lock(mutex_); - // Only stash if we don't already have it - if (exportedTxnData_.find(txnHash) == exportedTxnData_.end()) - { - exportedTxnData_.emplace(txnHash, std::move(txnData)); - JLOG(j_.trace()) << "Export: stashed txn data for " << txnHash; - - auto sigIt = signatures_.find(txnHash); - if (sigIt == signatures_.end()) - return; - - std::size_t pruned = 0; + auto parseTx = [](Serializer const& data) -> bool { try { - SerialIter sit(exportedTxnData_.at(txnHash).slice()); - auto stpTrans = std::make_shared(std::ref(sit)); + SerialIter sit(data.slice()); + auto tx = std::make_shared(std::ref(sit)); + (void)tx; + return true; + } + catch (std::exception const&) + { + return false; + } + }; - auto& signerMap = sigIt->second; - auto& verifiedSet = verified_[txnHash]; - for (auto it = signerMap.begin(); it != signerMap.end();) + if (!parseTx(txnData)) + { + JLOG(j_.warn()) << "Export: rejected invalid txn data for " << txnHash; + return; + } + + bool stored = false; + if (auto it = exportedTxnData_.find(txnHash); it == exportedTxnData_.end()) + { + exportedTxnData_.emplace(txnHash, std::move(txnData)); + stored = true; + JLOG(j_.trace()) << "Export: stashed txn data for " << txnHash; + } + else if (!parseTx(it->second)) + { + it->second = std::move(txnData); + stored = true; + JLOG(j_.warn()) << "Export: replaced invalid cached txn data for " + << txnHash; + } + + if (!stored) + return; + + if (firstSeenLedger_.find(txnHash) == firstSeenLedger_.end()) + { + firstSeenLedger_[txnHash] = currentSeq; + JLOG(j_.trace()) << "Export: first-seen (txn data) for " << txnHash + << " at ledger " << currentSeq; + } + + auto sigIt = signatures_.find(txnHash); + if (sigIt == signatures_.end()) + return; + + std::size_t pruned = 0; + try + { + SerialIter sit(exportedTxnData_.at(txnHash).slice()); + auto stpTrans = std::make_shared(std::ref(sit)); + + auto& signerMap = sigIt->second; + auto& verifiedSet = verified_[txnHash]; + for (auto it = signerMap.begin(); it != signerMap.end();) + { + auto const& validator = it->first; + auto const& signer = it->second; + + bool valid = false; + try { - auto const& validator = it->first; - auto const& signer = it->second; - - bool valid = false; - try + if (signer.isFieldPresent(sfSigningPubKey) && + signer.isFieldPresent(sfAccount) && + signer.isFieldPresent(sfTxnSignature)) { - if (signer.isFieldPresent(sfSigningPubKey) && - signer.isFieldPresent(sfAccount) && - signer.isFieldPresent(sfTxnSignature)) - { - auto const sigPubKey = - signer.getFieldVL(sfSigningPubKey); - auto const signingAcc = signer.getAccountID(sfAccount); - auto const signature = - signer.getFieldVL(sfTxnSignature); + auto const sigPubKey = signer.getFieldVL(sfSigningPubKey); + auto const signingAcc = signer.getAccountID(sfAccount); + auto const signature = signer.getFieldVL(sfTxnSignature); - if (!sigPubKey.empty() && - publicKeyType(makeSlice(sigPubKey))) + if (!sigPubKey.empty() && + publicKeyType(makeSlice(sigPubKey))) + { + PublicKey const signerPk{makeSlice(sigPubKey)}; + if (signerPk == validator && + signingAcc == calcAccountID(validator)) { - PublicKey const signerPk{makeSlice(sigPubKey)}; - if (signerPk == validator && - signingAcc == calcAccountID(validator)) - { - Serializer sigData = buildMultiSigningData( - *stpTrans, signingAcc); - valid = ripple::verify( - signerPk, - sigData.slice(), - makeSlice(signature), - true); - } + Serializer sigData = + buildMultiSigningData(*stpTrans, signingAcc); + valid = ripple::verify( + signerPk, + sigData.slice(), + makeSlice(signature), + true); } } } - catch (std::exception const&) - { - valid = false; - } - - if (valid) - { - verifiedSet.insert(validator); - ++it; - } - else - { - verifiedSet.erase(validator); - it = signerMap.erase(it); - ++pruned; - } } - - if (verifiedSet.empty()) - verified_.erase(txnHash); - - if (signerMap.empty()) + catch (std::exception const&) { - signatures_.erase(sigIt); - firstSeenLedger_.erase(txnHash); + valid = false; + } + + if (valid) + { + verifiedSet.insert(validator); + ++it; + } + else + { + verifiedSet.erase(validator); + it = signerMap.erase(it); + ++pruned; } } - catch (std::exception const& e) - { - JLOG(j_.warn()) << "Export: failed to parse stashed txn data for " - << txnHash << ": " << e.what(); - } - if (pruned > 0) + if (verifiedSet.empty()) + verified_.erase(txnHash); + + if (signerMap.empty()) { - JLOG(j_.warn()) << "Export: pruned " << pruned - << " invalid unverified signatures for " << txnHash; + signatures_.erase(sigIt); + firstSeenLedger_.erase(txnHash); } } + catch (std::exception const& e) + { + JLOG(j_.warn()) << "Export: failed to parse stashed txn data for " + << txnHash << ": " << e.what(); + } + + if (pruned > 0) + { + JLOG(j_.warn()) << "Export: pruned " << pruned + << " invalid unverified signatures for " << txnHash; + } } bool @@ -873,7 +910,7 @@ signPendingExports( // This must happen before checking for cached signature so that // peer signatures can be verified against this txn data. auto& collector = app.getExportSignatureCollector(); - collector.stashTxnData(txnHash, *s); + collector.stashTxnData(txnHash, *s, seq); // Check if we already have our signature cached in the collector. // This enables continuous broadcasting: we sign once, then keep