From 0d4ea1cf8a349cf59795ac68645afe70e98c6b3a Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Sat, 3 May 2014 10:20:58 +0200 Subject: [PATCH 1/6] util: add parseint32 function with strict error reporting None of the current integer parsing functions in util check whether the result is valid and fits in the range of the type. This is required for less sloppy error reporting. --- src/test/util_tests.cpp | 22 ++++++++++++++++++++++ src/util.cpp | 14 ++++++++++++++ src/util.h | 7 +++++++ 3 files changed, 43 insertions(+) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index b8f107f64..7e7c05a59 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -342,4 +342,26 @@ BOOST_AUTO_TEST_CASE(gettime) BOOST_CHECK((GetTime() & ~0xFFFFFFFFLL) == 0); } +BOOST_AUTO_TEST_CASE(test_ParseInt32) +{ + int32_t n; + // Valid values + BOOST_CHECK(ParseInt32("1234", NULL)); + BOOST_CHECK(ParseInt32("0", &n) && n == 0); + BOOST_CHECK(ParseInt32("1234", &n) && n == 1234); + BOOST_CHECK(ParseInt32("01234", &n) && n == 1234); // no octal + BOOST_CHECK(ParseInt32("2147483647", &n) && n == 2147483647); + BOOST_CHECK(ParseInt32("-2147483648", &n) && n == -2147483648); + BOOST_CHECK(ParseInt32("-1234", &n) && n == -1234); + // Invalid values + BOOST_CHECK(!ParseInt32("1a", &n)); + BOOST_CHECK(!ParseInt32("aap", &n)); + BOOST_CHECK(!ParseInt32("0x1", &n)); // no hex + // Overflow and underflow + BOOST_CHECK(!ParseInt32("-2147483649", NULL)); + BOOST_CHECK(!ParseInt32("2147483648", NULL)); + BOOST_CHECK(!ParseInt32("-32482348723847471234", NULL)); + BOOST_CHECK(!ParseInt32("32482348723847471234", NULL)); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/util.cpp b/src/util.cpp index a919b4b85..36ac23b1d 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -1427,3 +1427,17 @@ void RenameThread(const char* name) #endif } +bool ParseInt32(const std::string& str, int32_t *out) +{ + char *endp = NULL; + errno = 0; // strtol will not set errno if valid + long int n = strtol(str.c_str(), &endp, 10); + if(out) *out = (int)n; + // Note that strtol returns a *long int*, so even if strtol doesn't report a over/underflow + // we still have to check that the returned value is within the range of an *int32_t*. On 64-bit + // platforms the size of these types may be different. + return endp && *endp == 0 && !errno && + n >= std::numeric_limits::min() && + n <= std::numeric_limits::max(); +} + diff --git a/src/util.h b/src/util.h index fbd841f7a..fa1e664c9 100644 --- a/src/util.h +++ b/src/util.h @@ -256,6 +256,13 @@ inline int atoi(const std::string& str) return atoi(str.c_str()); } +/** + * Convert string to signed 32-bit integer with strict parse error feedback. + * @returns true if the entire string could be parsed as valid integer, + * false if not the entire string could be parsed or when overflow or underflow occured. + */ +bool ParseInt32(const std::string& str, int32_t *out); + inline int roundint(double d) { return (int)(d > 0 ? d + 0.5 : d - 0.5); From d8642752992799b0695cf97ada03c56d0526830c Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Sat, 3 May 2014 10:25:58 +0200 Subject: [PATCH 2/6] Use new function parseint32 in SplitHostPort Use the new function parseint32 in SplitHostPort instead of calling strtol directly. --- src/netbase.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/netbase.cpp b/src/netbase.cpp index ec275f738..4f7e3f6b7 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -47,12 +47,10 @@ void SplitHostPort(std::string in, int &portOut, std::string &hostOut) { bool fBracketed = fHaveColon && (in[0]=='[' && in[colon-1]==']'); // if there is a colon, and in[0]=='[', colon is not 0, so in[colon-1] is safe bool fMultiColon = fHaveColon && (in.find_last_of(':',colon-1) != in.npos); if (fHaveColon && (colon==0 || fBracketed || !fMultiColon)) { - char *endp = NULL; - int n = strtol(in.c_str() + colon + 1, &endp, 10); - if (endp && *endp == 0 && n >= 0) { + int32_t n; + if (ParseInt32(in.substr(colon + 1), &n) && n > 0 && n < 0x10000) { in = in.substr(0, colon); - if (n > 0 && n < 0x10000) - portOut = n; + portOut = n; } } if (in.size()>0 && in[0] == '[' && in[in.size()-1] == ']') From e16be73753d870c5ce77094d3a402bbe8e3bf542 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Mon, 28 Apr 2014 11:08:57 +0200 Subject: [PATCH 3/6] net: Add CSubNet class for subnet matching --- src/netbase.cpp | 123 ++++++++++++++++++++++++++++++++++++- src/netbase.h | 30 +++++++++ src/test/netbase_tests.cpp | 37 +++++++++++ 3 files changed, 187 insertions(+), 3 deletions(-) diff --git a/src/netbase.cpp b/src/netbase.cpp index 4f7e3f6b7..82a681281 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -546,6 +546,22 @@ void CNetAddr::SetIP(const CNetAddr& ipIn) memcpy(ip, ipIn.ip, sizeof(ip)); } +void CNetAddr::SetRaw(Network network, const uint8_t *ip_in) +{ + switch(network) + { + case NET_IPV4: + memcpy(ip, pchIPv4, 12); + memcpy(ip+12, ip_in, 4); + break; + case NET_IPV6: + memcpy(ip, ip_in, 16); + break; + default: + assert(!"invalid network"); + } +} + static const unsigned char pchOnionCat[] = {0xFD,0x87,0xD8,0x7E,0xEB,0x43}; bool CNetAddr::SetSpecial(const std::string &strName) @@ -569,13 +585,12 @@ CNetAddr::CNetAddr() CNetAddr::CNetAddr(const struct in_addr& ipv4Addr) { - memcpy(ip, pchIPv4, 12); - memcpy(ip+12, &ipv4Addr, 4); + SetRaw(NET_IPV4, (const uint8_t*)&ipv4Addr); } CNetAddr::CNetAddr(const struct in6_addr& ipv6Addr) { - memcpy(ip, &ipv6Addr, 16); + SetRaw(NET_IPV6, (const uint8_t*)&ipv6Addr); } CNetAddr::CNetAddr(const char *pszIp, bool fAllowLookup) @@ -1120,3 +1135,105 @@ void CService::SetPort(unsigned short portIn) { port = portIn; } + +CSubNet::CSubNet(): + valid(false) +{ + memset(netmask, 0, sizeof(netmask)); +} + +CSubNet::CSubNet(const std::string &strSubnet, bool fAllowLookup) +{ + size_t slash = strSubnet.find_last_of('/'); + std::vector vIP; + + valid = true; + // Default to /32 (IPv4) or /128 (IPv6), i.e. match single address + memset(netmask, 255, sizeof(netmask)); + + std::string strAddress = strSubnet.substr(0, slash); + if (LookupHost(strAddress.c_str(), vIP, 1, fAllowLookup)) + { + network = vIP[0]; + if (slash != strSubnet.npos) + { + std::string strNetmask = strSubnet.substr(slash + 1); + int32_t n; + // IPv4 addresses start at offset 12, and first 12 bytes must match, so just offset n + int noffset = network.IsIPv4() ? (12 * 8) : 0; + if (ParseInt32(strNetmask, &n)) // If valid number, assume /24 symtex + { + if(n >= 0 && n <= (128 - noffset)) // Only valid if in range of bits of address + { + n += noffset; + // Clear bits [n..127] + for (; n < 128; ++n) + netmask[n>>3] &= ~(1<<(n&7)); + } + else + { + valid = false; + } + } + else // If not a valid number, try full netmask syntax + { + if (LookupHost(strNetmask.c_str(), vIP, 1, false)) // Never allow lookup for netmask + { + // Remember: GetByte returns bytes in reversed order + // Copy only the *last* four bytes in case of IPv4, the rest of the mask should stay 1's as + // we don't want pchIPv4 to be part of the mask. + int asize = network.IsIPv4() ? 4 : 16; + for(int x=0; x Date: Mon, 28 Apr 2014 13:48:26 +0200 Subject: [PATCH 4/6] rpc: Use netmasks instead of wildcards for IP address matching `-rpcallowip` currently has a wacky wildcard-based format. After this commit it will accept the more standard format, for example: - Ranges with netmask 127.0.0.0/255.255.255.0, ::/0 - Ranges with cidr 12.3.4.5/24, 12:34:56:78:9a:bc:de:00/112 - Loose IPs ::1, 127.0.0.1 Trying to use the old *?-based format will result in an error message at launch. --- src/rpcserver.cpp | 59 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 12 deletions(-) diff --git a/src/rpcserver.cpp b/src/rpcserver.cpp index f78cb420f..5740cca13 100644 --- a/src/rpcserver.cpp +++ b/src/rpcserver.cpp @@ -38,6 +38,7 @@ static map > deadlineTimers; static ssl::context* rpc_ssl_context = NULL; static boost::thread_group* rpc_worker_group = NULL; static boost::asio::io_service::work *rpc_dummy_work = NULL; +static std::vector rpc_allow_subnets; //!< List of subnets to allow RPC connections from void RPCTypeCheck(const Array& params, const list& typesExpected, @@ -358,25 +359,34 @@ void ErrorReply(std::ostream& stream, const Object& objError, const Value& id) stream << HTTPReply(nStatus, strReply, false) << std::flush; } -bool ClientAllowed(const boost::asio::ip::address& address) +// Convert boost::asio address to CNetAddr +static CNetAddr BoostAsioToCNetAddr(boost::asio::ip::address address) { + CNetAddr netaddr; // Make sure that IPv4-compatible and IPv4-mapped IPv6 addresses are treated as IPv4 addresses if (address.is_v6() && (address.to_v6().is_v4_compatible() || address.to_v6().is_v4_mapped())) - return ClientAllowed(address.to_v6().to_v4()); + address = address.to_v6().to_v4(); - if (address == asio::ip::address_v4::loopback() - || address == asio::ip::address_v6::loopback() - || (address.is_v4() - // Check whether IPv4 addresses match 127.0.0.0/8 (loopback subnet) - && (address.to_v4().to_ulong() & 0xff000000) == 0x7f000000)) - return true; + if(address.is_v4()) + { + boost::asio::ip::address_v4::bytes_type bytes = address.to_v4().to_bytes(); + netaddr.SetRaw(NET_IPV4, &bytes[0]); + } + else + { + boost::asio::ip::address_v6::bytes_type bytes = address.to_v6().to_bytes(); + netaddr.SetRaw(NET_IPV6, &bytes[0]); + } + return netaddr; +} - const string strAddress = address.to_string(); - const vector& vAllow = mapMultiArgs["-rpcallowip"]; - BOOST_FOREACH(string strAllow, vAllow) - if (WildcardMatch(strAddress, strAllow)) +bool ClientAllowed(const boost::asio::ip::address& address) +{ + CNetAddr netaddr = BoostAsioToCNetAddr(address); + BOOST_FOREACH(const CSubNet &subnet, rpc_allow_subnets) + if (subnet.Match(netaddr)) return true; return false; } @@ -502,6 +512,31 @@ static void RPCAcceptHandler(boost::shared_ptr< basic_socket_acceptor& vAllow = mapMultiArgs["-rpcallowip"]; + BOOST_FOREACH(string strAllow, vAllow) + { + CSubNet subnet(strAllow); + if(!subnet.IsValid()) + { + uiInterface.ThreadSafeMessageBox( + strprintf("Invalid -rpcallowip subnet specification: %s", strAllow), + "", CClientUIInterface::MSG_ERROR); + StartShutdown(); + return; + } + rpc_allow_subnets.push_back(subnet); + } + } + std::string strAllowed; + BOOST_FOREACH(const CSubNet &subnet, rpc_allow_subnets) + strAllowed += subnet.ToString() + " "; + LogPrint("rpc", "Allowing RPC connections from: %s\n", strAllowed); + strRPCUserColonPass = mapArgs["-rpcuser"] + ":" + mapArgs["-rpcpassword"]; if (((mapArgs["-rpcpassword"] == "") || (mapArgs["-rpcuser"] == mapArgs["-rpcpassword"])) && Params().RequireRPCPassword()) From fdbd7075cab8d54f90c038e68afba868a9ff9f63 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Mon, 28 Apr 2014 13:48:57 +0200 Subject: [PATCH 5/6] Remove unused function WildcardMatch No longer necessary after implementing netmask-based matching. Also remove a longer-unused function `skipspaces`. --- src/test/util_tests.cpp | 11 ----------- src/util.cpp | 37 ------------------------------------- src/util.h | 9 --------- 3 files changed, 57 deletions(-) diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp index 7e7c05a59..f4ca8c053 100644 --- a/src/test/util_tests.cpp +++ b/src/test/util_tests.cpp @@ -165,17 +165,6 @@ BOOST_AUTO_TEST_CASE(util_GetArg) BOOST_CHECK_EQUAL(GetBoolArg("booltest4", false), true); } -BOOST_AUTO_TEST_CASE(util_WildcardMatch) -{ - BOOST_CHECK(WildcardMatch("127.0.0.1", "*")); - BOOST_CHECK(WildcardMatch("127.0.0.1", "127.*")); - BOOST_CHECK(WildcardMatch("abcdef", "a?cde?")); - BOOST_CHECK(!WildcardMatch("abcdef", "a?cde??")); - BOOST_CHECK(WildcardMatch("abcdef", "a*f")); - BOOST_CHECK(!WildcardMatch("abcdef", "a*x")); - BOOST_CHECK(WildcardMatch("", "*")); -} - BOOST_AUTO_TEST_CASE(util_FormatMoney) { BOOST_CHECK_EQUAL(FormatMoney(0, false), "0.00"); diff --git a/src/util.cpp b/src/util.cpp index 36ac23b1d..00e29446d 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -889,43 +889,6 @@ string DecodeBase32(const string& str) return string((const char*)&vchRet[0], vchRet.size()); } - -bool WildcardMatch(const char* psz, const char* mask) -{ - while (true) - { - switch (*mask) - { - case '\0': - return (*psz == '\0'); - case '*': - return WildcardMatch(psz, mask+1) || (*psz && WildcardMatch(psz+1, mask)); - case '?': - if (*psz == '\0') - return false; - break; - default: - if (*psz != *mask) - return false; - break; - } - psz++; - mask++; - } -} - -bool WildcardMatch(const string& str, const string& mask) -{ - return WildcardMatch(str.c_str(), mask.c_str()); -} - - - - - - - - static std::string FormatException(std::exception* pex, const char* pszThread) { #ifdef WIN32 diff --git a/src/util.h b/src/util.h index fa1e664c9..011a40e54 100644 --- a/src/util.h +++ b/src/util.h @@ -182,8 +182,6 @@ std::string DecodeBase32(const std::string& str); std::string EncodeBase32(const unsigned char* pch, size_t len); std::string EncodeBase32(const std::string& str); void ParseParameters(int argc, const char*const argv[]); -bool WildcardMatch(const char* psz, const char* mask); -bool WildcardMatch(const std::string& str, const std::string& mask); void FileCommit(FILE *fileout); bool TruncateFile(FILE *file, unsigned int length); int RaiseFileDescriptorLimit(int nMinFD); @@ -348,13 +346,6 @@ inline std::string DateTimeStrFormat(const char* pszFormat, int64_t nTime) return pszTime; } -template -void skipspaces(T& it) -{ - while (isspace(*it)) - ++it; -} - inline bool IsSwitchChar(char c) { #ifdef WIN32 From 21bf3d257b88c45e2bb0b47e36e73d7462760c2c Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Mon, 28 Apr 2014 15:23:29 +0200 Subject: [PATCH 6/6] Add tests for BoostAsioToCNetAddr --- src/rpcserver.cpp | 3 +-- src/rpcserver.h | 4 ++++ src/test/rpc_tests.cpp | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/rpcserver.cpp b/src/rpcserver.cpp index 5740cca13..ac40ea7cf 100644 --- a/src/rpcserver.cpp +++ b/src/rpcserver.cpp @@ -359,8 +359,7 @@ void ErrorReply(std::ostream& stream, const Object& objError, const Value& id) stream << HTTPReply(nStatus, strReply, false) << std::flush; } -// Convert boost::asio address to CNetAddr -static CNetAddr BoostAsioToCNetAddr(boost::asio::ip::address address) +CNetAddr BoostAsioToCNetAddr(boost::asio::ip::address address) { CNetAddr netaddr; // Make sure that IPv4-compatible and IPv4-mapped IPv6 addresses are treated as IPv4 addresses diff --git a/src/rpcserver.h b/src/rpcserver.h index 1092c691b..e8cd2cd0f 100644 --- a/src/rpcserver.h +++ b/src/rpcserver.h @@ -19,6 +19,7 @@ #include "json/json_spirit_writer_template.h" class CBlockIndex; +class CNetAddr; /* Start RPC threads */ void StartRPCThreads(); @@ -50,6 +51,9 @@ void RPCTypeCheck(const json_spirit::Object& o, */ void RPCRunLater(const std::string& name, boost::function func, int64_t nSeconds); +//! Convert boost::asio address to CNetAddr +extern CNetAddr BoostAsioToCNetAddr(boost::asio::ip::address address); + typedef json_spirit::Value(*rpcfn_type)(const json_spirit::Array& params, bool fHelp); class CRPCCommand diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index 5bc38ce2d..107c0f06e 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -6,6 +6,7 @@ #include "rpcclient.h" #include "base58.h" +#include "netbase.h" #include #include @@ -138,4 +139,19 @@ BOOST_AUTO_TEST_CASE(rpc_parse_monetary_values) BOOST_CHECK(AmountFromValue(ValueFromString("20999999.99999999")) == 2099999999999999LL); } +BOOST_AUTO_TEST_CASE(rpc_boostasiotocnetaddr) +{ + // Check IPv4 addresses + BOOST_CHECK_EQUAL(BoostAsioToCNetAddr(boost::asio::ip::address::from_string("1.2.3.4")).ToString(), "1.2.3.4"); + BOOST_CHECK_EQUAL(BoostAsioToCNetAddr(boost::asio::ip::address::from_string("127.0.0.1")).ToString(), "127.0.0.1"); + // Check IPv6 addresses + BOOST_CHECK_EQUAL(BoostAsioToCNetAddr(boost::asio::ip::address::from_string("::1")).ToString(), "::1"); + BOOST_CHECK_EQUAL(BoostAsioToCNetAddr(boost::asio::ip::address::from_string("123:4567:89ab:cdef:123:4567:89ab:cdef")).ToString(), + "123:4567:89ab:cdef:123:4567:89ab:cdef"); + // v4 compatible must be interpreted as IPv4 + BOOST_CHECK_EQUAL(BoostAsioToCNetAddr(boost::asio::ip::address::from_string("::0:127.0.0.1")).ToString(), "127.0.0.1"); + // v4 mapped must be interpreted as IPv4 + BOOST_CHECK_EQUAL(BoostAsioToCNetAddr(boost::asio::ip::address::from_string("::ffff:127.0.0.1")).ToString(), "127.0.0.1"); +} + BOOST_AUTO_TEST_SUITE_END()