Security Audit: Core Library Review (92 files) for CodeRabbit#2
Security Audit: Core Library Review (92 files) for CodeRabbit#2RahulRaj272727 wants to merge 1 commit intoreview-basefrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@ixwebsocket/IXBase64.h`:
- Around line 106-109: The code in IXBase64.h reads kDecodingTable using
static_cast<int>(input[i++]) which on platforms with signed char can produce
negative indices and OOB access; update the indexing to first convert the input
byte to an unsigned type before casting to int (e.g. use
static_cast<int>(static_cast<unsigned char>(input[i++])) or cast to uint8_t then
to int) for the four reads that assign a, b, c, d so bytes with high bit set
index kDecodingTable safely while preserving the existing '=' handling logic.
- Line 52: The code currently obtains a writable pointer via
const_cast<char*>(ret.c_str()) which is undefined behavior; change the pointer
acquisition to use a legal mutable buffer (e.g., use ret.data() in C++17+ or
ensure a mutable std::string copy) so that the pointer `p` points to a
modifiable buffer; update the reference where `p` is used accordingly (look for
`char *p = const_cast<char*>(ret.c_str())`, variable `p`, and the string `ret`
in IXBase64.h and replace with a safe approach such as `char *p = ret.data()` or
create a mutable copy before writing).
- Around line 54-59: The for-loop in the Base64 encoder (the block iterating
with i and using data, in_len, and sEncodingTable) can underflow because it uses
the unsigned expression "in_len - 2"; change the loop guard to a safe check such
as testing in_len before the loop or using an index comparison that cannot
underflow (e.g., use "i + 2 < in_len" or early-return/skip when in_len < 3) so
the body that reads data[i], data[i+1], data[i+2] cannot run on inputs of length
0, 1, or 2.
- Around line 96-101: The code reads input[in_len - 1] and input[in_len - 2]
without checking for empty input, causing OOB when in_len == 0; update the start
of the block in IXBase64.h (variables input, in_len, out_len) to handle empty
input early—e.g., if in_len == 0 return an empty output (or otherwise handle as
a special case) before computing out_len or accessing input[in_len - 1/ -2];
alternatively ensure you only access those indices when in_len >= 2 and adjust
out_len logic accordingly.
In `@ixwebsocket/IXHttpServer.cpp`:
- Around line 123-134: The code builds a file path by concatenating "." + uri
which allows path traversal; sanitize and canonicalize the URI before calling
readAsString. Specifically: strip any query/fragment from request->uri,
percent-decode it, reject or normalize segments like ".." and empty segments (or
collapse them), then join it against a fixed web root (e.g., webRoot +
sanitizedUri) using std::filesystem::path and obtain a canonical/resolved path
(std::filesystem::canonical or equivalent) and verify the resulting path has the
web root as its prefix; only then pass that canonical path to
readAsString(uriPath). Update the logic around variables uri, path, and the call
to readAsString to use the validated canonical path and keep
response_head_file(userAgent()/headers) usage unchanged.
In `@ixwebsocket/IXHttpServer.h`:
- Line 36: The default connection handler set in setDefaultConnectionCallback
currently builds file paths as "." + uri and is vulnerable to path traversal;
update setDefaultConnectionCallback to sanitize and normalize the incoming URI
before constructing the filesystem path: percent-decode and reject or normalize
sequences like "..", collapse multiple slashes, then resolve the resulting path
to an absolute path and verify it is inside your intended document root (e.g.
compare that resolvedPath.starts_with(documentRoot) or use canonicalization)
before calling readAsString/load/std::ifstream; if the check fails return a
403/404 response or serve a safe default. Ensure you perform these checks in the
code path used by setOnConnectionCallback/setDefaultConnectionCallback and reuse
the same sanitization helper for readAsString/load to avoid duplication.
In `@ixwebsocket/IXNetSystem.h`:
- Around line 28-39: The EAGAIN macro is incorrectly mapped to WSATRY_AGAIN;
update the mapping so EAGAIN maps to WSAEWOULDBLOCK (same as EWOULDBLOCK) so
socket non-blocking checks (e.g., the err == EWOULDBLOCK || err == EAGAIN || err
== EINPROGRESS checks in IXSocket.cpp and IXUdpSocket.cpp) work correctly on
Windows; modify the EAGAIN `#define` in the IXNetSystem.h macro block accordingly
and remove or retain WSATRY_AGAIN only for DNS-specific uses if needed.
In `@ixwebsocket/IXSocketServer.cpp`:
- Around line 310-329: The accept loop currently declares client as struct
sockaddr_in and later treats IPv6 connections as if they were sockaddr_in6,
causing invalid memory reads; change the client variable to a generic struct
sockaddr_storage (or declare both sockaddr_in and sockaddr_in6 and cast based on
sa_family) and when handling AF_INET6 cast the accepted address to (struct
sockaddr_in6*) and use its sin6_addr with inet_ntop (use sin_addr for AF_INET),
ensuring addressLen remains socklen_t and all casts and field accesses match the
actual sockaddr type used by accept in the run/accept handling code.
In `@ixwebsocket/IXUuid.cpp`:
- Around line 39-49: The constructor Uuid::Uuid() currently uses static local rd
and dist which causes a data race; make them thread-local instead (declare rd
and dist as thread_local) so each thread has its own std::random_device and
std::uniform_int_distribution instance, then keep the same calls to dist(rd) and
the existing bit-fiddling that sets _ab and _cd; ensure you only change the
storage specifier for rd and dist (or alternatively protect accesses with a
mutex) so uuid generation is safe under concurrent calls to Uuid::Uuid().
In `@ixwebsocket/IXWebSocketMessage.h`:
- Around line 18-56: The WebSocketMessage struct currently stores a const
std::string& (member str) which can dangle; change the member to own the string
(std::string str) and update the constructor(s) to accept an owning parameter
(either std::string by value or std::string&&) and initialize the member with
std::move(s) in the initializer list; remove the deleted rvalue overload since
temporaries will now be safely moved into the owned member. Ensure you update
all references to the constructor signature used by
WebSocketTransport::dispatch() so it passes/moves the string appropriately.
In `@ixwebsocket/IXWebSocketPerMessageDeflate.cpp`:
- Around line 93-96: Add a configurable max-decompressed-size to
WebSocketPerMessageDeflateOptions and enforce it during decompression: extend
WebSocketPerMessageDeflateOptions with a uint64_t maxDecompressedSize (default
reasonable value or 0==unlimited), pass that option into
WebSocketPerMessageDeflateDecompressor, and inside
WebSocketPerMessageDeflateDecompressor::decompress (the loop that calls
inflate() and appends to the output) check the cumulative output size after each
inflate chunk and abort/return false if it exceeds maxDecompressedSize; also
ensure WebSocketPerMessageDeflate::decompress forwards failures and that callers
(e.g., IXWebSocketTransport::emitMessage) treat a false return as a
decompression error.
In `@ixwebsocket/IXWebSocketTransport.cpp`:
- Around line 824-830: The getRandomUnsigned() implementation uses predictable
epoch seconds; replace it with a cryptographically secure RNG in
WebSocketTransport::getRandomUnsigned (e.g., use std::random_device +
std::uniform_int_distribution<unsigned> or call a platform CSPRNG like
getrandom()/BCryptGenRandom()/OpenSSL RAND_bytes() if stronger entropy is
needed), add the appropriate include (<random> or platform headers), and ensure
the returned unsigned is derived from securely generated bytes rather than time.
In `@ixwebsocket/IXWebSocketTransport.h`:
- Line 283: The getRandomUnsigned() implementation currently uses predictable
time-based bits for masking keys; replace its logic with a cryptographically
secure PRNG: initialize a static std::mt19937 (or std::mt19937_64) seeded from
std::random_device and use a std::uniform_int_distribution<uint32_t> to produce
the unsigned masking key in getRandomUnsigned(); ensure the engine is
static/thread-safe (or protected) and that getRandomUnsigned() returns the
uniformly distributed uint32_t value for WebSocket frame masking keys instead of
time-derived values.
🟠 Major comments (41)
ixwebsocket/IXUserAgent.cpp-26-45 (1)
26-45: Reorder platform detection macros: move generic__unix__check after specific OS checks.The current code places the
__unix__branch (line 26) before checks for HP-UX, AIX, Apple/Darwin, and Solaris. Since__unix__is defined on all these systems, the condition at line 26 matches first and prevents subsequent platform-specific branches from executing. On macOS and other systems where__unix__is defined butBSDis not set insys/param.h,PLATFORM_NAMEremains undefined, causing compilation to fail at line 67 where the macro is used directly.Move the
__unix__block after the specific OS checks (HP-UX, AIX, Apple/Darwin, Solaris) and add a fallback#define PLATFORM_NAME "unix"whenBSDis not defined:🔧 Suggested reordering + fallback
-#elif defined(__unix__) || !defined(__APPLE__) && defined(__MACH__) -#include <sys/param.h> -#if defined(BSD) -#define PLATFORM_NAME "bsd" // FreeBSD, NetBSD, OpenBSD, DragonFly BSD -#endif -#elif defined(__hpux) -#define PLATFORM_NAME "hp-ux" // HP-UX -#elif defined(_AIX) -#define PLATFORM_NAME "aix" // IBM AIX -#elif defined(__APPLE__) && defined(__MACH__) // Apple OSX and iOS (Darwin) +#elif defined(__hpux) +#define PLATFORM_NAME "hp-ux" // HP-UX +#elif defined(_AIX) +#define PLATFORM_NAME "aix" // IBM AIX +#elif defined(__sun) && defined(__SVR4) +#define PLATFORM_NAME "solaris" // Oracle Solaris, Open Indiana +#elif defined(__APPLE__) && defined(__MACH__) // Apple OSX and iOS (Darwin) `#include` <TargetConditionals.h> `#if` TARGET_IPHONE_SIMULATOR == 1 `#define` PLATFORM_NAME "ios" // Apple iOS `#elif` TARGET_OS_IPHONE == 1 `#define` PLATFORM_NAME "ios" // Apple iOS `#elif` TARGET_OS_MAC == 1 `#define` PLATFORM_NAME "macos" // Apple OSX `#endif` -#elif defined(__sun) && defined(__SVR4) -#define PLATFORM_NAME "solaris" // Oracle Solaris, Open Indiana +#elif defined(__unix__) || (!defined(__APPLE__) && defined(__MACH__)) +#include <sys/param.h> +#if defined(BSD) +#define PLATFORM_NAME "bsd" // FreeBSD, NetBSD, OpenBSD, DragonFly BSD +#else +#define PLATFORM_NAME "unix" +#endifixwebsocket/IXUniquePtr.h-9-16 (1)
9-16: Add explicit<utility>include forstd::forwardportability.
Line 16 usesstd::forward, which requires<utility>per the C++ standard. While this may compile on many platforms due to transitive includes through<memory>, explicitly including<utility>ensures portability across different standard library implementations.Proposed fix
`#include` <memory> +#include <utility>ixwebsocket/IXGzipCodec.cpp-74-118 (1)
74-118: Decompression has no output size limit — potential decompression bomb DoS.The
gzipDecompressfunction will decompress input indefinitely untilinflatereturnsZ_STREAM_END. A maliciously crafted small compressed payload can expand to gigabytes of output, exhausting memory.Consider adding a maximum output size parameter or internal limit to protect against this attack vector, especially since this is a networking library that may process untrusted input.
Do you want me to generate a modified implementation with a configurable maximum decompression size?
ixwebsocket/IXSelectInterruptPipe.cpp-64-86 (1)
64-86: Resource leak: pipe descriptors not closed onfcntlfailure.When
fcntlfails to setO_NONBLOCK, the code sets_fildesto -1 but does not close the successfully created pipe descriptors from line 55. This leaks two file descriptors per failed initialization.🔧 Proposed fix to close pipe on fcntl failure
if (fcntl(_fildes[kPipeReadIndex], F_SETFL, O_NONBLOCK) == -1) { std::stringstream ss; ss << "SelectInterruptPipe::init() failed in fcntl(..., O_NONBLOCK) call" << " : " << strerror(errno); errorMsg = ss.str(); + ::close(_fildes[kPipeReadIndex]); + ::close(_fildes[kPipeWriteIndex]); _fildes[kPipeReadIndex] = -1; _fildes[kPipeWriteIndex] = -1; return false; } if (fcntl(_fildes[kPipeWriteIndex], F_SETFL, O_NONBLOCK) == -1) { std::stringstream ss; ss << "SelectInterruptPipe::init() failed in fcntl(..., O_NONBLOCK) call" << " : " << strerror(errno); errorMsg = ss.str(); + ::close(_fildes[kPipeReadIndex]); + ::close(_fildes[kPipeWriteIndex]); _fildes[kPipeReadIndex] = -1; _fildes[kPipeWriteIndex] = -1; return false; }ixwebsocket/IXGzipCodec.cpp-31-36 (1)
31-36:deflateInit2return value not checked — undefined behavior on failure.If
deflateInit2fails (e.g., due to memory allocation failure), the z_stream remains uninitialized and subsequent calls todeflate()anddeflateEnd()will operate on invalid state, causing undefined behavior or crashes.🔧 Proposed fix to check initialization
- deflateInit2(&zs, + int initResult = deflateInit2(&zs, Z_DEFAULT_COMPRESSION, Z_DEFLATED, windowBits | GZIP_ENCODING, 8, Z_DEFAULT_STRATEGY); + + if (initResult != Z_OK) + { + return std::string(); + } zs.next_in = (Bytef*) str.data();ixwebsocket/IXGetFreePort.h-9-12 (1)
9-12: Infinite loop risk and unreachable code ingetFreePort()implementation.The
getFreePort()function inIXGetFreePort.cpp(lines 71–91) contains awhile(true)loop that only exits whenport > 1024. SincegetAnyFreePort()binds to port 0 to request an OS-assigned ephemeral port, it depends on the system's ephemeral port range configuration. On systems where this range includes ports ≤ 1024, the loop would run indefinitely—a potential DoS vector. While uncommon, this is possible on misconfigured systems.Additionally, the
return -1;statement at line 91 is unreachable dead code, as the loop either returns inside the condition or runs indefinitely.ixwebsocket/IXHttp.cpp-132-173 (1)
132-173: Add a hard cap onContent-Lengthto prevent memory exhaustion.
readBytes(contentLength, …)can allocate/read arbitrarily large bodies under attacker control. This is a classic DoS vector; enforce a maximum (configurable) size before reading.🛡️ Suggested guard
if (contentLength < 0) { return std::make_tuple( false, "Error: 'Content-Length' should be a positive integer", httpRequest); } + constexpr int kMaxBodySize = 10 * 1024 * 1024; // 10MB (tune/configure) + if (contentLength > kMaxBodySize) + { + return std::make_tuple( + false, "Error: 'Content-Length' exceeds maximum allowed size", httpRequest); + } auto res = socket->readBytes(contentLength, nullptr, nullptr, isCancellationRequested);ixwebsocket/IXWebSocketHttpHeaders.cpp-23-67 (1)
23-67: Reject overlong/unterminated header lines to avoid OOB/underflow.If a header line exceeds 1023 bytes or lacks CRLF, the current code can (a) silently truncate and (b) access out of bounds or underflow when computing
substr. Fail fast when the line doesn’t terminate properly and guardstart/length before indexing.🔧 Proposed hardening
for (i = 0; i < 2 || (i < 1023 && line[i - 2] != '\r' && line[i - 1] != '\n'); ++i) { if (!socket->readByte(line + i, isCancellationRequested)) { return std::make_pair(false, headers); } if (line[i] == ':' && colon == 0) { colon = i; } } + if (i >= 1023 && (line[i - 2] != '\r' || line[i - 1] != '\n')) + { + // header line too long or unterminated + return std::make_pair(false, headers); + } if (line[0] == '\r' && line[1] == '\n') { break; } @@ if (colon > 0) { line[i] = '\0'; std::string lineStr(line); + if (lineStr.size() < 2 || static_cast<size_t>(colon + 1) >= lineStr.size()) + { + return std::make_pair(false, headers); + } int start = colon + 1; - while (lineStr[start] == ' ') + while (start < static_cast<int>(lineStr.size()) && lineStr[start] == ' ') { start++; } + if (lineStr.size() < static_cast<size_t>(start + 2)) + { + return std::make_pair(false, headers); + } std::string name(lineStr.substr(0, colon)); std::string value(lineStr.substr(start, lineStr.size() - start - 2));ixwebsocket/IXSocketFactory.cpp-43-55 (1)
43-55: Missing error message when TLS is enabled but no backend is matched.When
IXWEBSOCKET_USE_TLSis defined but none of the specific backend macros (IXWEBSOCKET_USE_MBED_TLS,IXWEBSOCKET_USE_OPEN_SSL,__APPLE__) are defined, thesocketpointer remainsnullptrand the function proceeds to callsocket->init()at line 57, causing a null pointer dereference.🐛 Proposed fix
`#ifdef` IXWEBSOCKET_USE_TLS `#if` defined(IXWEBSOCKET_USE_MBED_TLS) socket = ix::make_unique<SocketMbedTLS>(tlsOptions, fd); `#elif` defined(IXWEBSOCKET_USE_OPEN_SSL) socket = ix::make_unique<SocketOpenSSL>(tlsOptions, fd); `#elif` defined(__APPLE__) socket = ix::make_unique<SocketAppleSSL>(tlsOptions, fd); +#else + errorMsg = "TLS is enabled but no TLS backend was configured."; + return nullptr; `#endif` `#else` errorMsg = "TLS support is not enabled on this platform."; return nullptr; `#endif`ixwebsocket/IXSocketConnect.cpp-59-93 (1)
59-93: Performance issue:SelectInterruptPtrcreated repeatedly inside polling loop.A new
SelectInterruptPtris allocated viacreateSelectInterrupt()on every iteration of the polling loop (line 70). This is inefficient and creates unnecessary allocation overhead. Move the creation outside the loop.Proposed fix
+ SelectInterruptPtr selectInterrupt = ix::createSelectInterrupt(); for (;;) { if (isCancellationRequested && isCancellationRequested()) // Must handle timeout as well { Socket::closeSocket(fd); errMsg = "Cancelled"; return -1; } int timeoutMs = 10; bool readyToRead = false; - SelectInterruptPtr selectInterrupt = ix::createSelectInterrupt(); PollResultType pollResult = Socket::poll(readyToRead, timeoutMs, fd, selectInterrupt);ixwebsocket/IXWebSocketPerMessageDeflate.cpp-71-81 (1)
71-81: The window bits parameters are swapped for server-side usage, violating RFC 7692.The
init()method usesgetClientMaxWindowBits()for compression andgetServerMaxWindowBits()for decompression, which is correct only for client-side. Since the sameWebSocketPerMessageDeflateclass is used for both client (line 129 in IXWebSocketTransport.cpp) and server (line 184 in IXWebSocketTransport.cpp) without role differentiation, the parameters are reversed when used server-side:
- Client-side: compressor should use client_max_window_bits ✓, decompressor should use server_max_window_bits ✓
- Server-side: compressor should use server_max_window_bits ✗ (currently uses client), decompressor should use client_max_window_bits ✗ (currently uses server)
This violates RFC 7692 and will cause interoperability issues when negotiating non-default window sizes. The class needs either role-aware parameter swapping or separate implementations for client and server.
ixwebsocket/IXWebSocketProxyServer.cpp-107-112 (1)
107-112: Add timeout/exit conditions to the remote‑connect wait loop.The loop waits until
ReadyState::Openwith no timeout (Line 109), so a failed backend connect can hang the handler indefinitely and tie up resources.⏱️ Suggested fix with timeout
- while (state->webSocket().getReadyState() != ReadyState::Open) - { - std::this_thread::sleep_for(std::chrono::milliseconds(10)); - } + auto start = std::chrono::steady_clock::now(); + const auto kConnectTimeout = std::chrono::seconds(10); + while (state->webSocket().getReadyState() != ReadyState::Open) + { + if (std::chrono::steady_clock::now() - start > kConnectTimeout) + { + state->setTerminated(); + return; + } + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + }ixwebsocket/IXWebSocketServer.cpp-190-215 (1)
190-215: Unbounded flush loop can stall broadcast thread.The loop waits until
bufferedAmount()reaches zero with repeated sleeps; a slow or stalled client can block message processing indefinitely (DoS risk). Add a maximum wait or drop/close slow clients.⏳ Suggested bounded flush
- do - { - std::chrono::duration<double, std::milli> duration(500); - std::this_thread::sleep_for(duration); - } while (client->bufferedAmount() != 0); + auto start = std::chrono::steady_clock::now(); + const auto kMaxFlushWait = std::chrono::seconds(5); + while (client->bufferedAmount() != 0 && + std::chrono::steady_clock::now() - start < kMaxFlushWait) + { + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + } + if (client->bufferedAmount() != 0) + { + client->close(); + }ixwebsocket/IXDNSLookup.h-48-49 (1)
48-49: Thread safety issue:getErrMsg()returns reference to protected data.Based on the implementation in
IXDNSLookup.cpp(lines 183-187),getErrMsg()acquires a lock, then returns aconst std::string&to_errMsg. Once the function returns, the lock is released, but the caller still holds a reference to_errMsg. If another thread callssetErrMsg(), this reference becomes a data race.The signature should return by value:
- const std::string& getErrMsg(); + std::string getErrMsg();And the implementation should copy:
std::string DNSLookup::getErrMsg() { std::lock_guard<std::mutex> lock(_errMsgMutex); return _errMsg; // return copy }ixwebsocket/IXConnectionState.cpp-13-17 (1)
13-17: Member_remotePortis not initialized.The constructor initializes
_terminatedbut leaves_remotePortuninitialized. Reading an uninitializedintingetRemotePort()beforesetRemotePort()is called results in undefined behavior.Proposed fix
ConnectionState::ConnectionState() : _terminated(false) + , _remotePort(0) { computeId(); }ixwebsocket/IXConnectionState.h-49-50 (1)
49-50: Member_remotePortshould have a default initializer.To prevent undefined behavior from reading uninitialized memory,
_remotePortshould be initialized in the class declaration or the constructor initializer list.Proposed fix in header
std::string _remoteIp; - int _remotePort; + int _remotePort = 0;ixwebsocket/IXUdpSocket.cpp-26-32 (1)
26-32: Race condition inclose()method.The check
if (_sockfd == -1)and subsequentcloseSocket(_sockfd); _sockfd = -1;is not atomic. If two threads callclose()simultaneously, both could pass the check before either sets_sockfd = -1, leading to a double-close. Since_sockfdisstd::atomic<int>, useexchangefor atomicity.Proposed fix using atomic exchange
void UdpSocket::close() { - if (_sockfd == -1) return; - - closeSocket(_sockfd); - _sockfd = -1; + int fd = _sockfd.exchange(-1); + if (fd == -1) return; + + closeSocket(fd); }ixwebsocket/IXNetSystem.cpp-220-240 (1)
220-240: Missing initialization check forfd->fdbefore use withFD_SET.Unlike the event-based path (line 129), this
select-based fallback does not check iffd->fd >= 0before using it withFD_SET. Negative file descriptors could cause undefined behavior.Proposed fix
for (nfds_t i = 0; i < nfds; ++i) { struct pollfd* fd = &fds[i]; + if (fd->fd < 0) + { + continue; + } if (fd->fd > maxfd) { maxfd = fd->fd; }ixwebsocket/IXUdpSocket.cpp-94-101 (1)
94-101: Incorrect error message source aftergetaddrinfofailure.When
getaddrinfofails, the error message should usegai_strerror(ret)(the GAI-specific error function), notstrerror(UdpSocket::getErrno()). The errno/WSAGetLastError is not set bygetaddrinfo; instead, it returns its own error codes.Proposed fix
int ret = getaddrinfo(host.c_str(), nullptr, &hints, &result); if (ret != 0) { - errMsg = strerror(UdpSocket::getErrno()); - freeaddrinfo(result); + errMsg = gai_strerror(ret); + // result may be nullptr on failure, but freeaddrinfo handles nullptr safely close(); return false; }Note: Also remove the
freeaddrinfo(result)call on the error path sinceresultmay be undefined/null whengetaddrinfofails.ixwebsocket/IXHttpServer.h-38-40 (1)
38-40:makeDebugServer()logs full request headers and body without sensitive data filtering.The implementation (lines 205-237 in IXHttpServer.cpp) logs all HTTP headers and the complete request body via
logInfo(). This publicly exposed method could inadvertently log sensitive information (authentication tokens, PII, credentials) if called by library users.Add a documentation warning that
makeDebugServer()is strictly for development/testing, or filter sensitive headers (Authorization, Cookie, etc.) before logging.ixwebsocket/IXWebSocketHandshake.cpp-329-336 (1)
329-336: Inconsistent case handling in Upgrade header validation.Line 329 uses
insensitiveStringComparefor the lowercase keyheaders["upgrade"], but line 330 checks the original caseheaders["Upgrade"]. Since the header parsing likely normalizes keys to lowercase,headers["Upgrade"]would not match and this condition would never be true for the Firefox special case.🔧 Proposed fix
- if (!insensitiveStringCompare(headers["upgrade"], "WebSocket") && - headers["Upgrade"] != "keep-alive, Upgrade") // special case for firefox + if (!insensitiveStringCompare(headers["upgrade"], "WebSocket") && + headers["upgrade"] != "keep-alive, Upgrade") // special case for firefoxNote: The Firefox special case value should also use case-insensitive comparison for robustness.
ixwebsocket/IXSocketTLSOptions.cpp-34-39 (1)
34-39: In-memory CA validation incorrectly attempts file existence check.When
caFilecontains an in-memory certificate (starting with"-----BEGIN CERTIFICATE-----"), this code attempts to open it as a file path, which will fail. The check should also excludeisUsingInMemoryCAs()case.🔧 Proposed fix
- if (!caFile.empty() && caFile != kTLSCAFileDisableVerify && - caFile != kTLSCAFileUseSystemDefaults && !std::ifstream(caFile)) + if (!caFile.empty() && caFile != kTLSCAFileDisableVerify && + caFile != kTLSCAFileUseSystemDefaults && + caFile.find(kTLSInMemoryMarker) == std::string::npos && + !std::ifstream(caFile)) { _errMsg = "caFile not found: " + caFile; return false; }ixwebsocket/IXHttpServer.cpp-94-95 (1)
94-95: Unhandled HTTP parse errors can lead to unexpected behavior.The FIXME comment indicates that errors from
Http::parseRequestare not handled. If parsing fails (e.g., malformed request), the code may proceed with invalid or uninitialized request data, potentially causing undefined behavior or security issues.🛡️ Proposed fix
auto ret = Http::parseRequest(socket, _timeoutSecs); - // FIXME: handle errors in parseRequest - if (std::get<0>(ret)) + if (!std::get<0>(ret)) + { + // Parsing failed - log and close connection + logError("Failed to parse HTTP request: " + std::get<1>(ret)); + connectionState->setTerminated(); + return; + } + { auto request = std::get<2>(ret);ixwebsocket/IXSocketMbedTLS.cpp-354-378 (1)
354-378: Unreachable code in recv() - while(true) loop always exits on first iteration.The
while (true)loop is misleading because every code path within the loop ends withreturn. The loop never actually iterates more than once. This appears to be leftover from a refactor or a bug.If the intent was to retry on
WANT_READ/WANT_WRITE, the current implementation doesn't do that - it returns-1withEWOULDBLOCKinstead of retrying.🔧 Proposed fix - remove misleading loop
ssize_t SocketMbedTLS::recv(void* buf, size_t nbyte) { - while (true) - { - std::lock_guard<std::mutex> lock(_mutex); + std::lock_guard<std::mutex> lock(_mutex); - ssize_t res = mbedtls_ssl_read(&_ssl, (unsigned char*) buf, (int) nbyte); + ssize_t res = mbedtls_ssl_read(&_ssl, (unsigned char*) buf, nbyte); - if (res > 0) - { - return res; - } + if (res > 0) + { + return res; + } - if (res == 0) - { - errno = ECONNRESET; - } + if (res == 0) + { + errno = ECONNRESET; + } + else if (res == MBEDTLS_ERR_SSL_WANT_READ || res == MBEDTLS_ERR_SSL_WANT_WRITE) + { + errno = EWOULDBLOCK; + } - if (res == MBEDTLS_ERR_SSL_WANT_READ || res == MBEDTLS_ERR_SSL_WANT_WRITE) - { - errno = EWOULDBLOCK; - } - return -1; - } + return -1; }ixwebsocket/IXDNSLookup.cpp-184-188 (1)
184-188: Race condition: returning reference to mutex-protected member after lock release.
getErrMsg()returns a reference to_errMsgafter the lock guard goes out of scope. The caller can read stale or torn data if another thread modifies_errMsgconcurrently.🔒 Proposed fix: return by value
- const std::string& DNSLookup::getErrMsg() + std::string DNSLookup::getErrMsg() { std::lock_guard<std::mutex> lock(_errMsgMutex); return _errMsg; }Note: The declaration in
IXDNSLookup.hmust also be updated to returnstd::stringinstead ofconst std::string&.ixwebsocket/IXSocketOpenSSL.cpp-464-488 (1)
464-488: Security issue: Certificate/key loading failures don't abort the TLS setup.When
hasCertAndKey()is true but loading fails (Lines 466-487), the function setserrMsgbut continues execution instead of returningfalse. This means the TLS connection may proceed without the intended client certificate, potentially failing mutual TLS silently or connecting with weaker authentication than expected.🔒 Proposed fix: return false on cert/key errors
if (_tlsOptions.hasCertAndKey()) { if (SSL_CTX_use_certificate_chain_file(_ssl_context, _tlsOptions.certFile.c_str()) != 1) { auto sslErr = ERR_get_error(); errMsg = "OpenSSL failed - SSL_CTX_use_certificate_chain_file(\"" + _tlsOptions.certFile + "\") failed: "; errMsg += ERR_error_string(sslErr, nullptr); + return false; } else if (SSL_CTX_use_PrivateKey_file( _ssl_context, _tlsOptions.keyFile.c_str(), SSL_FILETYPE_PEM) != 1) { auto sslErr = ERR_get_error(); errMsg = "OpenSSL failed - SSL_CTX_use_PrivateKey_file(\"" + _tlsOptions.keyFile + "\") failed: "; errMsg += ERR_error_string(sslErr, nullptr); + return false; } else if (!SSL_CTX_check_private_key(_ssl_context)) { auto sslErr = ERR_get_error(); errMsg = "OpenSSL failed - cert/key mismatch(\"" + _tlsOptions.certFile + ", " + _tlsOptions.keyFile + "\")"; errMsg += ERR_error_string(sslErr, nullptr); + return false; } }ixwebsocket/IXSocketOpenSSL.cpp-426-459 (1)
426-459: Potential DoS: Server TLS handshake loop lacks timeout/cancellation.
openSSLServerHandshakeloops indefinitely onSSL_ERROR_WANT_READ/SSL_ERROR_WANT_WRITEwithout any timeout or cancellation mechanism. A malicious client could keep the server stuck in this loop by sending partial TLS handshake data, consuming server resources.In contrast,
openSSLClientHandshake(Line 389) includes a cancellation check.🛡️ Proposed fix: add timeout/cancellation to server handshake
- bool SocketOpenSSL::openSSLServerHandshake(std::string& errMsg) + bool SocketOpenSSL::openSSLServerHandshake(std::string& errMsg, + const CancellationRequest& isCancellationRequested) { while (true) { if (_ssl_connection == nullptr || _ssl_context == nullptr) { return false; } + if (isCancellationRequested && isCancellationRequested()) + { + errMsg = "Cancellation requested"; + return false; + } + ERR_clear_error(); int accept_result = SSL_accept(_ssl_connection);The caller would need to provide a cancellation request with timeout.
ixwebsocket/IXDNSLookup.cpp-70-78 (1)
70-78: Potential undefined behavior:freeaddrinfo(nullptr)when lookup fails.When
getaddrinfofails,resis set tonullptr(Line 75), but the returnedAddrInfoPtrstill hasfreeaddrinfoas its deleter. When this shared_ptr is destroyed, it will callfreeaddrinfo(nullptr), which is undefined behavior on some platforms (POSIX doesn't guarantee it's safe).🐛 Proposed fix: use custom deleter that checks for null
struct addrinfo* res; int getaddrinfo_result = getaddrinfo(hostname.c_str(), sport.c_str(), &hints, &res); if (getaddrinfo_result) { errMsg = gai_strerror(getaddrinfo_result); - res = nullptr; + return nullptr; } - return AddrInfoPtr{ res, freeaddrinfo }; + return AddrInfoPtr(res, freeaddrinfo);Alternatively, use a custom deleter:
return AddrInfoPtr(res, [](addrinfo* p) { if (p) freeaddrinfo(p); });ixwebsocket/IXWebSocketHandshake.h-44-46 (1)
44-46: Use a cryptographically secure PRNG for WebSocket Sec-WebSocket-Key generation.The
genRandomStringimplementation usesstd::random_device+std::default_random_engine, which is not guaranteed to be cryptographically secure and may fall back to a deterministic PRNG on some platforms. RFC 6455 requires the 16-byte WebSocket key to be cryptographically unpredictable. Additionally, the 26-character alphabet ("0123456789ABCDEFGHabcdefgh") provides only ~75 bits of entropy instead of the required ~128 bits. Replace with a platform CSPRNG API (e.g.,BCryptGenRandomon Windows,/dev/urandomon POSIX) or a cryptographic library like OpenSSL.ixwebsocket/IXSocketServer.cpp-387-395 (1)
387-395: Null pointer dereference if connection state factory is not set.The static analysis correctly identifies that
connectionStatemay be null if_connectionStateFactoryis set but returns nullptr, or if_connectionStateFactoryis falsy and the block is skipped entirely. Lines 392-394 unconditionally dereferenceconnectionState.🐛 Proposed fix
std::shared_ptr<ConnectionState> connectionState; if (_connectionStateFactory) { connectionState = _connectionStateFactory(); } + + if (!connectionState) + { + logError("SocketServer::run() failed to create connection state"); + Socket::closeSocket(clientFd); + continue; + } + connectionState->setOnSetTerminatedCallback([this] { onSetTerminatedCallback(); }); connectionState->setRemoteIp(remoteIp); connectionState->setRemotePort(remotePort);ixwebsocket/IXWebSocketPerMessageDeflateCodec.cpp-209-251 (1)
209-251: Missing output size limit in decompress - potential decompression bomb vulnerability.The
decompressfunction has no upper bound on the decompressed output size. A malicious peer could send a small compressed payload that expands to gigabytes, causing memory exhaustion (zip bomb attack).Consider adding a maximum output size limit.
🛡️ Proposed mitigation
- bool WebSocketPerMessageDeflateDecompressor::decompress(const std::string& in, std::string& out) + bool WebSocketPerMessageDeflateDecompressor::decompress(const std::string& in, std::string& out, size_t maxOutputSize) { `#ifdef` IXWEBSOCKET_USE_ZLIB + const size_t defaultMaxOutputSize = 100 * 1024 * 1024; // 100MB default limit + if (maxOutputSize == 0) maxOutputSize = defaultMaxOutputSize; + // ... existing setup code ... do { _inflateState.avail_out = (uInt) _compressBuffer.size(); _inflateState.next_out = &_compressBuffer.front(); int ret = inflate(&_inflateState, Z_SYNC_FLUSH); if (ret == Z_NEED_DICT || ret == Z_DATA_ERROR || ret == Z_MEM_ERROR) { return false; // zlib error } out.append(reinterpret_cast<char*>(&_compressBuffer.front()), _compressBuffer.size() - _inflateState.avail_out); + + if (out.size() > maxOutputSize) + { + return false; // Decompression bomb protection + } } while (_inflateState.avail_out == 0);ixwebsocket/IXWebSocket.cpp-27-27 (1)
27-27: Static callback without synchronization poses data race risk.The static
_onTrafficTrackerCallbackis accessed from multiple threads (viainvokeTrafficTrackerCallback) butsetTrafficTrackerCallbackwrites to it without synchronization. This is a data race (undefined behavior in C++).🔒 Proposed fix using atomic or mutex
Consider using
std::atomic<OnTrafficTrackerCallback*>or protecting access with a static mutex. Alternatively, if the callback is only set once during initialization before any threads are started, document this constraint clearly.+ static std::mutex _trafficTrackerMutex; static OnTrafficTrackerCallback WebSocket::_onTrafficTrackerCallback = nullptr;And in
setTrafficTrackerCallbackandinvokeTrafficTrackerCallback:void WebSocket::setTrafficTrackerCallback(const OnTrafficTrackerCallback& callback) { + std::lock_guard<std::mutex> lock(_trafficTrackerMutex); _onTrafficTrackerCallback = callback; } void WebSocket::invokeTrafficTrackerCallback(size_t size, bool incoming) { + OnTrafficTrackerCallback callback; + { + std::lock_guard<std::mutex> lock(_trafficTrackerMutex); + callback = _onTrafficTrackerCallback; + } + if (callback) { - _onTrafficTrackerCallback(size, incoming); + callback(size, incoming); } }ixwebsocket/IXHttpClient.cpp-193-216 (1)
193-216: Potential HTTP header injection viaextraHeaders.User-provided header values from
args->extraHeaders(Line 194-197) are written directly into the HTTP request without sanitization. If a header value contains\r\n, an attacker could inject arbitrary headers or even split the request.🛡️ Proposed mitigation
Validate that header names and values don't contain CR/LF characters:
// Append extra headers for (auto&& it : args->extraHeaders) { + // Reject headers with CR/LF to prevent header injection + if (it.first.find_first_of("\r\n") != std::string::npos || + it.second.find_first_of("\r\n") != std::string::npos) + { + return std::make_shared<HttpResponse>(code, + description, + HttpErrorCode::Invalid, + headers, + payload, + "Invalid header: contains CR/LF", + uploadSize, + downloadSize); + } ss << it.first << ": " << it.second << "\r\n"; }ixwebsocket/IXHttpClient.cpp-415-444 (1)
415-444: Potential memory exhaustion from unbounded Content-Length.The
Content-Lengthheader is parsed and used directly to reserve payload memory (Line 442). A malicious server could send a hugeContent-Lengthvalue, causing the client to attempt allocating massive amounts of memory before receiving any data.Consider adding a configurable maximum response size limit.
🛡️ Proposed mitigation
+// Add to HttpRequestArgs or HttpClient +static const size_t kDefaultMaxResponseSize = 100 * 1024 * 1024; // 100MB +size_t maxResponseSize = kDefaultMaxResponseSize; // In the Content-Length handling: ss >> contentLength; +if (contentLength > args->maxResponseSize) { + return std::make_shared<HttpResponse>(code, + description, + HttpErrorCode::ResponseTooLarge, + headers, + payload, + "Response exceeds maximum allowed size", + uploadSize, + downloadSize); +}ixwebsocket/IXWebSocket.cpp-629-633 (1)
629-633: Returning reference to internal vector while holding lock causes use-after-unlock.
getSubProtocols()returns a reference to_subProtocolswhile holding_configMutex, but the lock is released when the function returns. Any caller using the returned reference after the function returns is accessing the vector without synchronization, which is a data race if another thread callsaddSubProtocol()concurrently.🔒 Proposed fix: return by value
-const std::vector<std::string>& WebSocket::getSubProtocols() +std::vector<std::string> WebSocket::getSubProtocols() const { std::lock_guard<std::mutex> lock(_configMutex); return _subProtocols; }This requires updating the header declaration accordingly.
ixwebsocket/IXHttpClient.cpp-446-524 (1)
446-524: Chunked transfer encoding lacks total size limit - DoS vector.The chunked transfer decoding loop (Lines 451-523) accumulates chunks indefinitely without any upper bound on total payload size. A malicious server could send an endless stream of chunks, exhausting client memory.
🛡️ Proposed mitigation
Track total bytes received and enforce a limit:
+size_t totalBytesReceived = 0; while (true) { // ... read chunk size ... + totalBytesReceived += chunkSize; + if (totalBytesReceived > args->maxResponseSize) { + return std::make_shared<HttpResponse>(code, + description, + HttpErrorCode::ResponseTooLarge, + headers, + payload, + "Chunked response exceeds maximum allowed size", + uploadSize, + downloadSize); + } // ... read chunk ...ixwebsocket/IXHttpClient.cpp-367-400 (1)
367-400: SSRF risk: Redirect handling follows arbitrary Location headers without validation.When following redirects (lines 367-400), the code extracts the
Locationheader directly and passes it torequest()without any validation. An attacker controlling a redirect response could redirect requests to internal services (e.g.,http://169.254.169.254/for cloud metadata, orhttp://localhost:8080/admin).The
HttpRequestArgsstructure offers onlyfollowRedirects(bool) andmaxRedirects(int) options—no mechanism to validate or restrict redirect destinations.Consider:
- Allowing only same-origin redirects by default
- Providing a callback to validate redirect URLs
- Blocking redirects to private IP ranges (10.x, 172.16-31.x, 192.168.x, 127.x, 169.254.x, link-local IPv6)
ixwebsocket/IXWebSocketTransport.cpp-342-346 (1)
342-346: Data race:_lastSendPingTimePointaccessed without mutex.Line 344 reads
_lastSendPingTimePointdirectly, but other code paths (Lines 246, 288, 298, 1044) access it while holding_lastSendPingTimePointMutex. This is a data race that could cause undefined behavior.🔒 Proposed fix to use mutex-protected access
if (_pingIntervalSecs > 0) { // compute lasting delay to wait for next ping / timeout, if at least one set auto now = std::chrono::steady_clock::now(); + std::chrono::steady_clock::time_point lastPing; + { + std::lock_guard<std::mutex> lock(_lastSendPingTimePointMutex); + lastPing = _lastSendPingTimePoint; + } int timeSinceLastPingMs = (int) std::chrono::duration_cast<std::chrono::milliseconds>( - now - _lastSendPingTimePoint) + now - lastPing) .count(); lastingTimeoutDelayInMs = (1000 * _pingIntervalSecs) - timeSinceLastPingMs; }ixwebsocket/IXWebSocketTransport.cpp-117-168 (1)
117-168: Redirect loop followsLocationheader without URL validation.The redirect handling at Lines 145-158 follows the
Locationheader without validating the scheme. A malicious server could redirect fromwss://tohttp://or to internal addresses (SSRF). Consider:
- Validating that the redirect target uses
ws://orwss://scheme only- Optionally restricting redirects to the same origin or allowing explicit opt-in for cross-origin redirects
🛡️ Proposed fix to validate redirect scheme
remoteUrl = it->second; + + // Validate redirect URL scheme to prevent protocol downgrade or SSRF + std::string redirectProtocol, redirectHost, redirectPath, redirectQuery; + int redirectPort; + if (!UrlParser::parse(remoteUrl, redirectProtocol, redirectHost, redirectPath, redirectQuery, redirectPort)) + { + return WebSocketInitResult(false, 0, "Invalid redirect URL"); + } + if (redirectProtocol != "ws" && redirectProtocol != "wss") + { + return WebSocketInitResult(false, 0, "Redirect to non-WebSocket protocol rejected"); + } + // Optional: reject downgrade from wss to ws + if (tls && redirectProtocol == "ws") + { + return WebSocketInitResult(false, 0, "TLS downgrade via redirect rejected"); + } + continue;ixwebsocket/IXWebSocketTransport.cpp-1275-1279 (1)
1275-1279: Race condition: Returning reference to mutex-protected member.
getCloseReason()returns aconst std::string&but releases the mutex before the caller uses the reference. Another thread could callsetCloseReason()and modify_closeReasonwhile the caller is still using the reference.🔒 Proposed fix to return by value
- const std::string& WebSocketTransport::getCloseReason() const + std::string WebSocketTransport::getCloseReason() const { std::lock_guard<std::mutex> lock(_closeReasonMutex); return _closeReason; }Also update the declaration in the header file to return
std::stringinstead ofconst std::string&.ixwebsocket/IXWebSocketTransport.cpp-541-552 (1)
541-552: Add payload size validation to prevent memory exhaustion DoS.The
maxFrameSizecheck of1ULL << 63is ineffective—all valid frame sizes pass through. A malicious peer can send a frame header claiming a multi-gigabyte payload, causing the receiver to set_rxbufWantedto an enormous value and buffer indefinitely via_rxbuf.insert()(line 1133), potentially exhausting memory.Add a configurable maximum payload size limit and validate frame size before setting
_rxbufWanted. Close the connection when a frame exceeds the limit.
| size_t out_len = 4 * ((in_len + 2) / 3); | ||
| std::string ret(out_len, '\0'); | ||
| size_t i; | ||
| char *p = const_cast<char*>(ret.c_str()); |
There was a problem hiding this comment.
Undefined behavior: modifying string through c_str().
const_cast<char*>(ret.c_str()) and then writing through p is undefined behavior. The c_str() method returns a pointer to a read-only buffer, and modifying it violates the C++ standard.
🐛 Proposed fix using legal mutable access
- char *p = const_cast<char*>(ret.c_str());
+ char *p = &ret[0]; // Legal mutable access in C++11 and laterAlternatively in C++17+: char *p = ret.data();
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| char *p = const_cast<char*>(ret.c_str()); | |
| char *p = &ret[0]; // Legal mutable access in C++11 and later |
🤖 Prompt for AI Agents
In `@ixwebsocket/IXBase64.h` at line 52, The code currently obtains a writable
pointer via const_cast<char*>(ret.c_str()) which is undefined behavior; change
the pointer acquisition to use a legal mutable buffer (e.g., use ret.data() in
C++17+ or ensure a mutable std::string copy) so that the pointer `p` points to a
modifiable buffer; update the reference where `p` is used accordingly (look for
`char *p = const_cast<char*>(ret.c_str())`, variable `p`, and the string `ret`
in IXBase64.h and replace with a safe approach such as `char *p = ret.data()` or
create a mutable copy before writing).
| for (i = 0; i < in_len - 2; i += 3) { | ||
| *p++ = sEncodingTable[(data[i] >> 2) & 0x3F]; | ||
| *p++ = sEncodingTable[((data[i] & 0x3) << 4) | ((int) (data[i + 1] & 0xF0) >> 4)]; | ||
| *p++ = sEncodingTable[((data[i + 1] & 0xF) << 2) | ((int) (data[i + 2] & 0xC0) >> 6)]; | ||
| *p++ = sEncodingTable[data[i + 2] & 0x3F]; | ||
| } |
There was a problem hiding this comment.
Integer underflow causes out-of-bounds access on small inputs.
The loop condition i < in_len - 2 uses unsigned size_t arithmetic. When in_len is 0, 1, or 2, the expression in_len - 2 underflows to a massive value (~2^64), causing the loop to execute and access data[i] out of bounds.
🐛 Proposed fix to prevent underflow
- for (i = 0; i < in_len - 2; i += 3) {
+ for (i = 0; in_len >= 3 && i < in_len - 2; i += 3) {Or restructure to check in_len before entering the loop.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (i = 0; i < in_len - 2; i += 3) { | |
| *p++ = sEncodingTable[(data[i] >> 2) & 0x3F]; | |
| *p++ = sEncodingTable[((data[i] & 0x3) << 4) | ((int) (data[i + 1] & 0xF0) >> 4)]; | |
| *p++ = sEncodingTable[((data[i + 1] & 0xF) << 2) | ((int) (data[i + 2] & 0xC0) >> 6)]; | |
| *p++ = sEncodingTable[data[i + 2] & 0x3F]; | |
| } | |
| for (i = 0; in_len >= 3 && i < in_len - 2; i += 3) { | |
| *p++ = sEncodingTable[(data[i] >> 2) & 0x3F]; | |
| *p++ = sEncodingTable[((data[i] & 0x3) << 4) | ((int) (data[i + 1] & 0xF0) >> 4)]; | |
| *p++ = sEncodingTable[((data[i + 1] & 0xF) << 2) | ((int) (data[i + 2] & 0xC0) >> 6)]; | |
| *p++ = sEncodingTable[data[i + 2] & 0x3F]; | |
| } |
🤖 Prompt for AI Agents
In `@ixwebsocket/IXBase64.h` around lines 54 - 59, The for-loop in the Base64
encoder (the block iterating with i and using data, in_len, and sEncodingTable)
can underflow because it uses the unsigned expression "in_len - 2"; change the
loop guard to a safe check such as testing in_len before the loop or using an
index comparison that cannot underflow (e.g., use "i + 2 < in_len" or
early-return/skip when in_len < 3) so the body that reads data[i], data[i+1],
data[i+2] cannot run on inputs of length 0, 1, or 2.
| size_t in_len = input.size(); | ||
| if (in_len % 4 != 0) return "Input data size is not a multiple of 4"; | ||
|
|
||
| size_t out_len = in_len / 4 * 3; | ||
| if (input[in_len - 1] == '=') out_len--; | ||
| if (input[in_len - 2] == '=') out_len--; |
There was a problem hiding this comment.
Out-of-bounds read on empty input.
When in_len == 0, the check in_len % 4 != 0 passes (0 % 4 == 0), but lines 100-101 access input[in_len - 1] and input[in_len - 2], which resolve to input[-1] and input[-2] — undefined behavior and memory corruption.
🐛 Proposed fix to handle empty input
size_t in_len = input.size();
+ if (in_len == 0) {
+ out.clear();
+ return "";
+ }
if (in_len % 4 != 0) return "Input data size is not a multiple of 4";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| size_t in_len = input.size(); | |
| if (in_len % 4 != 0) return "Input data size is not a multiple of 4"; | |
| size_t out_len = in_len / 4 * 3; | |
| if (input[in_len - 1] == '=') out_len--; | |
| if (input[in_len - 2] == '=') out_len--; | |
| size_t in_len = input.size(); | |
| if (in_len == 0) { | |
| out.clear(); | |
| return ""; | |
| } | |
| if (in_len % 4 != 0) return "Input data size is not a multiple of 4"; | |
| size_t out_len = in_len / 4 * 3; | |
| if (input[in_len - 1] == '=') out_len--; | |
| if (input[in_len - 2] == '=') out_len--; |
🤖 Prompt for AI Agents
In `@ixwebsocket/IXBase64.h` around lines 96 - 101, The code reads input[in_len -
1] and input[in_len - 2] without checking for empty input, causing OOB when
in_len == 0; update the start of the block in IXBase64.h (variables input,
in_len, out_len) to handle empty input early—e.g., if in_len == 0 return an
empty output (or otherwise handle as a special case) before computing out_len or
accessing input[in_len - 1/ -2]; alternatively ensure you only access those
indices when in_len >= 2 and adjust out_len logic accordingly.
| uint32_t a = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<int>(input[i++])]; | ||
| uint32_t b = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<int>(input[i++])]; | ||
| uint32_t c = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<int>(input[i++])]; | ||
| uint32_t d = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<int>(input[i++])]; |
There was a problem hiding this comment.
Signed char causes negative array index — out-of-bounds access.
static_cast<int>(input[i++]) preserves the sign of char on platforms where char is signed. If input contains bytes with the high bit set (values 128-255), the cast produces negative integers, causing kDecodingTable[negative_index] to read before the array — undefined behavior.
🐛 Proposed fix to ensure unsigned indexing
- uint32_t a = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<int>(input[i++])];
- uint32_t b = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<int>(input[i++])];
- uint32_t c = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<int>(input[i++])];
- uint32_t d = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<int>(input[i++])];
+ uint32_t a = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<unsigned char>(input[i++])];
+ uint32_t b = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<unsigned char>(input[i++])];
+ uint32_t c = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<unsigned char>(input[i++])];
+ uint32_t d = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<unsigned char>(input[i++])];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint32_t a = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<int>(input[i++])]; | |
| uint32_t b = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<int>(input[i++])]; | |
| uint32_t c = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<int>(input[i++])]; | |
| uint32_t d = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<int>(input[i++])]; | |
| uint32_t a = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<unsigned char>(input[i++])]; | |
| uint32_t b = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<unsigned char>(input[i++])]; | |
| uint32_t c = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<unsigned char>(input[i++])]; | |
| uint32_t d = input[i] == '=' ? 0 & i++ : kDecodingTable[static_cast<unsigned char>(input[i++])]; |
🤖 Prompt for AI Agents
In `@ixwebsocket/IXBase64.h` around lines 106 - 109, The code in IXBase64.h reads
kDecodingTable using static_cast<int>(input[i++]) which on platforms with signed
char can produce negative indices and OOB access; update the indexing to first
convert the input byte to an unsigned type before casting to int (e.g. use
static_cast<int>(static_cast<unsigned char>(input[i++])) or cast to uint8_t then
to int) for the four reads that assign a, b, c, d so bytes with high bit set
index kDecodingTable safely while preserving the existing '=' handling logic.
| std::string uri(request->uri); | ||
| if (uri.empty() || uri == "/") | ||
| { | ||
| uri = "/index.html"; | ||
| } | ||
|
|
||
| WebSocketHttpHeaders headers; | ||
| headers["Server"] = userAgent(); | ||
| headers["Content-Type"] = response_head_file(uri); | ||
|
|
||
| std::string path("." + uri); | ||
| auto res = readAsString(path); |
There was a problem hiding this comment.
Path traversal vulnerability allows accessing files outside web root.
The URI is directly concatenated to form a file path without sanitization. An attacker can use /../ sequences to escape the current directory and read arbitrary files:
- Request:
GET /../../../etc/passwd HTTP/1.1 - Constructed path:
./../../../etc/passwd
This is a critical security issue that enables arbitrary file reads from the server filesystem.
🛡️ Proposed fix to sanitize path
+ // Sanitize URI to prevent path traversal
+ std::string sanitizedUri = uri;
+
+ // Reject URIs with path traversal sequences
+ if (sanitizedUri.find("..") != std::string::npos ||
+ sanitizedUri.find("//") != std::string::npos)
+ {
+ return std::make_shared<HttpResponse>(
+ 403, "Forbidden", HttpErrorCode::Ok, WebSocketHttpHeaders(), std::string());
+ }
+
- std::string path("." + uri);
+ std::string path("." + sanitizedUri);Consider also resolving the path canonically and verifying it remains within the intended root directory.
🤖 Prompt for AI Agents
In `@ixwebsocket/IXHttpServer.cpp` around lines 123 - 134, The code builds a file
path by concatenating "." + uri which allows path traversal; sanitize and
canonicalize the URI before calling readAsString. Specifically: strip any
query/fragment from request->uri, percent-decode it, reject or normalize
segments like ".." and empty segments (or collapse them), then join it against a
fixed web root (e.g., webRoot + sanitizedUri) using std::filesystem::path and
obtain a canonical/resolved path (std::filesystem::canonical or equivalent) and
verify the resulting path has the web root as its prefix; only then pass that
canonical path to readAsString(uriPath). Update the logic around variables uri,
path, and the call to readAsString to use the validated canonical path and keep
response_head_file(userAgent()/headers) usage unchanged.
| Uuid::Uuid() | ||
| { | ||
| static std::random_device rd; | ||
| static std::uniform_int_distribution<uint64_t> dist(0, (uint64_t)(~0)); | ||
|
|
||
| _ab = dist(rd); | ||
| _cd = dist(rd); | ||
|
|
||
| _ab = (_ab & 0xFFFFFFFFFFFF0FFFULL) | 0x0000000000004000ULL; | ||
| _cd = (_cd & 0x3FFFFFFFFFFFFFFFULL) | 0x8000000000000000ULL; | ||
| } |
There was a problem hiding this comment.
Thread safety: Data race on static random_device and distribution.
The static local variables rd and dist are shared across all threads calling uuid4(). Neither std::random_device nor std::uniform_int_distribution is guaranteed to be thread-safe by the C++ standard, creating a data race when multiple threads generate UUIDs concurrently.
Given the PR's focus on thread safety and race conditions, consider one of these fixes:
- Add a mutex to protect access to the static RNG state.
- Use thread-local storage (
thread_local) to give each thread its own RNG instance.
🔒 Proposed fix using thread_local
Uuid::Uuid()
{
- static std::random_device rd;
- static std::uniform_int_distribution<uint64_t> dist(0, (uint64_t)(~0));
+ thread_local std::random_device rd;
+ thread_local std::uniform_int_distribution<uint64_t> dist(0, (uint64_t)(~0));
_ab = dist(rd);
_cd = dist(rd);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Uuid::Uuid() | |
| { | |
| static std::random_device rd; | |
| static std::uniform_int_distribution<uint64_t> dist(0, (uint64_t)(~0)); | |
| _ab = dist(rd); | |
| _cd = dist(rd); | |
| _ab = (_ab & 0xFFFFFFFFFFFF0FFFULL) | 0x0000000000004000ULL; | |
| _cd = (_cd & 0x3FFFFFFFFFFFFFFFULL) | 0x8000000000000000ULL; | |
| } | |
| Uuid::Uuid() | |
| { | |
| thread_local std::random_device rd; | |
| thread_local std::uniform_int_distribution<uint64_t> dist(0, (uint64_t)(~0)); | |
| _ab = dist(rd); | |
| _cd = dist(rd); | |
| _ab = (_ab & 0xFFFFFFFFFFFF0FFFULL) | 0x0000000000004000ULL; | |
| _cd = (_cd & 0x3FFFFFFFFFFFFFFFULL) | 0x8000000000000000ULL; | |
| } |
🤖 Prompt for AI Agents
In `@ixwebsocket/IXUuid.cpp` around lines 39 - 49, The constructor Uuid::Uuid()
currently uses static local rd and dist which causes a data race; make them
thread-local instead (declare rd and dist as thread_local) so each thread has
its own std::random_device and std::uniform_int_distribution instance, then keep
the same calls to dist(rd) and the existing bit-fiddling that sets _ab and _cd;
ensure you only change the storage specifier for rd and dist (or alternatively
protect accesses with a mutex) so uuid generation is safe under concurrent calls
to Uuid::Uuid().
| struct WebSocketMessage | ||
| { | ||
| WebSocketMessageType type; | ||
| const std::string& str; | ||
| size_t wireSize; | ||
| WebSocketErrorInfo errorInfo; | ||
| WebSocketOpenInfo openInfo; | ||
| WebSocketCloseInfo closeInfo; | ||
| bool binary; | ||
|
|
||
| WebSocketMessage(WebSocketMessageType t, | ||
| const std::string& s, | ||
| size_t w, | ||
| WebSocketErrorInfo e, | ||
| WebSocketOpenInfo o, | ||
| WebSocketCloseInfo c, | ||
| bool b = false) | ||
| : type(t) | ||
| , str(s) | ||
| , wireSize(w) | ||
| , errorInfo(e) | ||
| , openInfo(o) | ||
| , closeInfo(c) | ||
| , binary(b) | ||
| { | ||
| ; | ||
| } | ||
|
|
||
| /** | ||
| * @brief Deleted overload to prevent binding `str` to a temporary, which would cause | ||
| * undefined behavior since class members don't extend lifetime beyond the constructor call. | ||
| */ | ||
| WebSocketMessage(WebSocketMessageType t, | ||
| std::string&& s, | ||
| size_t w, | ||
| WebSocketErrorInfo e, | ||
| WebSocketOpenInfo o, | ||
| WebSocketCloseInfo c, | ||
| bool b = false) = delete; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all WebSocketMessage construction sites
rg -n "WebSocketMessage\s*\(" -A 3 -B 1Repository: RahulRaj272727/IXWebSocket
Length of output: 796
🏁 Script executed:
# Also search for places where WebSocketMessage is instantiated or created
rg -n "WebSocketMessage\(" -A 2Repository: RahulRaj272727/IXWebSocket
Length of output: 545
🏁 Script executed:
# Look at the header file to understand the full context
head -100 ixwebsocket/IXWebSocketMessage.hRepository: RahulRaj272727/IXWebSocket
Length of output: 1812
🏁 Script executed:
# Search for WebSocketMessage instantiation in .cpp files
rg -t cpp "WebSocketMessage\s*\{" -A 5 -B 2Repository: RahulRaj272727/IXWebSocket
Length of output: 52
🏁 Script executed:
# Also search for make_unique or other construction patterns
rg -t cpp "WebSocketMessage" --max-count 20 -B 1 -A 2 | head -80Repository: RahulRaj272727/IXWebSocket
Length of output: 4697
🏁 Script executed:
# Find all .cpp files in the repository
find . -name "*.cpp" -type f | head -20Repository: RahulRaj272727/IXWebSocket
Length of output: 672
🏁 Script executed:
# Find where emptyMsg is defined and used
rg -n "emptyMsg" ixwebsocket/IXWebSocket.cpp -B 5 -A 2Repository: RahulRaj272727/IXWebSocket
Length of output: 1736
🏁 Script executed:
# Look at the specific construction context in IXWebSocket.cpp
rg -n "make_unique<WebSocketMessage>" ixwebsocket/IXWebSocket.cpp -B 8 -A 10 | head -80Repository: RahulRaj272727/IXWebSocket
Length of output: 3336
🏁 Script executed:
# Find where msg is defined and used in WebSocketMessage construction
rg -n "ix::make_unique<WebSocketMessage>.*msg" ixwebsocket/IXWebSocket.cpp -B 15 -A 3 | head -100Repository: RahulRaj272727/IXWebSocket
Length of output: 52
🏁 Script executed:
# Also look for the broader context of msg variable definition
rg -n "std::string.*msg" ixwebsocket/IXWebSocket.cpp -B 2 -A 2 | head -80Repository: RahulRaj272727/IXWebSocket
Length of output: 278
🏁 Script executed:
# Get the full context of where msg is used in WebSocketMessage construction
rg -n "dispatch" ixwebsocket/IXWebSocket.cpp -B 3 -A 30 | head -120Repository: RahulRaj272727/IXWebSocket
Length of output: 1626
🏁 Script executed:
# Search for the actual WebSocketMessage construction with msg parameter
rg -n "WebSocketMessage.*msg" ixwebsocket/IXWebSocket.cpp -B 10 -A 5Repository: RahulRaj272727/IXWebSocket
Length of output: 52
🏁 Script executed:
# Get more of the dispatch lambda to see where WebSocketMessage is created with msg
rg -n "dispatch" ixwebsocket/IXWebSocket.cpp -A 50 | grep -A 15 "make_unique<WebSocketMessage>"Repository: RahulRaj272727/IXWebSocket
Length of output: 828
🏁 Script executed:
# Look for all WebSocketMessage constructions in IXWebSocket.cpp
rg -n "make_unique<WebSocketMessage>" ixwebsocket/IXWebSocket.cpp -A 8Repository: RahulRaj272727/IXWebSocket
Length of output: 2754
🏁 Script executed:
# Find the dispatch function implementation
rg -n "void.*dispatch\s*\(" ixwebsocket/ -A 20 | head -80Repository: RahulRaj272727/IXWebSocket
Length of output: 3430
🏁 Script executed:
# Find the WebSocketTransport class to see dispatch signature
rg -n "dispatch\s*\(" ixwebsocket/*.h -A 3Repository: RahulRaj272727/IXWebSocket
Length of output: 393
🏁 Script executed:
# Get more of the dispatch implementation to see what msg refers to
sed -n '461,700p' ixwebsocket/IXWebSocketTransport.cpp | head -150Repository: RahulRaj272727/IXWebSocket
Length of output: 6096
🏁 Script executed:
# Search for where onMessageCallback is invoked in dispatch
rg -n "onMessageCallback" ixwebsocket/IXWebSocketTransport.cpp -B 2 -A 2Repository: RahulRaj272727/IXWebSocket
Length of output: 2005
🏁 Script executed:
# Find the _onMessageCallback definition and usage
rg -n "_onMessageCallback" ixwebsocket/IXWebSocket.h -B 2 -A 5Repository: RahulRaj272727/IXWebSocket
Length of output: 390
🏁 Script executed:
# Look for setOnMessageCallback to understand what user callbacks might do
rg -n "setOnMessageCallback\|onMessageCallback" ixwebsocket/IXWebSocket.h -B 1 -A 3Repository: RahulRaj272727/IXWebSocket
Length of output: 52
🏁 Script executed:
# Find the OnMessageCallback type definition
rg -n "OnMessageCallback" ixwebsocket/ --type-listRepository: RahulRaj272727/IXWebSocket
Length of output: 459
🏁 Script executed:
# Find where OnMessageCallback is defined
rg -n "using OnMessageCallback\|typedef.*OnMessageCallback" ixwebsocket/Repository: RahulRaj272727/IXWebSocket
Length of output: 52
🏁 Script executed:
# Check IXWebSocket.h for callback registration
rg -n "setOnMessage" ixwebsocket/IXWebSocket.h -A 5Repository: RahulRaj272727/IXWebSocket
Length of output: 397
🏁 Script executed:
# Find OnMessageCallback typedef
rg -n "OnMessageCallback" ixwebsocket/*.h | grep -E "using|typedef"Repository: RahulRaj272727/IXWebSocket
Length of output: 249
🏁 Script executed:
# Also check IXWebSocket.h directly for callback types
head -80 ixwebsocket/IXWebSocket.h | tail -40Repository: RahulRaj272727/IXWebSocket
Length of output: 1649
🏁 Script executed:
# Look at the OnMessageCallback signature in IXWebSocketTransport.h
sed -n '70,75p' ixwebsocket/IXWebSocketTransport.hRepository: RahulRaj272727/IXWebSocket
Length of output: 342
🏁 Script executed:
# Search for examples of how the callback is used in tests or examples
rg -n "setOnMessageCallback" examples/ -A 5 | head -40Repository: RahulRaj272727/IXWebSocket
Length of output: 52
Dangling reference in WebSocketMessage::str due to unsafe reference storage.
The const std::string& str member stores a reference to a temporary/local string created in WebSocketTransport::dispatch(). The deleted rvalue overload cannot prevent this—const std::string& still binds to temporaries. When WebSocketMessage objects (via unique_ptr) are passed to user callbacks, the reference may dangle if the callback defers processing. Change to owning storage: std::string str and use std::move() in the constructor initializer list.
🤖 Prompt for AI Agents
In `@ixwebsocket/IXWebSocketMessage.h` around lines 18 - 56, The WebSocketMessage
struct currently stores a const std::string& (member str) which can dangle;
change the member to own the string (std::string str) and update the
constructor(s) to accept an owning parameter (either std::string by value or
std::string&&) and initialize the member with std::move(s) in the initializer
list; remove the deleted rvalue overload since temporaries will now be safely
moved into the owned member. Ensure you update all references to the constructor
signature used by WebSocketTransport::dispatch() so it passes/moves the string
appropriately.
| bool WebSocketPerMessageDeflate::decompress(const std::string& in, std::string& out) | ||
| { | ||
| return _decompressor->decompress(in, out); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg "class WebSocketPerMessageDeflateDecompressor" -A 50 --type cpp --type hRepository: RahulRaj272727/IXWebSocket
Length of output: 3090
🏁 Script executed:
rg "decompress" -B 5 -A 30 "WebSocketPerMessageDeflateDecompressor" --type cpp --type hRepository: RahulRaj272727/IXWebSocket
Length of output: 148
🏁 Script executed:
grep -n "bool WebSocketPerMessageDeflateDecompressor::decompress" -A 50 ixwebsocket/IXWebSocketPerMessageDeflateCodec.cppRepository: RahulRaj272727/IXWebSocket
Length of output: 1647
🏁 Script executed:
grep -n "WebSocketPerMessageDeflateOptions" -B 5 -A 20 ixwebsocket/IXWebSocketPerMessageDeflate.hRepository: RahulRaj272727/IXWebSocket
Length of output: 1063
🏁 Script executed:
grep -r "class WebSocketPerMessageDeflateOptions" -A 30 --type h --type cppRepository: RahulRaj272727/IXWebSocket
Length of output: 187
🏁 Script executed:
grep -r "maxMessageSize\|max_decompressed\|decompress_size" --type h --type cppRepository: RahulRaj272727/IXWebSocket
Length of output: 187
🏁 Script executed:
rg "class WebSocketPerMessageDeflateOptions" -A 30 --type hRepository: RahulRaj272727/IXWebSocket
Length of output: 4582
🏁 Script executed:
rg "maxDecompressedSize\|maxMessageSize\|max_decompressed" --type h --type cppRepository: RahulRaj272727/IXWebSocket
Length of output: 52
🏁 Script executed:
rg "\.decompress\(" -B 5 -A 10 --type cppRepository: RahulRaj272727/IXWebSocket
Length of output: 52
🏁 Script executed:
rg "decompress\(" -B 3 -A 3 --type cpp | head -100Repository: RahulRaj272727/IXWebSocket
Length of output: 3064
🏁 Script executed:
grep -n "decompress(message, _decompressedMessage)" -B 10 -A 15 ixwebsocket/IXWebSocketTransport.cppRepository: RahulRaj272727/IXWebSocket
Length of output: 1285
🏁 Script executed:
rg "maxMessageSize\|max_size\|message.*size.*limit" --type cpp --type hRepository: RahulRaj272727/IXWebSocket
Length of output: 52
🏁 Script executed:
rg "_decompressedMessage" --type cpp | head -20Repository: RahulRaj272727/IXWebSocket
Length of output: 530
🏁 Script executed:
rg "size()" --type cpp --type h | grep -i "decompress\|message" | head -20Repository: RahulRaj272727/IXWebSocket
Length of output: 1967
Decompression lacks size limits and is vulnerable to compression bomb attacks.
The decompress() method in WebSocketPerMessageDeflateDecompressor repeatedly calls inflate() and appends to the output string with no maximum size enforcement. The WebSocketPerMessageDeflateOptions provides no configurable decompression size limit, and at the call site in IXWebSocketTransport::emitMessage(), the decompressed output is only validated for UTF-8 correctness—not size. This allows a small compressed payload to expand into arbitrarily large memory allocations, enabling denial-of-service attacks through compression bombs.
Add a configurable maximum decompressed message size limit to WebSocketPerMessageDeflateOptions and enforce it within the decompression loop to reject payloads that exceed the threshold.
🤖 Prompt for AI Agents
In `@ixwebsocket/IXWebSocketPerMessageDeflate.cpp` around lines 93 - 96, Add a
configurable max-decompressed-size to WebSocketPerMessageDeflateOptions and
enforce it during decompression: extend WebSocketPerMessageDeflateOptions with a
uint64_t maxDecompressedSize (default reasonable value or 0==unlimited), pass
that option into WebSocketPerMessageDeflateDecompressor, and inside
WebSocketPerMessageDeflateDecompressor::decompress (the loop that calls
inflate() and appends to the output) check the cumulative output size after each
inflate chunk and abort/return false if it exceeds maxDecompressedSize; also
ensure WebSocketPerMessageDeflate::decompress forwards failures and that callers
(e.g., IXWebSocketTransport::emitMessage) treat a false return as a
decompression error.
| unsigned WebSocketTransport::getRandomUnsigned() | ||
| { | ||
| auto now = std::chrono::system_clock::now(); | ||
| auto seconds = | ||
| std::chrono::duration_cast<std::chrono::seconds>(now.time_since_epoch()).count(); | ||
| return static_cast<unsigned>(seconds); | ||
| } |
There was a problem hiding this comment.
Critical: Weak RNG for WebSocket masking keys.
getRandomUnsigned() returns seconds since epoch, which is highly predictable. RFC 6455 Section 5.3 requires that "the masking key needs to be unpredictable" to prevent attackers from predicting the masked payload. Using system time violates this requirement and could enable cache-poisoning or other attacks through intermediary proxies.
Replace with a cryptographically secure random number generator:
🔒 Proposed fix using platform-appropriate CSPRNG
- unsigned WebSocketTransport::getRandomUnsigned()
- {
- auto now = std::chrono::system_clock::now();
- auto seconds =
- std::chrono::duration_cast<std::chrono::seconds>(now.time_since_epoch()).count();
- return static_cast<unsigned>(seconds);
- }
+ unsigned WebSocketTransport::getRandomUnsigned()
+ {
+ std::random_device rd;
+ return rd();
+ }Note: Include <random> at the top of the file. If std::random_device entropy quality is a concern on certain platforms, consider using OpenSSL's RAND_bytes() or platform-specific APIs like getrandom() / BCryptGenRandom().
🤖 Prompt for AI Agents
In `@ixwebsocket/IXWebSocketTransport.cpp` around lines 824 - 830, The
getRandomUnsigned() implementation uses predictable epoch seconds; replace it
with a cryptographically secure RNG in WebSocketTransport::getRandomUnsigned
(e.g., use std::random_device + std::uniform_int_distribution<unsigned> or call
a platform CSPRNG like getrandom()/BCryptGenRandom()/OpenSSL RAND_bytes() if
stronger entropy is needed), add the appropriate include (<random> or platform
headers), and ensure the returned unsigned is derived from securely generated
bytes rather than time.
| uint64_t message_size, | ||
| uint8_t masking_key[4]); | ||
|
|
||
| unsigned getRandomUnsigned(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the implementation of getRandomUnsigned() for RNG quality
ast-grep --pattern 'unsigned $CLASS::getRandomUnsigned() {
$$$
}'
# Also search for random-related code in the transport implementation
rg -n "random|mt19937|random_device" --type=cpp -g "IXWebSocketTransport.cpp"Repository: RahulRaj272727/IXWebSocket
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Find files containing getRandomUnsigned
rg -l "getRandomUnsigned" --type=cpp
# Search for the function definition
rg -n "getRandomUnsigned" --type=cpp -A 10
# Look for random/RNG usage in the codebase
rg -n "random|rand|mt19937|random_device" --type=cpp | head -30Repository: RahulRaj272727/IXWebSocket
Length of output: 4141
🏁 Script executed:
#!/bin/bash
# Get more context on how getRandomUnsigned is used
rg -n "getRandomUnsigned" --type=cpp -B 5 -A 5
# Check RFC 6455 WebSocket masking requirements in any comments
rg -n "RFC|6455|masking|mask" --type=cpp | head -20Repository: RahulRaj272727/IXWebSocket
Length of output: 4346
Use cryptographically secure random number generation for WebSocket frame masking keys.
The getRandomUnsigned() implementation (IXWebSocketTransport.cpp:824-830) uses std::chrono::system_clock::now() truncated to seconds as the masking key source. This is predictable and violates RFC 6455, which requires strong randomness for client masking keys. Replace with a proper CSPRNG like std::random_device combined with std::mt19937, similar to the pattern already used elsewhere in the codebase (e.g., IXHttpClient.cpp:778-779).
🤖 Prompt for AI Agents
In `@ixwebsocket/IXWebSocketTransport.h` at line 283, The getRandomUnsigned()
implementation currently uses predictable time-based bits for masking keys;
replace its logic with a cryptographically secure PRNG: initialize a static
std::mt19937 (or std::mt19937_64) seeded from std::random_device and use a
std::uniform_int_distribution<uint32_t> to produce the unsigned masking key in
getRandomUnsigned(); ensure the engine is static/thread-safe (or protected) and
that getRandomUnsigned() returns the uniformly distributed uint32_t value for
WebSocket frame masking keys instead of time-derived values.
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
1 participant
Purpose
This PR contains ONLY the core IXWebSocket library files (ixwebsocket/ directory) for security review by CodeRabbit AI.
File Count: 92 files (under 100 limit)
Review Scope
Security Focus Areas
@coderabbitai full review
Please audit for:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.