ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2025-12-06 17:27:55 +00:00
Code Issues Packages Projects Releases Wiki Activity

Improve error message when signing fails (RIPD-1066):

Browse Source 
With the addition of multisigning there are a variety of reasons
a signature may fail.  We now return a more descriptive message
for the reason certain signature checks fail.
This commit is contained in:
Scott Schurr
2016-01-07 16:10:14 -08:00
committed by Nik Bougalis
parent ed9f5639a8
commit 2eaf211e9b
6 changed files with 178 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files

156
src/ripple/app/tests/MultiSign.test.cpp
View File

@@ -173,36 +173,6 @@ public:
env.close();
expect (env.seq(alice) == aliceSeq);
// Multisign, but leave a nonempty sfSigningPubKey. Should fail.
{
aliceSeq = env.seq (alice);
Json::Value multiSig =
env.json (noop (alice), msig(bogie), fee(2 * baseFee));
env (env.jt (multiSig), ter (temINVALID));
env.close();
expect (env.seq(alice) == aliceSeq);
}
// Attach both Signers and a TxnSignature with an empty sfPubKey.
// Should fail.
{
aliceSeq = env.seq (alice);
Json::Value multiSig = env.json (noop (alice),
fee(2 * baseFee), seq(aliceSeq), sig(alice), msig(bogie));
JTx jt = env.jt (multiSig);
jt.fill_fee = false;
jt.fill_seq = false;
jt.fill_sig = false;
jt.jv["SigningPubKey"] = "";
auto noSigPubKey = std::make_unique<STTx>(*(jt.stx));
noSigPubKey->setFieldVL (sfSigningPubKey, Blob());
jt.stx.reset (noSigPubKey.release());
env (jt, ter (temINVALID));
env.close();
expect (env.seq(alice) == aliceSeq);
}
// Don't meet the quorum. Should fail.
env(signers(alice, 2, {{bogie, 1}, {demon, 1}}));
aliceSeq = env.seq (alice);
@@ -424,7 +394,6 @@ public:
msig(msig::Reg{becky, beck}, msig::Reg{cheri, cher}));
env.close();
expect (env.seq(alice) == aliceSeq + 1);
}
void test_heterogeneousSigners()
@@ -731,6 +700,130 @@ public:
env.require (owners (alice, 6));
}
void test_badSignatureText()
{
// Verify that the text returned for signature failures is correct.
using namespace jtx;
Env env(*this, features(featureMultiSign));
// lambda that submits an STTx and returns the resulting JSON.
auto submitSTTx = [&env] (STTx const& stx)
{
Json::Value jvResult;
jvResult[jss::tx_blob] = strHex (stx.getSerializer().slice());
return env.rpc ("json", "submit", jvResult.toStyledString());
};
Account const alice {"alice"};
env.fund(XRP(1000), alice);
env(signers(alice, 1, {{bogie, 1}, {demon, 1}}), sig (alice));
auto const baseFee = env.current()->fees().base;
{
// Single-sign, but leave an empty SigningPubKey.
JTx tx = env.jt (noop (alice), sig(alice));
STTx local = *(tx.stx);
local.setFieldVL (sfSigningPubKey, Blob()); // Empty SigningPubKey
auto const info = submitSTTx (local);
expect (info[jss::result][jss::error_exception] ==
"fails local checks: Empty SigningPubKey.");
}
{
// Single-sign, but invalidate the signature.
JTx tx = env.jt (noop (alice), sig(alice));
STTx local = *(tx.stx);
// Flip some bits in the signature.
auto badSig = local.getFieldVL (sfTxnSignature);
badSig[20] ^= 0xAA;
local.setFieldVL (sfTxnSignature, badSig);
// Signature should fail.
auto const info = submitSTTx (local);
expect (info[jss::result][jss::error_exception] ==
"fails local checks: Invalid signature.");
}
{
// Multisign, but leave a nonempty sfSigningPubKey.
JTx tx = env.jt (noop (alice), fee(2 * baseFee), msig(bogie));
STTx local = *(tx.stx);
local[sfSigningPubKey] = alice.pk(); // Insert sfSigningPubKey
auto const info = submitSTTx (local);
expect (info[jss::result][jss::error_exception] ==
"fails local checks: Cannot both single- and multi-sign.");
}
{
// Both multi- and single-sign with an empty SigningPubKey.
JTx tx = env.jt (noop(alice), fee(2 * baseFee), msig(bogie));
STTx local = *(tx.stx);
local.sign (alice.pk(), alice.sk());
local.setFieldVL (sfSigningPubKey, Blob()); // Empty SigningPubKey
auto const info = submitSTTx (local);
expect (info[jss::result][jss::error_exception] ==
"fails local checks: Cannot both single- and multi-sign.");
}
{
// Multisign but invalidate one of the signatures.
JTx tx = env.jt (noop(alice), fee(2 * baseFee), msig(bogie));
STTx local = *(tx.stx);
// Flip some bits in the signature.
auto& signer = local.peekFieldArray (sfSigners).back();
auto badSig = signer.getFieldVL (sfTxnSignature);
badSig[20] ^= 0xAA;
signer.setFieldVL (sfTxnSignature, badSig);
// Signature should fail.
auto const info = submitSTTx (local);
expect (info[jss::result][jss::error_exception].asString().
find ("Invalid signature on account r") != std::string::npos);
}
{
// Multisign with an empty signers array should fail.
JTx tx = env.jt (noop(alice), fee(2 * baseFee), msig(bogie));
STTx local = *(tx.stx);
local.peekFieldArray (sfSigners).clear(); // Empty Signers array.
auto const info = submitSTTx (local);
expect (info[jss::result][jss::error_exception] ==
"fails local checks: Invalid Signers array size.");
}
{
// Multisign 9 times should fail.
JTx tx = env.jt (noop(alice), fee(2 * baseFee),
msig(bogie, bogie, bogie,
bogie, bogie, bogie, bogie, bogie, bogie));
STTx local = *(tx.stx);
auto const info = submitSTTx (local);
expect (info[jss::result][jss::error_exception] ==
"fails local checks: Invalid Signers array size.");
}
{
// The account owner may not multisign for themselves.
JTx tx = env.jt (noop(alice), fee(2 * baseFee), msig(alice));
STTx local = *(tx.stx);
auto const info = submitSTTx (local);
expect (info[jss::result][jss::error_exception] ==
"fails local checks: Invalid multisigner.");
}
{
// No duplicate multisignatures allowed.
JTx tx = env.jt (noop(alice), fee(2 * baseFee), msig(bogie, bogie));
STTx local = *(tx.stx);
auto const info = submitSTTx (local);
expect (info[jss::result][jss::error_exception] ==
"fails local checks: Duplicate Signers not allowed.");
}
{
// Multisignatures must be submitted in sorted order.
JTx tx = env.jt (noop(alice), fee(2 * baseFee), msig(bogie, demon));
STTx local = *(tx.stx);
// Unsort the Signers array.
auto& signers = local.peekFieldArray (sfSigners);
std::reverse (signers.begin(), signers.end());
// Signature should fail.
auto const info = submitSTTx (local);
expect (info[jss::result][jss::error_exception] ==
"fails local checks: Unsorted Signers array.");
}
}
void run() override
{
test_noReserve();
@@ -745,6 +838,7 @@ public:
test_keyDisable();
test_regKey();
test_txTypes();
test_badSignatureText();
}
};

