diff --git a/.github/workflows/php-unit-tests.yml b/.github/workflows/php-unit-tests.yml index 89eb1ad4..da6cba4a 100644 --- a/.github/workflows/php-unit-tests.yml +++ b/.github/workflows/php-unit-tests.yml @@ -72,8 +72,19 @@ jobs: # Thold contributes no parallel vendor directory or PHPUnit dependency. - name: Build Cacti test image run: | - docker build --tag cacti-web --file cacti-toolchain/docker/Dockerfile cacti-toolchain/docker - docker build --tag cacti-thold-test --file cacti-toolchain/docker/Dockerfile.test cacti-toolchain + for attempt in 1 2 3; do + if docker build --tag cacti-web --file cacti-toolchain/docker/Dockerfile cacti-toolchain/docker && \ + docker build --tag cacti-thold-test --file cacti-toolchain/docker/Dockerfile.test cacti-toolchain; then + exit 0 + fi + + if [ "$attempt" -lt 3 ]; then + sleep 10 + fi + done + + echo 'Cacti test image build failed after three attempts.' >&2 + exit 1 - name: Lint every PHP source file run: | diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index 5e4f3db6..73630b54 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -35,21 +35,12 @@ jobs: integration-test: runs-on: ${{ matrix.os }} - # A failure against the pinned release is a real failure. The develop entry - # is advisory: it is how a core regression becomes visible here, but it must - # not turn the plugin's own pull requests red. - continue-on-error: ${{ matrix.cacti != 'release/1.2.31' }} - strategy: fail-fast: false matrix: php: ['8.1', '8.2', '8.3', '8.4'] os: [ubuntu-latest] cacti: ['release/1.2.31'] - include: - - php: '8.4' - os: ubuntu-latest - cacti: 'develop' services: mariadb: @@ -95,7 +86,24 @@ jobs: echo "PHP_BINARY=$(command -v php)" >> "$GITHUB_ENV" - name: Run apt-get update - run: sudo apt-get update + run: | + for attempt in 1 2 3; do + if sudo timeout 3m apt-get \ + -o Dpkg::Lock::Timeout=60 \ + -o Acquire::Retries=3 \ + -o Acquire::http::Timeout=30 \ + -o Acquire::https::Timeout=30 \ + update; then + exit 0 + fi + + if [ "$attempt" -lt 3 ]; then + sleep 10 + fi + done + + echo 'apt-get update failed after three bounded attempts.' >&2 + exit 1 - name: Install System Dependencies run: sudo apt-get install -y apache2 snmp snmpd rrdtool fping @@ -141,7 +149,18 @@ jobs: run: | cd ${{ github.workspace }}/cacti if [ -f composer.json ]; then - sudo composer install --prefer-dist --no-progress + for attempt in 1 2 3; do + if sudo composer install --prefer-dist --no-progress --no-interaction; then + exit 0 + fi + + if [ "$attempt" -lt 3 ]; then + sleep 10 + fi + done + + echo 'Composer install failed after three attempts.' >&2 + exit 1 fi - name: Create Cacti config.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b7db7d7..fd521569 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ * issue#719: Plugin Disabled due to mix of string and int * issue: All Columns checkd on Thresholds page * issue: Special character previous value handling broken on data query indexes with special characters +* security: Replace eval() in RPN binary math operations with safe dispatch function (GHSA-vr4v-qvqm-gm9j, CWE-95) --- 1.8.2 --- diff --git a/tests/Unit/TholdRpnBinaryMathTest.php b/tests/Unit/TholdRpnBinaryMathTest.php new file mode 100644 index 00000000..2eb0170b --- /dev/null +++ b/tests/Unit/TholdRpnBinaryMathTest.php @@ -0,0 +1,51 @@ +toEqual($expected) + ->and($GLOBALS['rpn_error'])->toBeFalse(); +})->with([ + 'addition' => ['+', 8, 2, 10], + 'subtraction' => ['-', 8, 2, 6], + 'multiplication' => ['*', 8, 2, 16], + 'division' => ['/', 8, 2, 4], + 'modulo' => ['%', 8, 3, 2], + 'power' => ['^', 2, 3, 8], +]); + +test('unknown binary operators fail closed', function() { + expect(thold_rpn_math_binary('**', 2, 3))->toBe(0) + ->and($GLOBALS['rpn_error'])->toBeTrue() + ->and(CactiStubs::$log)->not->toBeEmpty(); +}); + +test('the expression evaluator routes binary math through the dispatcher', function() { + $stack = [8, 2]; + + thold_expression_math_rpn('-', $stack); + + expect($stack)->toBe([6]) + ->and($GLOBALS['rpn_error'])->toBeFalse(); +}); diff --git a/thold_functions.php b/thold_functions.php index 018cf99b..4dab88fc 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -338,6 +338,41 @@ function thold_expression_rpn_pop(&$stack) { } } +/** + * thold_rpn_math_binary - safely evaluates a binary arithmetic operation + * without using eval(). The operator must be one of the whitelisted RPN + * math tokens (+, -, *, /, %, ^). + * + * @param string $operator The arithmetic operator + * @param mixed $v2 The left operand (validated numeric) + * @param mixed $v1 The right operand (validated numeric) + * + * @return mixed The result of the operation + */ +function thold_rpn_math_binary($operator, $v2, $v1) { + global $rpn_error; + + switch ($operator) { + case '+': + return $v2 + $v1; + case '-': + return $v2 - $v1; + case '*': + return $v2 * $v1; + case '/': + return $v2 / $v1; + case '%': + return $v2 % $v1; + case '^': + return pow($v2, $v1); + default: + cacti_log("ERROR: RPN unknown binary operator '$operator'", false, 'THOLD'); + $rpn_error = true; + + return 0; + } +} + function thold_expression_math_rpn($operator, &$stack) { global $rpn_error; @@ -375,7 +410,7 @@ function thold_expression_math_rpn($operator, &$stack) { if ($rpn_evaled) { array_push($stack, $v3); } elseif (!$rpn_error) { - eval('$v3 = ' . $v2 . ' ' . $operator . ' ' . $v1 . ';'); // nosemgrep: php.lang.security.eval-use.eval-use -- pre-existing RPN expression evaluator; operator is constrained to whitelisted math tokens by the parser above + $v3 = thold_rpn_math_binary($operator, $v2, $v1); if ($v3 == '') { $v3 = 0;