From b33b8eb84380d88bc586e0d8404a772946578b6b Mon Sep 17 00:00:00 2001 From: snider Date: Wed, 31 Dec 2025 19:27:50 +0000 Subject: [PATCH] fix: Address 3 security findings from code review (batch 8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - HIGH-015: Improve TLS certificate validation - Enable SSL_VERIFY_PEER with system CA store - Support certificate pinning via fingerprint - Chain validation OR fingerprint match required - HIGH-019: Document libuv single-thread model for Client state - TOCTOU pattern is safe due to event loop serialization - MED-005: Fix potential alignment issues in Keccak - Use memcpy for unaligned uint8_t* to uint64_t access - Prevents undefined behavior on strict alignment architectures 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 --- miner/proxy/src/base/crypto/keccak.cpp | 7 +++- .../proxy/src/base/net/https/HttpsClient.cpp | 42 ++++++++++++++++--- miner/proxy/src/base/net/stratum/Client.cpp | 6 +++ 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/miner/proxy/src/base/crypto/keccak.cpp b/miner/proxy/src/base/crypto/keccak.cpp index aa60275..b38754e 100644 --- a/miner/proxy/src/base/crypto/keccak.cpp +++ b/miner/proxy/src/base/crypto/keccak.cpp @@ -179,8 +179,13 @@ void xmrig::keccak(const uint8_t *in, int inlen, uint8_t *md, int mdlen) memset(st, 0, sizeof(st)); for ( ; inlen >= rsiz; inlen -= rsiz, in += rsiz) { + // SECURITY FIX (MED-005): Use memcpy for potentially unaligned input data + // Direct cast of uint8_t* to uint64_t* is undefined behavior on strict + // alignment architectures (ARM, SPARC). The temp buffer is aligned. for (i = 0; i < rsizw; i++) { - st[i] ^= ((uint64_t *) in)[i]; + uint64_t val; + memcpy(&val, in + i * 8, sizeof(val)); + st[i] ^= val; } xmrig::keccakf(st, KECCAK_ROUNDS); diff --git a/miner/proxy/src/base/net/https/HttpsClient.cpp b/miner/proxy/src/base/net/https/HttpsClient.cpp index 8cd9099..572bc6a 100644 --- a/miner/proxy/src/base/net/https/HttpsClient.cpp +++ b/miner/proxy/src/base/net/https/HttpsClient.cpp @@ -46,6 +46,14 @@ xmrig::HttpsClient::HttpsClient(const char *tag, FetchRequest &&req, const std:: m_write = BIO_new(BIO_s_mem()); m_read = BIO_new(BIO_s_mem()); SSL_CTX_set_options(m_ctx, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3); + + // SECURITY FIX (HIGH-015): Enable certificate chain verification + // This works in conjunction with fingerprint verification in verify(). + // If a fingerprint is configured, we accept the cert if EITHER: + // 1. The chain validates against system CA store, OR + // 2. The fingerprint matches (for self-signed certs in mining contexts) + SSL_CTX_set_default_verify_paths(m_ctx); + SSL_CTX_set_verify(m_ctx, SSL_VERIFY_PEER, nullptr); } @@ -145,16 +153,38 @@ bool xmrig::HttpsClient::verify(X509 *cert) return false; } - if (!verifyFingerprint(cert)) { - if (!isQuiet()) { - LOG_ERR("[%s:%d] Failed to verify server certificate fingerprint", host(), port()); + // SECURITY FIX (HIGH-015): Check both certificate chain and fingerprint + // For proper security, we verify: + // 1. Chain validation (done by OpenSSL with SSL_VERIFY_PEER) + // 2. Fingerprint match (for pinning or self-signed certs) - if (strlen(m_fingerprint) == 64 && !req().fingerprint.isNull()) { - LOG_ERR("\"%s\" was given", m_fingerprint); - LOG_ERR("\"%s\" was configured", req().fingerprint.data()); + const long verifyResult = SSL_get_verify_result(m_ssl); + const bool chainValid = (verifyResult == X509_V_OK); + const bool fingerprintValid = verifyFingerprint(cert); + + // If fingerprint is configured, it MUST match (certificate pinning) + if (!req().fingerprint.isNull()) { + if (!fingerprintValid) { + if (!isQuiet()) { + LOG_ERR("[%s:%d] Failed to verify server certificate fingerprint", host(), port()); + if (strlen(m_fingerprint) == 64) { + LOG_ERR("\"%s\" was given", m_fingerprint); + LOG_ERR("\"%s\" was configured", req().fingerprint.data()); + } } + return false; } + // Fingerprint matches - accept even if chain doesn't validate + // (supports self-signed certificates with known fingerprint) + return true; + } + // No fingerprint configured - require valid certificate chain + if (!chainValid) { + if (!isQuiet()) { + LOG_ERR("[%s:%d] Certificate chain verification failed: %s", + host(), port(), X509_verify_cert_error_string(verifyResult)); + } return false; } diff --git a/miner/proxy/src/base/net/stratum/Client.cpp b/miner/proxy/src/base/net/stratum/Client.cpp index 2859ff7..bfb38d8 100644 --- a/miner/proxy/src/base/net/stratum/Client.cpp +++ b/miner/proxy/src/base/net/stratum/Client.cpp @@ -349,6 +349,12 @@ void xmrig::Client::onResolved(const DnsRecords &records, int status, const char } +// NOTE (HIGH-019): Thread safety of m_socket access +// This function checks m_socket != nullptr then uses it, which appears to be +// a TOCTOU (time-of-check-time-of-use) vulnerability. However, this is safe +// because libuv uses a single-threaded event loop model - all callbacks and +// operations on this Client run on the same thread. m_socket is only modified +// by connect() and onClose(), both called from the event loop. bool xmrig::Client::close() { if (m_state == ClosingState) {