Fix OS command injection via customCliArgs and harden settings validation - #395
Open
R0Wi wants to merge 1 commit into
Open
Fix OS command injection via customCliArgs and harden settings validation#395R0Wi wants to merge 1 commit into
R0Wi wants to merge 1 commit into
Conversation
…tion Follow-up to the `languages` command injection fix (#391): the same class of issue was still reachable through the `customCliArgs` workflow setting. `CommandLineUtils::escapeCustomCliArgs()` only stripped '&&' and ';', while the value is concatenated into a command string which is executed by mikehaertl/php-shellcommand via proc_open() as a raw shell string. Payloads using '$(...)', backticks, '|', '&', '>' or '<' therefore reached /bin/sh. Since the OCR operation is available for IManager::SCOPE_USER, any authenticated user could create a personal flow and execute arbitrary commands as the web server user once a matching file was processed. The same string is also forwarded to the remote backend as `ocrmypdf_parameters`. Changes: - CommandLineUtils now splits the custom CLI arguments into single tokens (honoring single/double quoted values) and quotes every token which contains anything but harmless characters via escapeshellarg(). Tokens consisting of harmless characters only are passed through unchanged, so the resulting commandline is byte identical for all valid inputs (also for the remote backend). The sidecar file path uses the same helper. - WorkflowSettings validates `customCliArgs` (string, no control characters, max length) and `ocrMode` (must be a known mode, otherwise the commandline parameter mapping is undefined). - WorkflowSettings::setProperty() now rejects a present but invalid value with an InvalidArgumentException instead of silently falling back to the default, and non-object JSON is rejected as invalid JSON. Operation surfaces the reason of the failed validation to the user. - ControllerBase logs exceptions and returns a generic error message instead of the internal exception text, which was reachable by any logged in user via the (NoAdminRequired) installed languages endpoint. - OcrBackendInfoController uses the #[NoAdminRequired] attribute instead of the deprecated annotation. - README documents that custom CLI arguments are passed as separate literal arguments without shell expansion. Adds regression tests for shell metacharacter payloads in customCliArgs on the CommandLineUtils and the PdfOcrProcessor level as well as tests for the stricter settings validation. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VAjZ1QPPCsKNxxkVN7ZMU8
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
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.
Follow-up to the
languagescommand injection fix (#391): the same class ofissue was still reachable through the
customCliArgsworkflow setting.CommandLineUtils::escapeCustomCliArgs()only stripped '&&' and ';', whilethe value is concatenated into a command string which is executed by
mikehaertl/php-shellcommand via proc_open() as a raw shell string. Payloads
using '$(...)', backticks, '|', '&', '>' or '<' therefore reached /bin/sh.
Since the OCR operation is available for IManager::SCOPE_USER, any
authenticated user could create a personal flow and execute arbitrary
commands as the web server user once a matching file was processed. The
same string is also forwarded to the remote backend as
ocrmypdf_parameters.Changes:
(honoring single/double quoted values) and quotes every token which
contains anything but harmless characters via escapeshellarg(). Tokens
consisting of harmless characters only are passed through unchanged, so
the resulting commandline is byte identical for all valid inputs (also
for the remote backend). The sidecar file path uses the same helper.
customCliArgs(string, no controlcharacters, max length) and
ocrMode(must be a known mode, otherwisethe commandline parameter mapping is undefined).
with an InvalidArgumentException instead of silently falling back to the
default, and non-object JSON is rejected as invalid JSON. Operation
surfaces the reason of the failed validation to the user.
instead of the internal exception text, which was reachable by any
logged in user via the (NoAdminRequired) installed languages endpoint.
of the deprecated annotation.
literal arguments without shell expansion.
Adds regression tests for shell metacharacter payloads in customCliArgs on
the CommandLineUtils and the PdfOcrProcessor level as well as tests for the
stricter settings validation.
Co-Authored-By: Claude noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01VAjZ1QPPCsKNxxkVN7ZMU8