fix(security): bounds-check untrusted shmem offsets in port and libunit

[andypost]

Summary

Audit-driven shared-memory bounds-check pass (PR-B from security-audit.md / PR #10). CVE-track — bundles five findings clustered around the "trust untrusted-input-from-shmem-peer" theme.

Findings addressed

• V5 [High] chunk_id no bounds check (port_memory.c)
• V5 [High] chunk_id + nchunks overflow (port_memory.c)
• V5 [Medium] TOCTOU on shmem mmap_id lookup (port_memory.c)
• V10 [Medium] response field count*size overflow (nxt_unit.c)
• V10 [Low] sptr offset dereferenced without bounds (nxt_unit.c)

Cross-cutting helper

File-static helper nxt_port_mmap_chunk_range_valid in src/nxt_port_memory.c and two siblings in src/nxt_unit.c (nxt_unit_response_buf_size, nxt_unit_sptr_in_buf). None exported through public headers; libunit ABI unchanged (nxt_unit.h, nxt_unit_sptr.h, nxt_unit_request.h, nxt_unit_field.h struct layouts and inline-helper bodies untouched).

Notes on the TOCTOU fix

nxt_port_get_port_incoming_mmap() had a real lookup-vs-deref window: the function released process->incoming.mutex before returning, then the caller dereferenced mmap_handler->hdr and only afterwards bumped the existing atomic refcount. The fix bumps the (existing) use_count refcount under the mutex inside the lookup — no new refcount field invented. The sole caller adopts that reference; the previously-explicit handler_use(+1) at the buf-parent assignment site is now removed, and every error path between lookup and buf creation drops the lookup ref.

Test plan

• ./configure --openssl && make builds unitd clean under -Werror.
• nxt_unit.c compile-checked standalone with stock CFLAGS (-Werror).
• pytest-3 --collect-only test/ collects 898 tests (imports parse).
• Full root-required pytest run — not exercised in the worktree; needs sudo pytest-3 test/ in a normal checkout.
• Targeted libunit smoke (Python WSGI / PHP) from a runtime-equipped environment to exercise the new sptr validation on real router→app traffic.

Upstream

Same fixes apply to freeunitorg/freeunit; will forward after merge. Maintainer should consider disclosure timing — V5 chunk_id bounds is the closest to RCE-shaped finding in this bundle.

Generated by Claude Code

[gemini-code-assist]

Code Review

This pull request introduces several security hardening measures, including bounds-checking for shared-memory offsets, addressing a TOCTOU vulnerability in mmap handler refcounting, preventing integer overflows during response buffer allocation, and validating string pointers (sptrs) in incoming requests. A critical issue was identified in the nxt_unit_sptr_in_buf function, where the bounds check for the sptr struct itself is insufficient, potentially allowing an out-of-bounds read when accessing its members.

[gemini-code-assist]

The current check is insufficient to prevent an out-of-bounds read. It does not ensure that the entire nxt_unit_sptr_t struct is within the buffer before its members are accessed. For example, if sptr_off is equal to buf_size, this check passes, but sptr would point just beyond the buffer, and accessing sptr->offset would be an out-of-bounds read. A similar issue occurs if sptr is only partially inside the buffer.

To fix this, you must validate that the sptr struct itself is fully contained within the buffer.

    if (sptr_off > buf_size - sizeof(nxt_unit_sptr_t)) {
        return 0;
    }

[andypost]

Gemini's [security-critical] finding addressed in d010e207: nxt_unit_sptr_in_buf now checks sptr_off > buf_size - sizeof(nxt_unit_sptr_t) before dereferencing sptr->offset, so the struct itself is fully bounded inside the buffer before any member access. Force-pushed; build clean.

[andypost]

Coming back to this with three coverage gaps surfaced by a parallel-POV review of the integration branch (audit-1.35.6, HEAD f44897a). All three sit squarely in PR-B's named scope — "bounds-check untrusted shmem offsets in port and libunit" — and extending this PR is the natural place rather than spinning new ones.

1. Send-side: nxt_port_mmap_at index cap (CR-class)

src/nxt_port_memory.c:46-86. The function trusts the caller's i parameter without a range cap. At i = 0xFFFFFFFF:

• cap = port_mmaps->cap (e.g. 16)
• if (cap == 0) cap = i + 1 skipped
• while (i + 1 > cap): i + 1 wraps to 0, 0 > 16 is false — growth loop skipped
• cap != port_mmaps->cap: 16 != 16 is false — realloc skipped
• return port_mmaps->elts + i → returns a pointer 4 GB past elts

The caller at line 261 (nxt_port_get_port_incoming_mmap) writes port_mmap->mmap_handler = mmap_handler to that OOB pointer. hdr->id is read from a peer-mapped region; a worker that maps its own region with hdr->id = 0xFFFFFFFF and sends the fd to main triggers a wild write inside the privileged process.

The nxt_port_mmap_chunk_range_valid helper this PR already introduces handles (chunk_id, nchunks) pairs but doesn't cover the mmap-region index used by nxt_port_mmap_at. Same shape, different consumer.

Suggested shape:

static nxt_port_mmap_t *
nxt_port_mmap_at(nxt_port_mmaps_t *port_mmaps, uint32_t i)
{
    uint32_t  cap;

    /*
     * Peer-supplied indices flow into this function via hdr->id on the
     * receive path.  Cap the index against a generous ceiling so a
     * forged hdr->id of 0xFFFFFFFF cannot wrap the growth loop and
     * return an out-of-range pointer.
     */
    if (nxt_slow_path(i >= NXT_PORT_MAX_MMAPS)) {
        return NULL;
    }

    cap = port_mmaps->cap;
    ...
}

Pick NXT_PORT_MAX_MMAPS generously (e.g. 1u << 20); the real bound is a Unit deployment characteristic, not a hard cap.

2. libunit write-side chunk validation

src/nxt_unit.c. The receive side now validates incoming sptr offsets via nxt_unit_sptr_in_buf (good). The write side — where libunit composes responses and stamps chunk_id into shared memory before notifying the router — uses the same (chunk_id, nchunks) shape but lacks the equivalent guard. The DedSec POV flagged a memset(0xA5) poison pattern reachable through a forged chunk index on the write path.

I haven't read this one line-by-line yet, so this needs your verification before fixing. Specifically: trace every site in nxt_unit.c that writes a chunk_id / nchunks into a nxt_port_mmap_msg_t (or similar). If the index comes from libunit-internal state, fine; if it can be influenced by app code, it needs validation.

If confirmed, the fix is the same shape: call nxt_port_mmap_chunk_range_valid (or the libunit-side equivalent) before the index is committed to shmem.

3. Response-side sptr validation

src/nxt_unit.c. This PR added nxt_unit_sptr_in_buf on request parsing (10 fields in nxt_unit_process_req_headers). The DedSec POV flagged a heap-disclosure path through response sptrs: an app builds a response whose sptr offsets encode arbitrary values, the router copies the encoded region out of router-process heap onto the wire, the client sees router secrets (TLS keys, other tenants' state).

Need to trace every site that builds a response sptr and verify the offset is bounded against the response buffer's [start, end) window. Likely candidates:

• nxt_unit_response_init field append paths
• nxt_unit_response_add_field / add_content
• nxt_unit_response_realloc sptr re-anchoring

Suggested helper: nxt_unit_response_sptr_in_buf(sptr, resp_start, resp_end) mirroring the existing request-side helper at line 254.

Why extend this PR rather than spin new ones

All three are "peer-supplied index into IPC structures, no bounds check before pointer arithmetic" — the named scope of PR-B. Splitting them across separate PRs would fragment the review (and the eventual CVE if these become reportable). Squashing them in here keeps the bundle coherent.

Happy to push the nxt_port_mmap_at patch as a separate commit on this branch if you want to take the libunit work yourself, or to do the full thing — say the word.

---

Generated by Claude Code