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.
This commit is contained in:
Scott Schurr
2019-01-23 16:29:20 -08:00
committed by Nik Bougalis
parent d8c450d272
commit 36d6758945
5 changed files with 184 additions and 88 deletions

View File

@@ -1,7 +1,7 @@
//------------------------------------------------------------------------------ //------------------------------------------------------------------------------
/* /*
This file is part of rippled: https://github.com/ripple/rippled 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 Permission to use, copy, modify, and/or distribute this software for any
purpose with or without fee is hereby granted, provided that the above purpose with or without fee is hereby granted, provided that the above
@@ -81,6 +81,8 @@ enum error_code_i
// Bad parameter // Bad parameter
rpcACT_BITCOIN, rpcACT_BITCOIN,
rpcACT_MALFORMED, rpcACT_MALFORMED,
rpcALREADY_MULTISIG,
rpcALREADY_SINGLE_SIG,
rpcQUALITY_MALFORMED, rpcQUALITY_MALFORMED,
rpcBAD_BLOB, rpcBAD_BLOB,
rpcBAD_FEATURE, rpcBAD_FEATURE,
@@ -106,7 +108,7 @@ enum error_code_i
rpcPAYS_AMT_MALFORMED, rpcPAYS_AMT_MALFORMED,
rpcPORT_MALFORMED, rpcPORT_MALFORMED,
rpcPUBLIC_MALFORMED, rpcPUBLIC_MALFORMED,
rpcSIGN_FOR_MALFORMED, rpcSIGNING_MALFORMED,
rpcSENDMAX_MALFORMED, rpcSENDMAX_MALFORMED,
rpcSRC_ACT_MALFORMED, rpcSRC_ACT_MALFORMED,
rpcSRC_ACT_MISSING, rpcSRC_ACT_MISSING,

View File

@@ -1,7 +1,7 @@
//------------------------------------------------------------------------------ //------------------------------------------------------------------------------
/* /*
This file is part of rippled: https://github.com/ripple/rippled 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 Permission to use, copy, modify, and/or distribute this software for any
purpose with or without fee is hereby granted, provided that the above purpose with or without fee is hereby granted, provided that the above
@@ -50,78 +50,80 @@ public:
ErrorCategory () ErrorCategory ()
: m_unknown (rpcUNKNOWN, "unknown", "An unknown error code.") : m_unknown (rpcUNKNOWN, "unknown", "An unknown error code.")
{ {
add (rpcACT_BITCOIN, "actBitcoin", "Account is bitcoin address."); add (rpcACT_BITCOIN, "actBitcoin", "Account is bitcoin address.");
add (rpcACT_EXISTS, "actExists", "Account already exists."); add (rpcACT_EXISTS, "actExists", "Account already exists.");
add (rpcACT_MALFORMED, "actMalformed", "Account malformed."); add (rpcACT_MALFORMED, "actMalformed", "Account malformed.");
add (rpcACT_NOT_FOUND, "actNotFound", "Account not found."); add (rpcACT_NOT_FOUND, "actNotFound", "Account not found.");
add (rpcAMENDMENT_BLOCKED, "amendmentBlocked", "Amendment blocked, need upgrade."); add (rpcALREADY_MULTISIG, "alreadyMultisig", "Already multisigned.");
add (rpcATX_DEPRECATED, "deprecated", "Use the new API or specify a ledger range."); add (rpcALREADY_SINGLE_SIG, "alreadySingleSig", "Already single-signed.");
add (rpcBAD_BLOB, "badBlob", "Blob must be a non-empty hex string."); add (rpcAMENDMENT_BLOCKED, "amendmentBlocked", "Amendment blocked, need upgrade.");
add (rpcBAD_FEATURE, "badFeature", "Feature unknown or invalid."); add (rpcATX_DEPRECATED, "deprecated", "Use the new API or specify a ledger range.");
add (rpcBAD_ISSUER, "badIssuer", "Issuer account malformed."); add (rpcBAD_BLOB, "badBlob", "Blob must be a non-empty hex string.");
add (rpcBAD_MARKET, "badMarket", "No such market."); add (rpcBAD_FEATURE, "badFeature", "Feature unknown or invalid.");
add (rpcBAD_SECRET, "badSecret", "Secret does not match account."); add (rpcBAD_ISSUER, "badIssuer", "Issuer account malformed.");
add (rpcBAD_SEED, "badSeed", "Disallowed seed."); add (rpcBAD_MARKET, "badMarket", "No such market.");
add (rpcBAD_SYNTAX, "badSyntax", "Syntax error."); add (rpcBAD_SECRET, "badSecret", "Secret does not match account.");
add (rpcCHANNEL_MALFORMED, "channelMalformed", "Payment channel is malformed."); add (rpcBAD_SEED, "badSeed", "Disallowed seed.");
add (rpcCHANNEL_AMT_MALFORMED, "channelAmtMalformed","Payment channel amount is malformed."); add (rpcBAD_SYNTAX, "badSyntax", "Syntax error.");
add (rpcCOMMAND_MISSING, "commandMissing", "Missing command entry."); add (rpcCHANNEL_MALFORMED, "channelMalformed", "Payment channel is malformed.");
add (rpcDST_ACT_MALFORMED, "dstActMalformed", "Destination account is malformed."); add (rpcCHANNEL_AMT_MALFORMED, "channelAmtMalformed", "Payment channel amount is malformed.");
add (rpcDST_ACT_MISSING, "dstActMissing", "Destination account not provided."); add (rpcCOMMAND_MISSING, "commandMissing", "Missing command entry.");
add (rpcDST_ACT_NOT_FOUND, "dstActNotFound", "Destination account not found."); add (rpcDST_ACT_MALFORMED, "dstActMalformed", "Destination account is malformed.");
add (rpcDST_AMT_MALFORMED, "dstAmtMalformed", "Destination amount/currency/issuer is malformed."); add (rpcDST_ACT_MISSING, "dstActMissing", "Destination account not provided.");
add (rpcDST_ISR_MALFORMED, "dstIsrMalformed", "Destination issuer is malformed."); add (rpcDST_ACT_NOT_FOUND, "dstActNotFound", "Destination account not found.");
add (rpcFORBIDDEN, "forbidden", "Bad credentials."); add (rpcDST_AMT_MALFORMED, "dstAmtMalformed", "Destination amount/currency/issuer is malformed.");
add (rpcGENERAL, "general", "Generic error reason."); add (rpcDST_ISR_MALFORMED, "dstIsrMalformed", "Destination issuer is malformed.");
add (rpcGETS_ACT_MALFORMED, "getsActMalformed", "Gets account malformed."); add (rpcFORBIDDEN, "forbidden", "Bad credentials.");
add (rpcGETS_AMT_MALFORMED, "getsAmtMalformed", "Gets amount malformed."); add (rpcGENERAL, "general", "Generic error reason.");
add (rpcHIGH_FEE, "highFee", "Current transaction fee exceeds your limit."); add (rpcGETS_ACT_MALFORMED, "getsActMalformed", "Gets account malformed.");
add (rpcHOST_IP_MALFORMED, "hostIpMalformed", "Host IP is malformed."); add (rpcGETS_AMT_MALFORMED, "getsAmtMalformed", "Gets amount malformed.");
add (rpcINSUF_FUNDS, "insufFunds", "Insufficient funds."); add (rpcHIGH_FEE, "highFee", "Current transaction fee exceeds your limit.");
add (rpcINTERNAL, "internal", "Internal error."); add (rpcHOST_IP_MALFORMED, "hostIpMalformed", "Host IP is malformed.");
add (rpcINVALID_PARAMS, "invalidParams", "Invalid parameters."); add (rpcINSUF_FUNDS, "insufFunds", "Insufficient funds.");
add (rpcJSON_RPC, "json_rpc", "JSON-RPC transport error."); add (rpcINTERNAL, "internal", "Internal error.");
add (rpcLGR_IDXS_INVALID, "lgrIdxsInvalid", "Ledger indexes invalid."); add (rpcINVALID_PARAMS, "invalidParams", "Invalid parameters.");
add (rpcLGR_IDX_MALFORMED, "lgrIdxMalformed", "Ledger index malformed."); add (rpcJSON_RPC, "json_rpc", "JSON-RPC transport error.");
add (rpcLGR_NOT_FOUND, "lgrNotFound", "Ledger not found."); add (rpcLGR_IDXS_INVALID, "lgrIdxsInvalid", "Ledger indexes invalid.");
add (rpcLGR_NOT_VALIDATED, "lgrNotValidated", "Ledger not validated."); add (rpcLGR_IDX_MALFORMED, "lgrIdxMalformed", "Ledger index malformed.");
add (rpcLOAD_FAILED, "loadFailed", "Load failed"); add (rpcLGR_NOT_FOUND, "lgrNotFound", "Ledger not found.");
add (rpcMASTER_DISABLED, "masterDisabled", "Master key is disabled."); add (rpcLGR_NOT_VALIDATED, "lgrNotValidated", "Ledger not validated.");
add (rpcNOT_ENABLED, "notEnabled", "Not enabled in configuration."); add (rpcLOAD_FAILED, "loadFailed", "Load failed");
add (rpcNOT_IMPL, "notImpl", "Not implemented."); add (rpcMASTER_DISABLED, "masterDisabled", "Master key is disabled.");
add (rpcNOT_READY, "notReady", "Not ready to handle this request."); add (rpcNOT_ENABLED, "notEnabled", "Not enabled in configuration.");
add (rpcNOT_STANDALONE, "notStandAlone", "Operation valid in debug mode only."); add (rpcNOT_IMPL, "notImpl", "Not implemented.");
add (rpcNOT_SUPPORTED, "notSupported", "Operation not supported."); add (rpcNOT_READY, "notReady", "Not ready to handle this request.");
add (rpcNO_ACCOUNT, "noAccount", "No such account."); add (rpcNOT_STANDALONE, "notStandAlone", "Operation valid in debug mode only.");
add (rpcNO_CLOSED, "noClosed", "Closed ledger is unavailable."); add (rpcNOT_SUPPORTED, "notSupported", "Operation not supported.");
add (rpcNO_CURRENT, "noCurrent", "Current ledger is unavailable."); add (rpcNO_ACCOUNT, "noAccount", "No such account.");
add (rpcNO_EVENTS, "noEvents", "Current transport does not support events."); add (rpcNO_CLOSED, "noClosed", "Closed ledger is unavailable.");
add (rpcNO_NETWORK, "noNetwork", "Not synced to Ripple network."); add (rpcNO_CURRENT, "noCurrent", "Current ledger is unavailable.");
add (rpcNO_PATH, "noPath", "Unable to find a ripple path."); add (rpcNO_EVENTS, "noEvents", "Current transport does not support events.");
add (rpcNO_PERMISSION, "noPermission", "You don't have permission for this command."); add (rpcNO_NETWORK, "noNetwork", "Not synced to Ripple network.");
add (rpcNO_PF_REQUEST, "noPathRequest", "No pathfinding request in progress."); add (rpcNO_PATH, "noPath", "Unable to find a ripple path.");
add (rpcPASSWD_CHANGED, "passwdChanged", "Wrong key, password changed."); add (rpcNO_PERMISSION, "noPermission", "You don't have permission for this command.");
add (rpcPAYS_ACT_MALFORMED, "paysActMalformed", "Pays account malformed."); add (rpcNO_PF_REQUEST, "noPathRequest", "No pathfinding request in progress.");
add (rpcPAYS_AMT_MALFORMED, "paysAmtMalformed", "Pays amount malformed."); add (rpcPASSWD_CHANGED, "passwdChanged", "Wrong key, password changed.");
add (rpcPORT_MALFORMED, "portMalformed", "Port is malformed."); add (rpcPAYS_ACT_MALFORMED, "paysActMalformed", "Pays account malformed.");
add (rpcPUBLIC_MALFORMED, "publicMalformed", "Public key is malformed."); add (rpcPAYS_AMT_MALFORMED, "paysAmtMalformed", "Pays amount malformed.");
add (rpcQUALITY_MALFORMED, "qualityMalformed", "Quality malformed."); add (rpcPORT_MALFORMED, "portMalformed", "Port is malformed.");
add (rpcSIGN_FOR_MALFORMED, "signForMalformed", "Signing for account is malformed."); add (rpcPUBLIC_MALFORMED, "publicMalformed", "Public key is malformed.");
add (rpcSLOW_DOWN, "slowDown", "You are placing too much load on the server."); add (rpcQUALITY_MALFORMED, "qualityMalformed", "Quality malformed.");
add (rpcSRC_ACT_MALFORMED, "srcActMalformed", "Source account is malformed."); add (rpcSIGNING_MALFORMED, "signingMalformed", "Signing of transaction is malformed.");
add (rpcSRC_ACT_MISSING, "srcActMissing", "Source account not provided."); add (rpcSLOW_DOWN, "slowDown", "You are placing too much load on the server.");
add (rpcSRC_ACT_NOT_FOUND, "srcActNotFound", "Source account not found."); add (rpcSRC_ACT_MALFORMED, "srcActMalformed", "Source account is malformed.");
add (rpcSRC_AMT_MALFORMED, "srcAmtMalformed", "Source amount/currency/issuer is malformed."); add (rpcSRC_ACT_MISSING, "srcActMissing", "Source account not provided.");
add (rpcSRC_CUR_MALFORMED, "srcCurMalformed", "Source currency is malformed."); add (rpcSRC_ACT_NOT_FOUND, "srcActNotFound", "Source account not found.");
add (rpcSRC_ISR_MALFORMED, "srcIsrMalformed", "Source issuer is malformed."); add (rpcSRC_AMT_MALFORMED, "srcAmtMalformed", "Source amount/currency/issuer is malformed.");
add (rpcSRC_MISSING, "srcMissing", "Source is missing."); add (rpcSRC_CUR_MALFORMED, "srcCurMalformed", "Source currency is malformed.");
add (rpcSRC_UNCLAIMED, "srcUnclaimed", "Source account is not claimed."); add (rpcSRC_ISR_MALFORMED, "srcIsrMalformed", "Source issuer is malformed.");
add (rpcSTREAM_MALFORMED, "malformedStream", "Stream malformed."); add (rpcSRC_MISSING, "srcMissing", "Source is missing.");
add (rpcTOO_BUSY, "tooBusy", "The server is too busy to help you now."); add (rpcSRC_UNCLAIMED, "srcUnclaimed", "Source account is not claimed.");
add (rpcTXN_NOT_FOUND, "txnNotFound", "Transaction not found."); add (rpcSTREAM_MALFORMED, "malformedStream", "Stream malformed.");
add (rpcUNKNOWN_COMMAND, "unknownCmd", "Unknown method."); add (rpcTOO_BUSY, "tooBusy", "The server is too busy to help you now.");
add (rpcWRONG_SEED, "wrongSeed", "The regular key does not point as the master key."); add (rpcTXN_NOT_FOUND, "txnNotFound", "Transaction not found.");
add (rpcSENDMAX_MALFORMED, "sendMaxMalformed", "SendMax amount malformed."); 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 ErrorInfo const& get (error_code_i code) const

View File

@@ -80,6 +80,11 @@ public:
(multiSignature_ != nullptr)); (multiSignature_ != nullptr));
} }
bool isSingleSigning () const
{
return !isMultiSigning();
}
// When multi-signing we should not edit the tx_json fields. // When multi-signing we should not edit the tx_json fields.
bool editFields () const bool editFields () const
{ {
@@ -447,9 +452,20 @@ transactionPreProcessImpl (
tx_json[jss::Flags] = tfFullyCanonicalSig; 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 (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); signingArgs.setPublicKey (keypair.first);
}
else if (signingArgs.isSingleSigning())
{
if (tx_json.isMember (sfSigners.jsonName))
return rpcError (rpcALREADY_MULTISIG);
}
if (verify) if (verify)
{ {
@@ -524,7 +540,7 @@ transactionPreProcessImpl (
signingArgs.moveMultiSignature (std::move (multisig)); signingArgs.moveMultiSignature (std::move (multisig));
} }
else else if (signingArgs.isSingleSigning())
{ {
stpTrans->sign (keypair.first, keypair.second); stpTrans->sign (keypair.first, keypair.second);
} }
@@ -1131,6 +1147,10 @@ Json::Value transactionSubmitMultiSigned (
return RPC::make_error (rpcINVALID_PARAMS, err.str ()); 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. // The Fee field must be in XRP and greater than zero.
auto const fee = stpTrans->getFieldAmount (sfFee); auto const fee = stpTrans->getFieldAmount (sfFee);

View File

@@ -855,6 +855,36 @@ R"({
"Missing field 'tx_json.Sequence'.", "Missing field 'tx_json.Sequence'.",
"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__, { "Minimal sign_for.", __LINE__,
R"({ R"({
"command": "doesnt_matter", "command": "doesnt_matter",
@@ -1109,8 +1139,8 @@ R"({
} }
})", })",
{{ {{
"Secret does not match account.", "Already multisigned.",
"Secret does not match account.", "Already multisigned.",
"Duplicate Signers:Signer:Account entries (rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh) are not allowed.", "Duplicate Signers:Signer:Account entries (rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh) are not allowed.",
""}}}, ""}}},
@@ -1139,8 +1169,8 @@ R"({
} }
})", })",
{{ {{
"Secret does not match account.", "Already multisigned.",
"Secret does not match account.", "Already multisigned.",
"", "",
""}}}, ""}}},
@@ -1169,8 +1199,8 @@ R"({
} }
})", })",
{{ {{
"Secret does not match account.", "Already multisigned.",
"Secret does not match account.", "Already multisigned.",
"Invalid signature.", "Invalid signature.",
"Invalid signature."}}}, "Invalid signature."}}},
@@ -1215,6 +1245,37 @@ R"({
"Missing field 'tx_json.TransactionType'.", "Missing field 'tx_json.TransactionType'.",
"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__, { "Invalid field 'tx_json': string instead of object", __LINE__,
R"({ R"({
"command": "doesnt_matter", "command": "doesnt_matter",
@@ -2337,7 +2398,8 @@ public:
{ {
std::ostringstream description; std::ostringstream description;
description << txnTest.description << " Called " description << txnTest.description << " Called "
<< get<2>(testFunc) << "()"; << get<2>(testFunc) << "(). Got \'"
<< errStr << "\'";
fail (description.str(), __FILE__, txnTest.line); fail (description.str(), __FILE__, txnTest.line);
} }
} }

View File

@@ -6970,10 +6970,20 @@ public:
Json::Value exp; Json::Value exp;
Json::Reader{}.parse (rpcCallTest.exp, exp); Json::Reader{}.parse (rpcCallTest.exp, exp);
// If there is an "error_code" field, remove it. Error codes // Lambda to remove the "params[0u]:error_code" field if present.
// are not expected to stay stable between releases. // Error codes are not expected to be stable between releases.
got.removeMember ("error_code"); auto rmErrorCode = [] (Json::Value& json)
exp.removeMember ("error_code"); {
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. // Pass if we didn't expect a throw and we got what we expected.
if ((rpcCallTest.throwsWhat == RPCCallTestData::no_exception) && if ((rpcCallTest.throwsWhat == RPCCallTestData::no_exception) &&