From 84e3184106446540310fd846933c275467900595 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Fri, 6 May 2016 19:54:52 -0700 Subject: [PATCH] Stricter validation of JSON input (RIPD-1100): Passing in objects, arrays or non-string objects previously generated nondescript errors. Improve the error messages returned to clients. Add unit tests to ensure that incorrect inputs are reliably detected and generate descriptive and accurate errors. --- src/ripple/rpc/KeypairForSignature.h | 4 +- src/ripple/rpc/handlers/WalletPropose.cpp | 12 +- src/ripple/rpc/impl/KeypairForSignature.cpp | 134 ++++-- src/ripple/rpc/tests/JSONRPC.test.cpp | 10 +- src/ripple/rpc/tests/KeyGeneration.test.cpp | 496 ++++++++++++++++++++ 5 files changed, 606 insertions(+), 50 deletions(-) diff --git a/src/ripple/rpc/KeypairForSignature.h b/src/ripple/rpc/KeypairForSignature.h index 9e455f739f..c1398cc6da 100644 --- a/src/ripple/rpc/KeypairForSignature.h +++ b/src/ripple/rpc/KeypairForSignature.h @@ -24,12 +24,14 @@ #include #include #include +#include +#include namespace ripple { namespace RPC { boost::optional -getSeedFromRPC (Json::Value const& params); +getSeedFromRPC (Json::Value const& params, Json::Value& error); std::pair keypairForSignature (Json::Value const& params, Json::Value& error); diff --git a/src/ripple/rpc/handlers/WalletPropose.cpp b/src/ripple/rpc/handlers/WalletPropose.cpp index d113f07298..dad3e9d0cc 100644 --- a/src/ripple/rpc/handlers/WalletPropose.cpp +++ b/src/ripple/rpc/handlers/WalletPropose.cpp @@ -77,6 +77,12 @@ Json::Value walletPropose (Json::Value const& params) if (params.isMember (jss::key_type)) { + if (! params[jss::key_type].isString()) + { + return RPC::expected_field_error ( + jss::key_type, "string"); + } + keyType = keyTypeFromString ( params[jss::key_type].asString()); @@ -88,10 +94,10 @@ Json::Value walletPropose (Json::Value const& params) params.isMember (jss::seed) || params.isMember (jss::seed_hex)) { - seed = RPC::getSeedFromRPC (params); - + Json::Value err; + seed = RPC::getSeedFromRPC (params, err); if (!seed) - return rpcError(rpcBAD_SEED); + return err; } else { diff --git a/src/ripple/rpc/impl/KeypairForSignature.cpp b/src/ripple/rpc/impl/KeypairForSignature.cpp index 562c86cb35..1d53010967 100644 --- a/src/ripple/rpc/impl/KeypairForSignature.cpp +++ b/src/ripple/rpc/impl/KeypairForSignature.cpp @@ -18,47 +18,76 @@ //============================================================================== #include +#include #include #include #include #include -#include namespace ripple { namespace RPC { boost::optional -getSeedFromRPC (Json::Value const& params) +getSeedFromRPC (Json::Value const& params, Json::Value& error) { - bool const hasPassphrase = params.isMember (jss::passphrase); - bool const hasSeed = params.isMember (jss::seed); - bool const hasHexSeed = params.isMember (jss::seed_hex); - - int const count = - (hasPassphrase ? 1 : 0) + - (hasSeed ? 1 : 0) + - (hasHexSeed ? 1 : 0); - - if (count == 1) + // The array should be constexpr, but that makes Visual Studio unhappy. + static char const* const seedTypes[] { - if (hasSeed) - return parseBase58 (params[jss::seed].asString()); + jss::passphrase.c_str(), + jss::seed.c_str(), + jss::seed_hex.c_str() + }; - if (hasPassphrase) - return parseGenericSeed (params[jss::passphrase].asString()); - - if (hasHexSeed) + // Identify which seed type is in use. + char const* seedType = nullptr; + int count = 0; + for (auto t : seedTypes) + { + if (params.isMember (t)) { - uint128 seed; - - if (seed.SetHexExact (params[jss::seed_hex].asString())) - return Seed { Slice(seed.data(), seed.size()) }; - - return boost::none; + ++count; + seedType = t; } } - return boost::none; + if (count != 1) + { + error = RPC::make_param_error ( + "Exactly one of the following must be specified: " + + std::string(jss::passphrase) + ", " + + std::string(jss::seed) + " or " + + std::string(jss::seed_hex)); + return boost::none; + } + + // Make sure a string is present + if (! params[seedType].isString()) + { + error = RPC::expected_field_error (seedType, "string"); + return boost::none; + } + + auto const fieldContents = params[seedType].asString(); + + // Convert string to seed. + boost::optional seed; + + if (seedType == jss::seed.c_str()) + seed = parseBase58 (fieldContents); + else if (seedType == jss::passphrase.c_str()) + seed = parseGenericSeed (fieldContents); + else if (seedType == jss::seed_hex.c_str()) + { + uint128 s; + + if (s.SetHexExact (fieldContents)) + seed.emplace (Slice(s.data(), s.size())); + } + + if (!seed) + error = rpcError (rpcBAD_SEED); + + return seed; } std::pair @@ -78,33 +107,30 @@ keypairForSignature (Json::Value const& params, Json::Value& error) // Identify which secret type is in use. char const* secretType = nullptr; - int secretCount = 0; + int count = 0; for (auto t : secretTypes) { if (params.isMember (t)) { - ++secretCount; + ++count; secretType = t; } } - if (secretCount == 0 || secretType == nullptr) + if (count == 0 || secretType == nullptr) { error = RPC::missing_field_error (jss::secret); return { }; } - if (secretCount > 1) + if (count > 1) { - // `passphrase`, `secret`, `seed`, and `seed_hex` are mutually exclusive. - error = rpcError (rpcBAD_SECRET); - return { }; - } - - if (has_key_type && (secretType == jss::secret.c_str())) - { - // `secret` is deprecated. - error = rpcError (rpcBAD_SECRET); + error = RPC::make_param_error ( + "Exactly one of the following must be specified: " + + std::string(jss::passphrase) + ", " + + std::string(jss::secret) + ", " + + std::string(jss::seed) + " or " + + std::string(jss::seed_hex)); return { }; } @@ -113,27 +139,53 @@ keypairForSignature (Json::Value const& params, Json::Value& error) if (has_key_type) { + if (! params[jss::key_type].isString()) + { + error = RPC::expected_field_error ( + jss::key_type, "string"); + return { }; + } + keyType = keyTypeFromString ( params[jss::key_type].asString()); if (keyType == KeyType::invalid) { - error = rpcError (rpcBAD_SEED); + error = RPC::invalid_field_error(jss::key_type); return { }; } - seed = getSeedFromRPC (params); + if (secretType == jss::secret.c_str()) + { + error = RPC::make_param_error ( + "The secret field is not allowed if " + + std::string(jss::key_type) + " is used."); + return { }; + } + + seed = getSeedFromRPC (params, error); } else { + if (! params[jss::secret].isString()) + { + error = RPC::expected_field_error ( + jss::secret, "string"); + return { }; + } + seed = parseGenericSeed ( params[jss::secret].asString ()); } if (!seed) { - error = RPC::make_error (rpcBAD_SEED, + if (!contains_error (error)) + { + error = RPC::make_error (rpcBAD_SEED, RPC::invalid_field_message (secretType)); + } + return { }; } diff --git a/src/ripple/rpc/tests/JSONRPC.test.cpp b/src/ripple/rpc/tests/JSONRPC.test.cpp index 05456e7007..ad368ca8c5 100644 --- a/src/ripple/rpc/tests/JSONRPC.test.cpp +++ b/src/ripple/rpc/tests/JSONRPC.test.cpp @@ -575,8 +575,8 @@ R"({ } })", { -"Invalid field 'seed'.", -"Invalid field 'seed'.", +"Disallowed seed.", +"Disallowed seed.", "Missing field 'tx_json.Sequence'.", "Missing field 'tx_json.Sequence'."}}, @@ -929,9 +929,9 @@ R"({ } })", { -"Invalid field 'seed'.", -"Invalid field 'seed'.", -"Invalid field 'seed'.", +"Disallowed seed.", +"Disallowed seed.", +"Disallowed seed.", "Missing field 'tx_json.Signers'."}}, { "Missing 'Account' in sign_for.", diff --git a/src/ripple/rpc/tests/KeyGeneration.test.cpp b/src/ripple/rpc/tests/KeyGeneration.test.cpp index 5a75652f54..b5edd2b16a 100644 --- a/src/ripple/rpc/tests/KeyGeneration.test.cpp +++ b/src/ripple/rpc/tests/KeyGeneration.test.cpp @@ -187,11 +187,507 @@ public: expect (contains_error (walletPropose (params))); } + void testBadInput () + { + testcase ("Bad inputs"); + + // Passing non-strings where strings are required + { + Json::Value params; + params[jss::key_type] = "secp256k1"; + params[jss::passphrase] = 20160506; + auto result = walletPropose (params); + expect (contains_error (result)); + expect (result[jss::error_message] == + "Invalid field 'passphrase', not string."); + } + + { + Json::Value params; + params[jss::key_type] = "secp256k1"; + params[jss::seed] = Json::objectValue; + auto result = walletPropose (params); + expect (contains_error (result)); + expect (result[jss::error_message] == + "Invalid field 'seed', not string."); + } + + { + Json::Value params; + params[jss::key_type] = "ed25519"; + params[jss::seed_hex] = Json::arrayValue; + auto result = walletPropose (params); + expect (contains_error (result)); + expect (result[jss::error_message] == + "Invalid field 'seed_hex', not string."); + } + + // Specifying multiple items at once + { + Json::Value params; + params[jss::key_type] = "secp256k1"; + params[jss::passphrase] = common::master_key; + params[jss::seed_hex] = common::master_seed_hex; + params[jss::seed] = common::master_seed; + auto result = walletPropose (params); + expect (contains_error (result)); + expect (result[jss::error_message] == + "Exactly one of the following must be specified: passphrase, seed or seed_hex"); + } + + // Specifying bad key types: + { + Json::Value params; + params[jss::key_type] = "prime256v1"; + params[jss::passphrase] = common::master_key; + auto result = walletPropose (params); + expect (contains_error (result)); + expect (result[jss::error_message] == + "Invalid parameters."); + } + + { + Json::Value params; + params[jss::key_type] = Json::objectValue; + params[jss::seed_hex] = common::master_seed_hex; + auto result = walletPropose (params); + expect (contains_error (result)); + expect (result[jss::error_message] == + "Invalid field 'key_type', not string."); + } + + { + Json::Value params; + params[jss::key_type] = Json::arrayValue; + params[jss::seed] = common::master_seed; + auto result = walletPropose (params); + expect (contains_error (result)); + expect (result[jss::error_message] == + "Invalid field 'key_type', not string."); + } + } + + void testKeypairForSignature ( + boost::optional keyType, + key_strings const& strings) + { + testcase ("keypairForSignature - " + + (keyType ? *keyType : "no key_type")); + + auto const publicKey = parseBase58( + TokenType::TOKEN_ACCOUNT_PUBLIC, strings.public_key); + expect (publicKey); + + if (!keyType) + { + { + Json::Value params; + Json::Value error; + params[jss::secret] = strings.master_seed; + + auto ret = keypairForSignature (params, error); + expect (! contains_error (error)); + expect (ret.first.size() != 0); + expect (ret.first == publicKey); + } + + { + Json::Value params; + Json::Value error; + params[jss::secret] = strings.master_seed_hex; + + auto ret = keypairForSignature (params, error); + expect (! contains_error (error)); + expect (ret.first.size() != 0); + expect (ret.first == publicKey); + } + + { + Json::Value params; + Json::Value error; + params[jss::secret] = strings.master_key; + + auto ret = keypairForSignature (params, error); + expect (! contains_error (error)); + expect (ret.first.size() != 0); + expect (ret.first == publicKey); + } + + keyType.emplace ("secp256k1"); + } + + { + Json::Value params; + Json::Value error; + + params[jss::key_type] = *keyType; + params[jss::seed] = strings.master_seed; + + auto ret = keypairForSignature (params, error); + expect (! contains_error (error)); + expect (ret.first.size() != 0); + expect (ret.first == publicKey); + } + + { + Json::Value params; + Json::Value error; + + params[jss::key_type] = *keyType; + params[jss::seed_hex] = strings.master_seed_hex; + + auto ret = keypairForSignature (params, error); + expect (! contains_error (error)); + expect (ret.first.size() != 0); + expect (ret.first == publicKey); + } + + { + Json::Value params; + Json::Value error; + + params[jss::key_type] = *keyType; + params[jss::passphrase] = strings.master_key; + + auto ret = keypairForSignature (params, error); + expect (! contains_error (error)); + expect (ret.first.size() != 0); + expect (ret.first == publicKey); + } + } + + void testKeypairForSignatureErrors() + { + // Specify invalid "secret" + { + Json::Value params; + Json::Value error; + params[jss::secret] = 314159265; + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'secret', not string."); + expect (ret.first.size() == 0); + } + + { + Json::Value params; + Json::Value error; + params[jss::secret] = Json::arrayValue; + params[jss::secret].append ("array:0"); + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'secret', not string."); + expect (ret.first.size() == 0); + } + + { + Json::Value params; + Json::Value error; + params[jss::secret] = Json::objectValue; + params[jss::secret]["string"] = "string"; + params[jss::secret]["number"] = 702; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (ret.first.size() == 0); + expect (error[jss::error_message] == + "Invalid field 'secret', not string."); + } + + // Specify "secret" and "key_type" + { + Json::Value params; + Json::Value error; + params[jss::key_type] = "ed25519"; + params[jss::secret] = common::master_seed; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "The secret field is not allowed if key_type is used."); + expect (ret.first.size() == 0); + } + + // Specify unknown or bad "key_type" + { + Json::Value params; + Json::Value error; + params[jss::key_type] = "prime256v1"; + params[jss::passphrase] = common::master_key; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'key_type'."); + expect (ret.first.size() == 0); + } + + { + Json::Value params; + Json::Value error; + params[jss::key_type] = Json::objectValue; + params[jss::seed_hex] = common::master_seed_hex; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'key_type', not string."); + expect (ret.first.size() == 0); + } + + { + Json::Value params; + Json::Value error; + params[jss::key_type] = Json::arrayValue; + params[jss::seed] = common::master_seed; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'key_type', not string."); + expect (ret.first.size() == 0); + } + + // Specify non-string passphrase + { // not a passphrase: number + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::passphrase] = 1234567890; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'passphrase', not string."); + expect (ret.first.size() == 0); + } + + { // not a passphrase: object + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::passphrase] = Json::objectValue; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'passphrase', not string."); + expect (ret.first.size() == 0); + } + + { // not a passphrase: array + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::passphrase] = Json::arrayValue; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'passphrase', not string."); + expect (ret.first.size() == 0); + } + + { // not a passphrase: empty string + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::passphrase] = ""; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Disallowed seed."); + expect (ret.first.size() == 0); + } + + + // Specify non-string or invalid seed + { // not a seed: number + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::seed] = 443556; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'seed', not string."); + expect (ret.first.size() == 0); + } + + { // not a string: object + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::seed] = Json::objectValue; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'seed', not string."); + expect (ret.first.size() == 0); + } + + { // not a string: array + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::seed] = Json::arrayValue; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'seed', not string."); + expect (ret.first.size() == 0); + } + + { // not a seed: empty + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::seed] = ""; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Disallowed seed."); + expect (ret.first.size() == 0); + } + + { // not a seed: invalid characters + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::seed] = "s M V s h z D F p t Z E m h s"; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Disallowed seed."); + expect (ret.first.size() == 0); + } + + { // not a seed: random string + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::seed] = "pnnjkbnobnml43679nbvjdsklnbjs"; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Disallowed seed."); + expect (ret.first.size() == 0); + } + + // Specify non-string or invalid seed_hex + { // not a string: number + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::seed_hex] = 443556; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'seed_hex', not string."); + expect (ret.first.size() == 0); + } + + { // not a string: object + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::seed_hex] = Json::objectValue; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'seed_hex', not string."); + expect (ret.first.size() == 0); + } + + { // not a string: array + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::seed_hex] = Json::arrayValue; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Invalid field 'seed_hex', not string."); + expect (ret.first.size() == 0); + } + + { // empty + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::seed_hex] = ""; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Disallowed seed."); + expect (ret.first.size() == 0); + } + + { // short + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::seed_hex] = "A670A19B"; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Disallowed seed."); + expect (ret.first.size() == 0); + } + + { // not hex + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::seed_hex] = common::passphrase; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Disallowed seed."); + expect (ret.first.size() == 0); + } + + { // overlong + Json::Value params; + Json::Value error; + params[jss::key_type] = "secp256k1"; + params[jss::seed_hex] = "BE6A670A19B209E112146D0A7ED2AAD72567D0FC913"; + + auto ret = keypairForSignature (params, error); + expect (contains_error (error)); + expect (error[jss::error_message] == + "Disallowed seed."); + expect (ret.first.size() == 0); + } + } + void run() { testKeyType (boost::none, secp256k1_strings); testKeyType (std::string("secp256k1"), secp256k1_strings); testKeyType (std::string("ed25519"), ed25519_strings); + testBadInput (); + + testKeypairForSignature (boost::none, secp256k1_strings); + testKeypairForSignature (std::string("secp256k1"), secp256k1_strings); + testKeypairForSignature (std::string("ed25519"), ed25519_strings); + testKeypairForSignatureErrors (); } };