Files
TheWitnessandThomas Vincent 629971cc73 Harden RRDtool proxy request handling (#20)
* Harden RRDtool proxy request handling

* Address PR review feedback: encrypt oversized-request errors, fix env inheritance, spacing, and symlink test

Addresses review comments on PR #17:
- lib/client.php: encrypt the 'Request too large' error frame once the
  client is authenticated, matching the existing encrypted-error pattern
- lib/client.php: build the removespikes child-process environment from
  getenv() instead of $_ENV so the full current environment (e.g. TEMP on
  Windows) is inherited, with RRDP_RRDTOOL_PATH merged on top
- lib/client.php: add the missing space in the RRDTOOL_PIPE_UNAVAILABLE
  error frame so it matches the other 'ERROR: ...' responses
- tests/run.php: check the symlink() fixture return value and skip the
  symlink-escape assertion (instead of producing a false positive) when
  symlink() is unavailable

* Upgrade phpseclib 3.x to phpseclib/phpseclib ^4.0

- composer.json/composer.lock: require phpseclib/phpseclib ^4.0 (was ^3.0)
- Update phpseclib3\\... namespace usages to phpseclib4\\... across
  rrdtool-proxy.php, lib/client.php, lib/functions.php, lib/wizard.php,
  and tests/run.php
- lib/functions.php: phpseclib4 removed the Random class, so
  Random::string(32) is replaced with random_bytes(32)
- Fix lib/__master__.php and lib/replicator.php, which PR #17 never
  migrated off the old pre-Composer phpseclib API (new \\phpseclib\\Crypt\\RSA(),
  ->loadKey(), ->getPublicKeyFingerprint() -- none of which exist in
  phpseclib 3.x or 4.x, so these MSR/replicator peer-authentication code
  paths would fatal at runtime). Both now use
  RSA::loadPublicKey()->getFingerprint('md5') wrapped in try/catch,
  matching the pattern already used in lib/client.php, plus a
  constant-time hash_equals() fingerprint comparison
- tests/run.php: relabel the phpseclib round-trip check from 'phpseclib 3'
  to 'phpseclib 4'

* Bump minimum PHP version to 8.2 and add CHANGELOG.md

- composer.json: require php ^8.2 (was ^8.1)
- include/global.php: bump RRDP_PHP_VERSION_REQUIRED to 80200 to match
- README.md: update minimum/recommended PHP version to 8.2
- composer.lock: refreshed for the new php platform constraint
- Add CHANGELOG.md tracking changes prefixed security:/issue:/feature:

* Add PHPDoc blocks to all functions

Adds a PHPDoc block above every function in rrdtool-proxy.php,
lib/functions.php, lib/client.php, lib/__master__.php, lib/replicator.php,
lib/wizard.php, cli/removespikes.php, tests/run.php, and
.phpstan-bootstrap.php, each with a brief description and inferred
@param/@return types. Function signatures themselves are unchanged; PHPStan
will flag any inferred types that turn out to be inaccurate.

* Add .github/copilot-instructions.md

Generated from README.md and this session's learnings: project architecture
(daemon entrypoint, shared lib helpers, client/master/replicator roles,
wizard, removespikes CLI), build/lint/test commands (composer scripts,
php tests/run.php, Windows platform-req caveats), and conventions (tabs,
GPL header block, PHPDoc-only typing, phpseclib4 gotchas, CHANGELOG entry
prefixes).

Also fix .gitignore: the '.git*' pattern was unintentionally also matching
and ignoring the .github/ directory; added a '!.github/' exception so files
under .github/ (like this one) are tracked.

* Add .gitattributes enforcing LF line endings; normalize existing CRLF files

- Add .gitattributes with '* text=auto eol=lf' so Git normalizes line
  endings to LF on checkout/commit regardless of local core.autocrlf
  settings.
- .gitignore: add '!.gitattributes' alongside the existing '!.github/'
  exception, since '.git*' was also unintentionally ignoring .gitattributes
  itself.
- Normalize the two tracked files that had CRLF line endings
  (.github/copilot-instructions.md, CHANGELOG.md) to LF; no other tracked
  files were affected.

* Manual PSR coding-style adjustments

Manual formatting/coding-style cleanup in cli/removespikes.php,
lib/__master__.php, lib/client.php, and lib/replicator.php.

* Address automated review feedback: symlink confinement, key-pair validation, and misc fixes

Addresses 10 unresolved copilot-pull-request-reviewer comments on PR #20:

- lib/functions.php: rewrite rrdp_resolve_path_within() to walk the path one
  segment at a time (instead of only checking the nearest existing ancestor
  via file_exists()), so a symlink anywhere along a not-yet-fully-existing
  path is resolved and confined even if the symlink itself is dangling.
- lib/functions.php: add rrdp_resolve_command_paths(), which resolves every
  RRD file path operand referenced by an RRDtool command (leading bare file
  arguments and DEF:/SDEF: clauses) against the RRA root and rewrites the
  command to use the canonical resolved paths, closing the symlink-escape
  gap left by the lexical-only rrdp_command_has_unsafe_path() check.
- lib/client.php: wire rrdp_resolve_command_paths() into the RRDtool command
  dispatch path, rejecting/rewriting unsafe operands before they reach the
  RRDtool pipe.
- lib/client.php: enforce the request-size limit on each completed
  transaction and the trailing unfinished remainder, instead of on the
  aggregate receive buffer before splitting on _EOT_, so multiple legitimate
  transactions in one read no longer trip a false 'Request too large'.
- rrdtool-proxy.php: validate at startup that the loaded public/private key
  files actually form a matching pair (comparing derived fingerprints)
  instead of only deriving a fingerprint from the public key, failing
  startup instead of accepting requests that will all fail to decrypt.
- lib/__master__.php, lib/replicator.php: resolve peer-supplied FULLSCAN
  folder/file names (and RRDLAST/RRDDUMP request filenames) through
  rrdp_resolve_rra_path() before using them in mkdir()/touch()/
  file_put_contents()/RRDtool restore-dump-last-flushcached operations,
  instead of concatenating raw peer-supplied text; reject and skip entries
  that fail confinement.
- lib/replicator.php, lib/__master__.php: guard rrdtool_pipe_init() failures
  in the main replication loop the same way lib/client.php already does,
  instead of indexing into a false return value and never retrying.
- cli/removespikes.php: make createRRDFileFromXML() return whether the
  RRDtool restore actually succeeded, and exit non-zero when it fails
  instead of silently exiting 0.
- .phpstan.neon: restore level 6 (was lowered to 2, contradicting
  .github/copilot-instructions.md) and add .phpstan-baseline.neon to
  capture the 42 pre-existing findings surfaced at level 6 without hiding
  future regressions.
- tests/run.php: add the standard Cacti Group GPL header block required for
  every PHP file.

---------

Co-authored-by: Thomas Vincent <thomasvincent@gmail.com>
2026-09-19 09:04:05 -04:00

158 lines
7.3 KiB
PHP

<?php
/*
+-------------------------------------------------------------------------+
| Copyright (C) 2004-2026 The Cacti Group |
| |
| This program is free software; you can redistribute it and/or |
| modify it under the terms of the GNU General Public License |
| as published by the Free Software Foundation; either version 2 |
| of the License, or (at your option) any later version. |
| |
| This program is distributed in the hope that it will be useful, |
| but WITHOUT ANY WARRANTY; without even the implied warranty of |
| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
| GNU General Public License for more details. |
+-------------------------------------------------------------------------+
| Cacti: The Complete RRDTool-based Graphing Solution |
+-------------------------------------------------------------------------+
| This code is designed, written, and maintained by the Cacti Group. See |
| about.php and/or the AUTHORS file for specific developer information. |
+-------------------------------------------------------------------------+
| http://www.cacti.net/ |
+-------------------------------------------------------------------------+
*/
require dirname(__DIR__) . '/vendor/autoload.php';
require dirname(__DIR__) . '/include/global.php';
require dirname(__DIR__) . '/lib/functions.php';
$failures = [];
$checks = 0;
/**
* Records a single test assertion: increments the running check count and, if
* $condition is falsy, appends $message to the list of failures.
*
* @param mixed $condition
* @param string $message
*
* @return void
*/
function check($condition, $message) {
global $checks, $failures;
$checks++;
if (!$condition) {
$failures[] = $message;
}
}
$test_root = sys_get_temp_dir() . '/rrdproxy-tests-' . bin2hex(random_bytes(8));
$rra_root = $test_root . '/rra';
$outside = $test_root . '/outside';
mkdir($rra_root, 0700, true);
mkdir($outside, 0700, true);
file_put_contents($rra_root . '/sample.rrd', 'rrd');
file_put_contents($outside . '/outside.rrd', 'outside');
$symlink_created = @symlink($outside, $rra_root . '/escape');
if (!$symlink_created) {
fwrite(STDERR, 'SKIP: symlink() is not available in this environment; skipping the symlink escape check.' . PHP_EOL);
}
$rrdp_config = ['path_rra' => $rra_root];
$canonical_rra_root = realpath($rra_root);
check(rrdp_resolve_rra_path('sample.rrd') === realpath($rra_root . '/sample.rrd'), 'A valid RRA path should resolve');
check(rrdp_resolve_rra_path('nested/new', false) === $canonical_rra_root . '/nested/new', 'A future path beneath the RRA root should resolve');
check(rrdp_resolve_rra_path('../outside/outside.rrd') === false, 'Parent traversal must be rejected');
check(rrdp_resolve_rra_path($outside . '/outside.rrd') === false, 'Absolute paths must be rejected');
if ($symlink_created) {
check(rrdp_resolve_rra_path('escape/outside.rrd') === false, 'Symlink escapes must be rejected');
} else {
fwrite(STDERR, 'SKIP: Symlink escapes must be rejected (no symlink() support)' . PHP_EOL);
}
check(rrdp_command_has_unsafe_path("update foo.rrd\ncreate escaped.rrd"), 'Embedded commands must be rejected');
check(rrdp_command_has_unsafe_path('update ../outside.rrd'), 'RRDtool traversal must be rejected');
check(rrdp_command_has_unsafe_path('graph graph.png DEF:x=../outside.rrd:value:AVERAGE'), 'RRDtool option traversal must be rejected');
check(rrdp_command_has_unsafe_path('update /tmp/outside.rrd'), 'Absolute RRDtool paths must be rejected');
check(rrdp_command_has_unsafe_path('graph graph.png --font TITLE:12:/tmp/font.ttf'), 'Embedded absolute RRDtool paths must be rejected');
$options = rrdp_parse_removespikes_options('-R=sample.rrd --method=stddev --dryrun');
check(is_array($options) && str_starts_with($options[0], '-R=' . $canonical_rra_root), 'Valid removespikes options should be normalized');
check(rrdp_parse_removespikes_options('-R=sample.rrd ;id') === false, 'Shell metacharacter arguments must be rejected');
check(rrdp_parse_removespikes_options('-R=../outside/outside.rrd') === false, 'removespikes traversal must be rejected');
check(rrdp_parse_removespikes_options('--dryrun') === false, 'removespikes must require exactly one RRD file');
check(rrdp_resolve_path_within($outside, 'future.rrd', false) === realpath($outside) . '/future.rrd', 'Generic containment should resolve safe future paths');
check(rrdp_resolve_path_within($outside, '../rra/sample.rrd', false) === false, 'Generic containment must reject traversal');
$process = rrdp_run_process([PHP_BINARY, '-r', 'echo $argv[1];', ';touch-not-executed']);
check($process !== false && $process['stdout'] === ';touch-not-executed', 'Process arguments must not be interpreted by a shell');
$rsa_class = 'phpseclib4\\Crypt\\RSA';
set_error_handler(static function ($severity, $message) {
return str_contains($message, 'Unable to write random state');
});
$private = $rsa_class::createKey(2048);
restore_error_handler();
$encryption = true;
$rrdp_config['encryption']['private_key'] = (string) $private;
$frame = encrypt('round trip', (string) $private->getPublicKey());
check(is_string($frame) && decrypt($frame) === 'round trip', 'phpseclib 4 encryption must round-trip');
check(decrypt('not-a-frame') === false, 'Malformed encrypted frames must fail closed');
$secret = $test_root . '/private.key';
check(rrdp_write_secure_file($secret, 'secret', 0600) !== false, 'Private key writes should succeed');
check((fileperms($secret) & 0777) === 0600, 'Private keys must use mode 0600');
$public_path = $test_root . '/public.key';
$private_path = $test_root . '/pair-private.key';
check(rrdp_write_key_pair($public_path, (string) $private->getPublicKey(), $private_path, (string) $private), 'Validated key pairs should be written together');
check((fileperms($private_path) & 0777) === 0600, 'Key-pair private keys must use mode 0600');
check(!rrdp_write_key_pair($public_path, 'invalid', $private_path, (string) $private), 'Invalid key pairs must fail closed');
$sockets = [];
check(socket_create_pair(AF_UNIX, SOCK_STREAM, 0, $sockets), 'A socket pair should be available');
if (count($sockets) === 2) {
check(rrdp_socket_write_all($sockets[0], 'complete') === 8, 'Socket writes should report the complete byte count');
check(socket_read($sockets[1], 8) === 'complete', 'Socket writes should deliver the complete payload');
socket_close($sockets[0]);
socket_close($sockets[1]);
}
/**
* Recursively deletes a file, symlink, or directory tree used by the test
* fixtures.
*
* @param string $path
*
* @return void
*/
function remove_test_tree($path) {
if (is_link($path) || is_file($path)) {
unlink($path);
return;
}
foreach (scandir($path) as $entry) {
if ($entry !== '.' && $entry !== '..') {
remove_test_tree($path . DIRECTORY_SEPARATOR . $entry);
}
}
rmdir($path);
}
remove_test_tree($test_root);
if ($failures) {
fwrite(STDERR, implode(PHP_EOL, $failures) . PHP_EOL);
exit(1);
}
print "OK ($checks checks)" . PHP_EOL;