diff --git a/src/test/app/ExportSignatureCollector_test.cpp b/src/test/app/ExportSignatureCollector_test.cpp index f6deb0031..86775bbfe 100644 --- a/src/test/app/ExportSignatureCollector_test.cpp +++ b/src/test/app/ExportSignatureCollector_test.cpp @@ -311,6 +311,99 @@ class ExportSignatureCollector_test : public beast::unit_test::suite BEAST_EXPECT(collector.getPendingExports().empty()); } + void + testAddSignatureRejectsIdentityMismatch() + { + testcase("addSignature rejects 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); + std::string const tag = "add-signature-identity-mismatch"; + auto const txnHash = sha512Half(makeSlice(tag)); + + STObject signer(sfSigner); + signer.setAccountID(sfAccount, validatorBAcc); + signer.setFieldVL(sfSigningPubKey, validatorB.first.slice()); + signer.setFieldVL(sfTxnSignature, Blob{0x01, 0x02}); + + collector.addSignature(txnHash, validatorA.first, signer, 600); + + BEAST_EXPECT(!collector.hasSignatureFrom(txnHash, validatorA.first)); + BEAST_EXPECT(collector.signatureCount(txnHash) == 0); + } + + void + testAddSignatureVerifiesWhenTxnDataPresent() + { + testcase("addSignature verifies when txn data present"); + + 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()); + auto signer = makeSigner(validator.first, validatorAcc, goodSig); + + collector.addSignature(txnHash, validator.first, signer, 601); + + BEAST_EXPECT(collector.hasSignatureFrom(txnHash, validator.first)); + BEAST_EXPECT(collector.isSignatureVerified(txnHash, validator.first)); + } + + void + testAddSignatureRejectsInvalidReplacementWhenVerified() + { + testcase("addSignature rejects invalid replacement when verified"); + + 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()); + auto goodSigner = makeSigner(validator.first, validatorAcc, goodSig); + collector.addSignature(txnHash, validator.first, goodSigner, 602); + + Blob badSig = goodSig; + badSig.back() ^= 0x01; + auto badSigner = makeSigner(validator.first, validatorAcc, badSig); + collector.addSignature(txnHash, validator.first, badSigner, 603); + + auto const stored = + collector.getSignatureFrom(txnHash, validator.first); + BEAST_EXPECT(stored); + if (stored) + BEAST_EXPECT(stored->getFieldVL(sfTxnSignature) == goodSig); + BEAST_EXPECT(collector.isSignatureVerified(txnHash, validator.first)); + } + public: void run() override @@ -322,6 +415,9 @@ public: testInvalidEarlyDataDoesNotAgeOutValidLater(); testAddSignatureUpdatesExisting(); testRejectedSignatureDoesNotCreatePendingEntry(); + testAddSignatureRejectsIdentityMismatch(); + testAddSignatureVerifiesWhenTxnDataPresent(); + testAddSignatureRejectsInvalidReplacementWhenVerified(); } }; diff --git a/src/xrpld/app/misc/detail/ExportSignatureCollector.cpp b/src/xrpld/app/misc/detail/ExportSignatureCollector.cpp index a9cb918d4..3fd1743fc 100644 --- a/src/xrpld/app/misc/detail/ExportSignatureCollector.cpp +++ b/src/xrpld/app/misc/detail/ExportSignatureCollector.cpp @@ -101,33 +101,131 @@ ExportSignatureCollector::addSignature( { std::lock_guard lock(mutex_); - // Track first-seen time for cleanup - if (firstSeenLedger_.find(txnHash) == firstSeenLedger_.end()) + // Validate signer payload binds to the validator key. + if (!signer.isFieldPresent(sfSigningPubKey) || + !signer.isFieldPresent(sfAccount) || + !signer.isFieldPresent(sfTxnSignature)) { - firstSeenLedger_[txnHash] = currentSeq; - JLOG(j_.debug()) << "Export: first signature for " << txnHash - << " at ledger " << currentSeq; + JLOG(j_.warn()) << "Export: addSignature rejected malformed signer for " + << txnHash << " from " + << toBase58(TokenType::NodePublic, validator); + return; } - // Add or update signature for this validator + auto const sigPubKey = signer.getFieldVL(sfSigningPubKey); + if (sigPubKey.empty() || !publicKeyType(makeSlice(sigPubKey))) + { + JLOG(j_.warn()) << "Export: addSignature rejected invalid pubkey for " + << txnHash << " from " + << toBase58(TokenType::NodePublic, validator); + return; + } + + auto const signingAcc = signer.getAccountID(sfAccount); + PublicKey const signerPubKey{makeSlice(sigPubKey)}; + if (signerPubKey != validator || signingAcc != calcAccountID(validator)) + { + JLOG(j_.warn()) + << "Export: addSignature rejected identity mismatch for " << txnHash + << " from " << toBase58(TokenType::NodePublic, validator); + return; + } + + // Verify immediately if tx data is available. + auto verifyWithStashedData = + [&](STObject const& candidate) -> std::optional { + auto txnIt = exportedTxnData_.find(txnHash); + if (txnIt == exportedTxnData_.end()) + return std::nullopt; + + try + { + SerialIter sit(txnIt->second.slice()); + auto stpTrans = std::make_shared(std::ref(sit)); + + auto const candAcc = candidate.getAccountID(sfAccount); + auto const candPub = candidate.getFieldVL(sfSigningPubKey); + auto const candSig = candidate.getFieldVL(sfTxnSignature); + Serializer sigData = buildMultiSigningData(*stpTrans, candAcc); + + return ripple::verify( + PublicKey(makeSlice(candPub)), + sigData.slice(), + makeSlice(candSig), + true); + } + catch (std::exception const& e) + { + JLOG(j_.warn()) + << "Export: addSignature verification exception for " << txnHash + << ": " << e.what(); + return false; + } + }; + + auto const verified = verifyWithStashedData(signer); + if (verified && !*verified) + { + JLOG(j_.warn()) + << "Export: addSignature rejected invalid signature for " << txnHash + << " from " << toBase58(TokenType::NodePublic, validator); + return; + } + + auto ensureFirstSeen = [&]() { + if (firstSeenLedger_.find(txnHash) == firstSeenLedger_.end()) + { + firstSeenLedger_[txnHash] = currentSeq; + JLOG(j_.debug()) << "Export: first signature for " << txnHash + << " at ledger " << currentSeq; + } + }; + auto& signerMap = signatures_[txnHash]; - auto [it, inserted] = - signerMap.insert_or_assign(validator, std::move(signer)); + auto verIt = verified_.find(txnHash); + bool const alreadyVerified = verIt != verified_.end() && + verIt->second.find(validator) != verIt->second.end(); - if (inserted) - { - JLOG(j_.trace()) << "Export: added signature from " - << toBase58(TokenType::NodePublic, validator) - << " for " << txnHash - << " (total: " << signerMap.size() << ")"; - } - else + if (auto existing = signerMap.find(validator); existing != signerMap.end()) { + // Never downgrade a verified cached signature to an unverified one. + if (!verified && alreadyVerified) + { + JLOG(j_.trace()) + << "Export: addSignature ignored unverified duplicate for " + << txnHash << " from " + << toBase58(TokenType::NodePublic, validator); + return; + } + + ensureFirstSeen(); + existing->second = std::move(signer); + if (verified && *verified) + verified_[txnHash].insert(validator); + else if (auto it = verified_.find(txnHash); it != verified_.end()) + { + it->second.erase(validator); + if (it->second.empty()) + verified_.erase(it); + } + JLOG(j_.trace()) << "Export: updated signature from " << toBase58(TokenType::NodePublic, validator) << " for " << txnHash - << " (total: " << signerMap.size() << ")"; + << " (verified=" << (verified && *verified) + << ", total=" << signerMap.size() << ")"; + return; } + + ensureFirstSeen(); + signerMap.emplace(validator, std::move(signer)); + if (verified && *verified) + verified_[txnHash].insert(validator); + + JLOG(j_.trace()) << "Export: added signature from " + << toBase58(TokenType::NodePublic, validator) << " for " + << txnHash << " (verified=" << (verified && *verified) + << ", total=" << signerMap.size() << ")"; } STArray