-
Notifications
You must be signed in to change notification settings - Fork 18
hardening: enforce POST+CSRF for purge syslog devices utility #318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
somethingwithproof
wants to merge
3
commits into
Cacti:develop
Choose a base branch
from
somethingwithproof:fix/259-csrf-purge-utility
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
75e4ad6
hardening: enforce POST+CSRF for purge syslog devices utility
somethingwithproof 5fe348a
fix(csrf): distinct error codes, audit log on non-POST, honest lint h…
somethingwithproof 98cde21
fix(security): harden syslog bulk form and nav encoding
somethingwithproof File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| <?php | ||
| /* | ||
| * Source-scan lint for the #259 purge-syslog-hosts CSRF hardening. | ||
| * | ||
| * This is a lint, NOT a behavioral test. It asserts that the expected | ||
| * CSRF-enforcement snippets exist in setup.php so regressions that | ||
| * silently delete the POST/csrf_check/JS-post-flow are caught at CI | ||
| * time. A full behavioral test would require bootstrapping Cacti's | ||
| * session, CSRF token, and database, which this plugin's regression | ||
| * harness cannot currently provide. | ||
| * | ||
| * If the guard is refactored in a way that preserves the strings but | ||
| * breaks the logic, this lint will NOT catch it — follow-up issue for | ||
| * real behavioral coverage once a DB-backed test harness exists. | ||
| */ | ||
|
|
||
| $setup = file_get_contents(dirname(__DIR__, 2) . '/setup.php'); | ||
|
|
||
| if ($setup === false) { | ||
| fwrite(STDERR, "Failed to read setup.php\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| $required = array( | ||
| "if (\$_SERVER['REQUEST_METHOD'] !== 'POST')", | ||
| "if (function_exists('csrf_check'))", | ||
| "if (!csrf_check(false))", | ||
| "__csrf_magic: csrfMagicToken" | ||
| ); | ||
|
|
||
| foreach ($required as $snippet) { | ||
| if (strpos($setup, $snippet) === false) { | ||
| fwrite(STDERR, "Missing expected CSRF hardening snippet: $snippet\n"); | ||
| exit(1); | ||
| } | ||
| } | ||
|
|
||
| if (strpos($setup, "href='utilities.php?action=purge_syslog_hosts'") !== false) { | ||
| fwrite(STDERR, "Legacy GET purge link still present.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // Verify fail-closed: the else branch (csrf_check unavailable) must reject, not fall through. | ||
| // Assert globally safe properties rather than parsing the else block via brittle regex. | ||
|
|
||
| // The fallback path must not attempt manual token checking | ||
| if (strpos($setup, "\$_POST['__csrf_magic']") !== false) { | ||
| fwrite(STDERR, "Fallback CSRF branch must not check token presence; must fail closed.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // The fallback path must log the blocked attempt | ||
| if (strpos($setup, "cacti_log('WARNING: syslog purge blocked") === false) { | ||
| fwrite(STDERR, "Fail-closed branch must call cacti_log() to audit blocked purge attempts.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // Log message must name the specific failure reason for incident response | ||
| if (strpos($setup, 'CSRF validation unavailable') === false) { | ||
| fwrite(STDERR, "Log message must specify 'CSRF validation unavailable' for operational clarity.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // Verify JS confirm() uses json_encode, not __esc() inside JS string | ||
| if (preg_match("/confirm\(\s*'/", $setup)) { | ||
| fwrite(STDERR, "JS confirm() must use json_encode() for safe encoding, not __esc() in a quoted string.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| if (strpos($setup, 'json_encode(__(') === false) { | ||
| fwrite(STDERR, "Expected json_encode(__(...)) for JS-safe encoding of confirm message.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // Verify json_encode uses JSON_HEX_TAG to prevent </script> breakout in HTML script context | ||
| if (strpos($setup, 'JSON_HEX_TAG') === false) { | ||
| fwrite(STDERR, "json_encode() must use JSON_HEX_TAG to prevent script-context breakout.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| if (strpos($setup, 'JSON_HEX_AMP') === false) { | ||
| fwrite(STDERR, "json_encode() must use JSON_HEX_AMP to escape ampersands in script context.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| if (strpos($setup, 'JSON_HEX_APOS') === false) { | ||
| fwrite(STDERR, "json_encode() must use JSON_HEX_APOS.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| if (strpos($setup, 'JSON_HEX_QUOT') === false) { | ||
| fwrite(STDERR, "json_encode() must use JSON_HEX_QUOT.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // Verify user-facing messages do not expose CSRF internals (log messages may use "CSRF") | ||
| if (preg_match("/raise_message\\('syslog_[a-z_]*', __\\('CSRF/", $setup)) { | ||
| fwrite(STDERR, "User-facing raise_message must not expose CSRF internals to end users.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // Verify generic user-facing message is present | ||
| if (strpos($setup, "Invalid request. Please try again.") === false) { | ||
| fwrite(STDERR, "Fail-closed branch must use generic 'Invalid request. Please try again.' message.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // Verify fail-closed raise_message uses MESSAGE_LEVEL_ERROR severity | ||
| if (strpos($setup, "raise_message('syslog_csrf_unavailable', __('Invalid request. Please try again.', 'syslog'), MESSAGE_LEVEL_ERROR)") === false) { | ||
| fwrite(STDERR, "Fail-closed branch raise_message must use MESSAGE_LEVEL_ERROR severity.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // Verify log message does not expose internal function name | ||
| if (strpos($setup, 'csrf_check() unavailable') !== false) { | ||
| fwrite(STDERR, "Log message must not name internal validation function.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| echo "issue259_csrf_purge_test passed\n"; |
58 changes: 58 additions & 0 deletions
58
tests/regression/issue279_bulk_form_and_nav_encoding_test.php
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| <?php | ||
|
|
||
| $targets = array( | ||
| 'syslog_removal.php' => file_get_contents(__DIR__ . '/../../syslog_removal.php'), | ||
| 'syslog_alerts.php' => file_get_contents(__DIR__ . '/../../syslog_alerts.php'), | ||
| 'syslog_reports.php' => file_get_contents(__DIR__ . '/../../syslog_reports.php'), | ||
| 'syslog.php' => file_get_contents(__DIR__ . '/../../syslog.php'), | ||
| ); | ||
|
|
||
| foreach ($targets as $file => $contents) { | ||
| if ($contents === false) { | ||
| fwrite(STDERR, "Unable to read $file\n"); | ||
| exit(1); | ||
| } | ||
| } | ||
|
|
||
| foreach (array('syslog_removal.php', 'syslog_alerts.php', 'syslog_reports.php') as $file) { | ||
| if (strpos($targets[$file], "html_escape(get_request_var('drp_action'))") === false) { | ||
| fwrite(STDERR, "Expected escaped drp_action hidden field in $file\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| if (strpos($targets[$file], "rawurlencode(get_request_var('filter'))") === false) { | ||
| fwrite(STDERR, "Expected URL-encoded filter nav value in $file\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| if (strpos($targets[$file], "<input type='hidden' name='drp_action' value='\" . get_request_var('drp_action') . \"'>") !== false) { | ||
| fwrite(STDERR, "Legacy raw drp_action hidden field remains in $file\n"); | ||
| exit(1); | ||
| } | ||
| } | ||
|
|
||
| $syslog = $targets['syslog.php']; | ||
|
|
||
| if (strpos($syslog, "pageTab: <?php print json_encode(get_request_var('tab'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT);?>,") === false) { | ||
| fwrite(STDERR, "Expected JSON-encoded syslog pageTab value\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| foreach (array( | ||
| "json_encode(__('Enter a search term', 'syslog'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT)", | ||
| "json_encode(__('Select Device(s)', 'syslog'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT)", | ||
| "json_encode(__('Devices Selected', 'syslog'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT)", | ||
| "json_encode(__('All Devices Selected', 'syslog'), JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT)", | ||
| ) as $needle) { | ||
| if (strpos($syslog, $needle) === false) { | ||
| fwrite(STDERR, "Expected JS-safe initSyslogMain text encoding\n"); | ||
| exit(1); | ||
| } | ||
| } | ||
|
|
||
| if (strpos($syslog, "pageTab: '<?php print get_request_var('tab'); ?>'") !== false) { | ||
| fwrite(STDERR, "Legacy raw pageTab JS assignment still present\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| echo "OK\n"; |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR description mentions an "__csrf_magic fallback if csrf helper is unavailable", but the current else-branch when csrf_check() is unavailable hard-blocks the purge action. Either implement the documented fallback behavior (so purge can still work on supported environments without csrf_check) or update the PR description/tests to reflect the intentional fail-closed behavior.