Harden RRDtool proxy request handling - #17
Open
somethingwithproof wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are a few reliability/compatibility issues in the hardened paths (notably encrypted error framing, potential fatal type errors on crypto failure, child-process environment handling, and a test fixture that can silently degrade coverage).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens RRDtool Proxy request handling by tightening input/frame validation, removing shell interpolation, confining filesystem access to configured roots, and updating the legacy crypto implementation to phpseclib 3 while maintaining Cacti wire compatibility.
Changes:
- Migrates encryption/key handling to phpseclib 3, adds key-pair validation + secure/atomic-ish writes, and improves socket write reliability.
- Adds strict bounds and “fail closed” behavior for encrypted/compressed client frames and rejects unsafe RRDtool paths/command framing.
- Removes shell execution for RRDtool/removespikes, adds focused security/regression coverage, and simplifies PHPStan configuration.
File summaries
| File | Description |
|---|---|
| tests/run.php | Adds targeted regression/security checks for path confinement, encryption framing, process spawning, key writes, and socket writes. |
| rrdtool-proxy.php | Improves admin socket cleanup, hardens key init/write path, and fixes partial socket writes via write-all helper. |
| README.md | Updates security documentation and clearly states legacy protocol limitations and deployment expectations. |
| lib/wizard.php | Fixes globals initialization and switches RSA key persistence to validated key-pair writer. |
| lib/replicator.php | Minor formatting-only adjustment in request handling loop. |
| lib/functions.php | Migrates crypto to phpseclib3 and adds path containment, safe process execution, secure file/key writes, and write-all sockets. |
| lib/client.php | Adds request size limits, safer decompression, unsafe-path rejection, safer env setting, and removes shell-based removespikes execution. |
| include/global.php | Raises minimum PHP version and introduces request/key/decompression size limits; removes “encryption” client param. |
| composer.json | Adds composer test and adjusts PHPStan script invocation flags. |
| cli/removespikes.php | Removes shell execution, uses proc_open safely, and switches to secure temp file creation for dumps. |
| .phpstan.neon | Simplifies PHPStan scope/config to active daemon paths and reduces process parallelism. |
| .phpstan-bootstrap.php | Adds stubs to satisfy PHPStan analysis without loading full Cacti runtime. |
Review details
- Files reviewed: 11/12 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+114
to
+116
| if (strlen($input) > $max_input_size) { | ||
| rrdp_system__socket_write($socket_client, RRD_ERROR . ' Request too large' . $end_of_sequence); | ||
| __logging(LOGGING_LOCATION_BUFFERED, 'Client request exceeded the maximum frame size', 'IPC', SEVERITY_LEVEL_WARNING); |
Comment on lines
+422
to
+426
| $parsed_options = rrdp_parse_removespikes_options($cmd_options); | ||
| $environment = array_merge($_ENV, ['RRDP_RRDTOOL_PATH' => $rrdp_config['path_rrdtool']]); | ||
| $result = $parsed_options === false ? false : rrdp_run_process(array_merge([PHP_BINARY, '-q', $rrdp_config['path_cli'] . '/removespikes.php'], $parsed_options), $environment); | ||
| $rrdp_exec_status = $result !== false && $result['status'] === 0; | ||
| $rrdp_exec_return = $result !== false ? $result['stdout'] . $result['stderr'] : 'Invalid removespikes options'; |
Comment on lines
+221
to
+225
| if ($rrdtool_process_pipes === false) { | ||
| rrdp_system__count('rrd_pipe_broken'); | ||
| rrdp_system__socket_write($socket_client, encrypt(RRD_ERROR . 'RRDTOOL_PIPE_UNAVAILABLE', $client_public_key) . $end_of_sequence); | ||
|
|
||
| continue; |
| mkdir($outside, 0700, true); | ||
| file_put_contents($rra_root . '/sample.rrd', 'rrd'); | ||
| file_put_contents($outside . '/outside.rrd', 'outside'); | ||
| symlink($outside, $rra_root . '/escape'); |
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.
Summary
Why
The proxy mixed phpseclib 1 calls with a phpseclib 3 dependency, accepted unbounded frames, passed client-controlled removespikes arguments through a shell, and allowed several filesystem operations without canonical root confinement. Socket and key-management paths also had reliability and permission issues.
Impact
Requests containing traversal, absolute paths, embedded command framing, malformed encryption frames, or oversized payloads now fail closed. Existing Cacti clients retain the current RSA/Rijndael-CBC wire format.
The compatibility protocol still lacks signed proof of private-key possession and authenticated message integrity. The README now documents that limitation and recommends a trusted, firewalled management network until a versioned client-and-server protocol upgrade is available.
Validation
composer test— 27 checks passedcomposer run phpstan— no errors at the configured baselinecomposer run lint— 11 files passedcomposer run phpcsfixer— cleancomposer validate --strict— cleancomposer audit— no vulnerability advisoriesgit diff --check— cleanCloses #18