fix(server): allow newlines in exec command arguments#1965
Conversation
Single-quoted POSIX strings treat newlines as literal characters, not command separators. The newline rejection prevented multi-line scripts passed via sandbox exec (e.g. python3 -c with a multi-line body). Null byte rejection is preserved since null bytes truncate C-level shell parsing. Closes NVIDIA#1963 Signed-off-by: Adel Zaalouk <azaalouk@redhat.com>
Add reject_null_bytes() for exec command argument validation, replacing reject_control_chars() which also rejected newlines. Multi-line scripts passed to bash -c or python3 -c are legitimate and safe because shell_escape() wraps them in POSIX single quotes where newlines are literal. Newline rejection is preserved for workdir (no legitimate path contains newlines) and environment values. Closes NVIDIA#1963 Signed-off-by: Adel Zaalouk <azaalouk@redhat.com>
Split reject_control_chars into reject_null_char and reject_newline_chars so callers can compose the checks they need. reject_control_chars now delegates to both. Exec command arguments call reject_null_char directly. Addresses review feedback from @elezar on PR NVIDIA#1965. Signed-off-by: Adel Zaalouk <azaalouk@redhat.com>
|
nit: |
|
@zanetworker is this PR still active? |
- Escape command_preview in both relay and interactive exec logging paths to prevent log injection via user-controlled CR/LF - Update validate_exec_request_fields doc comment to clarify that command args only reject NUL while env/workdir reject both - Rename test to validate_exec_rejects_newlines_in_workdir to match what it actually exercises - Replace multiline script test with CR/LF + single-quote combo to cover the POSIX escaping interaction Signed-off-by: Adel Zaalouk <azaalouk@redhat.com>
|
@TaylorMutch yes, applied the changes requested. |
|
/ok to test 32add0c |
|
Thanks for the updates. The core validation change looks sound, and the targeted tests pass locally. One requested change still appears to be missing from the current head ( Now that command arguments may contain CR/LF, both let command_preview: String = command
.chars()
.take(120)
.flat_map(char::escape_default)
.collect();Please also add the previously requested regression case combining CR/LF with an embedded single quote—for example, The latest commit message indicates these were addressed, but neither change appears in the current diff. Once those are included and CI passes, this should be ready for another review. |
|
@zanetworker please have a look when you get a chance. Probably just the change above is needed then we can merge this in. |
Escape CR/LF in command_preview before logging in both relay exec paths to prevent log injection via user-controlled newlines. Add regression test combining CR/LF with embedded single quotes to verify POSIX escaping composes correctly with multiline scripts. Signed-off-by: Adel Zaalouk <azaalouk@redhat.com>
|
@TaylorMutch updated. |
|
/ok to test zanetworker@e30ac3b |
Summary
Allow newline characters in
sandbox execcommand arguments so multi-line scripts (python3 -c "def f():\n ...",bash -c "echo 'line1\nline2'") work without base64 workarounds.Related Issue
Fixes #1963
Changes
shell_escape()(sandbox.rs): Remove newline rejection. Newlines inside POSIX single quotes are literal characters, not command separators. Null byte rejection preserved.validate_exec_request_fields()(validation.rs): Addreject_null_bytes()for exec command arguments instead ofreject_control_chars(). Newline rejection preserved for workdir and environment values.Testing
Verified end-to-end against a live gateway with podman driver on macOS:
cargo test --package openshell-server— 857 tests pass, 0 failuressandbox exec— workssandbox exec— worksChecklist