Harden command execution and state-changing API routes - #103
Open
dupremathieu wants to merge 6 commits into
Open
dupremathieu wants to merge 6 commits into
dupremathieu wants to merge 6 commits into
Conversation
Pacemaker.is_valid_host() interpolated the caller-supplied host into a bash -c command and ran it with shell=True, so a crafted --pinned-host/--preferred-host (or Cockpit plugin value) could execute arbitrary commands as root. Run crm node server with an argv list and match the host against the reported node names in Python instead. The True/False semantics are unchanged and the value never reaches a shell. Add unit tests covering valid, unknown and exact-match hosts plus shell metacharacter payloads such as $(id) and x; id; #. Signed-off-by: Mathieu Dupré <mathieu.dupre@savoirfairelinux.com>
find_resource() built a "crm status | grep | grep | awk" pipeline by concatenating the resource name and ran it through shell=True. The name reaches it from the console command's positional argument, so an argument like "$(id)" executed as root. Run ["crm", "status"] as an argv list and reproduce the pipeline in Python instead: match lines anchored at the resource name, keep the ones reporting Started, and return their last field, or None. The name goes through re.escape(), which the grep pattern did not. Signed-off-by: Mathieu Dupré <mathieu.dupre@savoirfairelinux.com>
console() passed its positional name straight to Pacemaker.find_resource(). Commit removing the shell from find_resource closes the injection there, but the cluster console entry point was the only caller that never validated the name, unlike set_metadata, add_colocation and add_pacemaker_remote. Run the existing _check_name() first, so 'x; id; #' or '$(id)' is refused before any subprocess, Pacemaker or libvirt call. The tests record find_resource and assert it is never reached, and that the recording itself works, so the guard cannot go vacuous unnoticed. Signed-off-by: Mathieu Dupré <mathieu.dupre@savoirfairelinux.com>
pinned_host/preferred_host are persisted to the shared Ceph image metadata and later re-read by root-run HA paths (enable_vm, clone) that hand them to node placement helpers. A value written once could therefore re-fire on every node acting on the VM. Validate both values with a strict host pattern before any RBD write, and validate them again when read back from the image, raising a clear ValueError on a bad value. The shell sink itself is fixed separately. Signed-off-by: Mathieu Dupré <mathieu.dupre@savoirfairelinux.com>
LibVirtManager.export_configuration() interpolated the caller-supplied domain and destination path into a "virsh ... > path" string and ran it with shell=True, so a crafted domain such as "$(id)" could execute arbitrary commands as root. It is reachable through the libvirt_cmd export subcommand. Run virsh with an argv list, capture stdout and write it to the destination path in Python. check=True still propagates failures, stderr is still inherited and the XML is written byte-for-byte. Add unit tests covering the argv list, shell metacharacters kept in a single argument and error propagation, using a stubbed virsh and no libvirt daemon. Signed-off-by: Mathieu Dupré <mathieu.dupre@savoirfairelinux.com>
/stop/<guest> and /start/<guest> answered GET, so any page a browser
loaded could trigger them cross-site once the API was reachable. They
are now POST-only, which makes Flask answer 405 to a GET; the read-only
/ and /status/<guest> routes stay on GET.
The existing CSRFProtect was never functional: it had no secret key and
no route issuing tokens, so it did nothing while every route was a GET,
and it would have turned every POST into a 500 ("A secret key is
required to use CSRF"). It is removed; the stateless API relies on the
authenticated nginx in front of it, which the module and the overview
now document.
The tests POST on the success paths and add one 405 check per
state-changing route, with the backend monkeypatched to fail if a GET
reaches it.
Signed-off-by: Mathieu Dupré <mathieu.dupre@savoirfairelinux.com>
dupremathieu
force-pushed
the
security-fixes
branch
from
October 4, 2026 14:37
ccde8da to
971c1b5
Compare
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.
This series removes shell string interpolation from the value-handling paths
and tightens a few entry points.
Pacemaker.is_valid_host()andPacemaker.find_resource()no longer builda
bash -c "grep ..."command or acrm status | grep | awkpipeline. Theyrun
crmwith an argument list and match the value in Python, so a host orresource name such as
$(id)orx; id; #is treated as a literal.pinned_host/preferred_hostbeforepersisting them to RBD image metadata and again when reading them back.
Those values are consumed by root-run HA paths on other nodes.
LibVirtManager.export_configuration()runsvirshwith an argument listand writes the dump in Python, instead of a shell redirection.
console()validates the VM name with the existing_check_name()helperbefore touching subprocess, Pacemaker or libvirt.
/stopand/startas POST-only, so a cross-site GETcan no longer trigger them.
Tests are updated and added. The CI command
(
pytest tests/ --ignore=tests/test_vm_manager_cluster.py --ignore=tests/test_vm_manager_cmd_cluster.py)passes and coverage stays at 97%.