Merge branch 'develop' into mvadari/fix-bad-cast

The `ledger_entry` and `deposit_preauth` requests require an array of credentials. However, the array size is not checked before is gets processing. This fix adds checks and return errors in case array size is too big.

include/xrpl/protocol/jss.h

@@ -480,7 +480,6 @@ JSS(ports);                   // out: NetworkOPs
 JSS(previous);                // out: Reservations
 JSS(previous_ledger);         // out: LedgerPropose
 JSS(price);                   // out: amm_info, AuctionSlot
-JSS(proof);                   // in: BookOffers
 JSS(propose_seq);             // out: LedgerPropose
 JSS(proposers);               // out: NetworkOPs, LedgerConsensus
 JSS(protocol);                // out: NetworkOPs, PeerImp

src/test/rpc/LedgerEntry_test.cpp

@@ -1103,7 +1103,7 @@ class LedgerEntry_test : public beast::unit_test::suite
             checkErrorValue(
                 jrr[jss::result],
                 "malformedAuthorizedCredentials",
-                "Invalid field 'authorized_credentials', not array.");
+                "Invalid field 'authorized_credentials', array empty.");
         }
 
         {
@@ -1144,7 +1144,7 @@ class LedgerEntry_test : public beast::unit_test::suite
             checkErrorValue(
                 jrr[jss::result],
                 "malformedAuthorizedCredentials",
-                "Invalid field 'authorized_credentials', not array.");
+                "Invalid field 'authorized_credentials', array too long.");
         }
     }
 

src/xrpld/app/misc/NetworkOPs.cpp

@@ -356,7 +356,6 @@ public:
         std::shared_ptr<ReadView const>& lpLedger,
         Book const&,
         AccountID const& uTakerID,
-        bool const bProof,
         unsigned int iLimit,
         Json::Value const& jvMarker,
         Json::Value& jvResult) override;
@@ -4440,7 +4439,6 @@ NetworkOPsImp::getBookPage(
     std::shared_ptr<ReadView const>& lpLedger,
     Book const& book,
     AccountID const& uTakerID,
-    bool const bProof,
     unsigned int iLimit,
     Json::Value const& jvMarker,
     Json::Value& jvResult)
@@ -4647,7 +4645,6 @@ NetworkOPsImp::getBookPage(
     std::shared_ptr<ReadView const> lpLedger,
     Book const& book,
     AccountID const& uTakerID,
-    bool const bProof,
     unsigned int iLimit,
     Json::Value const& jvMarker,
     Json::Value& jvResult)

src/xrpld/app/misc/NetworkOPs.h

@@ -151,7 +151,6 @@ public:
         std::shared_ptr<ReadView const>& lpLedger,
         Book const& book,
         AccountID const& uTakerID,
-        bool const bProof,
         unsigned int iLimit,
         Json::Value const& jvMarker,
         Json::Value& jvResult) = 0;

src/xrpld/rpc/detail/RPCCall.cpp

@@ -135,6 +135,38 @@ private:
         }
     }
 
