From 9b05c49ade729311a0f4388a109530ff8d0ed1f9 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 4 Mar 2021 16:05:33 +0100 Subject: fuzz: implement unimplemented FuzzedSock methods We want `Get()` to always return the same value, otherwise it will look like the `FuzzedSock` implementation itself is broken. So assign `m_socket` a random number in the `FuzzedSock` constructor. There is nothing to fuzz about the `Get()` and `Release()` methods, so use the ones from the base class `Sock`. `Reset()` is just setting our socket to `INVALID_SOCKET`. We don't want to use the base `Reset()` because it will close `m_socket` and given that our `m_socket` is just a random number it may end up closing a real opened file descriptor if it coincides with our random `m_socket`. --- src/test/fuzz/util.h | 30 ++++++++++++++++-------------- src/util/sock.h | 2 +- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index cdddad82b3..818246f73f 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -558,33 +558,27 @@ class FuzzedSock : public Sock public: explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider) : m_fuzzed_data_provider{fuzzed_data_provider} { + m_socket = fuzzed_data_provider.ConsumeIntegral(); } ~FuzzedSock() override { + // Sock::~Sock() will be called after FuzzedSock::~FuzzedSock() and it will call + // Sock::Reset() (not FuzzedSock::Reset()!) which will call CloseSocket(m_socket). + // Avoid closing an arbitrary file descriptor (m_socket is just a random number which + // may concide with a real opened file descriptor). + Reset(); } FuzzedSock& operator=(Sock&& other) override { - assert(false && "Not implemented yet."); + assert(false && "Move of Sock into FuzzedSock not allowed."); return *this; } - SOCKET Get() const override - { - assert(false && "Not implemented yet."); - return INVALID_SOCKET; - } - - SOCKET Release() override - { - assert(false && "Not implemented yet."); - return INVALID_SOCKET; - } - void Reset() override { - assert(false && "Not implemented yet."); + m_socket = INVALID_SOCKET; } ssize_t Send(const void* data, size_t len, int flags) const override @@ -667,6 +661,14 @@ public: { return m_fuzzed_data_provider.ConsumeBool(); } + + bool IsConnected(std::string& errmsg) const override { + if (m_fuzzed_data_provider.ConsumeBool()) { + return true; + } + errmsg = "disconnected at random by the fuzzer"; + return false; + } }; [[nodiscard]] inline FuzzedSock ConsumeSock(FuzzedDataProvider& fuzzed_data_provider) diff --git a/src/util/sock.h b/src/util/sock.h index 4b0618dcff..d923de531f 100644 --- a/src/util/sock.h +++ b/src/util/sock.h @@ -153,7 +153,7 @@ public: */ virtual bool IsConnected(std::string& errmsg) const; -private: +protected: /** * Contained socket. `INVALID_SOCKET` designates the object is empty. */ -- cgit v1.2.3 From 3088f83d016e7ebb6e6aa559e6326fa0ef0d6282 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 5 Mar 2021 14:50:49 +0100 Subject: fuzz: extend FuzzedSock::Recv() to support MSG_PEEK A conforming `recv(2)` call is supposed to return the same data on a call following `recv(..., MSG_PEEK)`. Extend `FuzzedSock::Recv()` to do that. For simplicity we only return 1 byte when `MSG_PEEK` is used. If we would return a buffer of N bytes, then we would have to keep track how many of them were consumed on subsequent non-`MSG_PEEK` calls. --- src/test/fuzz/util.h | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index 818246f73f..4f44394d2b 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -555,6 +555,13 @@ class FuzzedSock : public Sock { FuzzedDataProvider& m_fuzzed_data_provider; + /** + * Data to return when `MSG_PEEK` is used as a `Recv()` flag. + * If `MSG_PEEK` is used, then our `Recv()` returns some random data as usual, but on the next + * `Recv()` call we must return the same data, thus we remember it here. + */ + mutable std::optional m_peek_data; + public: explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider) : m_fuzzed_data_provider{fuzzed_data_provider} { @@ -635,8 +642,26 @@ public: } return r; } - const std::vector random_bytes = m_fuzzed_data_provider.ConsumeBytes( - m_fuzzed_data_provider.ConsumeIntegralInRange(0, len)); + std::vector random_bytes; + bool pad_to_len_bytes{m_fuzzed_data_provider.ConsumeBool()}; + if (m_peek_data.has_value()) { + // `MSG_PEEK` was used in the preceding `Recv()` call, return `m_peek_data`. + random_bytes.assign({m_peek_data.value()}); + if ((flags & MSG_PEEK) == 0) { + m_peek_data.reset(); + } + pad_to_len_bytes = false; + } else if ((flags & MSG_PEEK) != 0) { + // New call with `MSG_PEEK`. + random_bytes = m_fuzzed_data_provider.ConsumeBytes(1); + if (!random_bytes.empty()) { + m_peek_data = random_bytes[0]; + pad_to_len_bytes = false; + } + } else { + random_bytes = m_fuzzed_data_provider.ConsumeBytes( + m_fuzzed_data_provider.ConsumeIntegralInRange(0, len)); + } if (random_bytes.empty()) { const ssize_t r = m_fuzzed_data_provider.ConsumeBool() ? 0 : -1; if (r == -1) { @@ -645,7 +670,7 @@ public: return r; } std::memcpy(buf, random_bytes.data(), random_bytes.size()); - if (m_fuzzed_data_provider.ConsumeBool()) { + if (pad_to_len_bytes) { if (len > random_bytes.size()) { std::memset((char*)buf + random_bytes.size(), 0, len - random_bytes.size()); } -- cgit v1.2.3 From 5a887d49b2b39e59d7cce8e9d5b89c21ad694f8b Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 8 Mar 2021 11:42:24 +0100 Subject: fuzz: avoid FuzzedSock::Recv() repeated errors with EAGAIN If `recv(2)` returns an error (`-1`) and sets `errno` to a temporary error like `EAGAIN` a proper application code is expected to retry the operation. If the fuzz data is exhausted, then `FuzzedSock::Recv()` will keep returning `-1` and setting `errno` to the first element of `recv_errnos[]` which happened to be `EAGAIN`. This may continue forever or cause the fuzz test to run for a long time before some higher level application "receive timeout" is triggered. Thus, put `ECONNREFUSED` as first element of `recv_errnos[]`. --- src/test/fuzz/util.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index 4f44394d2b..db399abcf1 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -622,10 +622,13 @@ public: ssize_t Recv(void* buf, size_t len, int flags) const override { + // Have a permanent error at recv_errnos[0] because when the fuzzed data is exhausted + // SetFuzzedErrNo() will always return the first element and we want to avoid Recv() + // returning -1 and setting errno to EAGAIN repeatedly. constexpr std::array recv_errnos{ + ECONNREFUSED, EAGAIN, EBADF, - ECONNREFUSED, EFAULT, EINTR, EINVAL, -- cgit v1.2.3 From b5861100f85fef77b00f55dcdf01ffb4a2a112d8 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 5 Mar 2021 16:29:43 +0100 Subject: net: add connect() and getsockopt() wrappers to Sock Extend the `Sock` class with wrappers to `connect()` and `getsockopt()`. This will make it possible to mock code which uses those. --- src/test/fuzz/util.h | 41 +++++++++++++++++++++++++++++++++++++++++ src/util/sock.cpp | 10 ++++++++++ src/util/sock.h | 17 +++++++++++++++-- 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index db399abcf1..5294adf48e 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -685,6 +685,47 @@ public: return random_bytes.size(); } + int Connect(const sockaddr*, socklen_t) const override + { + // Have a permanent error at connect_errnos[0] because when the fuzzed data is exhausted + // SetFuzzedErrNo() will always return the first element and we want to avoid Connect() + // returning -1 and setting errno to EAGAIN repeatedly. + constexpr std::array connect_errnos{ + ECONNREFUSED, + EAGAIN, + ECONNRESET, + EHOSTUNREACH, + EINPROGRESS, + EINTR, + ENETUNREACH, + ETIMEDOUT, + }; + if (m_fuzzed_data_provider.ConsumeBool()) { + SetFuzzedErrNo(m_fuzzed_data_provider, connect_errnos); + return -1; + } + return 0; + } + + int GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const override + { + constexpr std::array getsockopt_errnos{ + ENOMEM, + ENOBUFS, + }; + if (m_fuzzed_data_provider.ConsumeBool()) { + SetFuzzedErrNo(m_fuzzed_data_provider, getsockopt_errnos); + return -1; + } + if (opt_val == nullptr) { + return 0; + } + std::memcpy(opt_val, + ConsumeFixedLengthByteVector(m_fuzzed_data_provider, *opt_len).data(), + *opt_len); + return 0; + } + bool Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred = nullptr) const override { return m_fuzzed_data_provider.ConsumeBool(); diff --git a/src/util/sock.cpp b/src/util/sock.cpp index f9ecfef5d4..0bc9795db3 100644 --- a/src/util/sock.cpp +++ b/src/util/sock.cpp @@ -66,6 +66,16 @@ ssize_t Sock::Recv(void* buf, size_t len, int flags) const return recv(m_socket, static_cast(buf), len, flags); } +int Sock::Connect(const sockaddr* addr, socklen_t addr_len) const +{ + return connect(m_socket, addr, addr_len); +} + +int Sock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const +{ + return getsockopt(m_socket, level, opt_name, static_cast(opt_val), opt_len); +} + bool Sock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred) const { #ifdef USE_POLL diff --git a/src/util/sock.h b/src/util/sock.h index d923de531f..c4ad0cbc43 100644 --- a/src/util/sock.h +++ b/src/util/sock.h @@ -80,16 +80,29 @@ public: /** * send(2) wrapper. Equivalent to `send(this->Get(), data, len, flags);`. Code that uses this - * wrapper can be unit-tested if this method is overridden by a mock Sock implementation. + * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ virtual ssize_t Send(const void* data, size_t len, int flags) const; /** * recv(2) wrapper. Equivalent to `recv(this->Get(), buf, len, flags);`. Code that uses this - * wrapper can be unit-tested if this method is overridden by a mock Sock implementation. + * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ virtual ssize_t Recv(void* buf, size_t len, int flags) const; + /** + * connect(2) wrapper. Equivalent to `connect(this->Get(), addr, addrlen)`. Code that uses this + * wrapper can be unit tested if this method is overridden by a mock Sock implementation. + */ + virtual int Connect(const sockaddr* addr, socklen_t addr_len) const; + + /** + * getsockopt(2) wrapper. Equivalent to + * `getsockopt(this->Get(), level, opt_name, opt_val, opt_len)`. Code that uses this + * wrapper can be unit tested if this method is overridden by a mock Sock implementation. + */ + virtual int GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const; + using Event = uint8_t; /** -- cgit v1.2.3 From 82d360b5a88d9057b6c09b61cd69e426c7a2412d Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 5 Mar 2021 17:01:59 +0100 Subject: net: change ConnectSocketDirectly() to take a Sock argument Change `ConnectSocketDirectly()` to take a `Sock` argument instead of a bare `SOCKET`. With this, use the `Sock`'s (possibly mocked) methods `Connect()`, `Wait()` and `GetSockOpt()` instead of calling the OS functions directly. --- src/i2p.cpp | 2 +- src/net.cpp | 2 +- src/netbase.cpp | 59 ++++++++++++++++++++++----------------------------------- src/netbase.h | 4 ++-- 4 files changed, 27 insertions(+), 40 deletions(-) diff --git a/src/i2p.cpp b/src/i2p.cpp index d16c620d88..13e7ce3515 100644 --- a/src/i2p.cpp +++ b/src/i2p.cpp @@ -279,7 +279,7 @@ Sock Session::Hello() const throw std::runtime_error("Cannot create socket"); } - if (!ConnectSocketDirectly(m_control_host, sock->Get(), nConnectTimeout, true)) { + if (!ConnectSocketDirectly(m_control_host, *sock, nConnectTimeout, true)) { throw std::runtime_error(strprintf("Cannot connect to %s", m_control_host.ToString())); } diff --git a/src/net.cpp b/src/net.cpp index 1e4a6a9aa7..36065118d7 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -448,7 +448,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo if (!sock) { return nullptr; } - connected = ConnectSocketDirectly(addrConnect, sock->Get(), nConnectTimeout, + connected = ConnectSocketDirectly(addrConnect, *sock, nConnectTimeout, conn_type == ConnectionType::MANUAL); } if (!proxyConnectionFailed) { diff --git a/src/netbase.cpp b/src/netbase.cpp index ac2392ebed..462fa719bc 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -537,12 +537,12 @@ static void LogConnectFailure(bool manual_connection, const char* fmt, const Arg } } -bool ConnectSocketDirectly(const CService &addrConnect, const SOCKET& hSocket, int nTimeout, bool manual_connection) +bool ConnectSocketDirectly(const CService &addrConnect, const Sock& sock, int nTimeout, bool manual_connection) { // Create a sockaddr from the specified service. struct sockaddr_storage sockaddr; socklen_t len = sizeof(sockaddr); - if (hSocket == INVALID_SOCKET) { + if (sock.Get() == INVALID_SOCKET) { LogPrintf("Cannot connect to %s: invalid socket\n", addrConnect.ToString()); return false; } @@ -552,8 +552,7 @@ bool ConnectSocketDirectly(const CService &addrConnect, const SOCKET& hSocket, i } // Connect to the addrConnect service on the hSocket socket. - if (connect(hSocket, (struct sockaddr*)&sockaddr, len) == SOCKET_ERROR) - { + if (sock.Connect(reinterpret_cast(&sockaddr), len) == SOCKET_ERROR) { int nErr = WSAGetLastError(); // WSAEINVAL is here because some legacy version of winsock uses it if (nErr == WSAEINPROGRESS || nErr == WSAEWOULDBLOCK || nErr == WSAEINVAL) @@ -561,46 +560,34 @@ bool ConnectSocketDirectly(const CService &addrConnect, const SOCKET& hSocket, i // Connection didn't actually fail, but is being established // asynchronously. Thus, use async I/O api (select/poll) // synchronously to check for successful connection with a timeout. -#ifdef USE_POLL - struct pollfd pollfd = {}; - pollfd.fd = hSocket; - pollfd.events = POLLIN | POLLOUT; - int nRet = poll(&pollfd, 1, nTimeout); -#else - struct timeval timeout = MillisToTimeval(nTimeout); - fd_set fdset; - FD_ZERO(&fdset); - FD_SET(hSocket, &fdset); - int nRet = select(hSocket + 1, nullptr, &fdset, nullptr, &timeout); -#endif - // Upon successful completion, both select and poll return the total - // number of file descriptors that have been selected. A value of 0 - // indicates that the call timed out and no file descriptors have - // been selected. - if (nRet == 0) - { - LogPrint(BCLog::NET, "connection to %s timeout\n", addrConnect.ToString()); + const Sock::Event requested = Sock::RECV | Sock::SEND; + Sock::Event occurred; + if (!sock.Wait(std::chrono::milliseconds{nTimeout}, requested, &occurred)) { + LogPrintf("wait for connect to %s failed: %s\n", + addrConnect.ToString(), + NetworkErrorString(WSAGetLastError())); return false; - } - if (nRet == SOCKET_ERROR) - { - LogPrintf("select() for %s failed: %s\n", addrConnect.ToString(), NetworkErrorString(WSAGetLastError())); + } else if (occurred == 0) { + LogPrint(BCLog::NET, "connection attempt to %s timed out\n", addrConnect.ToString()); return false; } - // Even if the select/poll was successful, the connect might not + // Even if the wait was successful, the connect might not // have been successful. The reason for this failure is hidden away // in the SO_ERROR for the socket in modern systems. We read it into - // nRet here. - socklen_t nRetSize = sizeof(nRet); - if (getsockopt(hSocket, SOL_SOCKET, SO_ERROR, (sockopt_arg_type)&nRet, &nRetSize) == SOCKET_ERROR) - { + // sockerr here. + int sockerr; + socklen_t sockerr_len = sizeof(sockerr); + if (sock.GetSockOpt(SOL_SOCKET, SO_ERROR, (sockopt_arg_type)&sockerr, &sockerr_len) == + SOCKET_ERROR) { LogPrintf("getsockopt() for %s failed: %s\n", addrConnect.ToString(), NetworkErrorString(WSAGetLastError())); return false; } - if (nRet != 0) - { - LogConnectFailure(manual_connection, "connect() to %s failed after select(): %s", addrConnect.ToString(), NetworkErrorString(nRet)); + if (sockerr != 0) { + LogConnectFailure(manual_connection, + "connect() to %s failed after wait: %s", + addrConnect.ToString(), + NetworkErrorString(sockerr)); return false; } } @@ -668,7 +655,7 @@ bool IsProxy(const CNetAddr &addr) { bool ConnectThroughProxy(const proxyType& proxy, const std::string& strDest, int port, const Sock& sock, int nTimeout, bool& outProxyConnectionFailed) { // first connect to proxy server - if (!ConnectSocketDirectly(proxy.proxy, sock.Get(), nTimeout, true)) { + if (!ConnectSocketDirectly(proxy.proxy, sock, nTimeout, true)) { outProxyConnectionFailed = true; return false; } diff --git a/src/netbase.h b/src/netbase.h index e98a21ce1f..dc9a952d4f 100644 --- a/src/netbase.h +++ b/src/netbase.h @@ -194,7 +194,7 @@ extern std::function(const CService&)> CreateSock; * Try to connect to the specified service on the specified socket. * * @param addrConnect The service to which to connect. - * @param hSocket The socket on which to connect. + * @param sock The socket on which to connect. * @param nTimeout Wait this many milliseconds for the connection to be * established. * @param manual_connection Whether or not the connection was manually requested @@ -202,7 +202,7 @@ extern std::function(const CService&)> CreateSock; * * @returns Whether or not a connection was successfully made. */ -bool ConnectSocketDirectly(const CService &addrConnect, const SOCKET& hSocket, int nTimeout, bool manual_connection); +bool ConnectSocketDirectly(const CService &addrConnect, const Sock& sock, int nTimeout, bool manual_connection); /** * Connect to a specified destination service through a SOCKS5 proxy by first -- cgit v1.2.3 From 9947e44de0cbd79e99d883443a9ac441d8c69713 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 4 Mar 2021 14:31:49 +0100 Subject: i2p: use pointers to Sock to accommodate mocking Change the types of `i2p::Connection::sock` and `i2p::sam::Session::m_control_sock` from `Sock` to `std::unique_ptr`. Using pointers would allow us to sneak `FuzzedSock` instead of `Sock` and have the methods of the former called. After this change a test only needs to replace `CreateSock()` with a function that returns `FuzzedSock`. --- src/i2p.cpp | 40 +++++++++++++++++++++------------------- src/i2p.h | 12 +++++++----- src/net.cpp | 4 ++-- 3 files changed, 30 insertions(+), 26 deletions(-) diff --git a/src/i2p.cpp b/src/i2p.cpp index 13e7ce3515..a44f09f043 100644 --- a/src/i2p.cpp +++ b/src/i2p.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -115,7 +116,8 @@ namespace sam { Session::Session(const fs::path& private_key_file, const CService& control_host, CThreadInterrupt* interrupt) - : m_private_key_file(private_key_file), m_control_host(control_host), m_interrupt(interrupt) + : m_private_key_file(private_key_file), m_control_host(control_host), m_interrupt(interrupt), + m_control_sock(std::make_unique(INVALID_SOCKET)) { } @@ -145,7 +147,7 @@ bool Session::Accept(Connection& conn) try { while (!*m_interrupt) { Sock::Event occurred; - conn.sock.Wait(MAX_WAIT_FOR_IO, Sock::RECV, &occurred); + conn.sock->Wait(MAX_WAIT_FOR_IO, Sock::RECV, &occurred); if ((occurred & Sock::RECV) == 0) { // Timeout, no incoming connections within MAX_WAIT_FOR_IO. @@ -153,7 +155,7 @@ bool Session::Accept(Connection& conn) } const std::string& peer_dest = - conn.sock.RecvUntilTerminator('\n', MAX_WAIT_FOR_IO, *m_interrupt, MAX_MSG_SIZE); + conn.sock->RecvUntilTerminator('\n', MAX_WAIT_FOR_IO, *m_interrupt, MAX_MSG_SIZE); conn.peer = CService(DestB64ToAddr(peer_dest), Params().GetDefaultPort()); @@ -171,7 +173,7 @@ bool Session::Connect(const CService& to, Connection& conn, bool& proxy_error) proxy_error = true; std::string session_id; - Sock sock; + std::unique_ptr sock; conn.peer = to; try { @@ -184,12 +186,12 @@ bool Session::Connect(const CService& to, Connection& conn, bool& proxy_error) } const Reply& lookup_reply = - SendRequestAndGetReply(sock, strprintf("NAMING LOOKUP NAME=%s", to.ToStringIP())); + SendRequestAndGetReply(*sock, strprintf("NAMING LOOKUP NAME=%s", to.ToStringIP())); const std::string& dest = lookup_reply.Get("VALUE"); const Reply& connect_reply = SendRequestAndGetReply( - sock, strprintf("STREAM CONNECT ID=%s DESTINATION=%s SILENT=false", session_id, dest), + *sock, strprintf("STREAM CONNECT ID=%s DESTINATION=%s SILENT=false", session_id, dest), false); const std::string& result = connect_reply.Get("RESULT"); @@ -271,7 +273,7 @@ Session::Reply Session::SendRequestAndGetReply(const Sock& sock, return reply; } -Sock Session::Hello() const +std::unique_ptr Session::Hello() const { auto sock = CreateSock(m_control_host); @@ -285,7 +287,7 @@ Sock Session::Hello() const SendRequestAndGetReply(*sock, "HELLO VERSION MIN=3.1 MAX=3.1"); - return std::move(*sock); + return sock; } void Session::CheckControlSock() @@ -293,7 +295,7 @@ void Session::CheckControlSock() LOCK(m_mutex); std::string errmsg; - if (!m_control_sock.IsConnected(errmsg)) { + if (!m_control_sock->IsConnected(errmsg)) { Log("Control socket error: %s", errmsg); Disconnect(); } @@ -341,26 +343,26 @@ Binary Session::MyDestination() const void Session::CreateIfNotCreatedAlready() { std::string errmsg; - if (m_control_sock.IsConnected(errmsg)) { + if (m_control_sock->IsConnected(errmsg)) { return; } Log("Creating SAM session with %s", m_control_host.ToString()); - Sock sock = Hello(); + auto sock = Hello(); const auto& [read_ok, data] = ReadBinaryFile(m_private_key_file); if (read_ok) { m_private_key.assign(data.begin(), data.end()); } else { - GenerateAndSavePrivateKey(sock); + GenerateAndSavePrivateKey(*sock); } const std::string& session_id = GetRandHash().GetHex().substr(0, 10); // full is an overkill, too verbose in the logs const std::string& private_key_b64 = SwapBase64(EncodeBase64(m_private_key)); - SendRequestAndGetReply(sock, strprintf("SESSION CREATE STYLE=STREAM ID=%s DESTINATION=%s", - session_id, private_key_b64)); + SendRequestAndGetReply(*sock, strprintf("SESSION CREATE STYLE=STREAM ID=%s DESTINATION=%s", + session_id, private_key_b64)); m_my_addr = CService(DestBinToAddr(MyDestination()), Params().GetDefaultPort()); m_session_id = session_id; @@ -370,12 +372,12 @@ void Session::CreateIfNotCreatedAlready() m_my_addr.ToString()); } -Sock Session::StreamAccept() +std::unique_ptr Session::StreamAccept() { - Sock sock = Hello(); + auto sock = Hello(); const Reply& reply = SendRequestAndGetReply( - sock, strprintf("STREAM ACCEPT ID=%s SILENT=false", m_session_id), false); + *sock, strprintf("STREAM ACCEPT ID=%s SILENT=false", m_session_id), false); const std::string& result = reply.Get("RESULT"); @@ -393,14 +395,14 @@ Sock Session::StreamAccept() void Session::Disconnect() { - if (m_control_sock.Get() != INVALID_SOCKET) { + if (m_control_sock->Get() != INVALID_SOCKET) { if (m_session_id.empty()) { Log("Destroying incomplete session"); } else { Log("Destroying session %s", m_session_id); } } - m_control_sock.Reset(); + m_control_sock->Reset(); m_session_id.clear(); } } // namespace sam diff --git a/src/i2p.h b/src/i2p.h index 1ebe7d0329..cb2efedba8 100644 --- a/src/i2p.h +++ b/src/i2p.h @@ -12,6 +12,7 @@ #include #include +#include #include #include #include @@ -29,7 +30,7 @@ using Binary = std::vector; */ struct Connection { /** Connected socket. */ - Sock sock; + std::unique_ptr sock; /** Our I2P address. */ CService me; @@ -166,7 +167,7 @@ private: * @return a connected socket * @throws std::runtime_error if an error occurs */ - Sock Hello() const EXCLUSIVE_LOCKS_REQUIRED(m_mutex); + std::unique_ptr Hello() const EXCLUSIVE_LOCKS_REQUIRED(m_mutex); /** * Check the control socket for errors and possibly disconnect. @@ -204,10 +205,11 @@ private: /** * Open a new connection to the SAM proxy and issue "STREAM ACCEPT" request using the existing - * session id. Return the idle socket that is waiting for a peer to connect to us. + * session id. + * @return the idle socket that is waiting for a peer to connect to us * @throws std::runtime_error if an error occurs */ - Sock StreamAccept() EXCLUSIVE_LOCKS_REQUIRED(m_mutex); + std::unique_ptr StreamAccept() EXCLUSIVE_LOCKS_REQUIRED(m_mutex); /** * Destroy the session, closing the internally used sockets. @@ -248,7 +250,7 @@ private: * connections and make outgoing ones. * See https://geti2p.net/en/docs/api/samv3 */ - Sock m_control_sock GUARDED_BY(m_mutex); + std::unique_ptr m_control_sock GUARDED_BY(m_mutex); /** * Our .b32.i2p address. diff --git a/src/net.cpp b/src/net.cpp index 36065118d7..6ff51057b1 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -432,7 +432,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo i2p::Connection conn; if (m_i2p_sam_session->Connect(addrConnect, conn, proxyConnectionFailed)) { connected = true; - sock = std::make_unique(std::move(conn.sock)); + sock = std::move(conn.sock); addr_bind = CAddress{conn.me, NODE_NONE}; } } else if (GetProxy(addrConnect.GetNetwork(), proxy)) { @@ -2221,7 +2221,7 @@ void CConnman::ThreadI2PAcceptIncoming() continue; } - CreateNodeFromAcceptedSocket(conn.sock.Release(), NetPermissionFlags::PF_NONE, + CreateNodeFromAcceptedSocket(conn.sock->Release(), NetPermissionFlags::PF_NONE, CAddress{conn.me, NODE_NONE}, CAddress{conn.peer, NODE_NONE}); } } -- cgit v1.2.3 From 2d8ac779708322e1235e823edfc9c8f6e2dd65e4 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 4 Mar 2021 15:32:11 +0100 Subject: fuzz: add tests for the I2P Session public interface --- src/Makefile.test.include | 1 + src/test/fuzz/i2p.cpp | 57 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 src/test/fuzz/i2p.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 24b2879789..6f461a989b 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -239,6 +239,7 @@ test_fuzz_fuzz_SOURCES = \ test/fuzz/golomb_rice.cpp \ test/fuzz/hex.cpp \ test/fuzz/http_request.cpp \ + test/fuzz/i2p.cpp \ test/fuzz/integer.cpp \ test/fuzz/key.cpp \ test/fuzz/key_io.cpp \ diff --git a/src/test/fuzz/i2p.cpp b/src/test/fuzz/i2p.cpp new file mode 100644 index 0000000000..345d68502a --- /dev/null +++ b/src/test/fuzz/i2p.cpp @@ -0,0 +1,57 @@ +// Copyright (c) 2020-2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +void initialize_i2p() +{ + static const auto testing_setup = MakeNoLogFileContext<>(); +} + +FUZZ_TARGET_INIT(i2p, initialize_i2p) +{ + FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()}; + + // Mock CreateSock() to create FuzzedSock. + auto CreateSockOrig = CreateSock; + CreateSock = [&fuzzed_data_provider](const CService&) { + return std::make_unique(fuzzed_data_provider); + }; + + const CService sam_proxy; + CThreadInterrupt interrupt; + + i2p::sam::Session sess{GetDataDir() / "fuzzed_i2p_private_key", sam_proxy, &interrupt}; + + i2p::Connection conn; + + if (sess.Listen(conn)) { + if (sess.Accept(conn)) { + try { + conn.sock->RecvUntilTerminator('\n', 10ms, interrupt, i2p::sam::MAX_MSG_SIZE); + } catch (const std::runtime_error&) { + } + } + } + + const CService to; + bool proxy_error; + + if (sess.Connect(to, conn, proxy_error)) { + try { + conn.sock->SendComplete("verack\n", 10ms, interrupt); + } catch (const std::runtime_error&) { + } + } + + CreateSock = CreateSockOrig; +} -- cgit v1.2.3 From 40316a37cb02cf8a9a8b2cbd4d7153ffa57e7ec5 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 11 Mar 2021 16:27:22 +0100 Subject: test: add I2P test for a runaway SAM proxy Add a regression test for https://github.com/bitcoin/bitcoin/pull/21407. The test creates a socket that, upon read, returns some data, but never the expected terminator `\n`, injects that socket into the I2P code and expects `i2p::sam::Session::Connect()` to fail, printing a specific error message to the log. --- src/Makefile.test.include | 1 + src/test/i2p_tests.cpp | 44 ++++++++++++++++++++++++++++++ src/test/util/net.h | 69 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+) create mode 100644 src/test/i2p_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 6f461a989b..cf7f65e177 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -90,6 +90,7 @@ BITCOIN_TESTS =\ test/fs_tests.cpp \ test/getarg_tests.cpp \ test/hash_tests.cpp \ + test/i2p_tests.cpp \ test/interfaces_tests.cpp \ test/key_io_tests.cpp \ test/key_tests.cpp \ diff --git a/src/test/i2p_tests.cpp b/src/test/i2p_tests.cpp new file mode 100644 index 0000000000..334f71106c --- /dev/null +++ b/src/test/i2p_tests.cpp @@ -0,0 +1,44 @@ +// Copyright (c) 2021-2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include +#include + +#include + +#include +#include + +BOOST_FIXTURE_TEST_SUITE(i2p_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(unlimited_recv) +{ + auto CreateSockOrig = CreateSock; + + // Mock CreateSock() to create MockSock. + CreateSock = [](const CService&) { + return std::make_unique(std::string(i2p::sam::MAX_MSG_SIZE + 1, 'a')); + }; + + CThreadInterrupt interrupt; + i2p::sam::Session session(GetDataDir() / "test_i2p_private_key", CService{}, &interrupt); + + { + ASSERT_DEBUG_LOG("Creating SAM session"); + ASSERT_DEBUG_LOG("too many bytes without a terminator"); + + i2p::Connection conn; + bool proxy_error; + BOOST_REQUIRE(!session.Connect(CService{}, conn, proxy_error)); + } + + CreateSock = CreateSockOrig; +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/util/net.h b/src/test/util/net.h index e25036be26..2b7988413f 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -5,7 +5,13 @@ #ifndef BITCOIN_TEST_UTIL_NET_H #define BITCOIN_TEST_UTIL_NET_H +#include #include +#include + +#include +#include +#include struct ConnmanTestMsg : public CConnman { using CConnman::CConnman; @@ -61,4 +67,67 @@ constexpr ConnectionType ALL_CONNECTION_TYPES[]{ ConnectionType::ADDR_FETCH, }; +/** + * A mocked Sock alternative that returns a statically contained data upon read and succeeds + * and ignores all writes. The data to be returned is given to the constructor and when it is + * exhausted an EOF is returned by further reads. + */ +class StaticContentsSock : public Sock +{ +public: + explicit StaticContentsSock(const std::string& contents) : m_contents{contents}, m_consumed{0} + { + // Just a dummy number that is not INVALID_SOCKET. + static_assert(INVALID_SOCKET != 1000); + m_socket = 1000; + } + + ~StaticContentsSock() override { Reset(); } + + StaticContentsSock& operator=(Sock&& other) override + { + assert(false && "Move of Sock into MockSock not allowed."); + return *this; + } + + void Reset() override + { + m_socket = INVALID_SOCKET; + } + + ssize_t Send(const void*, size_t len, int) const override { return len; } + + ssize_t Recv(void* buf, size_t len, int flags) const override + { + const size_t consume_bytes{std::min(len, m_contents.size() - m_consumed)}; + std::memcpy(buf, m_contents.data() + m_consumed, consume_bytes); + if ((flags & MSG_PEEK) == 0) { + m_consumed += consume_bytes; + } + return consume_bytes; + } + + int Connect(const sockaddr*, socklen_t) const override { return 0; } + + int GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const override + { + std::memset(opt_val, 0x0, *opt_len); + return 0; + } + + bool Wait(std::chrono::milliseconds timeout, + Event requested, + Event* occurred = nullptr) const override + { + if (occurred != nullptr) { + *occurred = requested; + } + return true; + } + +private: + const std::string m_contents; + mutable size_t m_consumed; +}; + #endif // BITCOIN_TEST_UTIL_NET_H -- cgit v1.2.3