Merge bitcoin#21407: i2p: limit the size of incoming messages

7059e6d test: add a test to ensure RecvUntilTerminator() limit works (Vasil Dimov)
80a5a8e i2p: limit the size of incoming messages (Vasil Dimov)

Pull request description:

  Put a limit on the amount of data `Sock::RecvUntilTerminator()` can read
  if no terminator is received.

  In the case of I2P this avoids a runaway (or malicious) I2P proxy
  sending us tons of data without a terminator before a timeout is
  triggered.

ACKs for top commit:
  laanwj:
    Re-ACK 7059e6d

Tree-SHA512: 21f3f3468c765c726cdc877eae847cdb4dbe225d94c5bd1849bd752c9761fac881c554f16ea7a685ad40312159d554e126c481e21c5fd83a6d947060b920373d

Author: laanwj

src/i2p.cpp

@@ -153,7 +153,7 @@ bool Session::Accept(Connection& conn)
             }
 
             const std::string& peer_dest =
-                conn.sock.RecvUntilTerminator('\n', MAX_WAIT_FOR_IO, *m_interrupt);
+                conn.sock.RecvUntilTerminator('\n', MAX_WAIT_FOR_IO, *m_interrupt, MAX_MSG_SIZE);
 
             conn.peer = CService(DestB64ToAddr(peer_dest), Params().GetDefaultPort());
 
@@ -252,7 +252,7 @@ Session::Reply Session::SendRequestAndGetReply(const Sock& sock,
     // signaled.
     static constexpr auto recv_timeout = 3min;
 
-    reply.full = sock.RecvUntilTerminator('\n', recv_timeout, *m_interrupt);
+    reply.full = sock.RecvUntilTerminator('\n', recv_timeout, *m_interrupt, MAX_MSG_SIZE);
 
     for (const auto& kv : spanparsing::Split(reply.full, ' ')) {
         const auto& pos = std::find(kv.begin(), kv.end(), '=');

src/i2p.h

@@ -40,6 +40,14 @@ struct Connection {
 
 namespace sam {
 
+/**
+ * The maximum size of an incoming message from the I2P SAM proxy (in bytes).
+ * Used to avoid a runaway proxy from sending us an "unlimited" amount of data without a terminator.
+ * The longest known message is ~1400 bytes, so this is high enough not to be triggered during
+ * normal operation, yet low enough to avoid a malicious proxy from filling our memory.
+ */
+static constexpr size_t MAX_MSG_SIZE{65536};
+
 /**
  * I2P SAM session.
  */

src/test/sock_tests.cpp

@@ -4,11 +4,13 @@
 
 #include <compat.h>
 #include <test/util/setup_common.h>
+#include <threadinterrupt.h>
 #include <util/sock.h>
 #include <util/system.h>
 
 #include <boost/test/unit_test.hpp>
 
+#include <cassert>
 #include <thread>
 
 using namespace std::chrono_literals;
@@ -144,6 +146,35 @@ BOOST_AUTO_TEST_CASE(wait)
     waiter.join();
 }
 
+BOOST_AUTO_TEST_CASE(recv_until_terminator_limit)
+{
+    constexpr auto timeout = 1min; // High enough so that it is never hit.
+    CThreadInterrupt interrupt;
+    int s[2];
+    CreateSocketPair(s);
+
+    Sock sock_send(s[0]);
+    Sock sock_recv(s[1]);
+
+    std::thread receiver([&sock_recv, &timeout, &interrupt]() {
+        constexpr size_t max_data{10};
+        bool threw_as_expected{false};
+        // BOOST_CHECK_EXCEPTION() writes to some variables shared with the main thread which
+        // creates a data race. So mimic it manually.
+        try {
+            sock_recv.RecvUntilTerminator('\n', timeout, interrupt, max_data);
+        } catch (const std::runtime_error& e) {
+            threw_as_expected = HasReason("too many bytes without a terminator")(e);
+        }
+        assert(threw_as_expected);
+    });
+
+    BOOST_REQUIRE_NO_THROW(sock_send.SendComplete("1234567", timeout, interrupt));
+    BOOST_REQUIRE_NO_THROW(sock_send.SendComplete("89a\n", timeout, interrupt));
+
+    receiver.join();
+}
+
 #endif /* WIN32 */
 
 BOOST_AUTO_TEST_SUITE_END()

src/util/sock.cpp

@@ -175,7 +175,8 @@ void Sock::SendComplete(const std::string& data,
 
 std::string Sock::RecvUntilTerminator(uint8_t terminator,
                                       std::chrono::milliseconds timeout,
-                                      CThreadInterrupt& interrupt) const
+                                      CThreadInterrupt& interrupt,
+                                      size_t max_data) const
 {
     const auto deadline = GetTime<std::chrono::milliseconds>() + timeout;
     std::string data;
@@ -190,9 +191,14 @@ std::string Sock::RecvUntilTerminator(uint8_t terminator,
     // at a time is about 50 times slower.
 
     for (;;) {
+        if (data.size() >= max_data) {
+            throw std::runtime_error(
+                strprintf("Received too many bytes without a terminator (%u)", data.size()));
+        }
+
         char buf[512];
 
-        const ssize_t peek_ret{Recv(buf, sizeof(buf), MSG_PEEK)};
+        const ssize_t peek_ret{Recv(buf, std::min(sizeof(buf), max_data - data.size()), MSG_PEEK)};
 
         switch (peek_ret) {
         case -1: {

src/util/sock.h

@@ -135,13 +135,16 @@ class Sock
      * @param[in] terminator Character up to which to read from the socket.
      * @param[in] timeout Timeout for the entire operation.
      * @param[in] interrupt If this is signaled then the operation is canceled.
+     * @param[in] max_data The maximum amount of data (in bytes) to receive. If this many bytes
+     * are received and there is still no terminator, then this method will throw an exception.
      * @return The data that has been read, without the terminating character.
      * @throws std::runtime_error if the operation cannot be completed. In this case some bytes may
      * have been consumed from the socket.
      */
     virtual std::string RecvUntilTerminator(uint8_t terminator,
                                             std::chrono::milliseconds timeout,
-                                            CThreadInterrupt& interrupt) const;
+                                            CThreadInterrupt& interrupt,
+                                            size_t max_data) const;
 
     /**
      * Check if still connected.