+    static std::optional<std::int32_t>
+    jvParseInt(Json::Value const& param)
+    {
+        if (param.isUInt() || param.isInt())
+            return param.asInt();
+
+        if (param.isString())
+        {
+            std::int32_t v;
+            if (beast::lexicalCastChecked(v, param.asString()))
+                return v;
+        }
+
+        return std::nullopt;
+    }
+
+    static std::optional<std::uint32_t>
+    jvParseUInt(Json::Value const& param)
+    {
+        if (param.isUInt() || (param.isInt() && param.asInt() >= 0))
+            return param.asUInt();
+
+        if (param.isString())
+        {
+            std::uint32_t v;
+            if (beast::lexicalCastChecked(v, param.asString()))
+                return v;
+        }
+
+        return std::nullopt;
+    }
+
     static bool
     validPublicKey(
         std::string const& strPk,
@@ -266,24 +298,50 @@ private:
         }
         else
         {
-            std::int64_t uLedgerMin = jvParams[1u].asInt();
-            std::int64_t uLedgerMax = jvParams[2u].asInt();
+            std::int32_t ledgerMin, ledgerMax;
+            if (auto const ledgerMinOpt = jvParseInt(jvParams[1u]))
+            {
+                ledgerMin = *ledgerMinOpt;
+            }
+            else
+            {
+                return rpcError(rpcINVALID_LGR_RANGE);
+            }
 
-            if (uLedgerMax != -1 && uLedgerMax < uLedgerMin)
+            if (auto const ledgerMaxOpt = jvParseInt(jvParams[2u]))
+            {
+                ledgerMax = *ledgerMaxOpt;
+            }
+            else
+            {
+                return rpcError(rpcINVALID_LGR_RANGE);
+            }
+
+            if (ledgerMax != -1 && ledgerMax < ledgerMin)
             {
                 if (apiVersion_ == 1)
                     return rpcError(rpcLGR_IDXS_INVALID);
                 return rpcError(rpcNOT_SYNCED);
             }
 
-            jvRequest[jss::ledger_index_min] = jvParams[1u].asInt();
-            jvRequest[jss::ledger_index_max] = jvParams[2u].asInt();
+            jvRequest[jss::ledger_index_min] = ledgerMin;
+            jvRequest[jss::ledger_index_max] = ledgerMax;
 
             if (iParams >= 4)
-                jvRequest[jss::limit] = jvParams[3u].asInt();
+            {
+                if (auto const limit = jvParseInt(jvParams[3u]))
+                    jvRequest[jss::limit] = *limit;
+                else
+                    return RPC::invalid_field_error(jss::limit);
+            }
 
             if (iParams >= 5)
-                jvRequest[jss::offset] = jvParams[4u].asInt();
+            {
+                if (auto const offset = jvParseInt(jvParams[4u]))
+                    jvRequest[jss::offset] = *offset;
+                else
+                    return RPC::invalid_field_error(jss::offset);
+            }
         }
 
         return jvRequest;
@@ -332,19 +390,17 @@ private:
 
         if (jvParams.size() >= 5)
         {
-            int iLimit = jvParams[5u].asInt();
-
-            if (iLimit > 0)
-                jvRequest[jss::limit] = iLimit;
-        }
-
-        if (jvParams.size() >= 6 && jvParams[5u].asInt())
+            if (auto const limit = jvParseInt(jvParams[4u]))
             {
-            jvRequest[jss::proof] = true;
+                if (limit > 0)
+                    jvRequest[jss::limit] = *limit;
+            }
+            else
+                return RPC::invalid_field_error(jss::limit);
         }
 
-        if (jvParams.size() == 7)
-            jvRequest[jss::marker] = jvParams[6u];
+        if (jvParams.size() == 6)
+            jvRequest[jss::marker] = jvParams[5u];
 
         return jvRequest;
     }
@@ -360,7 +416,12 @@ private:
 
         std::string input = jvParams[0u].asString();
         if (input.find_first_not_of("0123456789") == std::string::npos)
-            jvRequest["can_delete"] = jvParams[0u].asUInt();
+        {
+            if (auto seq = jvParseUInt(jvParams[0u]))
+                jvRequest["can_delete"] = *seq;
+            else
+                return RPC::invalid_field_error(jss::can_delete);
+        }
         else
             jvRequest["can_delete"] = input;
 
@@ -376,7 +437,10 @@ private:
         if (jvParams.size() == 2)
         {
             jvRequest[jss::ip] = ip;
-            jvRequest[jss::port] = jvParams[1u].asUInt();
+            if (auto port = jvParseUInt(jvParams[1u]))
+                jvRequest[jss::port] = *port;
+            else
+                return RPC::invalid_field_error(jss::port);
             return jvRequest;
         }
 
@@ -385,8 +449,13 @@ private:
         {
             std::size_t colon = ip.find_last_of(":");
             jvRequest[jss::ip] = std::string{ip, 0, colon};
-            jvRequest[jss::port] =
-                Json::Value{std::string{ip, colon + 1}}.asUInt();
+
+            std::uint32_t port;
+            if (beast::lexicalCastChecked(port, std::string{ip, colon + 1}))
+                jvRequest[jss::port] = port;
+            else
+                return RPC::invalid_field_error(jss::port);
+
             return jvRequest;
         }
 
@@ -459,7 +528,12 @@ private:
         Json::Value jvRequest(Json::objectValue);
 
         if (jvParams.size())
-            jvRequest[jss::min_count] = jvParams[0u].asUInt();
+        {
+            if (auto minCount = jvParseUInt(jvParams[0u]))
+                jvRequest[jss::min_count] = *minCount;
+            else
+                return RPC::invalid_field_error(jss::min_count);
+        }
 
         return jvRequest;
     }
