streams: Fix read-past-the-end and integer overflows by maflcko · Pull Request #24231 · bitcoin/bitcoin

maflcko · 2022-02-01T20:51:30Z

This is a follow-up to commit e26b620 with the following fixes:

Fix unsigned integer overflow in ignore(), when nReadPos wraps.
Fix unsigned integer overflow in read(), when nReadPos wraps.
Fix read-past-the-end in read(), when nReadPos wraps.

This shouldn't be remote-exploitable, because it requires a stream of more than 1GB of size. However, it might be exploitable if the attacker controls the datadir (I haven't checked).

A unit test for the overflow in ignore() looks like following. It is left as an excercise to the reader to replace foo.ignore(7) with the appropriate call to read() to reproduce the overflow and read-error in read().

diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 922fd8e513..ec6ea93919 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -534,6 +534,20 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization)
     } catch (const std::ios_base::failure&) {
     }
 
+    CDataStream foo{0, 0};
+    auto size{std::numeric_limits<uint32_t>::max()};
+    foo.resize(size);
+    BOOST_CHECK_EQUAL(foo.size(), size);
+    foo.ignore(std::numeric_limits<int32_t>::max());
+    size -= std::numeric_limits<int32_t>::max();
+    BOOST_CHECK_EQUAL(foo.size(), size);
+    foo.ignore(std::numeric_limits<int32_t>::max());
+    size -= std::numeric_limits<int32_t>::max();
+    BOOST_CHECK_EQUAL(foo.size(), size);
+    BOOST_CHECK_EQUAL(foo.size(), 1);
+    foo.ignore(7); // Should overflow, as the size is only 1
+    BOOST_CHECK_EQUAL(foo.size(), uint32_t(1 - 7));
+
     // Very large scriptPubKey (3*10^9 bytes) past the end of the stream
     CDataStream tmp(SER_DISK, CLIENT_VERSION);
     uint64_t x = 3000000000ULL;

maflcko · 2022-02-02T17:19:14Z

Pushed another commit to fix a premature throw of "end of data" on 64-bit systems, when there is still data.

Unit test:

diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 922fd8e513..d7a266cf1c 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -534,6 +534,21 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization)
     } catch (const std::ios_base::failure&) {
     }
 
+    CDataStream foo{0, 0};
+    auto size{size_t{std::numeric_limits<uint32_t>::max()} + 100};
+    foo.resize(size);
+    BOOST_CHECK_EQUAL(foo.size(), size);
+    foo.ignore(std::numeric_limits<int32_t>::max());
+    size -= std::numeric_limits<int32_t>::max();
+    BOOST_CHECK_EQUAL(foo.size(), size);
+    foo.ignore(std::numeric_limits<int32_t>::max());
+    size -= std::numeric_limits<int32_t>::max();
+    BOOST_CHECK_EQUAL(foo.size(), size);
+    BOOST_CHECK_EQUAL(foo.size(), 101);
+    foo.ignore(7); // Should work fine
+    size -= 7;
+    BOOST_CHECK_EQUAL(foo.size(), size);
+
     // Very large scriptPubKey (3*10^9 bytes) past the end of the stream
     CDataStream tmp(SER_DISK, CLIENT_VERSION);
     uint64_t x = 3000000000ULL;

maflcko · 2022-02-03T18:39:02Z

mingw can't compile the bugfix:

In file included from /usr/lib/gcc/x86_64-w64-mingw32/9.3-posix/include/c++/cstring:42,
                 from ./serialize.h:13,
                 from ./streams.h:9,
                 from test/streams_tests.cpp:5:
In function ‘void* memcpy(void*, const void*, size_t)’,
    inlined from ‘void CDataStream::insert(CDataStream::iterator, std::vector<std::byte>::const_iterator, std::vector<std::byte>::const_iterator)’ at ./streams.h:256:19,
    inlined from ‘void streams_tests::streams_serializedata_xor::test_method()’ at test/streams_tests.cpp:181:14:
/usr/share/mingw-w64/include/string.h:202:32: error: ‘void* __builtin_memcpy(void*, const void*, long long unsigned int)’ specified size between 9223372036854775808 and 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
  202 |   return __builtin___memcpy_chk(__dst, __src, __n, __mingw_bos(__dst, 0));
      |          ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘void* memcpy(void*, const void*, size_t)’,
    inlined from ‘void CDataStream::insert(CDataStream::iterator, std::vector<std::byte>::const_iterator, std::vector<std::byte>::const_iterator)’ at ./streams.h:256:19,
    inlined from ‘void streams_tests::streams_serializedata_xor::test_method()’ at test/streams_tests.cpp:198:14:
/usr/share/mingw-w64/include/string.h:202:32: error: ‘void* __builtin_memcpy(void*, const void*, long long unsigned int)’ specified size between 9223372036854775808 and 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
  202 |   return __builtin___memcpy_chk(__dst, __src, __n, __mingw_bos(__dst, 0));
      |          ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [Makefile:16816: test/test_bitcoin-streams_tests.o] Error 1

