From 4b71673ee9a9aa5c4142e5a0649dbc917c0c36ca Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Thu, 6 Mar 2014 21:15:55 -0800 Subject: [PATCH] Improve validation of JSON inputs --- src/ripple/types/api/strHex.h | 16 +++++- src/ripple/types/impl/strHex.cpp | 38 ++++++++++++- src/ripple_app/rpc/RPCHandler.cpp | 9 ++-- src/ripple_basics/utility/StringUtilities.cpp | 50 ++++++----------- src/ripple_basics/utility/StringUtilities.h | 54 +++---------------- src/ripple_data/protocol/STParsedJSON.cpp | 8 ++- 6 files changed, 82 insertions(+), 93 deletions(-) diff --git a/src/ripple/types/api/strHex.h b/src/ripple/types/api/strHex.h index bc9d21c66d..7288d14701 100644 --- a/src/ripple/types/api/strHex.h +++ b/src/ripple/types/api/strHex.h @@ -27,12 +27,24 @@ namespace ripple { +/** Converts an integer to the corresponding hex digit + @param iDigit 0-15 inclusive + @return a character from '0'-'9' or 'A'-'F' on success; 0 on failure. +*/ char charHex (int iDigit); +/** Converts a hex digit to the corresponding integer + @param cDigit one of '0'-'9', 'A'-'F' or 'a'-'f' + @return an integer from 0 to 15 on success; -1 on failure. +*/ +int charUnHex (char cDigit); + +// NIKB TODO cleanup this function and reduce the need for the many overloads +// it has in various places. template std::string strHex (Iterator first, int iSize) { - std::string strDst; + std::string strDst; strDst.resize (iSize * 2); @@ -41,7 +53,7 @@ std::string strHex (Iterator first, int iSize) unsigned char c = *first++; strDst[i * 2] = charHex (c >> 4); - strDst[i * 2 + 1] = charHex (c & 15); + strDst[i * 2 + 1] = charHex (c & 15); } return strDst; diff --git a/src/ripple/types/impl/strHex.cpp b/src/ripple/types/impl/strHex.cpp index 2a90c7dfb6..c1310c1248 100644 --- a/src/ripple/types/impl/strHex.cpp +++ b/src/ripple/types/impl/strHex.cpp @@ -21,7 +21,43 @@ namespace ripple { char charHex (int iDigit) { - return iDigit < 10 ? '0' + iDigit : 'A' - 10 + iDigit; + if (iDigit >= 0) + { + if(iDigit < 10) + return '0' + iDigit; + if(iDigit < 16) + return 'A' - 10 + iDigit; + } + + return 0; +} + +int charUnHex (char cDigit) +{ + struct HexTab + { + int hex[256]; + + HexTab () + { + std::fill (std::begin (hex), std::end (hex), -1); + for (int i = 0; i < 10; ++i) + hex ['0'+i] = i; + for (int i = 0; i < 6; ++i) + { + hex ['A'+i] = 10 + i; + hex ['a'+i] = 10 + i; + } + } + int operator[] (int i) const + { + return hex[i]; + } + }; + + static HexTab xtab; + + return xtab[cDigit]; } } diff --git a/src/ripple_app/rpc/RPCHandler.cpp b/src/ripple_app/rpc/RPCHandler.cpp index 77b3c78368..fcd18c8088 100644 --- a/src/ripple_app/rpc/RPCHandler.cpp +++ b/src/ripple_app/rpc/RPCHandler.cpp @@ -2007,15 +2007,12 @@ Json::Value RPCHandler::doSubmit (Json::Value params, Resource::Charge& loadType Json::Value jvResult; - Blob vucBlob (strUnHex (params["tx_blob"].asString ())); + std::pair ret(strUnHex (params["tx_blob"].asString ())); - if (!vucBlob.size ()) - { + if (!ret.second || !ret.first.size ()) return rpcError (rpcINVALID_PARAMS); - } - - Serializer sTrans (vucBlob); + Serializer sTrans (ret.first); SerializerIterator sitTrans (sTrans); SerializedTransaction::pointer stpTrans; diff --git a/src/ripple_basics/utility/StringUtilities.cpp b/src/ripple_basics/utility/StringUtilities.cpp index 1043e200e1..c984a0d741 100644 --- a/src/ripple_basics/utility/StringUtilities.cpp +++ b/src/ripple_basics/utility/StringUtilities.cpp @@ -58,34 +58,6 @@ std::string strprintf (const char* format, ...) return str; } -int charUnHex (char cDigit) -{ - struct HexTab - { - int hex[256]; - - HexTab () - { - std::fill (std::begin (hex), std::end (hex), -1); - for (int i = 0; i < 10; ++i) - hex ['0'+i] = i; - for (int i = 0; i < 6; ++i) - { - hex ['A'+i] = 10 + i; - hex ['a'+i] = 10 + i; - } - } - int operator[] (int i) const - { - return hex[i]; - } - }; - - static HexTab xtab; - - return xtab[cDigit]; -} - // NIKB NOTE: This function is only used by strUnHex (const std::string& strSrc) // which results in a pointless copy from std::string into std::vector. Should // we just scrap this function altogether? @@ -130,22 +102,32 @@ int strUnHex (std::string& strDst, const std::string& strSrc) return strDst.size (); } -Blob strUnHex (const std::string& strSrc) +std::pair strUnHex (const std::string& strSrc) { std::string strTmp; if (strUnHex (strTmp, strSrc) == -1) - return Blob (); + return std::make_pair (Blob (), false); - return strCopy (strTmp); + return std::make_pair(strCopy (strTmp), true); } uint64_t uintFromHex (const std::string& strSrc) { - uint64_t uValue = 0; + uint64_t uValue (0); - BOOST_FOREACH (char c, strSrc) - uValue = (uValue << 4) | charUnHex (c); + if (strSrc.size () > 16) + throw std::invalid_argument("overlong 64-bit value"); + + for (auto c : strSrc) + { + int ret = charUnHex (c); + + if (ret == -1) + throw std::invalid_argument("invalid hex digit"); + + uValue = (uValue << 4) | ret; + } return uValue; } diff --git a/src/ripple_basics/utility/StringUtilities.h b/src/ripple_basics/utility/StringUtilities.h index d97fe8d39d..10e3f64370 100644 --- a/src/ripple_basics/utility/StringUtilities.h +++ b/src/ripple_basics/utility/StringUtilities.h @@ -27,60 +27,18 @@ // Ripple specific constant used for parsing qualities and other things // +// NIKB TODO Why is this here instead of somewhere more sensible? What +// "other things" is this being used for? #define QUALITY_ONE 1000000000 // 10e9 //------------------------------------------------------------------------------ -// Terminal output color codes -#define vt_f_black "\033[30m" -#define vt_f_red "\033[31m" -#define vt_f_green "\033[32m" -#define vt_f_yellow "\033[33m" -#define vt_f_blue "\033[34m" -#define vt_f_megenta "\033[35m" -#define vt_f_cyan "\033[36m" -#define vt_f_white "\033[37m" -#define vt_f_default "\033[39m" - -#define vt_b_black "\033[40m" -#define vt_b_red "\033[41m" -#define vt_b_green "\033[42m" -#define vt_b_yellow "\033[43m" -#define vt_b_blue "\033[44m" -#define vt_b_megenta "\033[45m" -#define vt_b_cyan "\033[46m" -#define vt_b_white "\033[47m" -#define vt_b_default "\033[49m" - -#define vt_f_bold_black "\033[1m\033[30m" -#define vt_f_bold_red "\033[1m\033[31m" -#define vt_f_bold_green "\033[1m\033[32m" -#define vt_f_bold_yellow "\033[1m\033[33m" -#define vt_f_bold_blue "\033[1m\033[34m" -#define vt_f_bold_megenta "\033[1m\033[35m" -#define vt_f_bold_cyan "\033[1m\033[36m" -#define vt_f_bold_white "\033[1m\033[37m" -#define vt_f_bold_default "\033[1m\033[39m" - -#define vt_bold "\033[1m" -#define vt_dim "\033[2m" // does not work for xterm -#define vt_normal "\033[22m" // intensity - -#define vt_n_enable "\033[7m" // negative -#define vt_n_disable "\033[27m" - -#define vt_u_single "\033[4m" // underline -#define vt_u_double "\033[21m" // does not work for xterm -#define vt_u_disable "\033[24m" - -#define vt_reset vt_f_default vt_b_default vt_normal vt_n_disable vt_u_disable - -//------------------------------------------------------------------------------ - extern std::string strprintf (const char* format, ...); extern std::string urlEncode (const std::string& strSrc); +// NIKB TODO remove this function - it's only used for some logging in the UNL +// code which can be trivially rewritten. template std::string strJoin (Iterator first, Iterator last, std::string strSeperator) { @@ -94,6 +52,7 @@ std::string strJoin (Iterator first, Iterator last, std::string strSeperator) return ossValues.str (); } +// NIKB TODO Remove the need for all these overloads. Move them out of here. inline const std::string strHex (const std::string& strSrc) { return strHex (strSrc.begin (), strSrc.size ()); @@ -143,12 +102,11 @@ inline static std::string sqlEscape (Blob const& vecSrc) return j; } -int charUnHex (char cDigit); int strUnHex (std::string& strDst, const std::string& strSrc); uint64_t uintFromHex (const std::string& strSrc); -Blob strUnHex (const std::string& strSrc); +std::pair strUnHex (const std::string& strSrc); Blob strCopy (const std::string& strSrc); std::string strCopy (Blob const& vucSrc); diff --git a/src/ripple_data/protocol/STParsedJSON.cpp b/src/ripple_data/protocol/STParsedJSON.cpp index dab344f462..49bfb99a08 100644 --- a/src/ripple_data/protocol/STParsedJSON.cpp +++ b/src/ripple_data/protocol/STParsedJSON.cpp @@ -383,8 +383,12 @@ bool STParsedJSON::parse (std::string const& json_name, try { - data.push_back (new STVariableLength (field, - strUnHex (value.asString ()))); + std::pair ret(strUnHex (value.asString ())); + + if (!ret.second) + throw std::invalid_argument ("invalid data"); + + data.push_back (new STVariableLength (field, ret.first)); } catch (...) {