From 2eaf211e9bfe60f351f9d02169c3ecee1aec3e67 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Thu, 7 Jan 2016 16:10:14 -0800 Subject: [PATCH] Improve error message when signing fails (RIPD-1066): 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. --- src/ripple/app/tests/MultiSign.test.cpp | 156 +++++++++++++++++++----- src/ripple/app/tx/impl/Transactor.cpp | 15 ++- src/ripple/app/tx/impl/apply.cpp | 20 +-- src/ripple/protocol/STTx.h | 10 +- src/ripple/protocol/impl/STTx.cpp | 45 ++++--- src/ripple/protocol/tests/STTx.test.cpp | 2 +- 6 files changed, 178 insertions(+), 70 deletions(-) diff --git a/src/ripple/app/tests/MultiSign.test.cpp b/src/ripple/app/tests/MultiSign.test.cpp index f0c0e2a61..70a519c2c 100644 --- a/src/ripple/app/tests/MultiSign.test.cpp +++ b/src/ripple/app/tests/MultiSign.test.cpp @@ -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(*(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(); } }; diff --git a/src/ripple/app/tx/impl/Transactor.cpp b/src/ripple/app/tx/impl/Transactor.cpp index 19d8e14c3..5f3603d60 100644 --- a/src/ripple/app/tx/impl/Transactor.cpp +++ b/src/ripple/app/tx/impl/Transactor.cpp @@ -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; } diff --git a/src/ripple/app/tx/impl/apply.cpp b/src/ripple/app/tx/impl/apply.cpp index 00608ed9c..93bb99ecc 100644 --- a/src/ripple/app/tx/impl/apply.cpp +++ b/src/ripple/app/tx/impl/apply.cpp @@ -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 diff --git a/src/ripple/protocol/STTx.h b/src/ripple/protocol/STTx.h index 0b887a350..9e196dbac 100644 --- a/src/ripple/protocol/STTx.h +++ b/src/ripple/protocol/STTx.h @@ -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 + 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 checkSingleSign () const; + std::pair checkMultiSign () const; uint256 tid_; TxType tx_type_; diff --git a/src/ripple/protocol/impl/STTx.cpp b/src/ripple/protocol/impl/STTx.cpp index 051d632b9..6d765a6d2 100644 --- a/src/ripple/protocol/impl/STTx.cpp +++ b/src/ripple/protocol/impl/STTx.cpp @@ -181,9 +181,9 @@ void STTx::sign ( tid_ = getHash(HashPrefix::transactionID); } -bool STTx::checkSign(bool allowMultiSign) const +std::pair STTx::checkSign(bool allowMultiSign) const { - bool sigGood = false; + std::pair 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 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 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, ""}; } //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/tests/STTx.test.cpp b/src/ripple/protocol/tests/STTx.test.cpp index 4ca3ca1a5..428dce507 100644 --- a/src/ripple/protocol/tests/STTx.test.cpp +++ b/src/ripple/protocol/tests/STTx.test.cpp @@ -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);