diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index cd2da12316..0092916ddc 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -296,6 +296,14 @@ STTx::checkBatchSign(Rules const& rules) const if (!isFieldPresent(sfBatchSigners)) return std::unexpected("Missing BatchSigners field."); // LCOV_EXCL_LINE STArray const& signers{getFieldArray(sfBatchSigners)}; + // Bound signature verification to the protocol cap. This runs in + // checkValidity (via checkSign) at relay / submit time, BEFORE preflight + // and passesLocalChecks enforce the cap. Without this guard a malicious + // peer could put an oversized sfBatchSigners array in a 1 MB blob and + // force one signature verification per entry before any of those checks + // (or the fee charge) runs. + if (signers.size() > kMaxBatchSigners) + return std::unexpected("BatchSigners array exceeds max entries."); for (auto const& signer : signers) { Blob const& signingPubKey = signer.getFieldVL(sfSigningPubKey); @@ -578,9 +586,19 @@ STTx::buildBatchTxnIds() auto const& raw = getFieldArray(sfRawTransactions); - // Seated for any batch with raw transactions. The count is validated in - // preflight and at the relay boundary, so build every id here; this keeps - // the invariant batchTxnIds_->size() == rawTransactions.size(). + // Bound the eager hashing to the protocol cap. This runs in the STTx + // constructor, i.e. at parse time, BEFORE preflight / passesLocalChecks + // enforce the cap and BEFORE checkValidity's signature check. Without this + // guard a malicious peer could put an oversized sfRawTransactions array in + // a 1 MB blob and force hashing of every inner id at deserialization. + // Leaving batchTxnIds_ unseated (nullopt) makes checkSign skip batch-signer + // verification for the oversized batch, which is then dropped by the count + // checks in preflight / passesLocalChecks downstream. + if (raw.size() > kMaxBatchTxCount) + return; + + // For an in-cap batch the ids are always fully built, preserving the + // invariant batchTxnIds_->size() == rawTransactions.size(). auto& ids = batchTxnIds_.emplace(); ids.reserve(raw.size()); for (STObject const& rb : raw) diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index f9c156fe06..64ef4998b1 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -1544,6 +1544,16 @@ class Batch_test : public beast::unit_test::Suite BEAST_EXPECT(!result.applied && result.ter == temARRAY_TOO_LARGE); return result.applied; }); + + // Regression (eager parse-time hashing): the relay boundary + // (checkValidity) rejects the oversized inner array. buildBatchTxnIds + // leaves batchTxnIds_ unseated past kMaxBatchTxCount, so no inner ids + // are hashed and checkSign skips batch-signer crypto; the batch is + // dropped by the local checks instead. + auto const [valid, reason] = xrpl::checkValidity( + env.app().getHashRouter(), *jt.stx, env.current()->rules()); + BEAST_EXPECT(valid == xrpl::Validity::SigGoodOnly); + BEAST_EXPECT(reason == "Raw Transactions array exceeds max entries."); } // Regression: the relay-boundary local check (isBatchRawTransactionOkay) @@ -1598,6 +1608,14 @@ class Batch_test : public beast::unit_test::Suite BEAST_EXPECT(!result.applied && result.ter == temARRAY_TOO_LARGE); return result.applied; }); + + // Regression (uncapped batch-signer verification): the relay + // boundary (checkValidity) rejects the oversized signers array via + // the checkBatchSign guard, BEFORE verifying a single signature. + auto const [valid, reason] = xrpl::checkValidity( + env.app().getHashRouter(), *jt.stx, env.current()->rules()); + BEAST_EXPECT(valid == xrpl::Validity::SigBad); + BEAST_EXPECT(reason == "BatchSigners array exceeds max entries."); } }