security: replace eval() in RPN binary math with safe dispatch (GHSA-vr4v-qvqm-gm9j) - #807
security: replace eval() in RPN binary math with safe dispatch (GHSA-vr4v-qvqm-gm9j)#807bmfmancini wants to merge 3 commits into
Conversation
somethingwithproof
left a comment
somethingwithproof
left a comment
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
1e29747 to
e19eb4e
Compare
|
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. |
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)+,-,*,/,%