security: replace eval() in RPN binary math with safe dispatch (GHSA-vr4v-qvqm-gm9j) - #807
Merged
Merged
Conversation
TheWitness
previously approved these changes
Aug 17, 2026
somethingwithproof
previously requested changes
Aug 17, 2026
somethingwithproof
left a comment
somethingwithproof
left a comment
Member
There was a problem hiding this comment.
This needs isolation and behavioral coverage before merge. #773 already replaces the same binary eval block, so merging both independently risks one implementation stomping the other. Rebase on the selected implementation, retain one safe dispatch, and add Cacti-Composer/Pest cases for operand order, + - * / % ^, 0/0, divide-by-zero, modulo-by-zero, and an invalid operator. The current implementation's default return of 0 silently masks an impossible operator, and all checks are red.
…1523) Replace the eval()-based binary arithmetic evaluation at line 378 with a new thold_rpn_math_binary() helper function that uses a switch statement for +, -, *, /, %, ^ operators. The operator was already whitelisted via in_array(, , true) and operands validated with is_numeric(), but removing eval() eliminates the code injection risk entirely (CWE-95). Addresses SonarQube finding: php:S1523 at thold_functions.php:378
bmfmancini
force-pushed
the
advisory-fix-eval-math
branch
from
August 17, 2026 22:12
1e29747 to
e19eb4e
Compare
Member
|
The focused tests and green CI are useful progress. Two blockers remain: #773 already changes this exact evaluator, so this must be rebased onto whichever implementation is selected; and the invalid/default path still returns numeric 0, converting an evaluator error into a plausible threshold value. Please make that path unambiguously fail closed and add divide-by-zero/modulo-by-zero coverage. Also split the shared workflow retry/matrix edits into one dedicated PR instead of repeating them across each security patch. |
xmacan
previously approved these changes
Sep 15, 2026
TheWitness
dismissed
somethingwithproof’s stale review
September 20, 2026 01:03
The unsafe eval was removed.
Same set of pre-existing gaps as develop (covered by tests but never
implemented in thold_functions.php), plus one bug specific to this PR:
- get_allowed_thresholds()/get_allowed_threshold_logs(): bind graph_id and
caller-supplied params instead of interpolating them into the SQL text,
and use the *_prepared fetchers.
- thold_replace_threshold_tags(): add a \ flag that quotes
device/threshold free-text values with cacti_escapeshellarg() when the
substituted text is a trigger command.
- thold_command_execution(): pass shell=true for the three trigger
commands, and fix a topic-string typo ('thold' vs 'thold_cmd') that
silently suppressed exit-status logging for inline command runs.
- thold_expression_math_rpn(): remove a stray break that exited the
operator switch before pushing the 0/0 result, flag modulo-by-zero
instead of letting PHP throw DivisionByZeroError, reject non-numeric
unary operands before eval(), and flag SQRT/LOG results that are NAN or
INF instead of pushing them onto the stack (unary math here is still
eval()-based; unrelated to this PR's binary-math dispatcher).
- thold_rpn_math_binary(): this PR's new dispatcher mapped '^' to pow(),
silently changing it from the pre-existing bitwise XOR behaviour
(eval('... ^ ...') always computed XOR). Restored XOR to avoid breaking
existing user thresholds, and corrected this PR's own
TholdRpnBinaryMathTest 'power' dataset, which had asserted the
incorrect pow() behaviour.
thold_functions.php includes includes/arrays.php with a plain include() (not include_once()) from several of its own functions (e.g. thold_log()), keyed off \['base_path']. Once any test in the shared Pest process exercised one of those call sites, PHP's include-once registry considered the file already included: a later thold_test_load() (require_once) on the same resolved path silently no-op'd and never (re)published \ to \, depending on which order the test files happened to run in. This intermittently broke every test that reads \ (TholdReplaceThresholdTagsTest's THOLDTYPE substitution) as well as tests several call frames downstream of thold_log()/thold_check_threshold() (NotificationEmailDeduplicationTest, ThresholdTimeBasedCharacterizationTest), depending on file execution order. Add thold_test_load_always()/loadPluginSourceAlways(), which use a plain require() instead of require_once(), for plugin files that only assign file-scope variables (no function/class declarations) and are therefore safe to load more than once. Switch the four test classes that load includes/arrays.php to the new helper.
cigamit
previously approved these changes
Sep 20, 2026
…lures" This reverts commit 21fbd16.
…math # Conflicts: # CHANGELOG.md # thold_functions.php
Merging develop brought in #796's fix for thold_check_threshold()'s type-2 (time-based) restoral logic (`<` -> `>=` for the warning/alert fail-count comparisons). This test harness always stubs the log-derived fail count to 0, and the two restoral tests here relied on the default trigger of 1, so under the corrected operator (0 >= 1 is false) neither restoral branch fired anymore and both tests broke. Override time_fail_trigger/time_warning_fail_trigger to 0 in both tests, representing a threshold with no minimum streak requirement, so the harness's stubbed zero count still satisfies the corrected `>=` check and the tests exercise real restoral behavior again instead of the inverted bug.
xmacan
approved these changes
Sep 21, 2026
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 Fix — GHSA-vr4v-qvqm-gm9j
Fixes GHSA-vr4v-qvqm-gm9j
Summary
The
thold_expression_math_rpn()function inthold_functions.phpused PHP'seval()to evaluate binary arithmetic operations (line 378) during RPN expression parsing for threshold expressions.Vulnerability Details
php:S1523(SonarQube)thold_functions.php, line 378Changes
Replaced
eval()with a safe arithmetic dispatch functionthold_rpn_math_binary()that uses a switch statement to perform binary math operations (+,-,*,/,%) without dynamic code execution.Testing
php -l thold_functions.php)+,-,*,/,%