fix(export): age and validate stashed tx data for signature checks

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();
     }
 };
 

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.
 

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<STTx const>(std::ref(sit));
+            SerialIter sit(data.slice());
+            auto tx = std::make_shared<STTx const>(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<STTx const>(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