From f5629c352927d5c182160753a4b4ffcbc41e249d Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Wed, 1 Jul 2026 10:40:29 -0400 Subject: [PATCH] fix: bad merge --- src/libxrpl/protocol/STTx.cpp | 32 ++++++++++++++++----- src/test/app/Batch_test.cpp | 53 +++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 7 deletions(-) diff --git a/src/libxrpl/protocol/STTx.cpp b/src/libxrpl/protocol/STTx.cpp index cd2da12316..08f6bae29a 100644 --- a/src/libxrpl/protocol/STTx.cpp +++ b/src/libxrpl/protocol/STTx.cpp @@ -270,10 +270,14 @@ STTx::checkSign(Rules const& rules) const // Verify the batch signer signatures here too, so they are cached with the // rest of signature checking (checkValidity / SF_SIGGOOD) and stay out of - // the transaction engine. Gated on a batch (batchTxnIds_ seated) that - // actually carries signers; a batch whose inners are all from the outer - // account has no sfBatchSigners and needs no signer crypto. - if (batchTxnIds_ && isFieldPresent(sfBatchSigners)) + // the transaction engine. Keyed only on sfBatchSigners being present (which + // only a Batch can carry). Deliberately NOT gated on batchTxnIds_ or any + // other transaction data: signature checking must never be skipped based on + // whether some size check passed, otherwise an oversized batch could slip + // through as validly signed by leaning on the later local / preflight + // checks (which are bypassed on several paths). A batch whose inners are all + // from the outer account has no sfBatchSigners and needs no signer crypto. + if (isFieldPresent(sfBatchSigners)) { if (auto const ret = checkBatchSign(rules); !ret) return ret; @@ -296,6 +300,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 +590,15 @@ 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(). + // Always seat the vector for a batch, even when the array is oversized. Its + // existence is not used to gate signature checking (see checkSign), but the + // ids it holds are what checkBatchSign signs over, so it must always match + // sfRawTransactions. An oversized batch is still rejected - by the + // signer-count guard in checkBatchSign and by preflight - but its signers + // are verified rather than skipped. Building the ids only hashes each inner + // and the array is bounded by the 1 MB transaction-size limit, so this is + // cheap. This preserves 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..486e9bbec8 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -1544,6 +1544,51 @@ class Batch_test : public beast::unit_test::Suite BEAST_EXPECT(!result.applied && result.ter == temARRAY_TOO_LARGE); return result.applied; }); + + // Regression: at the relay boundary (checkValidity) this batch has + // no BatchSigners (all inners are from the outer account), so + // checkBatchSign is not invoked and the oversized inner array is + // rejected by the local checks. + 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 (Ed Hennis review): batch-signer verification is keyed only + // on sfBatchSigners being present - never gated on the inner-id vector + // or any size check - so an oversized batch that carries BatchSigners is + // still routed through checkBatchSign rather than slipping past as + // validly signed. Here the inner array AND the signers array are both + // oversized; checkBatchSign's signer-count guard returns SigBad instead + // of the batch flagging SigGoodOnly with its signers unchecked. + { + Env env{*this, features}; + + auto const alice = Account("alice"); + auto const bob = Account("bob"); + env.fund(XRP(10000), alice, bob); + env.close(); + + auto const aliceSeq = env.seq(alice); + auto const batchFee = batch::calcBatchFee(env, kMaxBatchSigners + 1, 9); + auto jt = env.jtnofill( + batch::outer(alice, aliceSeq, batchFee, tfAllOrNothing), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Inner(pay(alice, bob, XRP(1)), aliceSeq), + batch::Sig(std::vector(kMaxBatchSigners + 1, bob))); + + 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."); } // Regression: the relay-boundary local check (isBatchRawTransactionOkay) @@ -1598,6 +1643,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."); } }