Bound POSIX SHM mapping by the shared object size#479
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR hardens the POSIX shared-memory transport against malicious or malformed SHM headers by bounding mappings to the actual SHM object size, snapshotting and validating peer-writable header fields once, and enforcing alignment/size rules for request/response buffers. It also adds regression tests that squat the SHM name and validate both client-side rejection and server-side fail-fast behavior.
Changes:
- Derive
mmaplength fromfstat()object size and validate a single snapshot of header sizes before computing buffer offsets. - Enforce “non-zero multiple of
sizeof(whTransportMemCsr)” for request/response buffer sizes on both client and server creation paths. - Add negative tests for malformed SHM headers and for server misconfiguration leaving no stale SHM objects behind.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/wh_test_comm.c | Adds negative SHM layout/config tests by squatting the SHM name and writing malformed headers. |
| port/posix/posix_transport_shm.h | Documents the new req/resp buffer sizing/alignment rule and expected error behavior. |
| port/posix/posix_transport_shm.c | Maps SHM based on fstat() size, snapshots + validates header fields once, and uses validated sizes throughout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5a89810 to
637390e
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #479
Scan targets checked: wolfhsm-core-bugs, wolfhsm-src
No new issues found in the changed files. ✅
There was a problem hiding this comment.
The PR improves the POSIX SHM robustness.
What I see problematic is that it might hint that the POSIX SHM can provide a true isolation boundary between the client and the server.
Unfortunately there are plenty of other attack vectors for the client over POSIX SHM (both peers typically run with same UID, a malicious client can still ftruncate the file and SIGBUS the server, and an attacker can race to create a same-name object and interpose between client and server).
Moreover, I don't think POSIX SHM was ever meant to be a secure transport protocol but merely used for test, debug and as a reference.
I'm OK with the PR but this is the right moment to document the threat model explicitly in posix_transport_shm.h: same trust domain assumed; no peer authentication or isolation;
| /* Configure the underlying transport context from the validated | ||
| * sizes, not from the header the peer can still rewrite */ |
There was a problem hiding this comment.
Dropped, it's reverted to the original line.
| /* Configure the underlying transport context from the validated | ||
| * sizes, not from the header the peer can still rewrite */ |
637390e to
f9835a9
Compare
|
Hi @rizlik , Added an explicit Threat model section to the top of posix_transport_shm.h stating: same trust domain assumed (same uid, peer contents are trusted input), no peer authentication (whoever opens the name is the peer, and the client unlinks on successful mapping so the name can be recreated by anyone afterwards), and no isolation or availability guarantee (ftruncate(), or simply never advancing the CSRs). It closes by scoping what this PR actually buys: the header fields are validated against the real object size so a malformed or truncated object can't drive an out-of-bounds access in this process — robustness against a corrupt object, not protection against a hostile peer. Explicitly not a promotion to a trusted channel. |
Problem
The POSIX shared-memory transport built its
mmaplayout from header fields that live in the shared object itself, so a peer could set them freely. A local process that wins the race to create the named object (mode0660, same uid/gid) could:size = sizeof(header) + req_size + resp_size + dma_sizehad no overflow check and was never compared against the object's real size. The sum could wrap to a small value;Mapthen mapped that small region but derivedresp/dmafrom the raw fields, placing them outside the mapping, and propagated a near-SIZE_MAXdma_sizethat defeated the client DMA bounds check.req_size = resp_size = 65535on a 64-byte object gives a non-wrapping size past EOF; touching the last page is SIGBUS.Addressed by f_4223.
Fix (
port/posix/posix_transport_shm.c)fstatis the authoritative bound.posixTransportShm_Mapderives the mapping length from the object size, not the header.req_size/resp_size/dma_sizeare read once into locals, checked (sizeof(header) + req + resp <= st_size,dma_size <= remainder, overflow-free), and used only from those locals.HandleMapandServerInitconsume the validatedmap->req_size/map->resp_sizeinstead of re-reading the header.sizeof(whTransportMemCsr). A buffer under one word underflowed the length check inwh_TransportMem_SendRequest(req_sizeisuint16_t, soreq_size - sizeof(csr)wrapped nearSIZE_MAX), leaving an out-of-bounds write reachable; a non-multiple misaligned the response/DMA pointers cast tovolatile uint64_t.CreateMapapplies the same rule to the config beforeshm_open, so a misconfigured server returnsWH_ERROR_BADARGSinstead of leaving a stale object every client would reject; it also unlinks if the post-create map fails. Documented onposixTransportShmConfigin the header.Tests (
test/wh_test_comm.c)New single-process negative tests under
whTest_Comm._whTest_CommShmMalformedHeadersquats the name and presents 8 malformed layouts plus one exactly-fitting accept case;_whTest_CommShmServerBadConfigdrivesServerInitwith a bad size and asserts no object is left linked. Each clause of the predicate and both sides of the DMA bound have coverage.Verification
-std=c90 -Werror -Wall -Wextra -Wpointer-arith.test-refactor(37 passed, 0 failed) pass; POSIX example pair connects and runs its full crypto sequence.>→>=or>→>+1, drifting the test's header mirror, or removing the server config guard each makes the suite fail — the guards are load-bearing, not decorative.