This will leave the socket in a blocking mode. I think the rest of the P2P code expects and assumes non-blocking sockets. Better set this also to non-blocking and make the Socks5() more robust:
<details>
<summary>[patch] netbase: use reliable send() during SOCKS5 handshake</summary>
commit 4c8d08ca2b495da609d5586247dbd2b306e90e61 (HEAD -> tor-unix-domain-socket)
Parent: af4eb52ff611342dbd285f218474649f16badbaf
Author: Vasil Dimov <vd@FreeBSD.org>
AuthorDate: Thu Jul 13 14:17:30 2023 +0200
Commit: Vasil Dimov <vd@FreeBSD.org>
CommitDate: Thu Jul 13 14:17:30 2023 +0200
gpg: Signature made Thu Jul 13 14:23:03 2023 CEST
gpg: using RSA key E64D8D45614DB07545D9CCC154DF06F64B55CBBF
gpg: Good signature from "Vasil Dimov <vd@myforest.net>" [ultimate]
gpg: aka "Vasil Dimov <vd@FreeBSD.org>" [ultimate]
gpg: aka "Vasil Dimov <vasild@gmail.com>" [ultimate]
netbase: use reliable send() during SOCKS5 handshake
`send(2)` can be interrupted or for another reason it may not fully
complete sending all the bytes. We should be ready to retry the send
with the remaining bytes. This is what `Sock::SendComplete()` does,
thus use it in `Socks5()`.
Since `Sock::SendComplete()` takes a `CThreadInterrupt` argument,
change also the recv part of `Socks5()` to use `CThreadInterrupt`
instead of a boolean.
Easier reviewed with `git show -b` (ignore white-space changes).
diff --git a/src/net.cpp b/src/net.cpp
index a46cd25e90..4eb96f950d 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -2336,13 +2336,12 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
}
//
// Start threads
//
assert(m_msgproc);
- InterruptSocks5(false);
interruptNet.reset();
flagInterruptMsgProc = false;
{
LOCK(mutexMsgProc);
fMsgProcWake = false;
@@ -2408,13 +2407,13 @@ void CConnman::Interrupt()
LOCK(mutexMsgProc);
flagInterruptMsgProc = true;
}
condMsgProc.notify_all();
interruptNet();
- InterruptSocks5(true);
+ g_socks5_interrupt();
if (semOutbound) {
for (int i=0; i<m_max_outbound; i++) {
semOutbound->post();
}
}
diff --git a/src/netbase.cpp b/src/netbase.cpp
index 548e1483b8..cee50e67bb 100644
--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -32,13 +32,13 @@ static Proxy proxyInfo[NET_MAX] GUARDED_BY(g_proxyinfo_mutex);
static Proxy nameProxy GUARDED_BY(g_proxyinfo_mutex);
int nConnectTimeout = DEFAULT_CONNECT_TIMEOUT;
bool fNameLookup = DEFAULT_NAME_LOOKUP;
// Need ample time for negotiation for very slow proxies such as Tor
std::chrono::milliseconds g_socks5_recv_timeout = 20s;
-static std::atomic<bool> interruptSocks5Recv(false);
+CThreadInterrupt g_socks5_interrupt;
std::vector<CNetAddr> WrappedGetAddrInfo(const std::string& name, bool allow_lookup)
{
addrinfo ai_hint{};
// We want a TCP port, which is a streaming socket type
ai_hint.ai_socktype = SOCK_STREAM;
@@ -289,13 +289,13 @@ enum class IntrRecvError {
* [@param](/bitcoin-bitcoin/contributor/param/) sock The socket (has to be in non-blocking mode) from which to read bytes.
*
* [@returns](/bitcoin-bitcoin/contributor/returns/) An IntrRecvError indicating the resulting status of this read.
* IntrRecvError::OK only if all of the specified number of bytes were
* read.
*
- * [@see](/bitcoin-bitcoin/contributor/see/) This function can be interrupted by calling InterruptSocks5(bool).
+ * [@see](/bitcoin-bitcoin/contributor/see/) This function can be interrupted by calling g_socks5_interrupt().
* Sockets can be made non-blocking with Sock::SetNonBlocking().
*/
static IntrRecvError InterruptibleRecv(uint8_t* data, size_t len, std::chrono::milliseconds timeout, const Sock& sock)
{
auto curTime{Now<SteadyMilliseconds>()};
const auto endTime{curTime + timeout};
@@ -317,14 +317,15 @@ static IntrRecvError InterruptibleRecv(uint8_t* data, size_t len, std::chrono::m
return IntrRecvError::NetworkError;
}
} else {
return IntrRecvError::NetworkError;
}
}
- if (interruptSocks5Recv)
+ if (g_socks5_interrupt) {
return IntrRecvError::Interrupted;
+ }
curTime = Now<SteadyMilliseconds>();
}
return len == 0 ? IntrRecvError::OK : IntrRecvError::Timeout;
}
/** Convert SOCKS5 reply to an error message */
@@ -351,127 +352,120 @@ static std::string Socks5ErrorString(uint8_t err)
return "unknown";
}
}
bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* auth, const Sock& sock)
{
- IntrRecvError recvr;
- LogPrint(BCLog::NET, "SOCKS5 connecting %s\n", strDest);
- if (strDest.size() > 255) {
- return error("Hostname too long");
- }
- // Construct the version identifier/method selection message
- std::vector<uint8_t> vSocks5Init;
- vSocks5Init.push_back(SOCKSVersion::SOCKS5); // We want the SOCK5 protocol
- if (auth) {
- vSocks5Init.push_back(0x02); // 2 method identifiers follow...
- vSocks5Init.push_back(SOCKS5Method::NOAUTH);
- vSocks5Init.push_back(SOCKS5Method::USER_PASS);
- } else {
- vSocks5Init.push_back(0x01); // 1 method identifier follows...
- vSocks5Init.push_back(SOCKS5Method::NOAUTH);
- }
- ssize_t ret = sock.Send(vSocks5Init.data(), vSocks5Init.size(), MSG_NOSIGNAL);
- if (ret != (ssize_t)vSocks5Init.size()) {
- return error("Error sending to proxy");
- }
- uint8_t pchRet1[2];
- if (InterruptibleRecv(pchRet1, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
- LogPrintf("Socks5() connect to %s:%d failed: InterruptibleRecv() timeout or other failure\n", strDest, port);
- return false;
- }
- if (pchRet1[0] != SOCKSVersion::SOCKS5) {
- return error("Proxy failed to initialize");
- }
- if (pchRet1[1] == SOCKS5Method::USER_PASS && auth) {
- // Perform username/password authentication (as described in RFC1929)
- std::vector<uint8_t> vAuth;
- vAuth.push_back(0x01); // Current (and only) version of user/pass subnegotiation
- if (auth->username.size() > 255 || auth->password.size() > 255)
- return error("Proxy username or password too long");
- vAuth.push_back(auth->username.size());
- vAuth.insert(vAuth.end(), auth->username.begin(), auth->username.end());
- vAuth.push_back(auth->password.size());
- vAuth.insert(vAuth.end(), auth->password.begin(), auth->password.end());
- ret = sock.Send(vAuth.data(), vAuth.size(), MSG_NOSIGNAL);
- if (ret != (ssize_t)vAuth.size()) {
- return error("Error sending authentication to proxy");
- }
- LogPrint(BCLog::PROXY, "SOCKS5 sending proxy authentication %s:%s\n", auth->username, auth->password);
- uint8_t pchRetA[2];
- if (InterruptibleRecv(pchRetA, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
- return error("Error reading proxy authentication response");
+ try {
+ IntrRecvError recvr;
+ LogPrint(BCLog::NET, "SOCKS5 connecting %s\n", strDest);
+ if (strDest.size() > 255) {
+ return error("Hostname too long");
}
- if (pchRetA[0] != 0x01 || pchRetA[1] != 0x00) {
- return error("Proxy authentication unsuccessful");
+ // Construct the version identifier/method selection message
+ std::string vSocks5Init;
+ vSocks5Init.push_back(SOCKSVersion::SOCKS5); // We want the SOCK5 protocol
+ if (auth) {
+ vSocks5Init.push_back(0x02); // 2 method identifiers follow...
+ vSocks5Init.push_back(SOCKS5Method::NOAUTH);
+ vSocks5Init.push_back(SOCKS5Method::USER_PASS);
+ } else {
+ vSocks5Init.push_back(0x01); // 1 method identifier follows...
+ vSocks5Init.push_back(SOCKS5Method::NOAUTH);
}
- } else if (pchRet1[1] == SOCKS5Method::NOAUTH) {
- // Perform no authentication
- } else {
- return error("Proxy requested wrong authentication method %02x", pchRet1[1]);
- }
- std::vector<uint8_t> vSocks5;
- vSocks5.push_back(SOCKSVersion::SOCKS5); // VER protocol version
- vSocks5.push_back(SOCKS5Command::CONNECT); // CMD CONNECT
- vSocks5.push_back(0x00); // RSV Reserved must be 0
- vSocks5.push_back(SOCKS5Atyp::DOMAINNAME); // ATYP DOMAINNAME
- vSocks5.push_back(strDest.size()); // Length<=255 is checked at beginning of function
- vSocks5.insert(vSocks5.end(), strDest.begin(), strDest.end());
- vSocks5.push_back((port >> 8) & 0xFF);
- vSocks5.push_back((port >> 0) & 0xFF);
- ret = sock.Send(vSocks5.data(), vSocks5.size(), MSG_NOSIGNAL);
- if (ret != (ssize_t)vSocks5.size()) {
- return error("Error sending to proxy");
- }
- uint8_t pchRet2[4];
- if ((recvr = InterruptibleRecv(pchRet2, 4, g_socks5_recv_timeout, sock)) != IntrRecvError::OK) {
- if (recvr == IntrRecvError::Timeout) {
- /* If a timeout happens here, this effectively means we timed out while connecting
- * to the remote node. This is very common for Tor, so do not print an
- * error message. */
+ sock.SendComplete(vSocks5Init, g_socks5_recv_timeout, g_socks5_interrupt);
+ uint8_t pchRet1[2];
+ if (InterruptibleRecv(pchRet1, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
+ LogPrintf("Socks5() connect to %s:%d failed: InterruptibleRecv() timeout or other failure\n", strDest, port);
return false;
+ }
+ if (pchRet1[0] != SOCKSVersion::SOCKS5) {
+ return error("Proxy failed to initialize");
+ }
+ if (pchRet1[1] == SOCKS5Method::USER_PASS && auth) {
+ // Perform username/password authentication (as described in RFC1929)
+ std::string vAuth;
+ vAuth.push_back(0x01); // Current (and only) version of user/pass subnegotiation
+ if (auth->username.size() > 255 || auth->password.size() > 255)
+ return error("Proxy username or password too long");
+ vAuth.push_back(auth->username.size());
+ vAuth.insert(vAuth.end(), auth->username.begin(), auth->username.end());
+ vAuth.push_back(auth->password.size());
+ vAuth.insert(vAuth.end(), auth->password.begin(), auth->password.end());
+ sock.SendComplete(vAuth, g_socks5_recv_timeout, g_socks5_interrupt);
+ LogPrint(BCLog::PROXY, "SOCKS5 sending proxy authentication %s:%s\n", auth->username, auth->password);
+ uint8_t pchRetA[2];
+ if (InterruptibleRecv(pchRetA, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
+ return error("Error reading proxy authentication response");
+ }
+ if (pchRetA[0] != 0x01 || pchRetA[1] != 0x00) {
+ return error("Proxy authentication unsuccessful");
+ }
+ } else if (pchRet1[1] == SOCKS5Method::NOAUTH) {
+ // Perform no authentication
} else {
- return error("Error while reading proxy response");
+ return error("Proxy requested wrong authentication method %02x", pchRet1[1]);
}
- }
- if (pchRet2[0] != SOCKSVersion::SOCKS5) {
- return error("Proxy failed to accept request");
- }
- if (pchRet2[1] != SOCKS5Reply::SUCCEEDED) {
- // Failures to connect to a peer that are not proxy errors
- LogPrintf("Socks5() connect to %s:%d failed: %s\n", strDest, port, Socks5ErrorString(pchRet2[1]));
- return false;
- }
- if (pchRet2[2] != 0x00) { // Reserved field must be 0
- return error("Error: malformed proxy response");
- }
- uint8_t pchRet3[256];
- switch (pchRet2[3])
- {
+ std::string vSocks5;
+ vSocks5.push_back(SOCKSVersion::SOCKS5); // VER protocol version
+ vSocks5.push_back(SOCKS5Command::CONNECT); // CMD CONNECT
+ vSocks5.push_back(0x00); // RSV Reserved must be 0
+ vSocks5.push_back(SOCKS5Atyp::DOMAINNAME); // ATYP DOMAINNAME
+ vSocks5.push_back(strDest.size()); // Length<=255 is checked at beginning of function
+ vSocks5.insert(vSocks5.end(), strDest.begin(), strDest.end());
+ vSocks5.push_back((port >> 8) & 0xFF);
+ vSocks5.push_back((port >> 0) & 0xFF);
+ sock.SendComplete(vSocks5, g_socks5_recv_timeout, g_socks5_interrupt);
+ uint8_t pchRet2[4];
+ if ((recvr = InterruptibleRecv(pchRet2, 4, g_socks5_recv_timeout, sock)) != IntrRecvError::OK) {
+ if (recvr == IntrRecvError::Timeout) {
+ /* If a timeout happens here, this effectively means we timed out while connecting
+ * to the remote node. This is very common for Tor, so do not print an
+ * error message. */
+ return false;
+ } else {
+ return error("Error while reading proxy response");
+ }
+ }
+ if (pchRet2[0] != SOCKSVersion::SOCKS5) {
+ return error("Proxy failed to accept request");
+ }
+ if (pchRet2[1] != SOCKS5Reply::SUCCEEDED) {
+ // Failures to connect to a peer that are not proxy errors
+ LogPrintf("Socks5() connect to %s:%d failed: %s\n", strDest, port, Socks5ErrorString(pchRet2[1]));
+ return false;
+ }
+ if (pchRet2[2] != 0x00) { // Reserved field must be 0
+ return error("Error: malformed proxy response");
+ }
+ uint8_t pchRet3[256];
+ switch (pchRet2[3]) {
case SOCKS5Atyp::IPV4: recvr = InterruptibleRecv(pchRet3, 4, g_socks5_recv_timeout, sock); break;
case SOCKS5Atyp::IPV6: recvr = InterruptibleRecv(pchRet3, 16, g_socks5_recv_timeout, sock); break;
- case SOCKS5Atyp::DOMAINNAME:
- {
+ case SOCKS5Atyp::DOMAINNAME: {
recvr = InterruptibleRecv(pchRet3, 1, g_socks5_recv_timeout, sock);
if (recvr != IntrRecvError::OK) {
return error("Error reading from proxy");
}
int nRecv = pchRet3[0];
recvr = InterruptibleRecv(pchRet3, nRecv, g_socks5_recv_timeout, sock);
break;
}
default: return error("Error: malformed proxy response");
+ }
+ if (recvr != IntrRecvError::OK) {
+ return error("Error reading from proxy");
+ }
+ if (InterruptibleRecv(pchRet3, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
+ return error("Error reading from proxy");
+ }
+ LogPrint(BCLog::NET, "SOCKS5 connected %s\n", strDest);
+ return true;
+ } catch (const std::runtime_error& e) {
+ return error("Error during SOCKS5 proxy handshake: %s", e.what());
}
- if (recvr != IntrRecvError::OK) {
- return error("Error reading from proxy");
- }
- if (InterruptibleRecv(pchRet3, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
- return error("Error reading from proxy");
- }
- LogPrint(BCLog::NET, "SOCKS5 connected %s\n", strDest);
- return true;
}
std::unique_ptr<Sock> CreateSockOS(sa_family_t address_family)
{
// Not IPv4, IPv6 or UNIX
if (address_family == AF_UNSPEC) return nullptr;
@@ -743,17 +737,12 @@ bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out)
return subnet_out.IsValid();
}
}
return false;
}
-void InterruptSocks5(bool interrupt)
-{
- interruptSocks5Recv = interrupt;
-}
-
bool IsBadPort(uint16_t port)
{
/* Don't forget to update doc/p2p-bad-ports.md if you change this list. */
switch (port) {
case 1: // tcpmux
diff --git a/src/netbase.h b/src/netbase.h
index 997bccf30d..3c41ba3834 100644
--- a/src/netbase.h
+++ b/src/netbase.h
@@ -10,12 +10,13 @@
#endif
#include <compat/compat.h>
#include <netaddress.h>
#include <serialize.h>
#include <util/sock.h>
+#include <util/threadinterrupt.h>
#include <functional>
#include <memory>
#include <stdint.h>
#include <string>
#include <type_traits>
@@ -266,13 +267,16 @@ std::unique_ptr<Sock> ConnectDirectly(const CService& dest, bool manual_connecti
*/
std::unique_ptr<Sock> ConnectThroughProxy(const Proxy& proxy,
const std::string& dest,
uint16_t port,
bool& proxy_connection_failed);
-void InterruptSocks5(bool interrupt);
+/**
+ * Interrupt SOCKS5 reads or writes.
+ */
+extern CThreadInterrupt g_socks5_interrupt;
/**
* Connect to a specified destination service through an already connected
* SOCKS5 proxy.
*
* [@param](/bitcoin-bitcoin/contributor/param/) strDest The destination fully-qualified domain name.
diff --git a/src/test/fuzz/socks5.cpp b/src/test/fuzz/socks5.cpp
index 73235b7ced..7378cec7b8 100644
--- a/src/test/fuzz/socks5.cpp
+++ b/src/test/fuzz/socks5.cpp
@@ -29,13 +29,15 @@ void initialize_socks5()
FUZZ_TARGET_INIT(socks5, initialize_socks5)
{
FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
ProxyCredentials proxy_credentials;
proxy_credentials.username = fuzzed_data_provider.ConsumeRandomLengthString(512);
proxy_credentials.password = fuzzed_data_provider.ConsumeRandomLengthString(512);
- InterruptSocks5(fuzzed_data_provider.ConsumeBool());
+ if (fuzzed_data_provider.ConsumeBool()) {
+ g_socks5_interrupt();
+ }
// Set FUZZED_SOCKET_FAKE_LATENCY=1 to exercise recv timeout code paths. This
// will slow down fuzzing.
g_socks5_recv_timeout = (fuzzed_data_provider.ConsumeBool() && std::getenv("FUZZED_SOCKET_FAKE_LATENCY") != nullptr) ? 1ms : default_socks5_recv_timeout;
FuzzedSock fuzzed_sock = ConsumeSock(fuzzed_data_provider);
// This Socks5(...) fuzzing harness would have caught CVE-2017-18350 within
// a few seconds of fuzzing.
</details>