DrahtBot · 2022-02-04T04:31:01Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14485 (Try to use posix_fadvise with CBufferedFile by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

luke-jr · 2022-02-05T21:57:10Z

src/streams.h

Won't this truncate num_ignore to 32-bits before checking for overflows?

Yes, but this is already done on current master. The second commit fixes this.

luke-jr

utACK

maflcko · 2022-02-07T08:09:56Z

Please review #24253 first, while this stays in draft. (Can't be merged because mingw bug)

luke-jr · 2022-02-07T18:17:07Z

Could just disable the -Werror affected, but I guess #24253 is fine too...

…hods fa1b227 Remove broken and unused CDataStream methods (MarcoFalke) faee5f8 test: Create fresh CDataStream each time (MarcoFalke) fa71114 test: Inline expected_xor (MarcoFalke) Pull request description: The `insert` and `erase` methods have many issues: * They are unused * They are confusing and hard to read, as they implement "special cases" for optimization, that isn't needed * They are broken (See bitcoin/bitcoin#24231) * Fixing them leads to mingw compile errors (See bitcoin/bitcoin#24231 (comment)) Fix all issues by removing them ACKs for top commit: laanwj: Code review ACK fa1b227 Tree-SHA512: 9d9e5d42e6ffc5ae82bdb67cfb5b50b45977ae674acee6ff99092560aebf2fc7e4584ded614e190db0663226fa198e34350517cd7ee57d518de22e9568bc349a

-BEGIN VERIFY SCRIPT- sed -i 's/nReadPos/m_read_pos/g' ./src/streams.h -END VERIFY SCRIPT-

maflcko · 2022-02-09T16:33:37Z

Rebased. Should be trivial to re-ACK.

klementtan · 2022-02-10T14:53:21Z

Code Review ACK fa1b89a:

Verified that CheckedAdd is used to prevent overflow in read and ignore
Verified that the test in streams: Fix read-past-the-end and integer overflows #24231 (comment) passed on my local machine

Verified that an exception is thrown when there is an overflow with the following ad hoc test:

diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 82e4e1c90f..07758799f8 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -534,6 +534,21 @@ 
BOOST_AUTO_TEST_CASE(ccoins_serialization)
     } catch (const std::ios_base::failure&) {
     }

+    try {
+      std::byte write_bytes[100] =  {std::byte{0x3F}};
+      std::byte read_bytes[100] = {std::byte{0x0}};
+      CDataStream foo{0, 0};
+      auto size{100};
+      foo.resize(size);
+      foo.write({write_bytes, write_bytes + 99});
+      foo.read(read_bytes);
+      foo.ignore(std::numeric_limits<uint64_t>::max());
+      BOOST_CHECK_MESSAGE(false, "We should have thrown");
+    } catch (const std::ios_base::failure& f) {
+      std::string err(f.what());
+      BOOST_CHECK_EQUAL(err, "CDataStream::ignore(): end of 
  data: unspecified iostream_category error");
+    }
+
     // Very large scriptPubKey (3*10^9 bytes) past the end of the 
stream
     CDataStream tmp(SER_DISK, CLIENT_VERSION);
     uint64_t x = 3000000000ULL;

fa1b227 Remove broken and unused CDataStream methods (MarcoFalke) faee5f8 test: Create fresh CDataStream each time (MarcoFalke) fa71114 test: Inline expected_xor (MarcoFalke) Pull request description: The `insert` and `erase` methods have many issues: * They are unused * They are confusing and hard to read, as they implement "special cases" for optimization, that isn't needed * They are broken (See bitcoin#24231) * Fixing them leads to mingw compile errors (See bitcoin#24231 (comment)) Fix all issues by removing them ACKs for top commit: laanwj: Code review ACK fa1b227 Tree-SHA512: 9d9e5d42e6ffc5ae82bdb67cfb5b50b45977ae674acee6ff99092560aebf2fc7e4584ded614e190db0663226fa198e34350517cd7ee57d518de22e9568bc349a

luke-jr

re-utACK

…lows fa1b89a scripted-diff: Rename nReadPos to m_read_pos in streams.h (MarcoFalke) fa56c79 Make CDataStream work properly on 64-bit systems (MarcoFalke) fab02f7 streams: Fix read-past-the-end and integer overflows (MarcoFalke) Pull request description: This is a follow-up to commit e26b620 with the following fixes: * Fix unsigned integer overflow in `ignore()`, when `nReadPos` wraps. * Fix unsigned integer overflow in `read()`, when `nReadPos` wraps. * Fix read-past-the-end in `read()`, when `nReadPos` wraps. This shouldn't be remote-exploitable, because it requires a stream of more than 1GB of size. However, it might be exploitable if the attacker controls the datadir (I haven't checked). A unit test for the overflow in `ignore()` looks like following. It is left as an excercise to the reader to replace `foo.ignore(7)` with the appropriate call to `read()` to reproduce the overflow and read-error in `read()`. ```diff diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 922fd8e..ec6ea93919 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -534,6 +534,20 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization) } catch (const std::ios_base::failure&) { } + CDataStream foo{0, 0}; + auto size{std::numeric_limits<uint32_t>::max()}; + foo.resize(size); + BOOST_CHECK_EQUAL(foo.size(), size); + foo.ignore(std::numeric_limits<int32_t>::max()); + size -= std::numeric_limits<int32_t>::max(); + BOOST_CHECK_EQUAL(foo.size(), size); + foo.ignore(std::numeric_limits<int32_t>::max()); + size -= std::numeric_limits<int32_t>::max(); + BOOST_CHECK_EQUAL(foo.size(), size); + BOOST_CHECK_EQUAL(foo.size(), 1); + foo.ignore(7); // Should overflow, as the size is only 1 + BOOST_CHECK_EQUAL(foo.size(), uint32_t(1 - 7)); + // Very large scriptPubKey (3*10^9 bytes) past the end of the stream CDataStream tmp(SER_DISK, CLIENT_VERSION); uint64_t x = 3000000000ULL; ``` ACKs for top commit: klementtan: Code Review ACK fa1b89a: Tree-SHA512: 67f0a1baafe88eaf1dc844ac55b638d5cf168a18c945e3bf7a2cb03c9a5976674a8e3af2487d8a2c3eae21e5c0e7a519c8b16ee7f104934442e2769d100660e9

