From 079f346efde3519ce966c2135284ec33cf0c5a02 Mon Sep 17 00:00:00 2001
From: Brad Chase <bchase@ripple.com>
Date: Thu, 15 Feb 2018 15:55:59 -0500
Subject: [PATCH] Improve handling of malformed JSON-RPC requests

---
 src/ripple/rpc/impl/ServerHandlerImp.cpp | 37 ++++++++++++------
 src/test/server/ServerStatus_test.cpp    | 50 ++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 11 deletions(-)

diff --git a/src/ripple/rpc/impl/ServerHandlerImp.cpp b/src/ripple/rpc/impl/ServerHandlerImp.cpp
index 49eac8c12..d42f32747 100644
--- a/src/ripple/rpc/impl/ServerHandlerImp.cpp
+++ b/src/ripple/rpc/impl/ServerHandlerImp.cpp
@@ -567,7 +567,7 @@ ServerHandlerImp::processRequest (Port const& port,
         if ((request.size () > RPC::Tuning::maxRequestSize) ||
             ! reader.parse (request, jsonOrig) ||
             ! jsonOrig ||
-            ! (jsonOrig.isObject () || jsonOrig.isArray()))
+            ! jsonOrig.isObject ())
         {
             HTTPReply (400, "Unable to parse request: " +
                        reader.getFormatedErrorMessages(), output, rpcJ);
@@ -576,27 +576,41 @@ ServerHandlerImp::processRequest (Port const& port,
     }
 
     bool batch = false;
+    unsigned size = 1;
     if (jsonOrig.isMember(jss::method) && jsonOrig[jss::method] == "batch")
+    {
         batch = true;
-    auto size = batch ? jsonOrig[jss::params].size() : 1;
+        if(!jsonOrig.isMember(jss::params) || !jsonOrig[jss::params].isArray())
+        {
+            HTTPReply (400, "Malformed batch request", output, rpcJ);
+            return;
+        }
+        size = jsonOrig[jss::params].size();
+    }
+
     Json::Value reply(batch ? Json::arrayValue : Json::objectValue);
     auto const start (std::chrono::high_resolution_clock::now ());
     for (unsigned i = 0; i < size; ++i)
     {
-        Json::Value const& jsonRPC = batch ? jsonOrig[jss::params][i] : jsonOrig;
-        /* ---------------------------------------------------------------------- */
-        // Determine role/usage so we can charge for invalid requests
-        Json::Value const& method = jsonRPC [jss::method];
-
+        Json::Value const& jsonRPC =
+            batch ? jsonOrig[jss::params][i] : jsonOrig;
+        /* ------------------------------------------------------------------ */
         auto role = Role::FORBID;
-        auto required = RPC::roleRequired(method.asString());
+        auto required = Role::FORBID;
+        if (jsonRPC.isMember(jss::method) && jsonRPC[jss::method].isString())
+            required = RPC::roleRequired(jsonRPC[jss::method].asString());
+
         if (jsonRPC.isMember(jss::params) &&
             jsonRPC[jss::params].isArray() &&
             jsonRPC[jss::params].size() > 0 &&
             jsonRPC[jss::params][Json::UInt(0)].isObject())
         {
-            role = requestRole(required, port, jsonRPC[jss::params][Json::UInt(0)],
-                remoteIPAddress, user);
+            role = requestRole(
+                required,
+                port,
+                jsonRPC[jss::params][Json::UInt(0)],
+                remoteIPAddress,
+                user);
         }
         else
         {
@@ -641,7 +655,7 @@ ServerHandlerImp::processRequest (Port const& port,
             continue;
         }
 
-        if (method.isNull())
+        if (!jsonRPC.isMember(jss::method) || jsonRPC[jss::method].isNull())
         {
             usage.charge(Resource::feeInvalidRPC);
             if (!batch)
@@ -655,6 +669,7 @@ ServerHandlerImp::processRequest (Port const& port,
             continue;
         }
 
+        Json::Value const& method = jsonRPC[jss::method];
         if (! method.isString ())
         {
             usage.charge(Resource::feeInvalidRPC);
diff --git a/src/test/server/ServerStatus_test.cpp b/src/test/server/ServerStatus_test.cpp
index ff1f830cb..7d6872a50 100644
--- a/src/test/server/ServerStatus_test.cpp
+++ b/src/test/server/ServerStatus_test.cpp
@@ -896,6 +896,56 @@ class ServerStatus_test :
             BEAST_EXPECT(resp.body == "Unable to parse request: \r\n");
         }
 
+        {
+            beast::http::response<beast::http::string_body> resp;
+            Json::Value jv;
+            jv["invalid"] = 1;
+            doHTTPRequest(env, yield, false, resp, ec, to_string(jv));
+            BEAST_EXPECT(resp.result() == beast::http::status::bad_request);
+            BEAST_EXPECT(resp.body == "Null method\r\n");
+        }
+
+        {
+            beast::http::response<beast::http::string_body> resp;
+            Json::Value jv(Json::arrayValue);
+            jv.append("invalid");
+            doHTTPRequest(env, yield, false, resp, ec, to_string(jv));
+            BEAST_EXPECT(resp.result() == beast::http::status::bad_request);
+            BEAST_EXPECT(resp.body == "Unable to parse request: \r\n");
+        }
+
+        {
+            beast::http::response<beast::http::string_body> resp;
+            Json::Value jv(Json::arrayValue);
+            Json::Value j;
+            j["invalid"] = 1;
+            jv.append(j);
+            doHTTPRequest(env, yield, false, resp, ec, to_string(jv));
+            BEAST_EXPECT(resp.result() == beast::http::status::bad_request);
+            BEAST_EXPECT(resp.body == "Unable to parse request: \r\n");
+        }
+
+        {
+            beast::http::response<beast::http::string_body> resp;
+            Json::Value jv;
+            jv[jss::method] = "batch";
+            jv[jss::params] = 2;
+            doHTTPRequest(env, yield, false, resp, ec, to_string(jv));
+            BEAST_EXPECT(resp.result() == beast::http::status::bad_request);
+            BEAST_EXPECT(resp.body == "Malformed batch request\r\n");
+        }
+
+        {
+            beast::http::response<beast::http::string_body> resp;
+            Json::Value jv;
+            jv[jss::method] = "batch";
+            jv[jss::params] = Json::objectValue;
+            jv[jss::params]["invalid"] = 3;
+            doHTTPRequest(env, yield, false, resp, ec, to_string(jv));
+            BEAST_EXPECT(resp.result() == beast::http::status::bad_request);
+            BEAST_EXPECT(resp.body == "Malformed batch request\r\n");
+        }
+
         Json::Value jv;
         {
             beast::http::response<beast::http::string_body> resp;