mirror of
https://github.com/Cacti/cacti.git
synced 2026-11-10 14:33:25 +09:00
4e5b2852dc
* security(batch12): constant-time 2FA cookie and reset-token compares (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). * security(batch12): route package-repo manifest fetches through cacti_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). * security(batch12): shed matched quotes around resolved data-input tokens (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. * security(batch12): regression tests + CHANGELOG for batch-12 develop 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. * security(batch12): update batch-11 fq9x regression test for quote-aware 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. * security(batch12): narrow reset-hash row with is_array for PHPStan L6 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. * security(batch12): drop the executable bit on the new security test files * security(batch12): route import-time repo manifest fetches through cacti_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. * security(batch12): make substitute_script_path fully quote-aware (GHSA-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). * security(batch12): relocate batch-12 regressions into vuln-class subfolders 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. * security: preserve template quoting for raw path tokens in substitute_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. * security: keep substituting a token that follows a backslash in substitute_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. * security: normalize dangling backslash before substituted value in substitute_script_path (GHSA-5v3j) A template backslash immediately before a <field> token (\<arg1>) escaped the already-escaped value's opening quote, producing \'...' which the shell rejects with an unmatched-quote error (odd backslash runs) or prepended a stray literal backslash (even runs). The quote-aware scanner now sheds one backslash when the run immediately preceding a self-quoted value is odd, in both the unquoted in-place branch and the close-around branch, so the value's own quotes always function as the shell's quotes and $(...)/backticks inside it can never re-activate. The backslash regression test now executes each substituted template through /bin/sh (printf %s) and asserts a clean parse plus a literal round-trip across odd/even backslash counts, rather than pinning the generated string. * fix(security): preserve literal backslashes in quoted script fields Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test(security): cover changed guards and gate PR coverage Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * ci: measure committed PR changes on a policy-compliant runner Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * security: address review - nested cmd-sub/backtick handling, coverage gate, test skip substitute_script_path() now follows the template's own $(...) and backtick command substitutions as nested parsing contexts with their own quote state, so a token placed inside one ("$(printf %s <field>)") is judged against that fresh context instead of a suspended outer double quote. This closes the nested-reinterpretation bypass where an escaped value sitting inside a command substitution could be re-activated by an outer double quote. Backslash pairing in both the unquoted and double-quoted contexts now also neutralizes $ and backtick, so an escaped command substitution (\$(...) or \`...`) is not mis-detected as live. .github/workflows/pr-changed-coverage.yml: drop the hard-coded PR-number allowlist so the job runs whenever the path filter selects it, rather than silently skipping every unlisted PR once these merge. tests/Unit/Security/ShellInjection/ScriptFieldShellRoundTripTest.php: skip when either exec() or proc_open() is unavailable (the test launches the fixture with exec() while the fixture itself uses proc_open()). The fixture adds 7 nested command-substitution and backtick round-trip cases (90 -> 97), each proving the field value echoes back verbatim with nothing inside it executing. * fix(security): clear quote-empty flag on single-quote content; cover new parser branches The stack-based substitute_script_path() rewrite left quoteEmpty set while emitting literal content inside a single-quoted region, so a template like '\<field>' wrongly took the exact-wrap branch and dropped the backslash (ScriptSubstitutionMatrix 'shell 56'). Clear the frame's quoteEmpty whenever a single-quoted character is emitted, matching the pre-rewrite fallthrough. ScriptSubstitutionMatrix.php: add 8 explicit parser cases exercising the \/backtick nested contexts and escaped \$ / backtick pairing so every rewritten line is covered by the changed-coverage probe (1285 -> 1293). * style: short array syntax and php-cs-fixer formatting in substitute_script_path * test(security): tolerate formatting in spikekill escaping contract Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(spikekill): bind menu requests and protect mutations on develop Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test(spikekill): validate real CSRF tokens and inline coverage Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * ci(coverage): install locked dependencies for Symfony CSRF probes Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * perf(spikekill): skip disabled float debug formatting Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(spikekill): enforce mutation scope and propagate RRD failures Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(spikekill): escape repair notes and cover window boundaries Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: remove unused SpikeKill preference binding Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(security): reject unsupported nested shell grammar in #8237 (#8359) * fix(security): reject unsupported nested shell grammar Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test(security): exercise nested grammar rejection in CI coverage Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(security): reject script fields in backtick substitutions Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: retain native security fixture evidence in changed coverage Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(security): reject ambiguous shell expansion contexts Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test(ssrf): assemble insecure manifest schemes at runtime The SSRF negative-test inputs in PackageManifestMatrix.php are rejected by the import preflight policy, but storing them as clear-text URL literals (http://, ftp://, file://) tripped SonarCloud php:S5332 and dropped the New Code security rating to B. Join the scheme separator at runtime so the fixture no longer carries clear-text literals while the assembled values keep the exact insecure schemes the policy must reject. All 32 manifest cases still pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(spikekill): drop dangling href in removeSpikes redirect check The CSRF POST rewrite of removeSpikes replaced the GET href builder but left checkForRedirects(data, href) referencing the now-undefined href, throwing ReferenceError once a POST completed. Call checkForRedirects(data) so a redirect response reloads (a POST has no GET href to follow). Update the merged dom_dynamic_boundaries boundary test to exercise the POST path: define csrfMagicToken and mock $.post instead of $.getJSON, so it still verifies DOMPurify sanitization of spike results. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <copilot@users.noreply.github.com> Co-authored-by: TheWitness <1439914+TheWitness@users.noreply.github.com> Co-authored-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
74 lines
2.5 KiB
YAML
74 lines
2.5 KiB
YAML
name: PR changed-code coverage
|
|
|
|
on:
|
|
pull_request:
|
|
paths:
|
|
- 'auth_2fa.php'
|
|
- 'auth_resetpassword.php'
|
|
- 'package_import.php'
|
|
- 'package_repos.php'
|
|
- 'lib/functions.php'
|
|
- 'lib/html.php'
|
|
- 'include/layout.js'
|
|
- 'include/css/graph_drilldown.css'
|
|
- 'include/themes/**/main.css'
|
|
- 'tests/Helpers/FixtureCoverage.php'
|
|
- 'tests/Helpers/SourceSlice.php'
|
|
- 'tests/fixtures/Core/**'
|
|
- 'tests/e2e/tests/**'
|
|
- 'tests/tools/report_changed_coverage.py'
|
|
- 'tests/tools/run_pr_change_tests.sh'
|
|
- '.github/workflows/pr-changed-coverage.yml'
|
|
workflow_dispatch:
|
|
inputs:
|
|
base_commit:
|
|
description: Base commit for changed-line coverage
|
|
required: true
|
|
|
|
permissions:
|
|
contents: read
|
|
|
|
jobs:
|
|
changed-coverage:
|
|
runs-on: ubuntu-24.04
|
|
timeout-minutes: 15
|
|
steps:
|
|
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1
|
|
with:
|
|
fetch-depth: 0
|
|
ref: ${{ github.event.pull_request.head.sha || github.sha }}
|
|
persist-credentials: false
|
|
- name: Set up PHP and Cacti test dependencies
|
|
uses: ./.github/actions/setup-php-composer
|
|
with:
|
|
php-version: '8.3'
|
|
extensions: intl, mysql, gd, ldap, gmp, xml, curl, json, mbstring, dom
|
|
coverage: xdebug
|
|
ini-values: memory_limit=1G
|
|
dependency-mode: locked-tests
|
|
install-args: --prefer-dist --no-progress --no-interaction
|
|
- uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1
|
|
with:
|
|
node-version: '22'
|
|
- name: Install browser test dependencies
|
|
if: hashFiles('tests/e2e/tests/fixture-coverage.js') != ''
|
|
working-directory: tests/e2e
|
|
run: |
|
|
set -euo pipefail
|
|
npm ci --ignore-scripts
|
|
npx --no-install playwright install --with-deps chromium
|
|
- name: Run changed-code tests and require 100 percent changed-line coverage
|
|
env:
|
|
BASE_COMMIT: ${{ github.event.pull_request.base.sha || inputs.base_commit }}
|
|
CACTI_COVERAGE_DIR: ${{ runner.temp }}/cacti-changed-coverage
|
|
run: bash tests/tools/run_pr_change_tests.sh "$BASE_COMMIT"
|
|
- name: Preserve coverage evidence
|
|
if: always()
|
|
uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9
|
|
with:
|
|
name: changed-code-coverage
|
|
path: |
|
|
${{ runner.temp }}/cacti-changed-coverage
|
|
tests/e2e/test-results
|
|
if-no-files-found: warn
|