@@ -1071,7 +1145,10 @@ private:
     {
         Json::Value jvRequest{Json::objectValue};
 
-        jvRequest[jss::start] = jvParams[0u].asUInt();
+        if (auto const start = jvParseUInt(jvParams[0u]))
+            jvRequest[jss::start] = *start;
+        else
+            return RPC::invalid_field_error(jss::start);
 
         return jvRequest;
     }
@@ -1220,7 +1297,7 @@ public:
             {"amm_info", &RPCParser::parseAsIs, 1, 2},
             {"vault_info", &RPCParser::parseVault, 1, 2},
             {"book_changes", &RPCParser::parseLedgerId, 1, 1},
-            {"book_offers", &RPCParser::parseBookOffers, 2, 7},
+            {"book_offers", &RPCParser::parseBookOffers, 2, 6},
             {"can_delete", &RPCParser::parseCanDelete, 0, 1},
             {"channel_authorize", &RPCParser::parseChannelAuthorize, 3, 4},
             {"channel_verify", &RPCParser::parseChannelVerify, 4, 4},

src/xrpld/rpc/handlers/BookOffers.cpp

@@ -180,8 +180,6 @@ doBookOffers(RPC::JsonContext& context)
     if (auto err = readLimitField(limit, RPC::Tuning::bookOffers, context))
         return *err;
 
-    bool const bProof(context.params.isMember(jss::proof));
-
     Json::Value const jvMarker(
         context.params.isMember(jss::marker) ? context.params[jss::marker]
                                              : Json::Value(Json::nullValue));
@@ -190,7 +188,6 @@ doBookOffers(RPC::JsonContext& context)
         lpLedger,
         {{pay_currency, pay_issuer}, {get_currency, get_issuer}, domain},
         takerID ? *takerID : beast::zero,
-        bProof,
         limit,
         jvMarker,
         jvResult);

src/xrpld/rpc/handlers/LedgerEntry.cpp

@@ -16,8 +16,6 @@
 #include <xrpl/protocol/STXChainBridge.h>
 #include <xrpl/protocol/jss.h>
 
-#include <functional>
-
 namespace ripple {
 
 static Expected<uint256, Json::Value>
@@ -178,18 +176,41 @@ static Expected<STArray, Json::Value>
 parseAuthorizeCredentials(Json::Value const& jv)
 {
     if (!jv.isArray())
+    {
         return LedgerEntryHelpers::invalidFieldError(
             "malformedAuthorizedCredentials",
             jss::authorized_credentials,
             "array");
-    STArray arr(sfAuthorizeCredentials, jv.size());
+    }
+
+    std::uint32_t const n = jv.size();
+    if (n > maxCredentialsArraySize)
+    {
+        return Unexpected(LedgerEntryHelpers::malformedError(
+            "malformedAuthorizedCredentials",
+            "Invalid field '" + std::string(jss::authorized_credentials) +
+                "', array too long."));
+    }
+
+    if (n == 0)
+    {
+        return Unexpected(LedgerEntryHelpers::malformedError(
+            "malformedAuthorizedCredentials",
+            "Invalid field '" + std::string(jss::authorized_credentials) +
+                "', array empty."));
+    }
+
+    STArray arr(sfAuthorizeCredentials, n);
     for (auto const& jo : jv)
     {
         if (!jo.isObject())
+        {
             return LedgerEntryHelpers::invalidFieldError(
                 "malformedAuthorizedCredentials",
                 jss::authorized_credentials,
                 "array");
+        }
+
         if (auto const value = LedgerEntryHelpers::hasRequired(
                 jo,
                 {jss::issuer, jss::credential_type},
@@ -260,13 +281,6 @@ parseDepositPreauth(Json::Value const& dp, Json::StaticString const fieldName)
     auto const arr = parseAuthorizeCredentials(ac);
     if (!arr.has_value())
         return Unexpected(arr.error());
-    if (arr->empty() || (arr->size() > maxCredentialsArraySize))
-    {
-        return LedgerEntryHelpers::invalidFieldError(
-            "malformedAuthorizedCredentials",
-            jss::authorized_credentials,
-            "array");
-    }
 
     auto const& sorted = credentials::makeSorted(arr.value());
     if (sorted.empty())

src/xrpld/rpc/handlers/Subscribe.cpp

@@ -333,7 +333,6 @@ doSubscribe(RPC::JsonContext& context)
                             lpLedger,
                             field == jss::asks ? reversed(book) : book,
                             takerID ? *takerID : noAccount(),
-                            false,
                             RPC::Tuning::bookOffers.rdefault,
                             jvMarker,
                             jvOffers);