maflcko mentioned this pull request Feb 1, 2022

refactor: Use unsigned ignore() consistently #24194

Closed

DrahtBot added the Utils/log/libs label Feb 1, 2022

maflcko force-pushed the 2201-streamIgnore branch 2 times, most recently from fa0bf76 to fa0af11 Compare February 2, 2022 07:57

maflcko mentioned this pull request Feb 3, 2022

Remove broken and unused CDataStream methods #24253

Merged

luke-jr reviewed Feb 5, 2022

View reviewed changes

luke-jr approved these changes Feb 6, 2022

View reviewed changes

DrahtBot mentioned this pull request Feb 6, 2022

Try to use posix_fadvise with CBufferedFile #14485

Closed

maflcko marked this pull request as draft February 7, 2022 08:09

maflcko marked this pull request as ready for review February 9, 2022 16:19

MarcoFalke added 3 commits February 9, 2022 17:20

streams: Fix read-past-the-end and integer overflows

fab02f7

Make CDataStream work properly on 64-bit systems

fa56c79

scripted-diff: Rename nReadPos to m_read_pos in streams.h

fa1b89a

-BEGIN VERIFY SCRIPT- sed -i 's/nReadPos/m_read_pos/g' ./src/streams.h -END VERIFY SCRIPT-

DrahtBot added the Needs rebase label Feb 9, 2022

maflcko force-pushed the 2201-streamIgnore branch from fa65d44 to fa1b89a Compare February 9, 2022 16:31

DrahtBot removed the Needs rebase label Feb 9, 2022

klementtan approved these changes Feb 10, 2022

View reviewed changes

luke-jr approved these changes Feb 20, 2022

View reviewed changes

maflcko merged commit abaf943 into bitcoin:master Feb 21, 2022

maflcko deleted the 2201-streamIgnore branch February 21, 2022 07:10

bitcoin locked and limited conversation to collaborators Feb 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

streams: Fix read-past-the-end and integer overflows#24231

streams: Fix read-past-the-end and integer overflows#24231
maflcko merged 3 commits intobitcoin:masterfrom
maflcko:2201-streamIgnore

maflcko commented Feb 1, 2022 •

edited

Loading

Uh oh!

maflcko commented Feb 2, 2022

Uh oh!

maflcko commented Feb 3, 2022

Uh oh!

DrahtBot commented Feb 4, 2022 •

edited

Loading

Uh oh!

luke-jr Feb 5, 2022

Uh oh!

maflcko Feb 6, 2022

Uh oh!

luke-jr left a comment

Uh oh!

maflcko commented Feb 7, 2022

Uh oh!

luke-jr commented Feb 7, 2022

Uh oh!

maflcko commented Feb 9, 2022

Uh oh!

klementtan commented Feb 10, 2022 •

edited

Loading

Uh oh!

luke-jr left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

maflcko commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Feb 2, 2022

Uh oh!

maflcko commented Feb 3, 2022

Uh oh!

DrahtBot commented Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

luke-jr Feb 5, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 6, 2022

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 7, 2022

Uh oh!

luke-jr commented Feb 7, 2022

Uh oh!

maflcko commented Feb 9, 2022

Uh oh!

klementtan commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maflcko commented Feb 1, 2022 •

edited

Loading

DrahtBot commented Feb 4, 2022 •

edited

Loading

klementtan commented Feb 10, 2022 •

edited

Loading