Repository navigation
security: batch 12 (develop) - GHSA-4qx8 timing, GHSA-54fg SSRF, GHSA-5v3j data-input RCE, GHSA-vhh2 confirm - #8237
Open
TheWitness wants to merge 12 commits into
Open
TheWitness wants to merge 12 commits into
TheWitness wants to merge 12 commits into
Conversation
added 4 commits
October 8, 2026 18:58
…(GHSA-4qx8-jj2h-p7q2) auth_2fa.php used === to compare the 2FA bypass-cookie HMAC and auth_resetpassword.php used != for the reset-token check; both are byte-by-byte and leak timing. Switch to hash_equals() so the comparison is constant-time. Develop-only paths (no released 1.2.x equivalent).
…http (GHSA-54fg-q9h9-88mm) package_repos.php fetched the package.manifest for the GitHub and Direct-URL (repo_type 0/2) repository types with raw file_get_contents() - the Direct-URL path additionally disabled SSL verification - which bypassed the hardened cacti_http() client and allowed SSRF (cloud metadata, internal host/port probing) plus MITM. Both paths now use cacti_http(), which restricts the scheme to http/https, blocks private/loopback/link-local targets, pins DNS to prevent rebinding, and verifies TLS. The GitHub Bearer token is passed via the headers option. Develop-only (package_repos.php does not exist in released 1.2.x).
…ens (GHSA-5v3j-wcrr-jxjg) Field values are escaped with cacti_escapeshellarg(), which wraps them in single quotes. When a Data Input template author placed the <field> placeholder inside a matched pair of quotes (e.g. "<arg1>"), those surrounding quotes made the value's own quoting literal, so $(...) or backticks in the value executed via the shell on the next poll - a realm-3 user reaching OS command execution, bypassing the CVE-2026-39902 hardening and not addressed by the single-pass fq9x fix. substitute_script_path() now consumes one matched pair of surrounding quotes when a token resolves, so the escaped value's own quoting is authoritative. Unresolved tokens keep their literal form (including any quotes), and the single-pass behaviour that fixed GHSA-fq9x is retained. Affects released 1.2.x (<= 1.2.31) and develop; this is the develop fix, to be backported to 1.2.x.
…fixes Add tests/Unit/Security/Batch12SecurityRegressionTest.php guarding the batch-12 sinks (GHSA-4qx8, GHSA-54fg, GHSA-5v3j) with a behavioral proof that a double-quoted Data Input placeholder sheds its quotes, plus a confirmation test that poller_spikekill.php escapes --rrdfile (GHSA-vhh2, already fixed in #7235). List the four GHSAs in the CHANGELOG.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Remote import SSRF and quoted-placeholder command injection remain reachable, and an existing regression test will fail.
5 open findings
What changed in this PR
Hardens authentication comparisons, repository fetching, and data-input command substitution for four security advisories.
Changes:
- Uses constant-time authentication token comparisons.
- Routes repository probes through the SSRF-hardened HTTP client.
- Adds quote-aware command substitution and security regression tests.
| File | Description |
|---|---|
auth_2fa.php |
Uses hash_equals() for 2FA cookies. |
auth_resetpassword.php |
Uses constant-time reset-token comparisons. |
package_repos.php |
Hardens repository reachability probes. |
lib/functions.php |
Changes data-input token substitution. |
tests/Unit/Security/Batch12SecurityRegressionTest.php |
Adds advisory regression coverage. |
CHANGELOG |
Documents the security changes. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
added 3 commits
October 8, 2026 19:33
…re substitute_script_path The GHSA-5v3j quote-shedding changed substitute_script_path()'s regex and capture group, which the batch-11 fq9x test pinned verbatim. Update those two assertions to the quote-aware pattern marker and $matches[2]; the single-pass guarantee the test covers is unchanged.
The hash_equals() guard left $hash inferred as non-empty-array|true, so $hash['user_id'] tripped PHPStan level 6. Guard with is_array() instead of a truthy check; db_fetch_row_prepared returns false for no row, so behavior is unchanged.
TheWitness
pushed a commit
that referenced
this pull request
Oct 8, 2026
…5v3j) + spikekill confirm (GHSA-vhh2)
GHSA-5v3j-wcrr-jxjg: a Data Input template that wraps a <field>
placeholder in a matched pair of quotes ("<arg1>") neutralized the
value's own cacti_escapeshellarg() single-quoting, letting $(...) or
backticks in a realm-3 user's field value execute on the next poll
(bypassing the CVE-2026-39902 hardening). substitute_script_path() now
consumes one matched pair of surrounding quotes when a token resolves,
so the escaped value's quoting stays authoritative; unresolved tokens
keep their literal form and the single-pass GHSA-fq9x behaviour holds.
GHSA-vhh2-gghg-whjg: add a confirmation regression test that the
spikekill poller escapes the --rrdfile path (fixed in #7235).
Tests: tests/Unit/Security/DataInputQuotedPlaceholderTest.php (source +
behavioral) and tests/Unit/Security/SpikekillRrdfileEscapeTest.php.
Backport of the develop batch-12 fix (PR #8237).
…cti_http (GHSA-54fg) get_repo_file() still used raw file_get_contents() for the GitHub and Direct-URL repo types, bypassing the SSRF hardening added to form_save(). Route both remote branches through cacti_http() (scheme/host validation, private-range block, DNS pinning, TLS verify; Bearer token via the headers option) and add a regression assertion covering the import sink.
…A-5v3j)
The matched-pair quote shed only neutralised a token the template wrapped
exactly ("<arg1>"). A token embedded in a larger quoted word ("prefix<arg1>",
"<arg1>suffix", "<a><b>") kept the outer double quotes, so backticks/$(...) in
the already-escaped value stayed live - the bypass Copilot flagged.
Replace the regex with a single-pass shell-quote-state scanner: it still sheds
an exact wrapping pair, but for any token inside a larger quoted word it briefly
closes the template's quoting around the escaped value so it cannot be
re-interpreted. Values are never re-scanned (single-pass, GHSA-fq9x retained).
Updates the batch-11/12 source assertions, adds prefix/suffix/mixed regression
cases, and drops the Batch12 eval fallback (the unit bootstrap already loads
lib/functions.php via include/global.php).
bmfmancini
approved these changes
Oct 9, 2026
bmfmancini
previously approved these changes
Oct 9, 2026
TheWitness
added a commit
that referenced
this pull request
Oct 9, 2026
…A-5v3j) Backport of the develop fix (#8237). The matched-pair quote shed only neutralised a token the template wrapped exactly ("<arg1>"). A token embedded in a larger quoted word ("prefix<arg1>", "<arg1>suffix", "<a><b>") kept the outer double quotes, so backticks/$(...) in the already-escaped value stayed live. Replace the regex with a single-pass shell-quote-state scanner: it still sheds an exact wrapping pair, but for any token inside a larger quoted word it briefly closes the template's quoting around the escaped value so it cannot be re-interpreted. Values are never re-scanned (single-pass, GHSA-fq9x retained). Updates the source assertions in DataInputQuotedPlaceholderTest and ShellInjection/ShellInjectionRegressionTest, and adds prefix/suffix/mixed regression cases. The eval-extract fallback is kept here because the 1.2.x unit bootstrap does not load lib/functions.php (unlike develop).
…olders Split tests/Unit/Security/Batch12SecurityRegressionTest.php into behavior-named files under the existing vulnerability-class layout, per the review feedback: Auth/TwoFaCookieAndResetTokenConstantTimeTest.php GHSA-4qx8 Ssrf/PackageRepoManifestFetchTest.php GHSA-54fg ShellInjection/DataInputQuotedPlaceholderTest.php GHSA-5v3j ShellInjection/SpikekillRrdfileEscapeTest.php GHSA-vhh2 Each file reaches the repo root via dirname(__DIR__, 4) for its one subfolder-deep location. No assertion changes; pure relocation.
…_script_path Keep the develop fix in sync with the 1.2.x backport (#8238): substitute_script_path() only sheds/closes the template's quotes for self-quoting (cacti_escapeshellarg) values. Trusted raw path_* tokens are substituted in place so a quoted "<path_php_binary>" keeps its quotes for the shell and a bare <path_cacti>/scripts/x.php stays unquoted for the PHP script server, which resolves the first token as a filesystem path. Reformatted per php-cs-fixer and added a raw-path-token regression test.
…itute_script_path Mirror the 1.2.x fix (#8238): the scanner's backslash branches consumed the next character unconditionally, so a legacy/custom template token such as \<arg> never reached the matcher and the trailing cleanup stripped it to a bare backslash. Both branches now only pair the backslash with a following quote/backslash (so an escaped quote still cannot toggle quote-state); any other character - notably a <token> start - leaves the backslash on its own and is processed on the next iteration. Reformatted per php-cs-fixer and added a backslash-before-token regression test.
bmfmancini
approved these changes
Oct 9, 2026
This branch has not been deployed
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.
Security batch 12 (develop)
Consolidated fixes for four privately-reported advisories. Each has a guard
under
tests/Unit/Security/in a vulnerability-class subfolder, plus a CHANGELOGentry.
hash_equals()instead of===/!=inauth_2fa.phpandauth_resetpassword.phpfile_get_contents, TLS verification disabled)package_repos.php::form_save) and the import-time read (package_import.php::get_repo_file) throughcacti_http()(scheme/host validation, private-range block, DNS pinning, TLS verify); Bearer token via theheadersoptionpackage_repos.phpabsent in released 1.2.x)<field>placeholder in quotes ("<arg1>") — or embeds it in a larger quoted word ("prefix<arg1>") — neutralizescacti_escapeshellarg()'s own quoting, so$(...)/backticks in a realm-3 user's field value execute on the next pollsubstitute_script_path()is now a single-pass, quote-aware scanner: it sheds an exact wrapping pair and, for a token embedded inside a larger quoted word, briefly closes the template's quoting around the already-escaped value so it cannot be re-interpreted (single-pass fq9x behaviour retained)--rrdfilepathcacti_escapeshellarg($f)); this batch adds a confirmation regression test onlyTests
Regressions live under the existing vulnerability-class layout:
tests/Unit/Security/Auth/TwoFaCookieAndResetTokenConstantTimeTest.php— GHSA-4qx8 constant-time compares.tests/Unit/Security/Ssrf/PackageRepoManifestFetchTest.php— GHSA-54fg: both the save-time and import-time repo fetches route throughcacti_http()and drop rawfile_get_contents/verify_peer.tests/Unit/Security/ShellInjection/DataInputQuotedPlaceholderTest.php— GHSA-5v3j: a double-quoted placeholder sheds its quotes, a token embedded in a larger quoted word keeps the value safely quoted, bare/unknown tokens are unchanged, and the single-pass fq9x behaviour is retained.tests/Unit/Security/ShellInjection/SpikekillRrdfileEscapeTest.php— GHSA-vhh2: the--rrdfilepath is escaped.tests/Unit/Security/Batch11SecurityRegressionTest.phpis updated to assert thequote-aware scanner markers rather than the obsolete regex.
Deliberately not in this batch
tracked develop CSRF epic. A correct fix couples the guard change with
converting the affected action links to POST (the "guard and links move
together" rule from batch 11); a guard-only patch would regress the plugins and
clog UIs, so it is intentionally excluded here.
CVE status
Verified:
php -lon all changed files; the quote-aware scanner's output waschecked under
/bin/shto confirm the escaped value stays a literal argument forevery token shape; behavioral + source assertions pass (Pest runs in CI).