ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2025-11-09 05:35:51 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Transaction sig checking functions do not get a full context (#5829)

Browse Source 
Fixes a (currently harmless) bug introduced by PR #5594
This commit is contained in:
Ed Hennis
2025-10-01 16:58:43 -04:00
committed by GitHub
parent 330a3215bc
commit 51ef35ab55
2 changed files with 52 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

76
src/xrpld/app/tx/detail/Transactor.cpp
View File

@@ -660,14 +660,15 @@ Transactor::apply()
NotTEC NotTEC
Transactor::checkSign( Transactor::checkSign(
PreclaimContext const& ctx, ReadView const& view,
ApplyFlags flags,
AccountID const& idAccount, AccountID const& idAccount,
STObject const& sigObject) STObject const& sigObject,
beast::Journal const j)
{ {
auto const pkSigner = sigObject.getFieldVL(sfSigningPubKey); auto const pkSigner = sigObject.getFieldVL(sfSigningPubKey);
// Ignore signature check on batch inner transactions // Ignore signature check on batch inner transactions
if (sigObject.isFlag(tfInnerBatchTxn) && if (sigObject.isFlag(tfInnerBatchTxn) && view.rules().enabled(featureBatch))
ctx.view.rules().enabled(featureBatch))
{ {
// Defensive Check: These values are also checked in Batch::preflight // Defensive Check: These values are also checked in Batch::preflight
if (sigObject.isFieldPresent(sfTxnSignature) || !pkSigner.empty() || if (sigObject.isFieldPresent(sfTxnSignature) || !pkSigner.empty() ||
@@ -678,7 +679,7 @@ Transactor::checkSign(
return tesSUCCESS; return tesSUCCESS;
} }
if ((ctx.flags & tapDRY_RUN) && pkSigner.empty() && if ((flags & tapDRY_RUN) && pkSigner.empty() &&
!sigObject.isFieldPresent(sfSigners)) !sigObject.isFieldPresent(sfSigners))
{ {
// simulate: skip signature validation when neither SigningPubKey nor // simulate: skip signature validation when neither SigningPubKey nor
@@ -688,9 +689,9 @@ Transactor::checkSign(
// If the pk is empty and not simulate or simulate and signers, // If the pk is empty and not simulate or simulate and signers,
// then we must be multi-signing. // then we must be multi-signing.
if (ctx.tx.isFieldPresent(sfSigners)) if (sigObject.isFieldPresent(sfSigners))
{ {
return checkMultiSign(ctx, idAccount, sigObject); return checkMultiSign(view, flags, idAccount, sigObject, j);
} }
// Check Single Sign // Check Single Sign
@@ -699,7 +700,7 @@ Transactor::checkSign(
if (!publicKeyType(makeSlice(pkSigner))) if (!publicKeyType(makeSlice(pkSigner)))
{ {
JLOG(ctx.j.trace()) << "checkSign: signing public key type is unknown"; JLOG(j.trace()) << "checkSign: signing public key type is unknown";
return tefBAD_AUTH; // FIXME: should be better error! return tefBAD_AUTH; // FIXME: should be better error!
} }
@@ -707,11 +708,11 @@ Transactor::checkSign(
auto const idSigner = pkSigner.empty() auto const idSigner = pkSigner.empty()
? idAccount ? idAccount
: calcAccountID(PublicKey(makeSlice(pkSigner))); : calcAccountID(PublicKey(makeSlice(pkSigner)));
auto const sleAccount = ctx.view.read(keylet::account(idAccount)); auto const sleAccount = view.read(keylet::account(idAccount));
if (!sleAccount) if (!sleAccount)
return terNO_ACCOUNT; return terNO_ACCOUNT;
return checkSingleSign(ctx, idSigner, idAccount, sleAccount); return checkSingleSign(view, idSigner, idAccount, sleAccount, j);
} }
NotTEC NotTEC
@@ -720,7 +721,7 @@ Transactor::checkSign(PreclaimContext const& ctx)
auto const idAccount = ctx.tx.isFieldPresent(sfDelegate) auto const idAccount = ctx.tx.isFieldPresent(sfDelegate)
? ctx.tx.getAccountID(sfDelegate) ? ctx.tx.getAccountID(sfDelegate)
: ctx.tx.getAccountID(sfAccount); : ctx.tx.getAccountID(sfAccount);
return checkSign(ctx, idAccount, ctx.tx); return checkSign(ctx.view, ctx.flags, idAccount, ctx.tx, ctx.j);
} }
NotTEC NotTEC
@@ -735,7 +736,8 @@ Transactor::checkBatchSign(PreclaimContext const& ctx)
Blob const& pkSigner = signer.getFieldVL(sfSigningPubKey); Blob const& pkSigner = signer.getFieldVL(sfSigningPubKey);
if (pkSigner.empty()) if (pkSigner.empty())
{ {
if (ret = checkMultiSign(ctx, idAccount, signer); if (ret = checkMultiSign(
ctx.view, ctx.flags, idAccount, signer, ctx.j);
!isTesSuccess(ret)) !isTesSuccess(ret))
return ret; return ret;
} }
@@ -759,7 +761,8 @@ Transactor::checkBatchSign(PreclaimContext const& ctx)
return tesSUCCESS; return tesSUCCESS;
} }
if (ret = checkSingleSign(ctx, idSigner, idAccount, sleAccount); if (ret = checkSingleSign(
ctx.view, idSigner, idAccount, sleAccount, ctx.j);
!isTesSuccess(ret)) !isTesSuccess(ret))
return ret; return ret;
} }
@@ -769,14 +772,15 @@ Transactor::checkBatchSign(PreclaimContext const& ctx)
NotTEC NotTEC
Transactor::checkSingleSign( Transactor::checkSingleSign(
PreclaimContext const& ctx, ReadView const& view,
AccountID const& idSigner, AccountID const& idSigner,
AccountID const& idAccount, AccountID const& idAccount,
std::shared_ptr<SLE const> sleAccount) std::shared_ptr<SLE const> sleAccount,
beast::Journal const j)
{ {
bool const isMasterDisabled = sleAccount->isFlag(lsfDisableMaster); bool const isMasterDisabled = sleAccount->isFlag(lsfDisableMaster);
if (ctx.view.rules().enabled(fixMasterKeyAsRegularKey)) if (view.rules().enabled(fixMasterKeyAsRegularKey))
{ {
// Signed with regular key. // Signed with regular key.
if ((*sleAccount)[~sfRegularKey] == idSigner) if ((*sleAccount)[~sfRegularKey] == idSigner)
@@ -813,16 +817,14 @@ Transactor::checkSingleSign(
else if (sleAccount->isFieldPresent(sfRegularKey)) else if (sleAccount->isFieldPresent(sfRegularKey))
{ {
// Signing key does not match master or regular key. // Signing key does not match master or regular key.
JLOG(ctx.j.trace()) JLOG(j.trace()) << "checkSingleSign: Not authorized to use account.";
<< "checkSingleSign: Not authorized to use account.";
return tefBAD_AUTH; return tefBAD_AUTH;
} }
else else
{ {
// No regular key on account and signing key does not match master key. // No regular key on account and signing key does not match master key.
// FIXME: Why differentiate this case from tefBAD_AUTH? // FIXME: Why differentiate this case from tefBAD_AUTH?
JLOG(ctx.j.trace()) JLOG(j.trace()) << "checkSingleSign: Not authorized to use account.";
<< "checkSingleSign: Not authorized to use account.";
return tefBAD_AUTH_MASTER; return tefBAD_AUTH_MASTER;
} }
@@ -831,17 +833,19 @@ Transactor::checkSingleSign(
NotTEC NotTEC
Transactor::checkMultiSign( Transactor::checkMultiSign(
PreclaimContext const& ctx, ReadView const& view,
ApplyFlags flags,
AccountID const& id, AccountID const& id,
STObject const& sigObject) STObject const& sigObject,
beast::Journal const j)
{ {
// Get id's SignerList and Quorum. // Get id's SignerList and Quorum.
std::shared_ptr<STLedgerEntry const> sleAccountSigners = std::shared_ptr<STLedgerEntry const> sleAccountSigners =
ctx.view.read(keylet::signers(id)); view.read(keylet::signers(id));
// If the signer list doesn't exist the account is not multi-signing. // If the signer list doesn't exist the account is not multi-signing.
if (!sleAccountSigners) if (!sleAccountSigners)
{ {
JLOG(ctx.j.trace()) JLOG(j.trace())
<< "applyTransaction: Invalid: Not a multi-signing account."; << "applyTransaction: Invalid: Not a multi-signing account.";
return tefNOT_MULTI_SIGNING; return tefNOT_MULTI_SIGNING;
} }
@@ -856,7 +860,7 @@ Transactor::checkMultiSign(
"ripple::Transactor::checkMultiSign : signer list ID is 0"); "ripple::Transactor::checkMultiSign : signer list ID is 0");
auto accountSigners = auto accountSigners =
SignerEntries::deserialize(*sleAccountSigners, ctx.j, "ledger"); SignerEntries::deserialize(*sleAccountSigners, j, "ledger");
if (!accountSigners) if (!accountSigners)
return accountSigners.error(); return accountSigners.error();
@@ -880,7 +884,7 @@ Transactor::checkMultiSign(
{ {
if (++iter == accountSigners->end()) if (++iter == accountSigners->end())
{ {
JLOG(ctx.j.trace()) JLOG(j.trace())
<< "applyTransaction: Invalid SigningAccount.Account."; << "applyTransaction: Invalid SigningAccount.Account.";
return tefBAD_SIGNATURE; return tefBAD_SIGNATURE;
} }
@@ -888,7 +892,7 @@ Transactor::checkMultiSign(
if (iter->account != txSignerAcctID) if (iter->account != txSignerAcctID)
{ {
// The SigningAccount is not in the SignerEntries. // The SigningAccount is not in the SignerEntries.
JLOG(ctx.j.trace()) JLOG(j.trace())
<< "applyTransaction: Invalid SigningAccount.Account."; << "applyTransaction: Invalid SigningAccount.Account.";
return tefBAD_SIGNATURE; return tefBAD_SIGNATURE;
} }
@@ -902,13 +906,13 @@ Transactor::checkMultiSign(
// STTx::checkMultiSign // STTx::checkMultiSign
if (!spk.empty() && !publicKeyType(makeSlice(spk))) if (!spk.empty() && !publicKeyType(makeSlice(spk)))
{ {
JLOG(ctx.j.trace()) JLOG(j.trace())
<< "checkMultiSign: signing public key type is unknown"; << "checkMultiSign: signing public key type is unknown";
return tefBAD_SIGNATURE; return tefBAD_SIGNATURE;
} }
XRPL_ASSERT( XRPL_ASSERT(
(ctx.flags & tapDRY_RUN) || !spk.empty(), (flags & tapDRY_RUN) || !spk.empty(),
"ripple::Transactor::checkMultiSign : non-empty signer or " "ripple::Transactor::checkMultiSign : non-empty signer or "
"simulation"); "simulation");
AccountID const signingAcctIDFromPubKey = spk.empty() AccountID const signingAcctIDFromPubKey = spk.empty()
@@ -940,8 +944,7 @@ Transactor::checkMultiSign(
// In any of these cases we need to know whether the account is in // In any of these cases we need to know whether the account is in
// the ledger. Determine that now. // the ledger. Determine that now.
auto const sleTxSignerRoot = auto const sleTxSignerRoot = view.read(keylet::account(txSignerAcctID));
ctx.view.read(keylet::account(txSignerAcctID));
if (signingAcctIDFromPubKey == txSignerAcctID) if (signingAcctIDFromPubKey == txSignerAcctID)
{ {
@@ -954,7 +957,7 @@ Transactor::checkMultiSign(
if (signerAccountFlags & lsfDisableMaster) if (signerAccountFlags & lsfDisableMaster)
{ {
JLOG(ctx.j.trace()) JLOG(j.trace())
<< "applyTransaction: Signer:Account lsfDisableMaster."; << "applyTransaction: Signer:Account lsfDisableMaster.";
return tefMASTER_DISABLED; return tefMASTER_DISABLED;
} }
@@ -966,21 +969,21 @@ Transactor::checkMultiSign(
// Public key must hash to the account's regular key. // Public key must hash to the account's regular key.
if (!sleTxSignerRoot) if (!sleTxSignerRoot)
{ {
JLOG(ctx.j.trace()) << "applyTransaction: Non-phantom signer " JLOG(j.trace()) << "applyTransaction: Non-phantom signer "
"lacks account root."; "lacks account root.";
return tefBAD_SIGNATURE; return tefBAD_SIGNATURE;
} }
if (!sleTxSignerRoot->isFieldPresent(sfRegularKey)) if (!sleTxSignerRoot->isFieldPresent(sfRegularKey))
{ {
JLOG(ctx.j.trace()) JLOG(j.trace())
<< "applyTransaction: Account lacks RegularKey."; << "applyTransaction: Account lacks RegularKey.";
return tefBAD_SIGNATURE; return tefBAD_SIGNATURE;
} }
if (signingAcctIDFromPubKey != if (signingAcctIDFromPubKey !=
sleTxSignerRoot->getAccountID(sfRegularKey)) sleTxSignerRoot->getAccountID(sfRegularKey))
{ {
JLOG(ctx.j.trace()) JLOG(j.trace())
<< "applyTransaction: Account doesn't match RegularKey."; << "applyTransaction: Account doesn't match RegularKey.";
return tefBAD_SIGNATURE; return tefBAD_SIGNATURE;
} }
@@ -992,8 +995,7 @@ Transactor::checkMultiSign(
// Cannot perform transaction if quorum is not met. // Cannot perform transaction if quorum is not met.
if (weightSum < sleAccountSigners->getFieldU32(sfSignerQuorum)) if (weightSum < sleAccountSigners->getFieldU32(sfSignerQuorum))
{ {
JLOG(ctx.j.trace()) JLOG(j.trace()) << "applyTransaction: Signers failed to meet quorum.";
<< "applyTransaction: Signers failed to meet quorum.";
return tefBAD_QUORUM; return tefBAD_QUORUM;
} }

19
src/xrpld/app/tx/detail/Transactor.h
View File

@@ -283,9 +283,11 @@ protected:
static NotTEC static NotTEC
checkSign( checkSign(
PreclaimContext const& ctx, ReadView const& view,
AccountID const& id, ApplyFlags flags,
STObject const& sigObject); AccountID const& idAccount,
STObject const& sigObject,
beast::Journal const j);
// Base class always returns true // Base class always returns true
static bool static bool
@@ -323,15 +325,18 @@ private:
payFee(); payFee();
static NotTEC static NotTEC
checkSingleSign( checkSingleSign(
PreclaimContext const& ctx, ReadView const& view,
AccountID const& idSigner, AccountID const& idSigner,
AccountID const& idAccount, AccountID const& idAccount,
std::shared_ptr<SLE const> sleAccount); std::shared_ptr<SLE const> sleAccount,
beast::Journal const j);
static NotTEC static NotTEC
checkMultiSign( checkMultiSign(
PreclaimContext const& ctx, ReadView const& view,
ApplyFlags flags,
AccountID const& id, AccountID const& id,
STObject const& sigObject); STObject const& sigObject,
beast::Journal const j);
void trapTransaction(uint256) const; void trapTransaction(uint256) const;