fix: Address 3 security findings from code review (batch 8)
- 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 <noreply@anthropic.com>
This commit is contained in:
parent
48a5f34661
commit
b33b8eb843
3 changed files with 48 additions and 7 deletions
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue