fix(conf): reject embedded NUL in rootfs and cgroup path validation

[andypost]

Summary

Reworked salvage of the C-hardening half of the stale draft andypost#26 , rebased onto current pre-1.35.6 and extended.

Two isolation config options — isolation.cgroup.path and isolation.rootfs — are validated as length-tracked strings but later reused as NUL-terminated C strings during isolation setup (cgroup path construction; chroot/pivot_root). An embedded NUL byte survives config validation and silently truncates the effective path downstream.

Changes in nxt_conf_vldt_cgroup_path() / new nxt_conf_vldt_rootfs_path():

• Reject empty values and embedded NUL bytes (memchr) for both options.
• Validate rootfs as an absolute path other than / at config time — previously this was only enforced at runtime in nxt_isolation_set_rootfs(), so a bad value was accepted by the control API and only failed on app start.
• Replace sprintf(path, "/%*s/", ...) with snprintf(path, sizeof(path), "/%.*s/", ...). %*s is a width, not a precision, specifier — it reads cgpath.start to a NUL terminator a length-tracked string does not guarantee. %.*s bounds the copy to the given length, and snprintf bounds the destination.

What I deliberately left out of #26

• The Trivy CI workflow + sort-trivy-results.py — belongs in a separate ci: PR, SHA-pinned, schedule-only.
• docs/security-audit-findings.md — a public list of suspected-unfixed vulnerabilities; kept out of the repo for disclosure hygiene. The one actionable item there (rootfs embedded-NUL, "SA-001") is fixed by this PR.

Testing

• New unitd built from this branch; drove the control API directly (PUT /config):
  • rootfs: relative path, /, and /app\u0000/injected → rejected with Invalid configuration + detail; /srv/app → passes validation.
  • cgroup.path: scope\u0000python and scope/../python → rejected; scope/python → passes.
  • Confirmed the JSON \u0000 escape decodes to a real 0x00 byte (nxt_utf8_encode), so memchr catches it.
• Added negative cases to test/test_python_isolation.py: test_python_isolation_cgroup_invalid gains a NUL case; new test_python_isolation_rootfs_invalid covers empty, /, relative, and embedded-NUL.
• nxt_conf_validation.o and full unitd compile clean.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[gemini-code-assist]

Code Review

This pull request adds configuration validation to reject embedded NUL bytes in the "rootfs" and cgroup "path" isolation options, and ensures "rootfs" is validated as an absolute path other than "/". However, a security vulnerability was identified where a rootfs path configured as // can bypass the validation and resolve to / at runtime, disabling isolation. It is recommended to strip trailing slashes during validation to prevent this bypass and to add a corresponding test case.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[andypost]

@claude review

[claude]

Claude finished @andypost's task in 1m 25s —— View job

---

Code Review

• Read PR diff (src/nxt_conf_validation.c, test/test_python_isolation.py, CHANGES)
• Cross-check runtime path in src/nxt_isolation.c
• Evaluate prior review comments (gemini)
• Post review

---

Reviewed against origin/pre-1.35.6. The core of this PR is solid — the %*s → %.*s fix and the memchr NUL checks are correct and well-motivated, and moving rootfs validation to config time is the right call. One real gap remains.

🔴 // (and ///…) bypass — the gemini finding is valid

The concern raised in the inline review is a genuine bypass, and I'd treat it as blocking for this PR's stated goal (reject bad rootfs at config time).

nxt_conf_vldt_rootfs_path (src/nxt_conf_validation.c:3499) rejects on rootfs.length <= 1, but "//" has length 2, so it passes. At runtime, nxt_isolation_set_rootfs (src/nxt_isolation.c:505-514) checks the length before stripping the trailing slash:

if (nxt_slow_path(str.length <= 1 || str.start[0] != '/')) { ... }  // "//" -> len 2, passes
if (str.start[str.length - 1] == '/') {
    str.length--;                                                    // "//" -> "/"
}

So "//" survives both checks and collapses to "/" — a no-op chroot, silently disabling rootfs isolation. This is exactly the outcome the length <= 1 guard exists to prevent.

One nuance beyond the inline suggestion: the runtime only strips a single trailing slash, so "///" → "//", which Linux still resolves to /. The suggested fix (strip one slash, then re-check) closes "//" but not "///". A robust config-time check should strip all trailing slashes before the length test:

size_t  len = rootfs.length;

while (len > 1 && rootfs.start[len - 1] == '/') {
    len--;
}

if (len <= 1
    || rootfs.start[0] != '/'
    || memchr(rootfs.start, '\0', rootfs.length) != NULL)
{
    return nxt_conf_vldt_error(vldt, ...);
}

For true defense-in-depth, the runtime in nxt_isolation_set_rootfs should likewise loop the strip and re-validate str.length > 1 after stripping (that file isn't touched by this PR, but the single-strip logic is the root cause). Fix this →

🟢 What's correct

• cgroup %*s → %.*s (nxt_conf_validation.c:3475): accurate diagnosis. %*s reads a width argument and then scans cgpath.start to a NUL that a length-tracked nxt_str_t doesn't guarantee — a real over-read. %.*s + snprintf(sizeof(path)) bounds both source and destination. Good.
• NUL rejection via memchr on both options: correct, and confirmed the JSON `` escape decodes to a real 0x00, so `memchr` catches it.
• Ordering in cgroup validator: the too-long check still runs first (line 3462), then the empty/NUL check, then the snprintf — no over-read reachable. Good.
• The rootfs empty case (length == 0) is already covered by length <= 1.

🟡 Minor

• test_python_isolation_rootfs_invalid covers empty, /, relative, and embedded-NUL — nice. Please add check_invalid('//') (and ideally '///') so the bypass above is regression-guarded. The existing check_invalid('/') would still pass today even though // does not, so this gap is invisible without the explicit case.
• Consider whether rootfs should also reject a trailing-/.. / embedded /../ the way cgroup does. It's less critical for a chroot target than for the cgroup path construction, so I'd consider it out of scope here — just flagging.

Verdict

The NUL and format-string fixes are correct and worth merging. I'd resolve the ////// gap (config-time strip + a // test) before merge, since without it the headline "validate rootfs is an absolute path other than /" guarantee is incomplete.

_{· branch fix/conf-path-validation}

[andypost]

Addressed in 2d099720.

The // / /// bypass is real and now fixed. Verified against the runtime: nxt_isolation_set_rootfs stripped only a single trailing slash (an if, not a loop), so // collapsed to / (no-op chroot) and /// collapsed to // (which Linux still resolves to /) — both silently disabling rootfs isolation despite passing the length <= 1 guard.

Fix strips all trailing slashes before the length check, in two places:

• nxt_conf_vldt_rootfs_path (config-time) — the primary gate.
• nxt_isolation_set_rootfs (runtime, if → while + validate after stripping) — defense in depth, since the single-strip there was the root cause. Thanks @gemini-code-assist / @claude for calling out that the runtime file also needed touching.

Regression cases check_invalid("//") and check_invalid("///") added.

Drove a live unitd via PUT /config: //, ///, /, relative, and embedded-NUL all rejected with Invalid configuration; /srv/app and /srv/app/ (legitimate trailing slash) pass validation.

Left cgroup-style /../ rejection for rootfs out of scope — a chroot target does not build a path buffer the way the cgroup path does, so the NUL + collapse-to-/ checks are the relevant ones here.

[claude]

Claude finished @andypost's task in 3m 8s —— View job

---

Re-review of 2d099720

• Verify // / /// fix in nxt_conf_vldt_rootfs_path (config-time)
• Verify runtime fix in nxt_isolation_set_rootfs (if → while)
• Check regression tests (//, ///)

The // / /// bypass is resolved. Both the config-time gate and the runtime defense-in-depth check are correct. ✅

Config-time — nxt_conf_vldt_rootfs_path (src/nxt_conf_validation.c:3492)

len = rootfs.length;
while (len > 1 && rootfs.start[len - 1] == '/') {
    len--;
}
if (len <= 1
    || rootfs.start[0] != '/'
    || memchr(rootfs.start, '\0', rootfs.length) != NULL)
{
    return nxt_conf_vldt_error(vldt, ...);
}

Traced every case:

input	after strip	verdict
""	len 0	✅ rejected (len <= 1)
/	len 1	✅ rejected
//	len 1	✅ rejected
///	len 1	✅ rejected
app/rootfs	—	✅ rejected (start[0] != '/')
/app\0/injected	—	✅ rejected (memchr)
/srv/app	len 8	✅ passes
/srv/app/	len 8	✅ passes (legit trailing slash preserved)

Two details I checked and they're right:

• memchr scans rootfs.length (the original, un-stripped length) — correct; it must see a NUL anywhere in the string, not just before the stripped tail.
• The loop guard len > 1 prevents stripping the leading / itself, so a bare /…/// never underflows — it bottoms out at len == 1 and gets rejected.

Runtime — nxt_isolation_set_rootfs (src/nxt_isolation.c:505)

while (str.length > 1 && str.start[str.length - 1] == '/') {
    str.length--;
}
if (nxt_slow_path(str.length <= 1 || str.start[0] != '/')) { ... }

The if → while change is the real root-cause fix: the old single-strip left /// → // (still resolving to /). Now all trailing slashes are removed before the length <= 1 re-check, so // and /// both fail. Storing the stripped str.length also normalizes /srv/app/ → /srv/app, which is harmless-to-better. Good defense-in-depth even though the config gate should catch these first.

Tests (test/test_python_isolation.py:252)

check_invalid('//') and check_invalid('///') added alongside '', /, relative, and embedded-NUL — the previously-invisible gap is now regression-guarded.

Out-of-scope call

Agree with leaving /../ rejection out for rootfs — a chroot target doesn't build a NUL-terminated path buffer the way the cgroup path does, so the kernel resolves .. against the real FS at chroot/pivot_root time. NUL + collapse-to-/ are the relevant classes here.

Verdict

LGTM. ✅ The NUL rejection, the %*s → %.*s / snprintf fix, and now the ////// collapse-to-/ bypass are all correctly closed at both config and runtime, with regression coverage. Nothing blocking from my side. (I confirmed against the PR-head blobs; I trust your live unitd + clean-compile verification for the build itself.)
· branch fix/conf-path-validation

[andypost]

/gemini review

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[andypost]

@codex review
@claude review
/gemini review

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[claude]

Claude finished @andypost's task in 1m 15s —— View job

---

Re-review — PR head (8f231a0, squashed to a single commit)

• Read current PR diff (nxt_conf_validation.c, nxt_isolation.c, tests, CHANGES)
• Verify // / ///, NUL, and format-string fixes at head
• Post review

Reviewed against origin/pre-1.35.6. The four commits were squashed into one; content is identical to 2d099720, which I traced case-by-case in my earlier re-review. Re-confirmed at head — everything still holds.

✅ Correct at head

cgroup — nxt_conf_vldt_cgroup_path (src/nxt_conf_validation.c:3454)
Ordering is safe: too-long guard (:3462) → empty/NUL via memchr (:3467) → snprintf(path, sizeof(path), "/%.*s/", …) (:3475) → /../ scan. %*s→%.*s bounds the source to cgpath.length (no NUL-scan over-read) and snprintf bounds the destination. No over-read reachable.

rootfs — nxt_conf_vldt_rootfs_path (src/nxt_conf_validation.c:3490)
trimmed_len strips all trailing slashes before the <= 1 test, and memchr scans the original rootfs.length — both correct:

input	after trim	verdict
"" / / / // / ///	≤1	✅ rejected
app/rootfs	—	✅ rejected (start[0] != '/')
/app/rootfs\0/injected	—	✅ rejected (memchr)
/srv/app , /srv/app/	8	✅ passes (legit trailing slash)

runtime — nxt_isolation_set_rootfs (src/nxt_isolation.c:505)
The if→while strip before the length <= 1 re-check is the root-cause fix for the ///→//→/ collapse. Correct defense-in-depth.

tests (test/test_python_isolation.py:228)
cgroup_invalid gains the \0 case; rootfs_invalid covers '', /, //, ///, relative, and embedded-NUL — the previously-invisible bypass is regression-guarded.

🟡 Minor (non-blocking, unchanged from before)

• /../ (path-traversal) is intentionally not rejected for rootfs. Agreed out-of-scope: a chroot target doesn't build a NUL-terminated path buffer the way the cgroup path does; the kernel resolves .. against the real FS at chroot/pivot_root time. NUL + collapse-to-/ are the relevant classes here.

Verdict

LGTM ✅ — the responses to the gemini thread are accurate (the // check runs on the trimmed length, and ////// cases are present in the test). NUL rejection, the %*s→%.*s/snprintf fix, and the ////// collapse bypass are all closed at both config and runtime with regression coverage. Nothing blocking. (I trust your live unitd + clean-compile verification for the build itself.)
· branch fix/conf-path-validation

[gemini-code-assist]

Code Review

This pull request introduces validation for the 'rootfs' and cgroup 'path' isolation options. Specifically, it rejects embedded NUL bytes in both options, ensures 'rootfs' is validated as an absolute path other than '/' at configuration time, and replaces an unsafe sprintf with snprintf in the cgroup path validation. Additionally, trailing slashes are now properly trimmed in a loop, and comprehensive unit tests have been added to verify these validation rules. I have no further feedback to provide as the changes are well-implemented and include appropriate test coverage.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.