From 1de1b7dccee56aa54f9278de70027a002fdf9609 Mon Sep 17 00:00:00 2001 From: Kayan Date: Thu, 20 Jun 2019 14:50:14 +0800 Subject: [PATCH 1/2] varint unpacking boundary check --- include/fc/io/raw.hpp | 10 +++- test/CMakeLists.txt | 1 + test/io_raw/CMakeLists.txt | 4 ++ test/io_raw/test_io_raw.cpp | 109 ++++++++++++++++++++++++++++++++++++ 4 files changed, 123 insertions(+), 1 deletion(-) create mode 100644 test/io_raw/CMakeLists.txt create mode 100644 test/io_raw/test_io_raw.cpp diff --git a/include/fc/io/raw.hpp b/include/fc/io/raw.hpp index 59018589c..e3adbb2f5 100644 --- a/include/fc/io/raw.hpp +++ b/include/fc/io/raw.hpp @@ -226,6 +226,10 @@ namespace fc { do { s.get(b); v |= uint32_t(uint8_t(b) & 0x7f) << by; + if (by == 28) { + FC_ASSERT( uint8_t(b) <= 0x0f, "signed_int out of bounds"); // upper 4 bits should be zero + break; + } by += 7; } while( uint8_t(b) & 0x80 ); vi.value= (v>>1) ^ (~(v&1)+1ull); //reverse zigzag encoding @@ -236,8 +240,12 @@ namespace fc { do { s.get(b); v |= uint32_t(uint8_t(b) & 0x7f) << by; + if (by == 28) { + FC_ASSERT( uint8_t(b) <= 0x0f, "unsigned_int out of bounds"); // upper 4 bits should be zero + break; + } by += 7; - } while( uint8_t(b) & 0x80 && by < 32 ); + } while( uint8_t(b) & 0x80 ); vi.value = static_cast(v); } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 6a0b339d1..b89ec9dc8 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -1,3 +1,4 @@ add_subdirectory( crypto ) add_subdirectory( static_variant ) add_subdirectory( variant ) +add_subdirectory( io_raw ) diff --git a/test/io_raw/CMakeLists.txt b/test/io_raw/CMakeLists.txt new file mode 100644 index 000000000..9bd28d1e1 --- /dev/null +++ b/test/io_raw/CMakeLists.txt @@ -0,0 +1,4 @@ +add_executable( test_io_raw test_io_raw.cpp ) +target_link_libraries( test_io_raw fc ) + +add_test(NAME test_io_raw COMMAND libraries/fc/test/io_raw/test_io_raw WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) diff --git a/test/io_raw/test_io_raw.cpp b/test/io_raw/test_io_raw.cpp new file mode 100644 index 000000000..6d5f82e8c --- /dev/null +++ b/test/io_raw/test_io_raw.cpp @@ -0,0 +1,109 @@ +#define BOOST_TEST_MODULE io_raw +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +using namespace fc; + +bool expect_assert_message(const fc::exception& ex, string expected) { + BOOST_TEST_MESSAGE("LOG : " << "expected: " << expected << ", actual: " << ex.get_log().at(0).get_message()); + return (ex.get_log().at(0).get_message().find(expected) != std::string::npos); +} + +BOOST_AUTO_TEST_SUITE(test_io_raw) + +BOOST_AUTO_TEST_CASE(varint_unpack) { + struct stream { + char buffer[4096]; + size_t read_pos = 0; + size_t write_pos = 0; + stream() { + memset(buffer, 0, sizeof(buffer)); + } + void get(char &b) { + ++read_pos; + b = buffer[read_pos - 1]; + } + void write(char *data, size_t len) { + memcpy(&(buffer[write_pos]), data, len); + write_pos += len; + } + }; + + std::vector slist = { + 0, 1, -1, 63, -63, 64, -64, 127, -127, 128, -128, 255, -255, 256, -256, 32768, -32768, 65535, -65535, + 65536, -65536, -2147483648, -2147483647, 2147483646, 2147483647}; + + std::vector ulist = { + 0, 1, 2, 3, 63, 64, 127, 128, 65535, 65536, 2147483646, 2147483647, 2147483648, 4294967294, 4294967295}; + + stream s1; + fc::raw::pack(s1, slist); + std::vector slist2; + fc::raw::unpack(s1, slist2); + + BOOST_CHECK_EQUAL(slist.size(), slist2.size()); + BOOST_CHECK_EQUAL(s1.read_pos, s1.write_pos); + for (int i = 0; i < slist.size(); ++i) { + BOOST_CHECK_EQUAL(slist[i], slist2[i]); + } + + stream s2; + fc::raw::pack(s2, ulist); + std::vector ulist2; + fc::raw::unpack(s2, ulist2); + + BOOST_CHECK_EQUAL(ulist.size(), ulist2.size()); + BOOST_CHECK_EQUAL(s2.read_pos, s2.write_pos); + for (int i = 0; i < ulist.size(); ++i) { + BOOST_CHECK_EQUAL(ulist[i], ulist2[i]); + } + + auto unpack_uint = [](unsigned char *buf, size_t len, uint32_t expect_value) { + stream s; + s.write((char *)buf, len); + unsigned_int ui; + fc::raw::unpack(s, ui); + BOOST_CHECK_EQUAL((uint32_t)ui, expect_value); + BOOST_CHECK_EQUAL(s.read_pos, len); + }; + + auto unpack_int = [](unsigned char *buf, size_t len, int32_t expect_value) { + stream s; + s.write((char *)buf, len); + signed_int si; + fc::raw::unpack(s, si); + BOOST_CHECK_EQUAL((int32_t)si, expect_value); + BOOST_CHECK_EQUAL(s.read_pos, len); + }; + + unsigned char buf[] = { 0xff, 0xff, 0xff, 0xff, 0x0f, 0x00}; + unpack_uint(buf, 5, UINT_MAX); + unpack_int(buf, 5, INT_MIN); + + unsigned char buf2[] = { 0xff, 0xff, 0xff, 0xff, 0x8f, 0x00}; // invalid stop bit + BOOST_CHECK_EXCEPTION( unpack_uint(buf2, 5, 0) , fc::assert_exception, [](const auto& e) { + return expect_assert_message(e, "unsigned_int out of bounds"); + }); + BOOST_CHECK_EXCEPTION( unpack_int(buf2, 5, 0) , fc::assert_exception, [](const auto& e) { + return expect_assert_message(e, "signed_int out of bounds"); + }); + + unsigned char buf3[] = { 0xff, 0xff, 0xff, 0xff, 0x10, 0x00}; // data out of bound + BOOST_CHECK_EXCEPTION( unpack_uint(buf3, 5, 0) , fc::assert_exception, [](const auto& e) { + return expect_assert_message(e, "unsigned_int out of bounds"); + }); + BOOST_CHECK_EXCEPTION( unpack_int(buf3, 5, 0) , fc::assert_exception, [](const auto& e) { + return expect_assert_message(e, "signed_int out of bounds"); + }); +} + +BOOST_AUTO_TEST_SUITE_END() From e2df4f57c77018beecaacfcc83adb984670e66c0 Mon Sep 17 00:00:00 2001 From: Bart Wyatt Date: Fri, 21 Jun 2019 10:29:03 -0400 Subject: [PATCH 2/2] refactor throws from `fc::raw::unpack` so that they are easily detected in calling code --- include/fc/io/raw.hpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/include/fc/io/raw.hpp b/include/fc/io/raw.hpp index e3adbb2f5..4d717db45 100644 --- a/include/fc/io/raw.hpp +++ b/include/fc/io/raw.hpp @@ -227,7 +227,9 @@ namespace fc { s.get(b); v |= uint32_t(uint8_t(b) & 0x7f) << by; if (by == 28) { - FC_ASSERT( uint8_t(b) <= 0x0f, "signed_int out of bounds"); // upper 4 bits should be zero + if ( UNLIKELY(uint8_t(b) > 0x0f) ) { + FC_THROW_EXCEPTION(overflow_exception, "signed_int out of bounds"); // upper 4 bits should be zero + } break; } by += 7; @@ -241,7 +243,9 @@ namespace fc { s.get(b); v |= uint32_t(uint8_t(b) & 0x7f) << by; if (by == 28) { - FC_ASSERT( uint8_t(b) <= 0x0f, "unsigned_int out of bounds"); // upper 4 bits should be zero + if ( UNLIKELY(uint8_t(b) > 0x0f) ) { + FC_THROW_EXCEPTION(overflow_exception, "unsigned_int out of bounds"); // upper 4 bits should be zero + } break; } by += 7;