From add803be89c1538d4b98d3fc0e25930b96a78fb2 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Mon, 6 Oct 2014 10:27:34 +0100 Subject: [PATCH 1/3] core_rpc_server: fix overreads in slow_memmem It would read data outside the allocated space in a couple cases. --- src/rpc/core_rpc_server.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/rpc/core_rpc_server.cpp b/src/rpc/core_rpc_server.cpp index e80451cda..97795801c 100644 --- a/src/rpc/core_rpc_server.cpp +++ b/src/rpc/core_rpc_server.cpp @@ -398,17 +398,19 @@ namespace cryptonote return true; } //------------------------------------------------------------------------------------------------------------------------------ - uint64_t slow_memmem(void* start_buff, size_t buflen,void* pat,size_t patlen) + // equivalent of strstr, but with arbitrary bytes (ie, NULs) + // This does not differentiate between "not found" and "found at offset 0" + uint64_t slow_memmem(const void* start_buff, size_t buflen,const void* pat,size_t patlen) { - void* buf = start_buff; - void* end=(char*)buf+buflen-patlen; - while((buf=memchr(buf,((char*)pat)[0],buflen))) + const void* buf = start_buff; + const void* end=(const char*)buf+buflen; + if (patlen > buflen || patlen == 0) return 0; + while(buflen>0 && (buf=memchr(buf,((const char*)pat)[0],buflen-patlen+1))) { - if(buf>end) - return 0; if(memcmp(buf,pat,patlen)==0) - return (char*)buf - (char*)start_buff; - buf=(char*)buf+1; + return (const char*)buf - (const char*)start_buff; + buf=(const char*)buf+1; + buflen = (const char*)end - (const char*)buf; } return 0; } From 5585a1c60a7125137674b5f39855319b73877211 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Mon, 6 Oct 2014 14:10:57 +0100 Subject: [PATCH 2/3] tests: add a test for slow_memmem --- tests/CMakeLists.txt | 2 +- tests/unit_tests/slow_memmem.cpp | 126 +++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 tests/unit_tests/slow_memmem.cpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 14695ddd8..a6fb85d2c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -68,7 +68,7 @@ target_link_libraries(functional_tests cryptonote_core wallet common crypto ${UN target_link_libraries(hash-tests crypto) target_link_libraries(hash-target-tests crypto cryptonote_core) target_link_libraries(performance_tests cryptonote_core common crypto ${UNBOUND_LIBRARIES} ${Boost_LIBRARIES}) -target_link_libraries(unit_tests gtest_main cryptonote_core wallet crypto common ${UNBOUND_LIBRARIES} ${Boost_LIBRARIES}) +target_link_libraries(unit_tests gtest_main rpc cryptonote_core wallet crypto common ${UNBOUND_LIBRARIES} ${Boost_LIBRARIES}) target_link_libraries(net_load_tests_clt cryptonote_core common crypto gtest_main ${UNBOUND_LIBRARIES} ${Boost_LIBRARIES}) target_link_libraries(net_load_tests_srv cryptonote_core common crypto gtest_main ${UNBOUND_LIBRARIES} ${Boost_LIBRARIES}) diff --git a/tests/unit_tests/slow_memmem.cpp b/tests/unit_tests/slow_memmem.cpp new file mode 100644 index 000000000..b1a7622b2 --- /dev/null +++ b/tests/unit_tests/slow_memmem.cpp @@ -0,0 +1,126 @@ +// Copyright (c) 2014, The Monero Project +// +// All rights reserved. +// +// Redistribution and use in source and binary forms, with or without modification, are +// permitted provided that the following conditions are met: +// +// 1. Redistributions of source code must retain the above copyright notice, this list of +// conditions and the following disclaimer. +// +// 2. Redistributions in binary form must reproduce the above copyright notice, this list +// of conditions and the following disclaimer in the documentation and/or other +// materials provided with the distribution. +// +// 3. Neither the name of the copyright holder nor the names of its contributors may be +// used to endorse or promote products derived from this software without specific +// prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY +// EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF +// MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL +// THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, +// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS +// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, +// STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF +// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +// +// Parts of this file are originally copyright (c) 2012-2013 The Cryptonote developers + +#include +#include +#include +#include +#include +#include +#include "gtest/gtest.h" + +//#define TEST_ORIGINAL +//#define VERBOSE + +#ifdef TEST_ORIGINAL +uint64_t slow_memmem_original(void* start_buff, size_t buflen,void* pat,size_t patlen) +{ + void* buf = start_buff; + void* end=(char*)buf+buflen-patlen; + while((buf=memchr(buf,((char*)pat)[0],buflen))) + { + if(buf>end) + return 0; + if(memcmp(buf,pat,patlen)==0) + return (char*)buf - (char*)start_buff; + buf=(char*)buf+1; + } + return 0; +} + +#define slow_memmem slow_memmem_original +#else +namespace cryptonote { + uint64_t slow_memmem(const void* start_buff, size_t buflen,const void* pat,size_t patlen); +} +using namespace cryptonote; +#endif + +static const struct { + size_t buflen; + const char *buf; + size_t patlen; + const char *pat; + uint64_t res; +} T[]={ + {0,"",0,"",0}, + {1,"",0,"",0}, + {0,"",1,"",0}, + {1,"x",1,"x",0}, + {2,"x",1,"",1}, + {1,"x",1,"",0}, + {1,"x",2,"",0}, + {1,"x",2,"x",0}, + {2,"ax",2,"x",0}, + {1,"xx",2,"xx",0}, + {4,"abcd",3,"abc",0}, + {4,"abcd",3,"bcd",1}, + {4,"abcd",4,"abcd",0}, + {4,"abcd",1,"d",3}, + {4,"abcd",1,"c",2}, + {4,"abcd",1,"bc",1}, + {4,"abcd",1,"",0}, + {3,"abcd",1,"d",0}, + {5,"aaaab",2,"ab",3}, + {7,"aaaabab",2,"ab",3}, + {7,"aaaabab",3,"abc",0}, + {4,"abcd",2,"cd",2}, + {3,"abcd",2,"cd",0}, + {3,"a\0b",1,"",1}, + {3,"a\0b",2,"\0b",1}, + {8,"xxxxxxab",3,"xyz",0}, + {8,"xxxxxxab",6,"abcdef",0}, + {9,"\0xxxxxab",3,"ab",6}, + {4,"\0\0a",3,"\0a",1}, +}; + +TEST(slowmem,Success) +{ + size_t n; + for (n=0;n Date: Mon, 6 Oct 2014 14:18:16 +0100 Subject: [PATCH 3/3] core_rpc_server: use do while(0) idiom in macros using if --- src/rpc/core_rpc_server.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rpc/core_rpc_server.cpp b/src/rpc/core_rpc_server.cpp index 97795801c..036cb64ff 100644 --- a/src/rpc/core_rpc_server.cpp +++ b/src/rpc/core_rpc_server.cpp @@ -111,7 +111,7 @@ namespace cryptonote } return true; } -#define CHECK_CORE_BUSY() if(!check_core_busy()){res.status = CORE_RPC_STATUS_BUSY;return true;} +#define CHECK_CORE_BUSY() do { if(!check_core_busy()){res.status = CORE_RPC_STATUS_BUSY;return true;} } while(0) //------------------------------------------------------------------------------------------------------------------------------ bool core_rpc_server::check_core_ready() { @@ -121,7 +121,7 @@ namespace cryptonote } return check_core_busy(); } -#define CHECK_CORE_READY() if(!check_core_ready()){res.status = CORE_RPC_STATUS_BUSY;return true;} +#define CHECK_CORE_READY() do { if(!check_core_ready()){res.status = CORE_RPC_STATUS_BUSY;return true;} } while(0) //------------------------------------------------------------------------------------------------------------------------------ bool core_rpc_server::on_get_height(const COMMAND_RPC_GET_HEIGHT::request& req, COMMAND_RPC_GET_HEIGHT::response& res, connection_context& cntx)