ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-07-26 16:40:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: bad merge

Browse Source
This commit is contained in:
Denis Angell
2026-07-01 10:40:29 -04:00
parent ea13be81b7
commit f5629c3529
2 changed files with 78 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
src/libxrpl/protocol/STTx.cpp
View File

@@ -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)

53
src/test/app/Batch_test.cpp
View File

@@ -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<Reg>(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.");
}
}