Skip to content

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

Open
andypost wants to merge 1 commit into
masterfrom
claude/audit-pr-b-shmem-bounds
Open

fix(security): bounds-check untrusted shmem offsets in port and libunit#21
andypost wants to merge 1 commit into
masterfrom
claude/audit-pr-b-shmem-bounds

Conversation

@andypost
Copy link
Copy Markdown
Owner

@andypost andypost commented May 8, 2026

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

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/nxt_unit.c Outdated
Comment on lines +264 to +266
if (sptr_off > buf_size) {
return 0;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-critical critical

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;
    }

Audit-driven hardening pass clustered around the trust boundary where
one process writes a struct into shared memory and another process
reads it.  Five findings (security-audit.md V5/V10), one helper per
file, no public-API or libunit-ABI changes.

V5 [High]   chunk_id no bounds check (src/nxt_port_memory.c:698 on
            master @ 7b12696).  Peer-supplied chunk_id was passed to
            nxt_port_mmap_chunk_start() without checking against
            PORT_MMAP_CHUNK_COUNT, yielding an OOB pointer.  Reject in
            nxt_port_mmap_get_incoming_buf() before pointer arithmetic.

V5 [High]   chunk_id + nchunks past mapped region (line 701).
            nchunks is computed from untrusted mmap_msg->size; a peer
            could land b->mem.end past the mapped segment.  Combined
            with #1 above into a single overflow-safe range check
            (chunk_id + nchunks <= PORT_MMAP_CHUNK_COUNT, written as
            nchunks <= PORT_MMAP_CHUNK_COUNT - chunk_id after the
            chunk_id bound holds).

V5 [Medium] TOCTOU on shmem mmap_id lookup (lines 676-678).
            process->incoming.mutex was released between locating
            mmap_handler and the caller's first dereference of
            mmap_handler->hdr; a concurrent peer-side close could free
            the handler in that window.  nxt_port_get_port_incoming_mmap
            now bumps the existing atomic refcount under the mutex; the
            sole caller adopts the reference (the redundant explicit
            handler_use(+1) at the buf-parent assignment site is
            removed) and releases it on every error path.

V10 [Medium] max_fields_count * sizeof(field) + 2 overflow on response
            (src/nxt_unit.c:2049-2051, 2128-2130).  Application-supplied
            counts/sizes were multiplied without overflow checking;
            wrap-around produced an undersized buffer that subsequent
            field memcpy()s overran.  New file-static helper
            nxt_unit_response_buf_size() computes the size with
            UINT32_MAX-bounded checks at both multiplication and
            addition; nxt_unit_response_init/realloc reject overflow
            with a libunit error.

V10 [Low]   sptr offset dereferenced without bounds (line 1329 etc.).
            Every nxt_unit_request_t sptr is now validated at
            request-arrival time inside nxt_unit_process_req_headers()
            before any consumer dereferences it.  New file-static
            helper nxt_unit_sptr_in_buf(sptr, length, buf, size)
            performs the underflow-safe check; one-shot validation
            keeps the per-deref hot paths and the libunit ABI
            unchanged.

Cross-cutting: file-static helpers nxt_port_mmap_chunk_range_valid()
in src/nxt_port_memory.c and nxt_unit_sptr_in_buf() /
nxt_unit_response_buf_size() in src/nxt_unit.c.  None exported
through public headers; nxt_unit_sptr.h, nxt_unit_request.h,
nxt_unit_field.h, and nxt_unit.h struct layouts are unchanged.

Builds clean with ./configure --openssl && make under -Werror.
nxt_unit.c also compile-checked standalone with the stock CFLAGS.
pytest-3 --collect-only test/ collects 898 tests; root-required
runs were not exercised in the worktree.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@andypost andypost force-pushed the claude/audit-pr-b-shmem-bounds branch from e855da4 to d010e20 Compare May 11, 2026 21:28
@andypost
Copy link
Copy Markdown
Owner Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant