From 36d6758945954fab757b024b2afc8c3a5ad76919 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Wed, 23 Jan 2019 16:29:20 -0800 Subject: [PATCH] Disallow both single- and multi-signing in RPC (RIPD-1713): The ledger already declared a transaction that is both single- and multi-signing malformed. This just adds some checking in the signing RPC commands (like submit and sign_for) which allows that sort of error to be identified a bit closer to the user. In the process of adding this code a bug was found in the RPCCall unit test. That bug is fixed as well. --- src/ripple/protocol/ErrorCodes.h | 6 +- src/ripple/protocol/impl/ErrorCodes.cpp | 148 ++++++++++++------------ src/ripple/rpc/impl/TransactionSign.cpp | 24 +++- src/test/rpc/JSONRPC_test.cpp | 76 ++++++++++-- src/test/rpc/RPCCall_test.cpp | 18 ++- 5 files changed, 184 insertions(+), 88 deletions(-) diff --git a/src/ripple/protocol/ErrorCodes.h b/src/ripple/protocol/ErrorCodes.h index 29673b45c..7819faf6a 100644 --- a/src/ripple/protocol/ErrorCodes.h +++ b/src/ripple/protocol/ErrorCodes.h @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of rippled: https://github.com/ripple/rippled - Copyright (c) 2012, 2013 Ripple Labs Inc. + Copyright (c) 2012 - 2019 Ripple Labs Inc. Permission to use, copy, modify, and/or distribute this software for any purpose with or without fee is hereby granted, provided that the above @@ -81,6 +81,8 @@ enum error_code_i // Bad parameter rpcACT_BITCOIN, rpcACT_MALFORMED, + rpcALREADY_MULTISIG, + rpcALREADY_SINGLE_SIG, rpcQUALITY_MALFORMED, rpcBAD_BLOB, rpcBAD_FEATURE, @@ -106,7 +108,7 @@ enum error_code_i rpcPAYS_AMT_MALFORMED, rpcPORT_MALFORMED, rpcPUBLIC_MALFORMED, - rpcSIGN_FOR_MALFORMED, + rpcSIGNING_MALFORMED, rpcSENDMAX_MALFORMED, rpcSRC_ACT_MALFORMED, rpcSRC_ACT_MISSING, diff --git a/src/ripple/protocol/impl/ErrorCodes.cpp b/src/ripple/protocol/impl/ErrorCodes.cpp index 4b78a0ff7..20078c3bd 100644 --- a/src/ripple/protocol/impl/ErrorCodes.cpp +++ b/src/ripple/protocol/impl/ErrorCodes.cpp @@ -1,7 +1,7 @@ //------------------------------------------------------------------------------ /* This file is part of rippled: https://github.com/ripple/rippled - Copyright (c) 2012, 2013 Ripple Labs Inc. + Copyright (c) 2012 - 2019 Ripple Labs Inc. Permission to use, copy, modify, and/or distribute this software for any purpose with or without fee is hereby granted, provided that the above @@ -50,78 +50,80 @@ public: ErrorCategory () : m_unknown (rpcUNKNOWN, "unknown", "An unknown error code.") { - add (rpcACT_BITCOIN, "actBitcoin", "Account is bitcoin address."); - add (rpcACT_EXISTS, "actExists", "Account already exists."); - add (rpcACT_MALFORMED, "actMalformed", "Account malformed."); - add (rpcACT_NOT_FOUND, "actNotFound", "Account not found."); - add (rpcAMENDMENT_BLOCKED, "amendmentBlocked", "Amendment blocked, need upgrade."); - add (rpcATX_DEPRECATED, "deprecated", "Use the new API or specify a ledger range."); - add (rpcBAD_BLOB, "badBlob", "Blob must be a non-empty hex string."); - add (rpcBAD_FEATURE, "badFeature", "Feature unknown or invalid."); - add (rpcBAD_ISSUER, "badIssuer", "Issuer account malformed."); - add (rpcBAD_MARKET, "badMarket", "No such market."); - add (rpcBAD_SECRET, "badSecret", "Secret does not match account."); - add (rpcBAD_SEED, "badSeed", "Disallowed seed."); - add (rpcBAD_SYNTAX, "badSyntax", "Syntax error."); - add (rpcCHANNEL_MALFORMED, "channelMalformed", "Payment channel is malformed."); - add (rpcCHANNEL_AMT_MALFORMED, "channelAmtMalformed","Payment channel amount is malformed."); - add (rpcCOMMAND_MISSING, "commandMissing", "Missing command entry."); - add (rpcDST_ACT_MALFORMED, "dstActMalformed", "Destination account is malformed."); - add (rpcDST_ACT_MISSING, "dstActMissing", "Destination account not provided."); - add (rpcDST_ACT_NOT_FOUND, "dstActNotFound", "Destination account not found."); - add (rpcDST_AMT_MALFORMED, "dstAmtMalformed", "Destination amount/currency/issuer is malformed."); - add (rpcDST_ISR_MALFORMED, "dstIsrMalformed", "Destination issuer is malformed."); - add (rpcFORBIDDEN, "forbidden", "Bad credentials."); - add (rpcGENERAL, "general", "Generic error reason."); - add (rpcGETS_ACT_MALFORMED, "getsActMalformed", "Gets account malformed."); - add (rpcGETS_AMT_MALFORMED, "getsAmtMalformed", "Gets amount malformed."); - add (rpcHIGH_FEE, "highFee", "Current transaction fee exceeds your limit."); - add (rpcHOST_IP_MALFORMED, "hostIpMalformed", "Host IP is malformed."); - add (rpcINSUF_FUNDS, "insufFunds", "Insufficient funds."); - add (rpcINTERNAL, "internal", "Internal error."); - add (rpcINVALID_PARAMS, "invalidParams", "Invalid parameters."); - add (rpcJSON_RPC, "json_rpc", "JSON-RPC transport error."); - add (rpcLGR_IDXS_INVALID, "lgrIdxsInvalid", "Ledger indexes invalid."); - add (rpcLGR_IDX_MALFORMED, "lgrIdxMalformed", "Ledger index malformed."); - add (rpcLGR_NOT_FOUND, "lgrNotFound", "Ledger not found."); - add (rpcLGR_NOT_VALIDATED, "lgrNotValidated", "Ledger not validated."); - add (rpcLOAD_FAILED, "loadFailed", "Load failed"); - add (rpcMASTER_DISABLED, "masterDisabled", "Master key is disabled."); - add (rpcNOT_ENABLED, "notEnabled", "Not enabled in configuration."); - add (rpcNOT_IMPL, "notImpl", "Not implemented."); - add (rpcNOT_READY, "notReady", "Not ready to handle this request."); - add (rpcNOT_STANDALONE, "notStandAlone", "Operation valid in debug mode only."); - add (rpcNOT_SUPPORTED, "notSupported", "Operation not supported."); - add (rpcNO_ACCOUNT, "noAccount", "No such account."); - add (rpcNO_CLOSED, "noClosed", "Closed ledger is unavailable."); - add (rpcNO_CURRENT, "noCurrent", "Current ledger is unavailable."); - add (rpcNO_EVENTS, "noEvents", "Current transport does not support events."); - add (rpcNO_NETWORK, "noNetwork", "Not synced to Ripple network."); - add (rpcNO_PATH, "noPath", "Unable to find a ripple path."); - add (rpcNO_PERMISSION, "noPermission", "You don't have permission for this command."); - add (rpcNO_PF_REQUEST, "noPathRequest", "No pathfinding request in progress."); - add (rpcPASSWD_CHANGED, "passwdChanged", "Wrong key, password changed."); - add (rpcPAYS_ACT_MALFORMED, "paysActMalformed", "Pays account malformed."); - add (rpcPAYS_AMT_MALFORMED, "paysAmtMalformed", "Pays amount malformed."); - add (rpcPORT_MALFORMED, "portMalformed", "Port is malformed."); - add (rpcPUBLIC_MALFORMED, "publicMalformed", "Public key is malformed."); - add (rpcQUALITY_MALFORMED, "qualityMalformed", "Quality malformed."); - add (rpcSIGN_FOR_MALFORMED, "signForMalformed", "Signing for account is malformed."); - add (rpcSLOW_DOWN, "slowDown", "You are placing too much load on the server."); - add (rpcSRC_ACT_MALFORMED, "srcActMalformed", "Source account is malformed."); - add (rpcSRC_ACT_MISSING, "srcActMissing", "Source account not provided."); - add (rpcSRC_ACT_NOT_FOUND, "srcActNotFound", "Source account not found."); - add (rpcSRC_AMT_MALFORMED, "srcAmtMalformed", "Source amount/currency/issuer is malformed."); - add (rpcSRC_CUR_MALFORMED, "srcCurMalformed", "Source currency is malformed."); - add (rpcSRC_ISR_MALFORMED, "srcIsrMalformed", "Source issuer is malformed."); - add (rpcSRC_MISSING, "srcMissing", "Source is missing."); - add (rpcSRC_UNCLAIMED, "srcUnclaimed", "Source account is not claimed."); - add (rpcSTREAM_MALFORMED, "malformedStream", "Stream malformed."); - add (rpcTOO_BUSY, "tooBusy", "The server is too busy to help you now."); - add (rpcTXN_NOT_FOUND, "txnNotFound", "Transaction not found."); - add (rpcUNKNOWN_COMMAND, "unknownCmd", "Unknown method."); - add (rpcWRONG_SEED, "wrongSeed", "The regular key does not point as the master key."); - add (rpcSENDMAX_MALFORMED, "sendMaxMalformed", "SendMax amount malformed."); + add (rpcACT_BITCOIN, "actBitcoin", "Account is bitcoin address."); + add (rpcACT_EXISTS, "actExists", "Account already exists."); + add (rpcACT_MALFORMED, "actMalformed", "Account malformed."); + add (rpcACT_NOT_FOUND, "actNotFound", "Account not found."); + add (rpcALREADY_MULTISIG, "alreadyMultisig", "Already multisigned."); + add (rpcALREADY_SINGLE_SIG, "alreadySingleSig", "Already single-signed."); + add (rpcAMENDMENT_BLOCKED, "amendmentBlocked", "Amendment blocked, need upgrade."); + add (rpcATX_DEPRECATED, "deprecated", "Use the new API or specify a ledger range."); + add (rpcBAD_BLOB, "badBlob", "Blob must be a non-empty hex string."); + add (rpcBAD_FEATURE, "badFeature", "Feature unknown or invalid."); + add (rpcBAD_ISSUER, "badIssuer", "Issuer account malformed."); + add (rpcBAD_MARKET, "badMarket", "No such market."); + add (rpcBAD_SECRET, "badSecret", "Secret does not match account."); + add (rpcBAD_SEED, "badSeed", "Disallowed seed."); + add (rpcBAD_SYNTAX, "badSyntax", "Syntax error."); + add (rpcCHANNEL_MALFORMED, "channelMalformed", "Payment channel is malformed."); + add (rpcCHANNEL_AMT_MALFORMED, "channelAmtMalformed", "Payment channel amount is malformed."); + add (rpcCOMMAND_MISSING, "commandMissing", "Missing command entry."); + add (rpcDST_ACT_MALFORMED, "dstActMalformed", "Destination account is malformed."); + add (rpcDST_ACT_MISSING, "dstActMissing", "Destination account not provided."); + add (rpcDST_ACT_NOT_FOUND, "dstActNotFound", "Destination account not found."); + add (rpcDST_AMT_MALFORMED, "dstAmtMalformed", "Destination amount/currency/issuer is malformed."); + add (rpcDST_ISR_MALFORMED, "dstIsrMalformed", "Destination issuer is malformed."); + add (rpcFORBIDDEN, "forbidden", "Bad credentials."); + add (rpcGENERAL, "general", "Generic error reason."); + add (rpcGETS_ACT_MALFORMED, "getsActMalformed", "Gets account malformed."); + add (rpcGETS_AMT_MALFORMED, "getsAmtMalformed", "Gets amount malformed."); + add (rpcHIGH_FEE, "highFee", "Current transaction fee exceeds your limit."); + add (rpcHOST_IP_MALFORMED, "hostIpMalformed", "Host IP is malformed."); + add (rpcINSUF_FUNDS, "insufFunds", "Insufficient funds."); + add (rpcINTERNAL, "internal", "Internal error."); + add (rpcINVALID_PARAMS, "invalidParams", "Invalid parameters."); + add (rpcJSON_RPC, "json_rpc", "JSON-RPC transport error."); + add (rpcLGR_IDXS_INVALID, "lgrIdxsInvalid", "Ledger indexes invalid."); + add (rpcLGR_IDX_MALFORMED, "lgrIdxMalformed", "Ledger index malformed."); + add (rpcLGR_NOT_FOUND, "lgrNotFound", "Ledger not found."); + add (rpcLGR_NOT_VALIDATED, "lgrNotValidated", "Ledger not validated."); + add (rpcLOAD_FAILED, "loadFailed", "Load failed"); + add (rpcMASTER_DISABLED, "masterDisabled", "Master key is disabled."); + add (rpcNOT_ENABLED, "notEnabled", "Not enabled in configuration."); + add (rpcNOT_IMPL, "notImpl", "Not implemented."); + add (rpcNOT_READY, "notReady", "Not ready to handle this request."); + add (rpcNOT_STANDALONE, "notStandAlone", "Operation valid in debug mode only."); + add (rpcNOT_SUPPORTED, "notSupported", "Operation not supported."); + add (rpcNO_ACCOUNT, "noAccount", "No such account."); + add (rpcNO_CLOSED, "noClosed", "Closed ledger is unavailable."); + add (rpcNO_CURRENT, "noCurrent", "Current ledger is unavailable."); + add (rpcNO_EVENTS, "noEvents", "Current transport does not support events."); + add (rpcNO_NETWORK, "noNetwork", "Not synced to Ripple network."); + add (rpcNO_PATH, "noPath", "Unable to find a ripple path."); + add (rpcNO_PERMISSION, "noPermission", "You don't have permission for this command."); + add (rpcNO_PF_REQUEST, "noPathRequest", "No pathfinding request in progress."); + add (rpcPASSWD_CHANGED, "passwdChanged", "Wrong key, password changed."); + add (rpcPAYS_ACT_MALFORMED, "paysActMalformed", "Pays account malformed."); + add (rpcPAYS_AMT_MALFORMED, "paysAmtMalformed", "Pays amount malformed."); + add (rpcPORT_MALFORMED, "portMalformed", "Port is malformed."); + add (rpcPUBLIC_MALFORMED, "publicMalformed", "Public key is malformed."); + add (rpcQUALITY_MALFORMED, "qualityMalformed", "Quality malformed."); + add (rpcSIGNING_MALFORMED, "signingMalformed", "Signing of transaction is malformed."); + add (rpcSLOW_DOWN, "slowDown", "You are placing too much load on the server."); + add (rpcSRC_ACT_MALFORMED, "srcActMalformed", "Source account is malformed."); + add (rpcSRC_ACT_MISSING, "srcActMissing", "Source account not provided."); + add (rpcSRC_ACT_NOT_FOUND, "srcActNotFound", "Source account not found."); + add (rpcSRC_AMT_MALFORMED, "srcAmtMalformed", "Source amount/currency/issuer is malformed."); + add (rpcSRC_CUR_MALFORMED, "srcCurMalformed", "Source currency is malformed."); + add (rpcSRC_ISR_MALFORMED, "srcIsrMalformed", "Source issuer is malformed."); + add (rpcSRC_MISSING, "srcMissing", "Source is missing."); + add (rpcSRC_UNCLAIMED, "srcUnclaimed", "Source account is not claimed."); + add (rpcSTREAM_MALFORMED, "malformedStream", "Stream malformed."); + add (rpcTOO_BUSY, "tooBusy", "The server is too busy to help you now."); + add (rpcTXN_NOT_FOUND, "txnNotFound", "Transaction not found."); + add (rpcUNKNOWN_COMMAND, "unknownCmd", "Unknown method."); + add (rpcWRONG_SEED, "wrongSeed", "The regular key does not point as the master key."); + add (rpcSENDMAX_MALFORMED, "sendMaxMalformed", "SendMax amount malformed."); } ErrorInfo const& get (error_code_i code) const diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index cd9e21880..d77baa6a6 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -80,6 +80,11 @@ public: (multiSignature_ != nullptr)); } + bool isSingleSigning () const + { + return !isMultiSigning(); + } + // When multi-signing we should not edit the tx_json fields. bool editFields () const { @@ -447,9 +452,20 @@ transactionPreProcessImpl ( tx_json[jss::Flags] = tfFullyCanonicalSig; } - // If multisigning then we need to return the public key. + // If multisigning there should not be a single signature and vice versa. if (signingArgs.isMultiSigning()) + { + if (tx_json.isMember (sfTxnSignature.jsonName)) + return rpcError (rpcALREADY_SINGLE_SIG); + + // If multisigning then we need to return the public key. signingArgs.setPublicKey (keypair.first); + } + else if (signingArgs.isSingleSigning()) + { + if (tx_json.isMember (sfSigners.jsonName)) + return rpcError (rpcALREADY_MULTISIG); + } if (verify) { @@ -524,7 +540,7 @@ transactionPreProcessImpl ( signingArgs.moveMultiSignature (std::move (multisig)); } - else + else if (signingArgs.isSingleSigning()) { stpTrans->sign (keypair.first, keypair.second); } @@ -1131,6 +1147,10 @@ Json::Value transactionSubmitMultiSigned ( return RPC::make_error (rpcINVALID_PARAMS, err.str ()); } + // There may not be a TxnSignature field. + if (stpTrans->isFieldPresent (sfTxnSignature)) + return rpcError (rpcSIGNING_MALFORMED); + // The Fee field must be in XRP and greater than zero. auto const fee = stpTrans->getFieldAmount (sfFee); diff --git a/src/test/rpc/JSONRPC_test.cpp b/src/test/rpc/JSONRPC_test.cpp index 543a0970d..4f5efaf50 100644 --- a/src/test/rpc/JSONRPC_test.cpp +++ b/src/test/rpc/JSONRPC_test.cpp @@ -855,6 +855,36 @@ R"({ "Missing field 'tx_json.Sequence'.", "Missing field 'tx_json.Sequence'."}}}, +{ "Single-sign a multisigned transaction.", __LINE__, +R"({ + "command": "doesnt_matter", + "account": "rPcNzota6B8YBokhYtcTNqQVCngtbnWfux", + "secret": "a", + "tx_json": { + "Account" : "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "Amount" : "1000000000", + "Destination" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "Fee" : "50", + "Sequence" : 0, + "Signers" : [ + { + "Signer" : { + "Account" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "SigningPubKey" : "0330E7FC9D56BB25D6893BA3F317AE5BCF33B3291BD63DB32654A313222F7FD020", + "TxnSignature" : "304502210080EB23E78A841DDC5E3A4F10DE6EAF052207D6B519BF8954467ADB221B3F349002202CA458E8D4E4DE7176D27A91628545E7B295A5DFC8ADF0B5CD3E279B6FA02998" + } + } + ], + "SigningPubKey" : "", + "TransactionType" : "Payment" + } +})", +{{ +"Already multisigned.", +"Already multisigned.", +"Secret does not match account.", +""}}}, + { "Minimal sign_for.", __LINE__, R"({ "command": "doesnt_matter", @@ -1109,8 +1139,8 @@ R"({ } })", {{ -"Secret does not match account.", -"Secret does not match account.", +"Already multisigned.", +"Already multisigned.", "Duplicate Signers:Signer:Account entries (rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh) are not allowed.", ""}}}, @@ -1139,8 +1169,8 @@ R"({ } })", {{ -"Secret does not match account.", -"Secret does not match account.", +"Already multisigned.", +"Already multisigned.", "", ""}}}, @@ -1169,8 +1199,8 @@ R"({ } })", {{ -"Secret does not match account.", -"Secret does not match account.", +"Already multisigned.", +"Already multisigned.", "Invalid signature.", "Invalid signature."}}}, @@ -1215,6 +1245,37 @@ R"({ "Missing field 'tx_json.TransactionType'.", "Missing field 'tx_json.TransactionType'."}}}, +{ "TxnSignature in sign_for.", __LINE__, +R"({ + "command": "doesnt_matter", + "account": "rPcNzota6B8YBokhYtcTNqQVCngtbnWfux", + "secret": "c", + "tx_json": { + "Account" : "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA", + "Amount" : "1000000000", + "Destination" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "Fee" : "50", + "Sequence" : 0, + "Signers" : [ + { + "Signer" : { + "Account" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh", + "SigningPubKey" : "0330E7FC9D56BB25D6893BA3F317AE5BCF33B3291BD63DB32654A313222F7FD020", + "TxnSignature" : "304502210080EB23E78A841DDC5E3A4F10DE6EAF052207D6B519BF8954467ADB221B3F349002202CA458E8D4E4DE7176D27A91628545E7B295A5DFC8ADF0B5CD3E279B6FA02998" + } + } + ], + "SigningPubKey" : "", + "TxnSignature" : "304502210080EB23E78A841DDC5E3A4F10DE6EAF052207D6B519BF8954467ADB221B3F349002202CA458E8D4E4DE7176D27A91628545E7B295A5DFC8ADF0B5CD3E279B6FA02998", + "TransactionType" : "Payment" + } +})", +{{ +"Already multisigned.", +"Already multisigned.", +"Already single-signed.", +"Signing of transaction is malformed."}}}, + { "Invalid field 'tx_json': string instead of object", __LINE__, R"({ "command": "doesnt_matter", @@ -2337,7 +2398,8 @@ public: { std::ostringstream description; description << txnTest.description << " Called " - << get<2>(testFunc) << "()"; + << get<2>(testFunc) << "(). Got \'" + << errStr << "\'"; fail (description.str(), __FILE__, txnTest.line); } } diff --git a/src/test/rpc/RPCCall_test.cpp b/src/test/rpc/RPCCall_test.cpp index 1f63b8316..6f7016ff0 100644 --- a/src/test/rpc/RPCCall_test.cpp +++ b/src/test/rpc/RPCCall_test.cpp @@ -6970,10 +6970,20 @@ public: Json::Value exp; Json::Reader{}.parse (rpcCallTest.exp, exp); - // If there is an "error_code" field, remove it. Error codes - // are not expected to stay stable between releases. - got.removeMember ("error_code"); - exp.removeMember ("error_code"); + // Lambda to remove the "params[0u]:error_code" field if present. + // Error codes are not expected to be stable between releases. + auto rmErrorCode = [] (Json::Value& json) + { + if (json.isMember (jss::params) && + json[jss::params].isArray() && + json[jss::params].size() > 0 && + json[jss::params][0u].isObject()) + { + json[jss::params][0u].removeMember (jss::error_code); + } + }; + rmErrorCode (got); + rmErrorCode (exp); // Pass if we didn't expect a throw and we got what we expected. if ((rpcCallTest.throwsWhat == RPCCallTestData::no_exception) &&