docs(security): add 14-vector audit + remediation tracker#10
Open
andypost wants to merge 1 commit into
Open
Conversation
Merged
3 tasks
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive security and leak audit report for the FreeUnit project, detailing findings across 14 different vectors. The audit identifies several vulnerabilities, including a critical TOCTOU issue in cgroup handling and multiple high-severity memory safety concerns. Review feedback pointed out numerical inconsistencies between the executive summary and the detailed findings list. Additionally, it was recommended to remove a local environment file path from the remediation tracker description to maintain project-wide relevance.
Comment on lines
+13
to
+19
| | Severity | Count | | ||
| |----------|-------| | ||
| | Critical | 1 | | ||
| | High | 11 | | ||
| | Medium | 24 | | ||
| | Low | 7 | | ||
| | Informational | 2 | |
There was a problem hiding this comment.
The counts in the executive summary table do not match the actual number of findings listed in the remediation tracker below. The tracker contains 17 'High', 28 'Medium', 9 'Low', and 3 'Informational' findings, which differs from the numbers reported here. Please update the summary table to be consistent with the detailed findings.
|
|
||
| ## Remediation tracker | ||
|
|
||
| PR slots map to the remediation plan in `~/.claude-my/plans/create-a-plan-to-abundant-rabin.md`. PR #8 on `andypost/unit` (`fix(port): plug mp-pool retain and fd/buffer leaks in IPC reply paths`) is the precedent and is listed in the Appendix; it does not appear here because its findings predate this audit. |
There was a problem hiding this comment.
The path ~/.claude-my/plans/create-a-plan-to-abundant-rabin.md appears to be a local file path from the environment where the audit was generated. It should be removed or replaced with a link to a project-wide tracking document or issue to ensure it is useful for all contributors.
Suggested change
| PR slots map to the remediation plan in `~/.claude-my/plans/create-a-plan-to-abundant-rabin.md`. PR #8 on `andypost/unit` (`fix(port): plug mp-pool retain and fd/buffer leaks in IPC reply paths`) is the precedent and is listed in the Appendix; it does not appear here because its findings predate this audit. | |
| PR slots map to the remediation plan. PR #8 on `andypost/unit` (`fix(port): plug mp-pool retain and fd/buffer leaks in IPC reply paths`) is the precedent and is listed in the Appendix; it does not appear here because its findings predate this audit. |
Static review of master @ 7b12696 across 14 vectors (HTTP parsing, routing, TLS, control API, port IPC, isolation, language SAPIs, libunit ABI, static files, WebSocket, memory pool, FD lifetime). Findings: 1 Critical, 11 High, 24 Medium, 7 Low, 2 Informational. Each finding includes file:line reference, class, trigger, impact, suggested fix, and a `PR:` slot tying it to one of nine planned remediation PRs (PR-A through PR-I). Two findings excluded by maintainer DoS policy are flagged as such. Top 5 picks (auditor): V6 cgroup TOCTOU (Critical), V3 missing SSL_CTX_check_private_key, V5 untrusted shmem chunk_id, V9 Java InputStream.readLine bounds, V12 WebSocket frame_size loop bug. The Remediation tracker section gives an at-a-glance view; per-finding `PR:` bullets flip to merged-PR references as fixes land, so the file doubles as a remediation log. PR #8 (port-IPC retain/fd leaks) is acknowledged in the Appendix as the precedent. No source changes; documentation only.
andypost
force-pushed
the
claude/security-audit-master
branch
from
2bd10d9 to
741d806
Compare
3 tasks
andypost
added a commit
that referenced
this pull request
May 8, 2026
…ol-socket Audit-driven hardening pass (PR-C from security-audit.md, #10). Carries the only Critical finding (cgroup TOCTOU). * V6 [Critical] Cgroup TOCTOU /proc/self/cgroup src/nxt_process.c, src/nxt_cgroup.c nxt_cgroup_proc_add() ran after fork() but before the child unshared into a new cgroup namespace; the parent read its own /proc/self/cgroup to resolve a relative path, which races a parent migration between cgroups in that fork→unshare window and may place the child in the wrong cgroup. Read /proc/<child_pid>/cgroup instead — that view is stable for the just-forked child and reflects its post-unshare position by the time the cgroup directory is created. The symmetric nxt_cgroup_cleanup() runs after the child has already exited, so /proc/<pid>/cgroup is gone; cache the resolved directory in process->isolation.cgroup.resolved_path during proc_add and read it back at cleanup. Recompute cgroot from /proc/self/cgroup (the still-alive parent's view) since cleanup only needs a stop boundary for the rmdir walk. * V6 [High] Mount-destination symlink TOCTOU src/nxt_isolation.c nxt_fs_mkdir_p() creates mount destinations under a possibly attacker-influenced rootfs; a path component can be swapped for a symlink between mkdir and mount(2). After mkdir and before nxt_fs_mount(), reopen the destination via openat2(rootfs_fd, …, RESOLVE_BENEATH | RESOLVE_NO_SYMLINKS); a tampered destination fails the open before mount(2) sees it. Gated on NXT_HAVE_OPENAT2. openat2(2) is Linux 5.6+ and returns ENOSYS on older kernels even when the userspace headers are present. Distinguish ENOSYS from real RESOLVE_BENEATH violations: warn once and fall through to the unguarded mount (matching the prior security posture) rather than failing every isolation setup on pre-5.6 hosts. * V5 [Medium] No sender-type ACL on port handler dispatch src/nxt_port.c, src/nxt_port_socket.c Add nxt_port_sender_acl[] mapping privileged message types to the process types permitted to send them, plus nxt_port_msg_acl_check() in nxt_port_handler(). Identity comes from the existing SCM_CREDENTIALS pid (NXT_USE_CMSG_PID), with a sanity-check fallback to the message header on platforms without it. Listed types: START_PROCESS, SOCKET, SOCKET_UNLINK, MODULES, CONF_STORE, CERT_GET/DELETE, SCRIPT_GET/DELETE, ACCESS_LOG, APP_RESTART. QUIT, REMOVE_PID, CHANGE_FILE, and NEW_PORT are intentionally not listed — their senders span main/router/prototype during shutdown and fd-publish flows, so per-caller analysis is needed before tightening. Documented inline. * V4 [High] Control socket lacks peer-cred check src/nxt_controller.c Any local user with write access to control.unit.sock could mutate config; the only barrier was filesystem permissions on the socket file. Add nxt_controller_check_peer_cred() invoked from nxt_controller_conn_init() that validates the peer UID against the process UID via SO_PEERCRED (Linux) or getpeereid() (BSD/macOS), rejecting connections from non-matching peers with a 403-style alert. Platforms with neither fall back to filesystem-perms-only (no startup-time alert; fix-on-demand if a deployment surfaces it). No protocol or config-surface changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced May 8, 2026
andypost
added a commit
that referenced
this pull request
May 11, 2026
…et stance Audit-driven routing-polish + isolation-docs pass (PR-I from security-audit.md / #10). Six low-priority findings; no behaviour change for any valid existing config. * V2 [Medium] IPv4 /32 CIDR fallthrough (nxt_http_route_addr.c) When cidr_prefix == 32 the parser today falls through to the inet_addr path below, which then sets match_type = EXACT. The current code is correct, but the fallthrough is fragile under refactor. Add a comment making the intent explicit so a future reader doesn't add an early goto and accidentally skip the inet_addr parse. * V2 [Medium] Asymmetric pattern/request URI decoding (nxt_http_route.c) Both configured pattern strings and request URIs decode through nxt_decode_uri / nxt_decode_uri_plus; %XX semantics are symmetric. Document the invariant at nxt_http_route_decode_str so a future helper change keeps the two paths in lock-step. * V2 [Medium] nxt_regex_match_create(.., 0) size argument (route.c) pcre2_match_data_create(0, ctx) is undefined per the PCRE2 docs; 1 is the documented minimum (one ovector pair, holding the overall match offsets — captures aren't consulted here). Bump the size from 0 to 1 and document. * V2 [Low] Port-range parsing off-by-one (route_addr.c) memchr(.., port.length - 1) silently accepted a trailing '-' in a 2-byte input ("N-"). Validate port.length >= 3 (the minimum for "N-N") before searching, drop the - 1, and let the single-port parse below reject the malformed range. * V2 [Low] Host matching always lowercased (nxt_http_route.c) Host matching is hardcoded to LOWCASE; admin-supplied case-sensitivity is silently ignored. Per RFC 9110 §4.2.3 hosts are case-insensitive, so the behaviour is correct; just document it so the surprise is in the comment, not in the operator's logs. * V6 [Informational] capset never called (nxt_capability.c) The module exposes capget for detection but does not call capset(2) to drop capabilities programmatically. Operators expecting an explicit cap-drop step should not assume one exists — privilege barriers are setuid + PR_SET_NO_NEW_PRIVS. Top-of-file comment. No behaviour change for any valid existing config; the only visible change is that a route pattern with a malformed port range like "X-" (2 bytes, trailing dash) is now rejected with NXT_ADDR_PATTERN_PORT_ERROR via the single-port parse branch instead of being silently accepted as a host-only entry. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
andypost
added a commit
that referenced
this pull request
May 11, 2026
Audit-driven HTTP/URI parser and string-helper bounds pass (PR-F from security-audit.md / PR #10). Four findings, no protocol or config-surface changes. V1 [High] Proxy response Content-Length underflow (src/nxt_h1proto.c:2881). When an upstream sent more body bytes than its Content-Length declared, h1p->remainder (signed nxt_off_t) silently underflowed and subsequent `remainder > 0` checks dropped excess data, masking the inconsistency. Guard the subtraction on the slow path: log the violation, set request->inconsistent immediately, clamp remainder to zero, and close the peer cleanly. Happy path stays branch-predicted. V1 [Medium] Proxy Content-Length > NXT_OFF_T_MAX (src/nxt_http_proxy.c:417). nxt_off_t_parse() returns -2 on overflow and -1 on parse error; the previous `n >= 0` guard collapsed both into "leave content_length_n at -1", indistinguishable from "header absent". Now log at NXT_LOG_WARN and mark r->inconsistent — symmetric with the underflow path above and matches the upstream-side handling in nxt_http_request.c. V13 [Medium] nxt_is_complex_uri_encoded() off-by-one (src/nxt_string.c:718). The bounds check `end - src < 2` is wrong: the two pre-increments below it read src+1 and src+2, so three bytes must remain. A URI ending in "%X" (one hex digit) read one byte past end. Bumped to `< 3` with a short comment explaining the pre-increment shape. V13 [Low] nxt_rmemstrn() length underflow (src/nxt_string.c:319). `s1 = end - length` had no defensive check; all current callers validate upstream but a future caller could trigger pointer underflow. Added the audit's recommended guard at function entry. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
andypost
added a commit
that referenced
this pull request
May 12, 2026
Audit-driven language-binding pass (PR-G from security-audit.md / #10). Nine findings on the SAPI-side trust boundary with app code; no protocol or libunit-ABI changes. * V7 [High] PHP isspace() skip past header end (nxt_php_sapi.c:1796) while(isspace(*value)) value++; was unbounded — a header like `header("X: ")` walks past h->header + h->header_len. Bound the loop with `value < h->header + h->header_len`. * V7 [Medium] PHP realpath leak of tmp buffer (nxt_php_sapi.c:884) Static-app config branch: nxt_realpath() failure returned NXT_ERROR without freeing the tmp buffer allocated above. Mirror the existing correct pattern at line ~953 (entrypoint path) — add nxt_free(tmp). * V7 [Low] PHP PATH_INFO not NUL-terminated (nxt_php_sapi.c:1473) ctx->path_info points into the shmem-mapped request buffer; it is length-only. PHP today consumes it length-safely via php_register_variable_safe. Add a comment so a future C-string consumer doesn't assume otherwise. * V8 [High] Python WSGI environ NULL after copy fail (nxt_python_wsgi.c:455) Refresh after request_done returned NULL silently — leaving the next request to dereference NULL. Surface the alert; the next request's existing NULL-check (line ~323) retries via copy_environ and fails cleanly with NXT_UNIT_ERROR if the retry also fails. * V8 [Medium] Perl ERRSV not cleared on PSGI init fail (nxt_perl_psgi.c fail label) eval_pv() / io_init() failure left ERRSV populated; the next interpreter created on this pctx slot would inherit the exception. Scrub with sv_setsv(ERRSV, &PL_sv_undef) in the fail label. * V8 [Low] Python ASGI lifespan checks wrong NULL var (nxt_python_asgi_lifespan.c:162,168) After `send = PyObject_GetAttrString(..)` and `done = PyObject_GetAttrString(..)`, both error checks tested `receive == NULL` instead of `send`/`done`. Two-token correction. * V9 [High] Java InputStream.readLine() no off/len bounds (nxt_jni_InputStream.c:89) GetPrimitiveArrayCritical() pointer + app-supplied off/len drove nxt_unit_request_read() into an OOB write. Validate against GetArrayLength() before the critical region; throw IllegalStateException on violation (matches the helper class used by PR-A's nxt_jni_Request.c sendWsFrame fix). * V9 [Medium] WASM offset arithmetic on guest-controlled fields (nxt_wasm.c request handler) The SET_REQ_MEMBER macro and per-field loop accumulated `offset` and memcpy'd at `wr + offset` without bounding against NXT_WASM_MEM_SIZE. Many/large request headers could drive the copy past the 32 MB sandbox region. Introduce a local WASM_OFFSET_GUARD macro and check before each write. * V9 [Medium] WASM send_headers / send_response offset (nxt_wasm.c:43,73) Guest-supplied `offset` was cast to a typed pointer without bounding against NXT_WASM_MEM_SIZE. Bound before the cast in both send_headers and send_response; in send_response also bound resp->size against the remaining region. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
andypost
added a commit
that referenced
this pull request
May 13, 2026
…oundary state Audit-driven controller robustness + request-boundary state pass (PR-H from security-audit.md / #10). Five findings; no protocol or config-surface changes. * V4 [High] Unbounded JSON recursion (nxt_conf.c) A POST /config with deeply-nested JSON like "[[[[...]]]]" recursed through nxt_conf_json_parse_value → parse_object / parse_array with no depth cap, blowing the controller's stack. Add a file-static depth counter checked at the '{' / '[' arms of parse_value (the only recursion sites); cap at 100, well above any legitimate Unit config (nesting > 6 levels does not occur in practice). Reset at every entry to nxt_conf_json_parse() so a stale value from an aborted prior call cannot leak. * V4 [High] Unbounded JSON array/object element count (nxt_conf.c) parse_object and parse_array looped with `count++` and no cap; a flat [1,2,...,1e6] passes a billion-byte allocation request to nxt_mp_get() at the end. Cap at 100k elements (well above any real config — the largest production configs we've seen have a few thousand routes). Reject with a clean parse error. * V4 [Medium] Validator trust-model annotation (nxt_conf_validation.c) Document at nxt_conf_vldt_app_isolation_members that allowing arbitrary "executable" and isolation = false is intentional: the privilege boundary lives at the control-socket layer (SO_PEERCRED, landed in #14) and not in this schema validator. Allow-listing executables here would be a deployment policy decision, not a config-schema concern. * V7 [Medium] PHP TrueAsync EG exception scrubbing (nxt_php_sapi.c) The TrueAsync entrypoint intentionally skips php_request_shutdown() so the callback zval persists across the prototype → worker fork. But if the entrypoint script raised an exception that wasn't caught before HttpServer-> onRequest() stored the callback, every forked worker would inherit the exception on its first request. Add a zend_clear_exception() before the function returns. Other EG globals (output buffers, error_reporting, symbol table) are reset implicitly by the worker's per-request init; the exception state is the one that early-exits php_execute_script() on the next request. * V8 [Medium] Ruby per-request IO NULL-deref hardening (nxt_ruby_stream_io.c) rctx->req is cleared after each request handler runs (src/ruby/nxt_ruby.c:657). Apps that capture rack.input or rack.errors across the request boundary — background threads, cached IO handles — would NULL-deref rctx->req when they called gets / read / puts / write. Add a NULL guard at each public entry point; gets/read return Qnil, write silently drops. The audit framed this as "buffers from a prior request remain accessible"; the actual hazard is NULL-deref, but the fix covers both shapes (all reads go through rctx->req, no buffers are held inside the IO object). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
andypost
added a commit
that referenced
this pull request
May 13, 2026
…ol-socket Audit-driven hardening pass (PR-C from security-audit.md, #10). Carries the only Critical finding (cgroup TOCTOU). * V6 [Critical] Cgroup TOCTOU /proc/self/cgroup src/nxt_process.c, src/nxt_cgroup.c nxt_cgroup_proc_add() ran after fork() but before the child unshared into a new cgroup namespace; the parent read its own /proc/self/cgroup to resolve a relative path, which races a parent migration between cgroups in that fork→unshare window and may place the child in the wrong cgroup. Read /proc/<child_pid>/cgroup instead — that view is stable for the just-forked child and reflects its post-unshare position by the time the cgroup directory is created. The symmetric nxt_cgroup_cleanup() runs after the child has already exited, so /proc/<pid>/cgroup is gone; cache the resolved directory in process->isolation.cgroup.resolved_path during proc_add and read it back at cleanup. Recompute cgroot from /proc/self/cgroup (the still-alive parent's view) since cleanup only needs a stop boundary for the rmdir walk. * V6 [High] Mount-destination symlink TOCTOU src/nxt_isolation.c nxt_fs_mkdir_p() creates mount destinations under a possibly attacker-influenced rootfs; a path component can be swapped for a symlink between mkdir and mount(2). After mkdir and before nxt_fs_mount(), reopen the destination via openat2(rootfs_fd, …, RESOLVE_BENEATH | RESOLVE_NO_SYMLINKS); a tampered destination fails the open before mount(2) sees it. Gated on NXT_HAVE_OPENAT2. openat2(2) is Linux 5.6+ and returns ENOSYS on older kernels even when the userspace headers are present. Distinguish ENOSYS from real RESOLVE_BENEATH violations: warn once and fall through to the unguarded mount (matching the prior security posture) rather than failing every isolation setup on pre-5.6 hosts. * V5 [Medium] No sender-type ACL on port handler dispatch src/nxt_port.c, src/nxt_port_socket.c Add nxt_port_sender_acl[] mapping privileged message types to the process types permitted to send them, plus nxt_port_msg_acl_check() in nxt_port_handler(). Identity comes from the existing SCM_CREDENTIALS pid (NXT_USE_CMSG_PID), with a sanity-check fallback to the message header on platforms without it. Listed types: START_PROCESS, SOCKET, SOCKET_UNLINK, MODULES, CONF_STORE, CERT_GET/DELETE, SCRIPT_GET/DELETE, ACCESS_LOG, APP_RESTART. QUIT, REMOVE_PID, CHANGE_FILE, and NEW_PORT are intentionally not listed — their senders span main/router/prototype during shutdown and fd-publish flows, so per-caller analysis is needed before tightening. Documented inline. * V4 [High] Control socket lacks peer-cred check src/nxt_controller.c Any local user with write access to control.unit.sock could mutate config; the only barrier was filesystem permissions on the socket file. Add nxt_controller_check_peer_cred() invoked from nxt_controller_conn_init() that validates the peer UID against the process UID via SO_PEERCRED (Linux) or getpeereid() (BSD/macOS), rejecting connections from non-matching peers with a 403-style alert. Platforms with neither fall back to filesystem-perms-only (no startup-time alert; fix-on-demand if a deployment surfaces it). No protocol or config-surface changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
andypost
added a commit
that referenced
this pull request
May 13, 2026
…oundary state Audit-driven controller robustness + request-boundary state pass (PR-H from security-audit.md / #10). Five findings; no protocol or config-surface changes. * V4 [High] Unbounded JSON recursion (nxt_conf.c) A POST /config with deeply-nested JSON like "[[[[...]]]]" recursed through nxt_conf_json_parse_value → parse_object / parse_array with no depth cap, blowing the controller's stack. Add a file-static depth counter checked at the '{' / '[' arms of parse_value (the only recursion sites); cap at 100, well above any legitimate Unit config (nesting > 6 levels does not occur in practice). Reset at every entry to nxt_conf_json_parse() so a stale value from an aborted prior call cannot leak. * V4 [High] Unbounded JSON array/object element count (nxt_conf.c) parse_object and parse_array looped with `count++` and no cap; a flat [1,2,...,1e6] passes a billion-byte allocation request to nxt_mp_get() at the end. Cap at 100k elements (well above any real config — the largest production configs we've seen have a few thousand routes). Reject with a clean parse error. * V4 [Medium] Validator trust-model annotation (nxt_conf_validation.c) Document at nxt_conf_vldt_app_isolation_members that allowing arbitrary "executable" and isolation = false is intentional: the privilege boundary lives at the control-socket layer (SO_PEERCRED, landed in #14) and not in this schema validator. Allow-listing executables here would be a deployment policy decision, not a config-schema concern. * V7 [Medium] PHP TrueAsync EG exception scrubbing (nxt_php_sapi.c) The TrueAsync entrypoint intentionally skips php_request_shutdown() so the callback zval persists across the prototype → worker fork. But if the entrypoint script raised an exception that wasn't caught before HttpServer-> onRequest() stored the callback, every forked worker would inherit the exception on its first request. Add a zend_clear_exception() before the function returns. Other EG globals (output buffers, error_reporting, symbol table) are reset implicitly by the worker's per-request init; the exception state is the one that early-exits php_execute_script() on the next request. * V8 [Medium] Ruby per-request IO NULL-deref hardening (nxt_ruby_stream_io.c) rctx->req is cleared after each request handler runs (src/ruby/nxt_ruby.c:657). Apps that capture rack.input or rack.errors across the request boundary — background threads, cached IO handles — would NULL-deref rctx->req when they called gets / read / puts / write. Add a NULL guard at each public entry point; gets/read return Qnil, write silently drops. The audit framed this as "buffers from a prior request remain accessible"; the actual hazard is NULL-deref, but the fix covers both shapes (all reads go through rctx->req, no buffers are held inside the IO object). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
andypost
added a commit
that referenced
this pull request
May 13, 2026
…ol-socket Audit-driven hardening pass (PR-C from security-audit.md, #10). Carries the only Critical finding (cgroup TOCTOU). * V6 [Critical] Cgroup TOCTOU /proc/self/cgroup src/nxt_process.c, src/nxt_cgroup.c nxt_cgroup_proc_add() ran after fork() but before the child unshared into a new cgroup namespace; the parent read its own /proc/self/cgroup to resolve a relative path, which races a parent migration between cgroups in that fork→unshare window and may place the child in the wrong cgroup. Read /proc/<child_pid>/cgroup instead — that view is stable for the just-forked child and reflects its post-unshare position by the time the cgroup directory is created. The symmetric nxt_cgroup_cleanup() runs after the child has already exited, so /proc/<pid>/cgroup is gone; cache the resolved directory in process->isolation.cgroup.resolved_path during proc_add and read it back at cleanup. Recompute cgroot from /proc/self/cgroup (the still-alive parent's view) since cleanup only needs a stop boundary for the rmdir walk. * V6 [High] Mount-destination symlink TOCTOU src/nxt_isolation.c nxt_fs_mkdir_p() creates mount destinations under a possibly attacker-influenced rootfs; a path component can be swapped for a symlink between mkdir and mount(2). After mkdir and before nxt_fs_mount(), reopen the destination via openat2(rootfs_fd, …, RESOLVE_BENEATH | RESOLVE_NO_SYMLINKS); a tampered destination fails the open before mount(2) sees it. Gated on NXT_HAVE_OPENAT2. openat2(2) is Linux 5.6+ and returns ENOSYS on older kernels even when the userspace headers are present. Distinguish ENOSYS from real RESOLVE_BENEATH violations: warn once and fall through to the unguarded mount (matching the prior security posture) rather than failing every isolation setup on pre-5.6 hosts. * V5 [Medium] No sender-type ACL on port handler dispatch src/nxt_port.c, src/nxt_port_socket.c Add nxt_port_sender_acl[] mapping privileged message types to the process types permitted to send them, plus nxt_port_msg_acl_check() in nxt_port_handler(). Identity comes from the existing SCM_CREDENTIALS pid (NXT_USE_CMSG_PID), with a sanity-check fallback to the message header on platforms without it. Listed types: START_PROCESS, SOCKET, SOCKET_UNLINK, MODULES, CONF_STORE, CERT_GET/DELETE, SCRIPT_GET/DELETE, ACCESS_LOG, APP_RESTART. QUIT, REMOVE_PID, CHANGE_FILE, and NEW_PORT are intentionally not listed — their senders span main/router/prototype during shutdown and fd-publish flows, so per-caller analysis is needed before tightening. Documented inline. * V4 [High] Control socket lacks peer-cred check src/nxt_controller.c Any local user with write access to control.unit.sock could mutate config; the only barrier was filesystem permissions on the socket file. Add nxt_controller_check_peer_cred() invoked from nxt_controller_conn_init() that validates the peer UID against the process UID via SO_PEERCRED (Linux) or getpeereid() (BSD/macOS), rejecting connections from non-matching peers with a 403-style alert. Platforms with neither fall back to filesystem-perms-only (no startup-time alert; fix-on-demand if a deployment surfaces it). No protocol or config-surface changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
andypost
added a commit
that referenced
this pull request
May 14, 2026
…oundary state Audit-driven controller robustness + request-boundary state pass (PR-H from security-audit.md / #10). Five findings; no protocol or config-surface changes. * V4 [High] Unbounded JSON recursion (nxt_conf.c) A POST /config with deeply-nested JSON like "[[[[...]]]]" recursed through nxt_conf_json_parse_value → parse_object / parse_array with no depth cap, blowing the controller's stack. Add a file-static depth counter checked at the '{' / '[' arms of parse_value (the only recursion sites); cap at 100, well above any legitimate Unit config (nesting > 6 levels does not occur in practice). Reset at every entry to nxt_conf_json_parse() so a stale value from an aborted prior call cannot leak. * V4 [High] Unbounded JSON array/object element count (nxt_conf.c) parse_object and parse_array looped with `count++` and no cap; a flat [1,2,...,1e6] passes a billion-byte allocation request to nxt_mp_get() at the end. Cap at 100k elements (well above any real config — the largest production configs we've seen have a few thousand routes). Reject with a clean parse error. * V4 [Medium] Validator trust-model annotation (nxt_conf_validation.c) Document at nxt_conf_vldt_app_isolation_members that allowing arbitrary "executable" and isolation = false is intentional: the privilege boundary lives at the control-socket layer (SO_PEERCRED, landed in #14) and not in this schema validator. Allow-listing executables here would be a deployment policy decision, not a config-schema concern. * V7 [Medium] PHP TrueAsync EG exception scrubbing (nxt_php_sapi.c) The TrueAsync entrypoint intentionally skips php_request_shutdown() so the callback zval persists across the prototype → worker fork. But if the entrypoint script raised an exception that wasn't caught before HttpServer-> onRequest() stored the callback, every forked worker would inherit the exception on its first request. Add a zend_clear_exception() before the function returns. Other EG globals (output buffers, error_reporting, symbol table) are reset implicitly by the worker's per-request init; the exception state is the one that early-exits php_execute_script() on the next request. * V8 [Medium] Ruby per-request IO NULL-deref hardening (nxt_ruby_stream_io.c) rctx->req is cleared after each request handler runs (src/ruby/nxt_ruby.c:657). Apps that capture rack.input or rack.errors across the request boundary — background threads, cached IO handles — would NULL-deref rctx->req when they called gets / read / puts / write. Add a NULL guard at each public entry point; gets/read return Qnil, write silently drops. The audit framed this as "buffers from a prior request remain accessible"; the actual hazard is NULL-deref, but the fix covers both shapes (all reads go through rctx->req, no buffers are held inside the IO object). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
andypost
added a commit
that referenced
this pull request
May 14, 2026
Audit-driven language-binding pass (PR-G from security-audit.md / #10). Nine findings on the SAPI-side trust boundary with app code; no protocol or libunit-ABI changes. * V7 [High] PHP isspace() skip past header end (nxt_php_sapi.c:1796) while(isspace(*value)) value++; was unbounded — a header like `header("X: ")` walks past h->header + h->header_len. Bound the loop with `value < h->header + h->header_len`. * V7 [Medium] PHP realpath leak of tmp buffer (nxt_php_sapi.c:884) Static-app config branch: nxt_realpath() failure returned NXT_ERROR without freeing the tmp buffer allocated above. Mirror the existing correct pattern at line ~953 (entrypoint path) — add nxt_free(tmp). * V7 [Low] PHP PATH_INFO not NUL-terminated (nxt_php_sapi.c:1473) ctx->path_info points into the shmem-mapped request buffer; it is length-only. PHP today consumes it length-safely via php_register_variable_safe. Add a comment so a future C-string consumer doesn't assume otherwise. * V8 [High] Python WSGI environ NULL after copy fail (nxt_python_wsgi.c:455) Refresh after request_done returned NULL silently — leaving the next request to dereference NULL. Surface the alert; the next request's existing NULL-check (line ~323) retries via copy_environ and fails cleanly with NXT_UNIT_ERROR if the retry also fails. * V8 [Medium] Perl ERRSV not cleared on PSGI init fail (nxt_perl_psgi.c fail label) eval_pv() / io_init() failure left ERRSV populated; the next interpreter created on this pctx slot would inherit the exception. Scrub with sv_setsv(ERRSV, &PL_sv_undef) in the fail label. * V8 [Low] Python ASGI lifespan checks wrong NULL var (nxt_python_asgi_lifespan.c:162,168) After `send = PyObject_GetAttrString(..)` and `done = PyObject_GetAttrString(..)`, both error checks tested `receive == NULL` instead of `send`/`done`. Two-token correction. * V9 [High] Java InputStream.readLine() no off/len bounds (nxt_jni_InputStream.c:89) GetPrimitiveArrayCritical() pointer + app-supplied off/len drove nxt_unit_request_read() into an OOB write. Validate against GetArrayLength() before the critical region; throw IllegalStateException on violation (matches the helper class used by PR-A's nxt_jni_Request.c sendWsFrame fix). * V9 [Medium] WASM offset arithmetic on guest-controlled fields (nxt_wasm.c request handler) The SET_REQ_MEMBER macro and per-field loop accumulated `offset` and memcpy'd at `wr + offset` without bounding against NXT_WASM_MEM_SIZE. Many/large request headers could drive the copy past the 32 MB sandbox region. Introduce a local WASM_OFFSET_GUARD macro and check before each write. * V9 [Medium] WASM send_headers / send_response offset (nxt_wasm.c:43,73) Guest-supplied `offset` was cast to a typed pointer without bounding against NXT_WASM_MEM_SIZE. Bound before the cast in both send_headers and send_response; in send_response also bound resp->size against the remaining region. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
Remediation plan & status overviewThis PR is the audit catalog; the remediation lives in a stack of nine focused PRs on this fork. All of them are CI-green and have addressed reviewer feedback (Gemini + ChatGPT Codex + phpclub). The mapping below is what the PR stack
Plus precedent: #8 ( Severity tally vs audit
Excluded findings are pure-DoS per the project's stated security policy and stay documented in Strategy / orderingThe stack was sliced for reviewability (single conceptual change per PR, ≤8 files, ≤500 lines), severity ordering (CVE-track items in the first wave), and decoupled diffs (no PR depends on another being merged — reviewer can land them in any order). Submission waves used:
Upstream forwarding patternStaging PR on this fork lands first, then the same diff is filed against What remains
The plan itself is preserved at |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Static security review of master @ `7b126961` across 14 vectors, captured as a single `security-audit.md` at the repo root. Documentation only — no source changes.
What's in the file
Each finding has: file:line, class (leak / UAF / OOB / logic / TOCTOU / crypto / injection / DoS), trigger, impact, suggested fix, and a `PR:` slot tying it to one of nine planned remediation PRs (`PR-A` … `PR-I`). Two findings are flagged `excluded (DoS policy)` per the standard `/security-review` exclusions.
A Remediation tracker table near the top gives an at-a-glance view; per-finding `PR:` bullets flip to merged-PR refs as fixes land, so the file doubles as a remediation log.
Top 5 (auditor's pick)
PR slot map (preview)
PR #8 (port-IPC retain/fd leaks) is acknowledged in the Appendix as the precedent and slicing template.
Methodology
Read-only static review across 14 vectors, one Explore subagent per vector. No code modified during audit.
Test plan
Out of scope
The fixes themselves. Each `PR:` slot is a separate follow-up PR; this PR is just the artifact that anchors them.
Generated by Claude Code