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

@@ -24,12 +24,14 @@
 #include <ripple/protocol/PublicKey.h>
 #include <ripple/protocol/SecretKey.h>
 #include <ripple/protocol/Seed.h>
+#include <boost/optional.hpp>
+#include <utility>
 
 namespace ripple {
 namespace RPC {
 
 boost::optional<Seed>
-getSeedFromRPC (Json::Value const& params);
+getSeedFromRPC (Json::Value const& params, Json::Value& error);
 
 std::pair<PublicKey, SecretKey>
 keypairForSignature (Json::Value const& params, Json::Value& error);

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
     {

src/ripple/rpc/impl/KeypairForSignature.cpp

@@ -18,47 +18,76 @@
 //==============================================================================
 
 #include <BeastConfig.h>
+#include <ripple/rpc/KeypairForSignature.h>
 #include <ripple/app/misc/NetworkOPs.h>
 #include <ripple/net/RPCErr.h>
 #include <ripple/protocol/ErrorCodes.h>
 #include <ripple/protocol/JsonFields.h>
-#include <ripple/rpc/KeypairForSignature.h>
 
 namespace ripple {
 namespace RPC {
 
 boost::optional<Seed>
-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<Seed> (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> seed;
+
+    if (seedType == jss::seed.c_str())
+        seed = parseBase58<Seed> (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<PublicKey, SecretKey>
@@ -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 { };
     }
 

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.",

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<std::string> keyType,
+        key_strings const& strings)
+    {
+        testcase ("keypairForSignature - " +
+            (keyType ? *keyType : "no key_type"));
+
+        auto const publicKey = parseBase58<PublicKey>(
+            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 ();
     }
 };