From 10d2a61fe1c848f44033e8450ff3a5ffa7f4322a Mon Sep 17 00:00:00 2001 From: Stefan Boberg Date: Mon, 4 May 2026 16:46:03 +0200 Subject: zenhttp improvements (robustness / correctness) (#968) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A collection of security, correctness, and robustness fixes in `zenhttp` and `zencore` surfaced by security review. Most items are small, independent commits grouped here because they all tighten trust boundaries or fix UB along the same code paths. ## WebSocket protocol hardening (RFC 6455) - **Enforce the client-side mask bit**. Server-side frame loops now reject unmasked frames with close code 1002 per §5.1. Prevents HTTP intermediary smuggling. - **Validate control frames and RSV bits**. Fragmented control frames, oversized (>125 B) control payloads, and any non-zero RSV bit now fail the connection before allocation. - **Lower per-frame payload cap** from 256 MB → 4 MB. Bounds per-connection accumulator memory. - **Implement message fragmentation**. Continuation frames are coalesced and delivered as a single message; interleaved non-control frames close with 1002; assembled messages are capped at 4 MB (1009 on overflow). Previously partial fragments were delivered to handlers, bypassing payload validation. - **Parse the 101 handshake response properly** in `HttpWsClient`. Status-line, `Upgrade`, `Connection`, and `Sec-WebSocket-Accept` are now matched exactly rather than via substring searches against the full body. ## Auth / OIDC hardening - **Constant-time password compare** in `PasswordSecurity::IsAllowed` (closes a remote length/content timing oracle). Adds a shared `ConstantTimeEquals` helper. - **Harden Basic-auth header parsing**: trim trailing LWS, reject control bytes and DEL in the credential. - **OIDC discovery pinning**: require HTTPS (loopback exempt), verify `issuer` matches `BaseUrl`, require `token_endpoint` / `userinfo_endpoint` / `jwks_uri` to share origin with `BaseUrl`, reject empty `token_endpoint`. - **Restrict `POST /auth/oidc/refreshtoken`** to local-machine requests. Previously unauthenticated in default deployments — remote callers could evict or replace cached tokens. - **Stop logging OIDC provider response bodies** on refresh failure (IdPs echo `refresh_token` back in error bodies). - **Drop the unused `IdentityToken` field** from `OidcClient` / `OpenIdToken` so nothing in the tree accidentally trusts an unverified JWT. ## Auth state encryption migration - Add `AesGcm` AEAD primitive (BCrypt / OpenSSL backends, mbedTLS stubbed) and `CryptoRandom::Fill` CSPRNG helper in `zencore/crypto.h`. - Migrate authstate file from AES-256-CBC with a fixed IV to AES-GCM with a fresh 12-byte random nonce per write and the 4-byte `ZEN1` magic bound as AAD. Legacy-CBC files are transparently read once and rewritten in the new format. ## Filesystem / IO robustness - `IoBufferExtendedCore::Materialize` now checks `MAP_FAILED` on POSIX (was comparing to `nullptr`, which let the failure sentinel propagate into later reads and `munmap(MAP_FAILED, ...)`). - `IoBufferBuilder::MakeFromFile / MakeFromTemporaryFile`: close the FD/HANDLE on exception via a dismissable `ScopeGuard`; actually check the `fstat()` return value (previously used an uninitialized `FileSize`). - `ReadFromFileMaybe`: loop short reads, retry `EINTR`, chunk Windows `ReadFile` at `0xFFFFFFFF` bytes (fixes silent truncation of multi-GiB reads). - `WipeDirectory`: compare `FindFirstFileW` handle against `INVALID_HANDLE_VALUE` rather than `nullptr`. - `RemoveFileNative` (Linux/macOS): report non-`ENOENT` stat failures via the `std::error_code` out-param and stop reading `st_mode` after a failed stat. ## Buffer / compression correctness - Avoid per-copy `IoBufferCore` heap allocations in `CompositeBuffer::CopyTo / ViewOrCopyRange` iterators; add fast path for `BufferHeader::Read` when the 64-byte header fits in the first plain-memory segment. - `BufferHeader`: add `IsHeaderValid()` gate covering `BlockSizeExponent` range, `BlockCount * BlockSize` overflow, and `TotalRawSize` bounds before any arithmetic uses them. Defends against attacker-controlled headers that can pass the CRC and trigger OOB writes in `DecompressBlock`. --- src/zenutil/process/asyncpipereader.cpp | 48 ++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 16 deletions(-) (limited to 'src/zenutil/process/asyncpipereader.cpp') diff --git a/src/zenutil/process/asyncpipereader.cpp b/src/zenutil/process/asyncpipereader.cpp index 8eac350c6..7d603aa3f 100644 --- a/src/zenutil/process/asyncpipereader.cpp +++ b/src/zenutil/process/asyncpipereader.cpp @@ -31,7 +31,7 @@ static constexpr size_t kReadBufferSize = 4096; #if !ZEN_PLATFORM_WINDOWS -struct AsyncPipeReader::Impl +struct AsyncPipeReader::Impl : public TRefCounted { asio::io_context& m_IoContext; std::unique_ptr m_Descriptor; @@ -82,22 +82,25 @@ struct AsyncPipeReader::Impl return; } - m_Descriptor->async_read_some(asio::buffer(m_Buffer), [this](const asio::error_code& Ec, size_t BytesRead) { + // Capture Self so the Impl outlives any completion the io_context has + // already picked up but not yet dispatched, even if the owning + // AsyncPipeReader is destroyed in the meantime. + m_Descriptor->async_read_some(asio::buffer(m_Buffer), [Self = Ref(this)](const asio::error_code& Ec, size_t BytesRead) { if (Ec) { - if (Ec != asio::error::operation_aborted && m_EofCallback) + if (Ec != asio::error::operation_aborted && Self->m_EofCallback) { - m_EofCallback(); + Self->m_EofCallback(); } return; } - if (BytesRead > 0 && m_DataCallback) + if (BytesRead > 0 && Self->m_DataCallback) { - m_DataCallback(std::string_view(m_Buffer.data(), BytesRead)); + Self->m_DataCallback(std::string_view(Self->m_Buffer.data(), BytesRead)); } - EnqueueRead(); + Self->EnqueueRead(); }); } }; @@ -183,7 +186,7 @@ CreateOverlappedStdoutPipe(StdoutPipeHandles& OutPipe) return true; } -struct AsyncPipeReader::Impl +struct AsyncPipeReader::Impl : public TRefCounted { asio::io_context& m_IoContext; std::unique_ptr m_StreamHandle; @@ -229,22 +232,25 @@ struct AsyncPipeReader::Impl return; } - m_StreamHandle->async_read_some(asio::buffer(m_Buffer), [this](const asio::error_code& Ec, size_t BytesRead) { + // Capture Self so the Impl outlives any completion the io_context has + // already picked up but not yet dispatched, even if the owning + // AsyncPipeReader is destroyed in the meantime. + m_StreamHandle->async_read_some(asio::buffer(m_Buffer), [Self = Ref(this)](const asio::error_code& Ec, size_t BytesRead) { if (Ec) { - if (Ec != asio::error::operation_aborted && m_EofCallback) + if (Ec != asio::error::operation_aborted && Self->m_EofCallback) { - m_EofCallback(); + Self->m_EofCallback(); } return; } - if (BytesRead > 0 && m_DataCallback) + if (BytesRead > 0 && Self->m_DataCallback) { - m_DataCallback(std::string_view(m_Buffer.data(), BytesRead)); + Self->m_DataCallback(std::string_view(Self->m_Buffer.data(), BytesRead)); } - EnqueueRead(); + Self->EnqueueRead(); }); } }; @@ -255,11 +261,21 @@ struct AsyncPipeReader::Impl // Common wrapper // ============================================================================ -AsyncPipeReader::AsyncPipeReader(asio::io_context& IoContext) : m_Impl(std::make_unique(IoContext)) +AsyncPipeReader::AsyncPipeReader(asio::io_context& IoContext) : m_Impl(new Impl(IoContext)) { } -AsyncPipeReader::~AsyncPipeReader() = default; +AsyncPipeReader::~AsyncPipeReader() +{ + // Explicitly stop pending async reads. The Impl may outlive this call if a + // completion is still in the io_context queue (the handler holds a strong + // Ref back to Impl to keep it alive). Stop() here guarantees reads stop + // even if nobody has called Stop() on the wrapper. + if (m_Impl) + { + m_Impl->Stop(); + } +} void AsyncPipeReader::Start(StdoutPipeHandles&& Pipe, std::function DataCallback, std::function EofCallback) -- cgit v1.2.3