security: fix filter SQLi and add CSRF protection to servcheck request handling - #90
Open
somethingwithproof wants to merge 3 commits into
Open
Conversation
…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
changed the title
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>
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.
Two request-security fixes found in a review of the plugin (the first on this repo).
SQL injection via the
filtersearch (CA, proxy, credential pages)servcheck_ca.php,servcheck_proxy.php, andservcheck_credential.phpregistered thefilterrequest variable asFILTER_DEFAULT(no sanitization) and concatenated it raw into aLIKEclause, so a servcheck-realm user could inject SQL through thefilterGET parameter (the CA query was also malformed).servcheck_test.phpalready does this correctly withsanitize_search_string; this aligns the other three and additionally builds theLIKEvalue withdb_qstr, closing the injection regardless of the sanitizer, and fixing the broken CA search.CSRF on the enable / disable / purge GET actions
servcheck_test.phpranenable,disable, andpurge(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 validatescsrf_guard()->validate()against the__csrf_magictoken, which the action links carry viacsrf_get_tokens()(Cacti's own CSRF API;csrf_check()only guards POST).Both post-auth (servcheck admin realm).
php -land php-cs-fixer clean; no behaviour change for a normal search or a legitimately-clicked action.Closes #92