fix: BatchV_1 add defensive checks (#6719)

This commit is contained in:
Denis Angell
2026-04-07 14:15:04 +02:00
committed by GitHub
parent 36503b5017
commit 871d60f910
2 changed files with 64 additions and 15 deletions

View File

@@ -181,15 +181,13 @@ Transactor::preflight1(PreflightContext const& ctx, std::uint32_t flagMask)
if (ctx.tx.isFlag(tfInnerBatchTxn) && !ctx.rules.enabled(featureBatchV1_1))
return temINVALID_FLAG;
if (ctx.rules.enabled(featureBatchV1_1) &&
ctx.tx.isFlag(tfInnerBatchTxn) != ctx.parentBatchId.has_value())
// Reject if the inner batch flag and parentBatchId are inconsistent.
// A standalone tx with tfInnerBatchTxn but no parentBatchId is an
// attack attempt. A tx with parentBatchId but without tfInnerBatchTxn
// is a programming error.
if (ctx.tx.isFlag(tfInnerBatchTxn) != ctx.parentBatchId.has_value())
return temINVALID_INNER_BATCH;
XRPL_ASSERT(
ctx.tx.isFlag(tfInnerBatchTxn) == ctx.parentBatchId.has_value() ||
!ctx.rules.enabled(featureBatchV1_1),
"Inner batch transaction must have a parent batch ID.");
return tesSUCCESS;
}
@@ -204,15 +202,18 @@ Transactor::preflight2(PreflightContext const& ctx)
return *ret;
}
// It should be impossible for the InnerBatchTxn flag to be set without
// featureBatchV1_1 being enabled
XRPL_ASSERT_PARTS(
!ctx.tx.isFlag(tfInnerBatchTxn) || ctx.rules.enabled(featureBatchV1_1),
"xrpl::Transactor::preflight2",
"InnerBatch flag only set if feature enabled");
// Skip signature check on batch inner transactions
if (ctx.tx.isFlag(tfInnerBatchTxn) && ctx.rules.enabled(featureBatchV1_1))
// Skip signature check on batch inner transactions, but only if
// this tx was actually submitted through the batch apply path
// (parentBatchId is set). Without this check, a standalone tx
// with tfInnerBatchTxn could bypass signature verification.
if (ctx.tx.isFlag(tfInnerBatchTxn))
{
if (!ctx.rules.enabled(featureBatchV1_1))
return temINVALID_FLAG;
if (!ctx.parentBatchId.has_value())
return temINVALID_INNER_BATCH;
return tesSUCCESS;
}
// Do not add any checks after this point that are relevant for
// batch inner transactions. They will be skipped.

View File

@@ -4411,6 +4411,53 @@ class Batch_test : public beast::unit_test::suite
}
}
void
testStandaloneInnerBatchFlag(FeatureBitset features)
{
testcase("standalone tx with tfInnerBatchTxn rejected");
using namespace test::jtx;
using namespace std::literals;
// A standalone Payment with tfInnerBatchTxn must be rejected.
// Without proper guards this would bypass signature verification
// in preflight2.
{
test::jtx::Env env{*this, features};
auto const alice = Account("alice");
auto const bob = Account("bob");
env.fund(XRP(10000), alice, bob);
env.close();
// Submit a normal Payment with tfInnerBatchTxn flag.
// preflight1 must reject with temINVALID_INNER_BATCH because
// the flag is set but no parentBatchId exists.
env(pay(alice, bob, XRP(1)), txflags(tfInnerBatchTxn), ter(telENV_RPC_FAILED));
env.close();
// Verify via direct apply path (bypassing RPC layer)
env.app().openLedger().modify([&](OpenView& view, beast::Journal j) {
// Construct a Payment STTx with tfInnerBatchTxn,
// empty signing pub key, and no signature — mimicking
// what an attacker would send to skip sig verification.
STTx const stx = STTx(ttPAYMENT, [&](auto& obj) {
obj.setAccountID(sfAccount, alice.id());
obj.setAccountID(sfDestination, bob.id());
obj.setFieldAmount(sfAmount, XRP(1));
obj.setFieldAmount(sfFee, XRP(0));
obj.setFieldU32(sfSequence, env.seq(alice));
obj.setFieldU32(sfFlags, tfInnerBatchTxn);
});
auto const result = xrpl::apply(env.app(), view, stx, tapNONE, j);
// Must NOT be applied — signature was never checked
BEAST_EXPECT(!result.applied);
return false;
});
}
}
void
testWithFeats(FeatureBitset features)
{
@@ -4445,6 +4492,7 @@ class Batch_test : public beast::unit_test::suite
testBatchDelegate(features);
testValidateRPCResponse(features);
testBatchCalculateBaseFee(features);
testStandaloneInnerBatchFlag(features);
}
public: