fix(sapi): bounds-check app-supplied arguments across language bindings#23
fix(sapi): bounds-check app-supplied arguments across language bindings#23andypost wants to merge 1 commit into
Conversation
![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 several security and stability fixes across multiple language bindings, including Java, PHP, Perl, Python, and WASM. Key changes involve implementing bounds-checking for application-supplied arguments to prevent out-of-bounds access, addressing memory leaks in PHP, and correcting logic errors in Python ASGI lifespan checks. Feedback from the review highlights a potential out-of-bounds read in WASM header processing due to insufficient validation of field counts and guest-supplied offsets. Additionally, a suggestion was made to cast characters to "unsigned char" in PHP header parsing to prevent undefined behavior with "isspace()".
| * `offset` arrives from the guest WASM module; bound it against | ||
| * the linear-memory size before dereferencing. | ||
| */ | ||
| if (offset > NXT_WASM_MEM_SIZE - sizeof(nxt_wasm_response_fields_t)) { |
There was a problem hiding this comment.
This bounds check only validates that the base nxt_wasm_response_fields_t header fits within the WASM linear memory. However, the subsequent code reads rh->nfields and iterates through rh->fields[i], which is an array of unknown size at this point. A malicious guest could provide a large nfields value, leading to an out-of-bounds read when accessing rh->fields[i]. Additionally, the offsets rh->fields[i].name_off and rh->fields[i].value_off (used at lines 72-73) are guest-supplied and must also be validated against NXT_WASM_MEM_SIZE to prevent arbitrary out-of-bounds reads from the host process memory.
|
|
||
| value = colon + 1; | ||
| while(isspace(*value)) { | ||
| while (value < h->header + h->header_len && isspace(*value)) { |
There was a problem hiding this comment.
The isspace() function's behavior is undefined if the argument is not representable as an unsigned char or is not EOF. When passing a char directly, values above 127 (which are negative on many platforms) can cause issues. It is safer to cast the dereferenced pointer to unsigned char.
while (value < h->header + h->header_len && isspace((unsigned char) *value)) {![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d265dce704
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (offset > NXT_WASM_MEM_SIZE - sizeof(nxt_wasm_response_fields_t)) { | ||
| nxt_unit_req_alert(ctx->req, | ||
| "WASM send_headers offset %u out of range", offset); | ||
| return; |
There was a problem hiding this comment.
This check only proves that the 4-byte nfields header fits in linear memory; it still trusts the guest-supplied rh->nfields, name_off, and value_off before the loops below dereference rh->fields[i] and add fields. A WASM app can pass an offset near the end of the 32 MB region with nfields > 0, causing the first loop to read rh->fields[0] past NXT_WASM_MEM_SIZE, or can keep the table in-bounds while pointing names/values outside memory. Please include the variable fields[] table and each field's string range in the bounds validation before using them.
Useful? React with 👍 / 👎.
| send = PyObject_GetAttrString((PyObject *) lifespan, "send"); | ||
| if (nxt_slow_path(receive == NULL)) { | ||
| if (nxt_slow_path(send == NULL)) { | ||
| nxt_unit_alert(NULL, "Python failed to get 'send' method"); | ||
| goto release_receive; |
There was a problem hiding this comment.
When creating the bound send method fails, this new early cleanup path decrefs lifespan before fields such as lifespan->state are initialized below. The deallocator unconditionally runs Py_CLEAR(lifespan->state), so an allocation failure in PyObject_GetAttrString(..., "send") or the analogous _done path can dereference an uninitialized pointer instead of failing cleanly. Initialize the object fields immediately after PyObject_New() or avoid the deallocator until they are initialized.
Useful? React with 👍 / 👎.
| nxt_unit_req_alert(req, \ | ||
| "WASM request buffer overflow at offset %zu", offset); \ | ||
| nxt_unit_request_done(req, NXT_UNIT_ERROR); \ | ||
| return; \ |
There was a problem hiding this comment.
For oversized WASM requests that hit this new guard, the handler returns immediately after NXT_WASM_FH_REQUEST_INIT has already run, skipping the NXT_WASM_FH_REQUEST_END hook used on the normal and 500-error exits below. Modules that allocate or track per-request state in request_init_handler will not get their matching cleanup/notification for these rejected requests; route these new error exits through the existing request-end path after marking the request done.
Useful? React with 👍 / 👎.
andypost
force-pushed
the
claude/audit-pr-g-binding-bounds
branch
from
d265dce to
ba76490
Compare
|
Five Gemini + Codex findings addressed in
Force-pushed; build clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba764907b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| nxt_unit_req_alert(req, \ | ||
| "WASM request buffer overflow at offset %zu", offset); \ | ||
| nxt_unit_request_done(req, NXT_UNIT_ERROR); \ | ||
| goto request_done; \ |
There was a problem hiding this comment.
On the new bounds-failure paths, this jumps to request_done before nxt_wasm_ctx.req is assigned later in the handler, so the NXT_WASM_FH_REQUEST_END hook runs with a stale request pointer from the previous request (or NULL on the first one). This only affects oversized/many-header requests that hit these guards, but a configured request-end handler that calls back into host functions through the global WASM context can act on the wrong request; assign nxt_wasm_ctx.req = req before any guard can route to the hook.
Useful? React with 👍 / 👎.
|
|
||
| if (abs_name < fields_table_end | ||
| || abs_name > NXT_WASM_MEM_SIZE | ||
| || name_len > NXT_WASM_MEM_SIZE - abs_name |
There was a problem hiding this comment.
This bounds check accepts any in-memory name_len, but the validated value is later passed to nxt_unit_response_add_field(), whose name_length parameter is uint8_t. A WASM app that supplies a header name length above 255 but still inside linear memory will have that length truncated when the field is added, so Unit sends a response with a different/truncated header name instead of rejecting the invalid guest data; include the UINT8_MAX protocol limit in this validation or widen the downstream API.
Useful? React with 👍 / 👎.
| if (abs_name < fields_table_end | ||
| || abs_name > NXT_WASM_MEM_SIZE | ||
| || name_len > NXT_WASM_MEM_SIZE - abs_name | ||
| || abs_val < fields_table_end | ||
| || abs_val > NXT_WASM_MEM_SIZE | ||
| || val_len > NXT_WASM_MEM_SIZE - abs_val) |
There was a problem hiding this comment.
These new range checks validate each header string independently, but they still allow a guest to advertise many fields whose name/value ranges overlap the same in-bounds bytes; the later fields_len sum can then exceed the uint32_t max_fields_size accepted by nxt_unit_response_init(), get truncated, and cause nxt_unit_response_add_field() failures that are ignored before response_send(). This only requires a malicious WASM app with a large nfields, so reject aggregate header bytes above UINT32_MAX (or handle init/add failures) as part of the new validation.
Useful? React with 👍 / 👎.
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
force-pushed
the
claude/audit-pr-g-binding-bounds
branch
from
ba76490 to
b535398
Compare
|
Three remaining Codex P2 items on
Local syntax-check ( |
1 participant
Summary
Audit-driven language-binding bounds pass (PR-G from `security-audit.md` / PR #10). Nine findings across PHP / Python / Perl / Java / WASM — the SAPI-side trust boundary with app code. No protocol or libunit-ABI changes.
Findings addressed
Notable choices
Files changed
7 files, +123 / -12.
Test plan
Upstream
Same fixes apply to `freeunitorg/freeunit`; will forward after merge here.
Generated by Claude Code