15
src/ripple/app/tx/impl/Transactor.cpp
View File

@@ -88,14 +88,17 @@ preflight1 (PreflightContext const& ctx)
TER
preflight2 (PreflightContext const& ctx)
{
if(!( ctx.flags & tapNO_CHECK_SIGN) &&
checkValidity(ctx.app.getHashRouter(),
ctx.tx, ctx.rules, ctx.app.config()).first == Validity::SigBad)
if(!( ctx.flags & tapNO_CHECK_SIGN))
{
JLOG(ctx.j.debug) << "preflight2: bad signature";
return temINVALID;
auto const sigValid = checkValidity(ctx.app.getHashRouter(),
ctx.tx, ctx.rules, ctx.app.config());
if (sigValid.first == Validity::SigBad)
{
JLOG(ctx.j.debug) <<
"preflight2: bad signature. " << sigValid.second;
return temINVALID;
}
}
return tesSUCCESS;
}

20
src/ripple/app/tx/impl/apply.cpp
View File

@@ -46,16 +46,16 @@ checkValidity(HashRouter& router,
auto const flags = router.getFlags(id);
if (flags & SF_SIGBAD)
// Signature is known bad
return std::make_pair(Validity::SigBad,
"Transaction has bad signature.");
return {Validity::SigBad, "Transaction has bad signature."};
if (!(flags & SF_SIGGOOD))
{
// Don't know signature state. Check it.
if (!tx.checkSign(allowMultiSign))
auto const sigVerify = tx.checkSign(allowMultiSign);
if (! sigVerify.first)
{
router.setFlags(id, SF_SIGBAD);
return std::make_pair(Validity::SigBad,
"Transaction has bad signature.");
return {Validity::SigBad, sigVerify.second};
}
router.setFlags(id, SF_SIGGOOD);
}
@@ -64,22 +64,22 @@ checkValidity(HashRouter& router,
if (flags & SF_LOCALBAD)
// ...but the local checks
// are known bad.
return std::make_pair(Validity::SigGoodOnly,
"Local checks failed.");
return {Validity::SigGoodOnly, "Local checks failed."};
if (flags & SF_LOCALGOOD)
// ...and the local checks
// are known good.
return std::make_pair(Validity::Valid, "");
return {Validity::Valid, ""};
// Do the local checks
std::string reason;
if (!passesLocalChecks(tx, reason))
{
router.setFlags(id, SF_LOCALBAD);
return std::make_pair(Validity::SigGoodOnly, reason);
return {Validity::SigGoodOnly, reason};
}
router.setFlags(id, SF_LOCALGOOD);
return std::make_pair(Validity::Valid, "");
return {Validity::Valid, ""};
}
void

10
src/ripple/protocol/STTx.h
View File

@@ -127,7 +127,11 @@ public:
PublicKey const& publicKey,
SecretKey const& secretKey);
bool checkSign(bool allowMultiSign) const;
/** Check the signature.
@return `true` if valid signature. If invalid, the error message string.
*/
std::pair<bool, std::string>
checkSign(bool allowMultiSign) const;
// SQL Functions with metadata.
static
@@ -144,8 +148,8 @@ public:
std::string const& escapedMetaData) const;
private:
bool checkSingleSign () const;
bool checkMultiSign () const;
std::pair<bool, std::string> checkSingleSign () const;
std::pair<bool, std::string> checkMultiSign () const;
uint256 tid_;
TxType tx_type_;

