Skip to content

security: fix filter SQLi and add CSRF protection to servcheck request handling - #90

Open
somethingwithproof wants to merge 4 commits into
Cacti:mainfrom
somethingwithproof:fix/servcheck-input-validation
Open

security: fix filter SQLi and add CSRF protection to servcheck request handling#90
somethingwithproof wants to merge 4 commits into
Cacti:mainfrom
somethingwithproof:fix/servcheck-input-validation

Conversation

@somethingwithproof

@somethingwithproof somethingwithproof commented Aug 30, 2026

Copy link
Copy Markdown
Member

Two request-security fixes found in a review of the plugin (the first on this repo).

SQL injection via the filter search (CA, proxy, credential pages)

servcheck_ca.php, servcheck_proxy.php, and servcheck_credential.php registered the filter request variable as FILTER_DEFAULT (no sanitization) and concatenated it raw into a LIKE clause, so a servcheck-realm user could inject SQL through the filter GET parameter (the CA query was also malformed). servcheck_test.php already does this correctly with sanitize_search_string; this aligns the other three and additionally builds the LIKE value with db_qstr, closing the injection regardless of the sanitizer, and fixing the broken CA search.

CSRF on the enable / disable / purge GET actions

servcheck_test.php ran enable, disable, and purge (which deletes a check's log history) from a plain GET with no CSRF check — a crafted link could disable monitoring or purge history when an authenticated admin loaded it. The action now validates csrf_guard()->validate() against the __csrf_magic token, which the action links carry via csrf_get_tokens() (Cacti's own CSRF API; csrf_check() only guards POST).

Both post-auth (servcheck admin realm). php -l and php-cs-fixer clean; no behaviour change for a normal search or a legitimately-clicked action.

Closes #92

…edential pages

These three list pages registered the filter request var as FILTER_DEFAULT and
concatenated it raw into a LIKE clause, so a servcheck-realm user could inject
SQL through the filter parameter (the CA query was also malformed). Register
filter with sanitize_search_string, matching servcheck_test.php, and build the
LIKE value with db_qstr.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
These state-changing actions ran from a plain GET with no CSRF check, so a
crafted link could disable a check or purge its history when an authenticated
admin loaded it. Validate csrf_guard()->validate() against the __csrf_magic
token, which the action links now carry via csrf_get_tokens().

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof somethingwithproof changed the title security: sanitize and bind the filter search (CA, proxy, credential pages) security: fix filter SQLi and add CSRF protection to servcheck request handling Aug 30, 2026
Cacti core now requires PHP >= 8.2, so the 8.1 integration job fails the
composer platform check; Ubuntu Noble also lacks libapache2-mod-php for
non-native versions without the ondrej PPA.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

servcheck_ca.php still stores request vars under the Proxy session key, which can cause cross-page filter/sort state collisions and should be corrected.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR strengthens request-handling security in the servcheck Cacti plugin by closing a SQL injection vector in list-page filtering and adding CSRF validation to previously unprotected state-changing GET actions.

Changes:

  • Add CSRF token generation and validation for enable, disable, and purge actions on the Service Checks page.
  • Sanitize the filter request variable and SQL-quote the LIKE parameter for CA, Proxy, and Credential list searches.
  • Update CI workflow PHP matrix and apt repository setup; document the security fixes in the changelog.
File summaries
File Description
servcheck_test.php Adds CSRF token validation for GET actions and propagates CSRF token in action links/JS.
servcheck_proxy.php Sanitizes filter input and safely builds LIKE expressions using db_qstr().
servcheck_credential.php Sanitizes filter input and safely builds LIKE expressions using db_qstr().
servcheck_ca.php Sanitizes filter input and safely builds LIKE expressions using db_qstr() (but see stored comment re: session key).
CHANGELOG.md Records the two security hardening changes under develop.
.github/workflows/plugin-ci-workflow.yml Adjusts CI PHP versions and adds Ondřej PHP PPA before installing system packages.
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread servcheck_ca.php
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix filter SQLi and add CSRF protection to servcheck request handling

2 participants