Repository navigation
Validate hosts including IPv6, keep credentials off the command line, fix script paths (stacked on #44) - #46
Open
somethingwithproof wants to merge 6 commits into
Conversation
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…ster path_wmi Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…s end to end Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
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.



Description
Stacked on #44. The base branch of this pull request is
fix/hardening-review-fixes, notdevelop, and it must merge after #44. It builds on #44's POST-only Query Tool, its database-clock scheduling, its Cacti-signature test stubs and its end-to-end harness. After #44 merges, retarget this pull request todevelop; no other change is needed.Four follow-up changes to the WMI client and its scripts. One of them (wmic credentials) is security-relevant: it keeps the Windows password out of the process list.
is_valid_host(), never called) refused every IPv6 address, short names such aswin-server,.localnames and top level labels over four letters. It is replaced bywmi_valid_host(), which is now used by the tool.--userand--passwordas arguments, which any local user can read withps. It now writes a private authentication file and passes--authentication-file.path_wmiis registered as a setting.Related Issue
None open for these. Open issue #15 (Windows hosts) is not touched.
Motivation and Context
Host validation (
includes/functions.php,wmi_tools.php):wmi_valid_host()accepts an IPv4 or IPv6 address (filter_var), a bracketed IPv6 address (brackets removed), and a host name under RFC 1123: labels of 1 to 63 letters, digits and hyphens that do not start or end with a hyphen, at most 253 characters, optional trailing dot. It refuses IPv6 zone ids (fe80::1%eth0), names whose last label is all digits (999.1.1.1,1.2.3), underscores, ports, slashes, spaces, control characters and non-ASCII.ERROR: The host must be an IPv4 or IPv6 address or a host nameotherwise. The message is not translated, like the otherERROR:strings in the client, so the translation template is unchanged by it.//, so it cannot be read as an option (tests cover-oProxy, an IPv6 address and the injection-looking inputs), and the PowerShell transport passes it in an environment variable.ip2long, dotted-quad regexes,FILTER_FLAG_IPV4). The only one wasis_valid_host(). The plugin stores no host addresses of its own; it uses Cacti'shost.hostname(varchar(100), any text), and the host selection is Cacti's device list.//hostargument accepts an IPv6 address, and whetherNew-CimSession -ComputerNameaccepts one. No wmic binary was available (the wmi-client package is not in the Ubuntu 18.04 or 20.04 repositories I could reach), and there is no Windows host here. An IPv6 address is passed as one escaped argument after//, exactly like a name; please try it against a real host.Credentials (
includes/linux_wmi.php):username=,password=and, forDOMAIN\userorDOMAIN/user,domain=into a file created exclusively with a random name (mode 0600), passes--authentication-file=<file>, and removes the file and directory in afinallyblock, so a failing or timed-out wmic leaves nothing behind.--userand--passwordarguments.-Areads (username=,password=,domain=). A password with leading or trailing spaces depends on how that parser trims values. The tests use a stand-in executable that supports-Aand records what it received.Script-server files:
templates/resource/script_server/exchange_messages.xmlandplugin_wmi_create_resource_xml()named|path_cacti|/scripts/wmi-script.php, which does not exist. They now name|path_cacti|/plugins/wmi/script/wmi-script.php.script/wmi-script.phprequired../include/cli_check.php(now../../../include/cli_check.php) and looked for awmicinside the plugin that is not shipped.wmi_script.phprequired../include/global.phpand hard-coded/usr/bin/wmic. Both now use thepath_wmisetting, falling back to/usr/bin/wmic.path_wmiwas read by the Query Tool and the poller but never registered. It is now a path setting with the default/usr/bin/wmic, so existing installs that have no stored value behave as before. The two new setting strings are added tolocales/po/cacti.pot.wmi_script.phpas a script, because it requires a live Cacti.script/wmi-script.phpruns under test through a stand-in wmic.wmic stderr:
ERROR: <stdout>with stderr added after a line break (orERROR: <stderr>when stdout is empty), capped at 2000 characters. On success stderr is ignored. The timeout is 120 seconds; a wmic that runs longer is killed and reported asERROR: wmic did not finish within 120 seconds.How Has This Been Tested?
Environment: PHP 8.4.27 with Xdebug in Docker, Cacti 1.2.x (1.2.32) and Cacti develop (1.3.0) checkouts with
composer installas the workflow does it, and MariaDB 11.8.9.WmiValidHostTest(IPv4, IPv6 compressed, expanded and bracketed, zone ids refused, short names,.local, long top level labels, labels at 63 and 64 characters, 253 and 254 character names, trailing dot, empty input, injection-looking input, the command line for-oProxyand an IPv6 address),WmiWmicAuthFileTest,WmiWmicStderrTestandWmiScriptServerTest(resource paths exist, the scripts'requiretargets exist in the Cacti tree, the setting is registered and keeps other path settings, the script-server function runs through a stand-in wmic usingpath_wmi).=, quotes, spaces,$(), backslashes,#and non-ASCII, refusal of line breaks and NUL, cleanup after success, failure and timeout, and the fallback when the directory cannot be made, the file cannot be created or the write is short.--authentication-file(it records the file's content, mode and path) and prints a line on stderr on every call, so the cache flows prove that stderr never reaches the parsed rows. New end-to-end checks:bad host,-oProxy=x,a..b,fe80::1%eth0,999.1.1.1andhost;idwithout calling wmic, and passeswin-server,[2001:db8::1](as//2001:db8::1),10.0.0.5,srv.corp.localand an expanded IPv6 address after the//prefix;script/wmi-script.phpandwmi_script.phprun from the command line against the real database and the fake wmic (index, key and value, and an unknown query).WMI_E2E=1and a database).composer installas the workflow does it, the pinned Pest harness for 1.2.x,install_cacti,plugin_manage --install --enableon a bare Cacti with MariaDB and no other plugin, the log-writable step, then Pest as a non-root user withWMI_E2E=1and PHP set toAmerica/New_York, then the workflow's log grep): 275 passed and 1 warning on Cacti 1.2.x, the same on Cacti develop, log grep clean on both. The warning is the existingPruneFilesTestpermission test.php tests/bin/patch-coverage.php <clover> origin/fix/hardening-review-fixes 100(the base of this stack): 100.00 percent (121 of 121 changed measured lines).php tests/bin/e2e-coverage-report.php: every changed executable line, including those inwmi_tools.php,wmi_script.phpandscript/wmi-script.php, ran in at least one suite.check-i18n-pot.php(pot included in the change) andvalidate-manifest.php: pass.php -lon every changed file: no errors.The conflicts with #44 (
tests/bootstrap-unit.php,CHANGELOG.md,includes/functions.php,wmi_tools.php,includes/linux_wmi.php) were resolved by keeping #44's version of each shared hunk and adding this change's code beside it. The only stub change left in this branch is a__test_configlookup inread_config_option; #44's row-map stub and its core-matching signatures are used as they are.Not run: the GitHub workflow itself, PHP 8.2 and 8.3 locally, a real wmic or Windows host, PHPStan and the coding-standards jobs beyond what origin/develop already reports, and Sonar.
Types of changes
Checklist:
Rollback: each commit reverts on its own (the script-path commit also touches
locales/po/cacti.pot). No schema change. The newpath_wmisetting only appears on the Paths tab; with no stored value the code uses/usr/bin/wmicas before. A host the Query Tool now refuses can still be queried through a device.