45
src/ripple/protocol/impl/STTx.cpp
View File

@@ -181,9 +181,9 @@ void STTx::sign (
tid_ = getHash(HashPrefix::transactionID);
}
bool STTx::checkSign(bool allowMultiSign) const
std::pair<bool, std::string> STTx::checkSign(bool allowMultiSign) const
{
bool sigGood = false;
std::pair<bool, std::string> ret {false, ""};
try
{
if (allowMultiSign)
@@ -192,18 +192,19 @@ bool STTx::checkSign(bool allowMultiSign) const
// at the SigningPubKey. It it's empty we must be
// multi-signing. Otherwise we're single-signing.
Blob const& signingPubKey = getFieldVL (sfSigningPubKey);
sigGood = signingPubKey.empty () ?
ret = signingPubKey.empty () ?
checkMultiSign () : checkSingleSign ();
}
else
{
sigGood = checkSingleSign ();
ret = checkSingleSign ();
}
}
catch (std::exception const&)
{
ret = {false, "Internal signature check failure."};
}
return sigGood;
return ret;
}
Json::Value STTx::getJson (int) const
@@ -261,14 +262,13 @@ STTx::getMetaSQL (Serializer rawTxn,
% getSequence () % inLedger % status % rTxn % escapedMetaData);
}
bool
STTx::checkSingleSign () const
std::pair<bool, std::string> STTx::checkSingleSign () const
{
// We don't allow both a non-empty sfSigningPubKey and an sfSigners.
// That would allow the transaction to be signed two ways. So if both
// fields are present the signature is invalid.
if (isFieldPresent (sfSigners))
return false;
return {false, "Cannot both single- and multi-sign."};
bool validSig = false;
try
@@ -293,27 +293,29 @@ STTx::checkSingleSign () const
// Assume it was a signature failure.
validSig = false;
}
return validSig;
if (validSig == false)
return {false, "Invalid signature."};
return {true, ""};
}
bool
STTx::checkMultiSign () const
std::pair<bool, std::string> STTx::checkMultiSign () const
{
// Make sure the MultiSigners are present. Otherwise they are not
// attempting multi-signing and we just have a bad SigningPubKey.
if (!isFieldPresent (sfSigners))
return false;
return {false, "Empty SigningPubKey."};
// We don't allow both an sfSigners and an sfTxnSignature. Both fields
// being present would indicate that the transaction is signed both ways.
if (isFieldPresent (sfTxnSignature))
return false;
return {false, "Cannot both single- and multi-sign."};
STArray const& signers {getFieldArray (sfSigners)};
// There are well known bounds that the number of signers must be within.
if (signers.size() < minMultiSigners || signers.size() > maxMultiSigners)
return false;
return {false, "Invalid Signers array size."};
// We can ease the computational load inside the loop a bit by
// pre-constructing part of the data that we hash. Fill a Serializer
@@ -335,11 +337,15 @@ STTx::checkMultiSign () const
// The account owner may not multisign for themselves.
if (accountID == txnAccountID)
return false;
return {false, "Invalid multisigner."};
// No duplicate signers allowed.
if (lastAccountID == accountID)
return {false, "Duplicate Signers not allowed."};
// Accounts must be in order by account ID. No duplicates allowed.
if (lastAccountID >= accountID)
return false;
if (lastAccountID > accountID)
return {false, "Unsorted Signers array."};
// The next signature must be greater than this one.
lastAccountID = accountID;
@@ -371,11 +377,12 @@ STTx::checkMultiSign () const
validSig = false;
}
if (!validSig)
return false;
return {false, std::string("Invalid signature on account ") +
toBase58(accountID) + "."};
}
// All signatures verified.
return true;
return {true, ""};
}
//------------------------------------------------------------------------------

2
src/ripple/protocol/tests/STTx.test.cpp
View File

@@ -52,7 +52,7 @@ public:
});
j.sign (keypair.first, keypair.second);
unexpected (!j.checkSign (true), "Transaction fails signature test");
unexpected (!j.checkSign (true).first, "Transaction fails signature test");
Serializer rawTxn;
j.add (rawTxn);