From c28c516b22e35c649792e622e03cb72fd0f4edbb Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Thu, 17 Sep 2015 17:56:12 -0700 Subject: [PATCH] sign_for RPC command fixes (RIPD-182): o Remove warning written to log by sign_for command. o The sign_for RPC command previously only worked in the "json sign_for" form. The command now works as a straight "sign_for". The "offline" parameter also works. o Don't autofill Fee or Paths when signing offline. --- src/ripple/app/main/Main.cpp | 2 +- src/ripple/app/tx/Transaction.h | 4 - src/ripple/app/tx/impl/Transaction.cpp | 31 ------- src/ripple/net/impl/RPCCall.cpp | 28 +++--- src/ripple/protocol/impl/Sign.cpp | 31 +++++++ src/ripple/rpc/impl/TransactionSign.cpp | 81 ++++++++++------- src/ripple/rpc/impl/TransactionSign.h | 13 ++- src/ripple/rpc/tests/JSONRPC.test.cpp | 110 ++++++++++++++++++++++-- 8 files changed, 212 insertions(+), 88 deletions(-) diff --git a/src/ripple/app/main/Main.cpp b/src/ripple/app/main/Main.cpp index d4b6bfe75..f4473e0d3 100644 --- a/src/ripple/app/main/Main.cpp +++ b/src/ripple/app/main/Main.cpp @@ -150,7 +150,7 @@ void printHelp (const po::options_description& desc) " version\n" " server_info\n" " sign [offline]\n" - " sign_for\n" + " sign_for [offline]\n" " stop\n" " submit |[ ]\n" " submit_multisigned\n" diff --git a/src/ripple/app/tx/Transaction.h b/src/ripple/app/tx/Transaction.h index f7ee52804..7750129f4 100644 --- a/src/ripple/app/tx/Transaction.h +++ b/src/ripple/app/tx/Transaction.h @@ -66,10 +66,6 @@ public: Transaction ( std::shared_ptr const&, std::string&, Application&) noexcept; - static - Transaction::pointer - sharedTransaction (Blob const&, Rules const& rules, Application& app); - static Transaction::pointer transactionFromSQL ( diff --git a/src/ripple/app/tx/impl/Transaction.cpp b/src/ripple/app/tx/impl/Transaction.cpp index 2d61cad9f..cb8084917 100644 --- a/src/ripple/app/tx/impl/Transaction.cpp +++ b/src/ripple/app/tx/impl/Transaction.cpp @@ -56,37 +56,6 @@ Transaction::Transaction (std::shared_ptr const& stx, mStatus = NEW; } -Transaction::pointer Transaction::sharedTransaction ( - Blob const& vucTransaction, Rules const& rules, Application& app) -{ - try - { - SerialIter sit (makeSlice(vucTransaction)); - auto txn = std::make_shared(sit); - std::string reason; - auto result = std::make_shared ( - txn, reason, app); - if (checkValidity(app.getHashRouter(), - *txn, rules, app.config()).first - != Validity::Valid) - { - result->setStatus(INVALID); - } - return result; - } - catch (std::exception& e) - { - JLOG (app.journal ("Ledger").warning) << "Exception constructing transaction" << - e.what (); - } - catch (...) - { - JLOG (app.journal ("Ledger").warning) << "Exception constructing transaction"; - } - - return {}; -} - // // Misc. // diff --git a/src/ripple/net/impl/RPCCall.cpp b/src/ripple/net/impl/RPCCall.cpp index 03ff38884..30bbb4e5a 100644 --- a/src/ripple/net/impl/RPCCall.cpp +++ b/src/ripple/net/impl/RPCCall.cpp @@ -429,23 +429,27 @@ private: return jvRequest; } - // sign_for + // sign_for offline + // sign_for Json::Value parseSignFor (Json::Value const& jvParams) { - Json::Value txJSON; - Json::Reader reader; + bool const bOffline = 4 == jvParams.size () && jvParams[3u].asString () == "offline"; - if ((4 == jvParams.size ()) - && reader.parse (jvParams[3u].asString (), txJSON)) + if (3 == jvParams.size () || bOffline) { - if (txJSON.type () == Json::objectValue) + Json::Value txJSON; + Json::Reader reader; + if (reader.parse (jvParams[2u].asString (), txJSON)) { - // Return SigningFor object for the submitted transaction. + // sign_for txJSON. Json::Value jvRequest; - jvRequest["signing_for"] = jvParams[0u].asString (); - jvRequest["account"] = jvParams[1u].asString (); - jvRequest["secret"] = jvParams[2u].asString (); - jvRequest["tx_json"] = txJSON; + + jvRequest[jss::account] = jvParams[0u].asString (); + jvRequest[jss::secret] = jvParams[1u].asString (); + jvRequest[jss::tx_json] = txJSON; + + if (bOffline) + jvRequest[jss::offline] = true; return jvRequest; } @@ -929,7 +933,7 @@ public: { "random", &RPCParser::parseAsIs, 0, 0 }, { "ripple_path_find", &RPCParser::parseRipplePathFind, 1, 2 }, { "sign", &RPCParser::parseSignSubmit, 2, 3 }, - { "sign_for", &RPCParser::parseSignFor, 4, 4 }, + { "sign_for", &RPCParser::parseSignFor, 3, 4 }, { "submit", &RPCParser::parseSignSubmit, 1, 3 }, { "submit_multisigned", &RPCParser::parseSubmitMultiSigned, 1, 1 }, { "server_info", &RPCParser::parseAsIs, 0, 0 }, diff --git a/src/ripple/protocol/impl/Sign.cpp b/src/ripple/protocol/impl/Sign.cpp index c57efe3ce..241745070 100644 --- a/src/ripple/protocol/impl/Sign.cpp +++ b/src/ripple/protocol/impl/Sign.cpp @@ -51,6 +51,37 @@ verify (STObject const& st, true); } +// Questions regarding buildMultiSigningData: +// +// Why do we include the Signer.Account in the blob to be signed? +// +// Unless you include the Account which is signing in the signing blob, +// you could swap out any Signer.Account for any other, which may also +// be on the SignerList and have a RegularKey matching the +// Signer.SigningPubKey. +// +// That RegularKey may be set to allow some 3rd party to sign transactions +// on the account's behalf, and that RegularKey could be common amongst all +// users of the 3rd party. That's just one example of sharing the same +// RegularKey amongst various accounts and just one vulnerability. +// +// "When you have something that's easy to do that makes entire classes of +// attacks clearly and obviously impossible, you need a damn good reason +// not to do it." -- David Schwartz +// +// Why would we include the signingFor account in the blob to be signed? +// +// In the current signing scheme, the account that a signer is `signing +// for/on behalf of` is the tx_json.Account. +// +// Later we might support more levels of signing. Suppose Bob is a signer +// for Alice, and Carol is a signer for Bob, so Carol can sign for Bob who +// signs for Alice. But suppose Alice has two signers: Bob and Dave. If +// Carol is a signer for both Bob and Dave, then the signature needs to +// distinguish between Carol signing for Bob and Carol signing for Dave. +// +// So, if we support multiple levels of signing, then we'll need to +// incorporate the "signing for" accounts into the signing data as well. Serializer buildMultiSigningData (STObject const& obj, AccountID const& signingID) { diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index 8d07065fe..092708e29 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -22,6 +22,7 @@ #include #include #include +#include // Validity::Valid #include #include #include @@ -386,7 +387,7 @@ transactionPreProcessImpl ( Json::Value err = checkFee ( params, role, - signingArgs.editFields(), + verify && signingArgs.editFields(), app.config(), app.getFeeTrack(), ledger); @@ -401,7 +402,7 @@ transactionPreProcessImpl ( role, app, ledger, - signingArgs.editFields()); + verify && signingArgs.editFields()); if (RPC::contains_error(err)) return std::move (err); @@ -427,6 +428,10 @@ transactionPreProcessImpl ( tx_json[jss::Flags] = tfFullyCanonicalSig; } + // If multisigning then we need to return the public key. + if (signingArgs.isMultiSigning()) + signingArgs.setPublicKey (keypair.publicKey); + if (verify) { if (! sle) @@ -437,12 +442,9 @@ transactionPreProcessImpl ( << "verify: " << toBase58(calcAccountID(keypair.publicKey)) << " : " << toBase58(srcAddressID); - // If multisigning then we need to return the public key. - if (signingArgs.isMultiSigning()) - { - signingArgs.setPublicKey (keypair.publicKey); - } - else + // Don't do this test if multisigning since the account and secret + // probably don't belong together in that case. + if (!signingArgs.isMultiSigning()) { // Make sure the account and secret belong together. error_code_i const err = @@ -507,7 +509,7 @@ transactionPreProcessImpl ( static std::pair transactionConstructImpl (std::shared_ptr const& stpTrans, - Rules const& rules, Application& app) + Rules const& rules, bool validateSig, Application& app, ApplyFlags flags) { std::pair ret; @@ -533,17 +535,33 @@ transactionConstructImpl (std::shared_ptr const& stpTrans, { Serializer s; tpTrans->getSTransaction ()->add (s); + Blob transBlob = s.getData (); + SerialIter sit {makeSlice(transBlob)}; - Transaction::pointer tpTransNew = - Transaction::sharedTransaction(s.getData(), rules, app); - - if (tpTransNew && ( - !tpTransNew->getSTransaction ()->isEquivalent ( - *tpTrans->getSTransaction ()))) + // Check the signature if that's called for. + auto sttxNew = std::make_shared (sit); + if (validateSig && + checkValidity(app.getHashRouter(), + *sttxNew, rules, app.config(), flags).first != Validity::Valid) { - tpTransNew.reset (); + ret.first = RPC::make_error (rpcINTERNAL, + "Invalid signature."); + return ret; + } + + std::string reason; + auto tpTransNew = + std::make_shared (sttxNew, reason, app); + + if (tpTransNew) + { + if (!tpTransNew->getSTransaction()->isEquivalent ( + *tpTrans->getSTransaction())) + { + tpTransNew.reset (); + } + tpTrans = std::move (tpTransNew); } - tpTrans = std::move (tpTransNew); } } catch (std::exception&) @@ -656,7 +674,8 @@ Json::Value transactionSign ( Role role, int validatedLedgerAge, Application& app, - std::shared_ptr ledger) + std::shared_ptr ledger, + ApplyFlags flags) { using namespace detail; @@ -674,8 +693,8 @@ Json::Value transactionSign ( // Make sure the STTx makes a legitimate Transaction. std::pair txn = - transactionConstructImpl (preprocResult.second, - ledger->rules(), app); + transactionConstructImpl ( + preprocResult.second, ledger->rules(), true, app, flags); if (!txn.second) return txn.first; @@ -691,7 +710,8 @@ Json::Value transactionSubmit ( int validatedLedgerAge, Application& app, std::shared_ptr ledger, - ProcessTransactionFn const& processTransaction) + ProcessTransactionFn const& processTransaction, + ApplyFlags flags) { using namespace detail; @@ -709,8 +729,8 @@ Json::Value transactionSubmit ( // Make sure the STTx makes a legitimate Transaction. std::pair txn = - transactionConstructImpl (preprocResult.second, - ledger->rules(), app); + transactionConstructImpl ( + preprocResult.second, ledger->rules(), true, app, flags); if (!txn.second) return txn.first; @@ -735,7 +755,7 @@ namespace detail { // There are a some field checks shared by transactionSignFor // and transactionSubmitMultiSigned. Gather them together here. -Json::Value checkMultiSignFields (Json::Value const& jvRequest) +static Json::Value checkMultiSignFields (Json::Value const& jvRequest) { if (! jvRequest.isMember (jss::tx_json)) return RPC::missing_field_error (jss::tx_json); @@ -767,7 +787,8 @@ Json::Value transactionSignFor ( Role role, int validatedLedgerAge, Application& app, - std::shared_ptr ledger) + std::shared_ptr ledger, + ApplyFlags flags) { auto j = app.journal ("RPCHandler"); JLOG (j.debug) << "transactionSignFor: " << jvRequest; @@ -828,8 +849,8 @@ Json::Value transactionSignFor ( // Make sure the STTx makes a legitimate Transaction. std::pair txn = - transactionConstructImpl (preprocResult.second, - ledger->rules(), app); + transactionConstructImpl ( + preprocResult.second, ledger->rules(), false, app, flags); if (!txn.second) return txn.first; @@ -870,7 +891,8 @@ Json::Value transactionSubmitMultiSigned ( int validatedLedgerAge, Application& app, std::shared_ptr ledger, - ProcessTransactionFn const& processTransaction) + ProcessTransactionFn const& processTransaction, + ApplyFlags flags) { auto j = app.journal ("RPCHandler"); JLOG (j.debug) @@ -1056,8 +1078,7 @@ Json::Value transactionSubmitMultiSigned ( // Make sure the SerializedTransaction makes a legitimate Transaction. std::pair txn = - transactionConstructImpl (stpTrans, - ledger->rules(), app); + transactionConstructImpl (stpTrans, ledger->rules(), true, app, flags); if (!txn.second) return txn.first; diff --git a/src/ripple/rpc/impl/TransactionSign.h b/src/ripple/rpc/impl/TransactionSign.h index ec38bb573..ef894a6b1 100644 --- a/src/ripple/rpc/impl/TransactionSign.h +++ b/src/ripple/rpc/impl/TransactionSign.h @@ -22,6 +22,7 @@ #include #include +#include namespace ripple { @@ -81,7 +82,8 @@ Json::Value transactionSign ( Role role, int validatedLedgerAge, Application& app, - std::shared_ptr ledger); + std::shared_ptr ledger, + ApplyFlags flags = tapNONE); /** Returns a Json::objectValue. */ Json::Value transactionSubmit ( @@ -91,7 +93,8 @@ Json::Value transactionSubmit ( int validatedLedgerAge, Application& app, std::shared_ptr ledger, - ProcessTransactionFn const& processTransaction); + ProcessTransactionFn const& processTransaction, + ApplyFlags flags = tapNONE); /** Returns a Json::objectValue. */ Json::Value transactionSignFor ( @@ -100,7 +103,8 @@ Json::Value transactionSignFor ( Role role, int validatedLedgerAge, Application& app, - std::shared_ptr ledger); + std::shared_ptr ledger, + ApplyFlags flags = tapNONE); /** Returns a Json::objectValue. */ Json::Value transactionSubmitMultiSigned ( @@ -110,7 +114,8 @@ Json::Value transactionSubmitMultiSigned ( int validatedLedgerAge, Application& app, std::shared_ptr ledger, - ProcessTransactionFn const& processTransaction); + ProcessTransactionFn const& processTransaction, + ApplyFlags flags = tapNONE); } // RPC } // ripple diff --git a/src/ripple/rpc/tests/JSONRPC.test.cpp b/src/ripple/rpc/tests/JSONRPC.test.cpp index fc5fe923f..1542945bd 100644 --- a/src/ripple/rpc/tests/JSONRPC.test.cpp +++ b/src/ripple/rpc/tests/JSONRPC.test.cpp @@ -557,6 +557,7 @@ R"({ "secret": "masterpassphrase", "offline": 1, "tx_json": { + "Fee": 10, "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", "Amount": "1000000000", "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", @@ -569,7 +570,7 @@ R"({ "Missing field 'tx_json.Sequence'.", "Missing field 'tx_json.Sequence'."}}, -{ "Valid transaction if 'offline' is true.", +{ "If 'offline' is true then a 'Fee' field must be supplied.", R"({ "command": "doesnt_matter", "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", @@ -584,11 +585,54 @@ R"({ } })", { +"Missing field 'tx_json.Fee'.", +"Missing field 'tx_json.Fee'.", +"Missing field 'tx_json.SigningPubKey'.", +"Missing field 'tx_json.SigningPubKey'."}}, + +{ "Valid transaction if 'offline' is true.", +R"({ + "command": "doesnt_matter", + "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "secret": "masterpassphrase", + "offline": 1, + "tx_json": { + "Sequence": 0, + "Fee": 10, + "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "Amount": "1000000000", + "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "TransactionType": "Payment" + } +})", +{ "", "", "Missing field 'tx_json.SigningPubKey'.", "Missing field 'tx_json.SigningPubKey'."}}, +{ "'offline' and 'build_path' are mutually exclusive.", +R"({ + "command": "doesnt_matter", + "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "secret": "masterpassphrase", + "offline": 1, + "build_path": 1, + "tx_json": { + "Sequence": 0, + "Fee": 10, + "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "Amount": "1000000000", + "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "TransactionType": "Payment" + } +})", +{ +"Field 'build_path' not allowed in this context.", +"Field 'build_path' not allowed in this context.", +"Missing field 'tx_json.SigningPubKey'.", +"Missing field 'tx_json.SigningPubKey'."}}, + { "A 'Flags' field may be specified.", R"({ "command": "doesnt_matter", @@ -667,6 +711,28 @@ R"({ "", "Missing field 'tx_json.Signers'."}}, +{ "Minimal offline sign_for.", +R"({ + "command": "doesnt_matter", + "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "secret": "masterpassphrase", + "offline": 1, + "tx_json": { + "Account": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "Amount": "1000000000", + "Destination": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "Fee": 50, + "Sequence": 0, + "SigningPubKey": "", + "TransactionType": "Payment" + } +})", +{ +"", +"", +"", +"Missing field 'tx_json.Signers'."}}, + { "Missing 'Account' in sign_for.", R"({ "command": "doesnt_matter", @@ -829,6 +895,34 @@ R"({ "Missing field 'tx_json.TransactionType'."}}, { "Minimal submit_multisigned.", +R"({ + "command": "submit_multisigned", + "tx_json": { + "Account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "Amount": "1000000000", + "Destination": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "Fee": 50, + "Sequence": 0, + "Signers" : [ + { + "Signer" : { + "Account" : "rPcNzota6B8YBokhYtcTNqQVCngtbnWfux", + "SigningPubKey" : "02FE36A690D6973D55F88553F5D2C4202DE75F2CF8A6D0E17C70AC223F044501F8", + "TxnSignature" : "3045022100909D01399AFFAD1E30D250CE61F93975B7F61E47B5244D78C3E86D9806535D95022012E389E0ACB016334052B7FE07FA6CEFDC8BE82CB410FA841D5049641C89DC8F" + } + } + ], + "SigningPubKey": "", + "TransactionType": "Payment" + } +})", +{ +"Missing field 'secret'.", +"Missing field 'secret'.", +"Missing field 'account'.", +""}}, + +{ "Minimal submit_multisigned with bad signature.", R"({ "command": "submit_multisigned", "tx_json": { @@ -854,7 +948,7 @@ R"({ "Missing field 'secret'.", "Missing field 'secret'.", "Missing field 'account'.", -""}}, +"Invalid signature."}}, { "Missing tx_json in submit_multisigned.", R"({ @@ -1463,7 +1557,8 @@ public: Role role, int validatedLedgerAge, Application& app, - std::shared_ptr ledger); + std::shared_ptr ledger, + ApplyFlags flags); using submitFunc = Json::Value (*) ( Json::Value params, @@ -1472,7 +1567,8 @@ public: int validatedLedgerAge, Application& app, std::shared_ptr ledger, - ProcessTransactionFn const& processTransaction); + ProcessTransactionFn const& processTransaction, + ApplyFlags flags); using TestStuff = std::tuple ; @@ -1512,7 +1608,8 @@ public: testRole, 1, env.app(), - ledger); + ledger, + tapENABLE_TESTING); } else { @@ -1525,7 +1622,8 @@ public: 1, env.app(), ledger, - processTxn); + processTxn, + tapENABLE_TESTING); } std::string errStr;