Validate hosts including IPv6, keep credentials off the command line, fix script paths (stacked on #44)

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 --- develop ---
 
+* dev: Extend the end-to-end fake wmic with --authentication-file and a stderr line, and cover host validation, the credentials file and the script-server scripts end to end
+* issue: Point the script-server resource and its scripts at the files that exist, load Cacti from where it is, and register the wmic path as the path_wmi setting (default /usr/bin/wmic)
+* security: Pass the Windows credentials to wmic in a private authentication file instead of --user and --password arguments, so they no longer show up in the process list
+* issue: Read wmic's stderr apart from its stdout, add it to the error text only, and stop a wmic that does not finish within two minutes
+* issue: Validate the Query Tool host as an IPv4 address, an IPv6 address or an RFC 1123 host name; the old check refused IPv6, short names and longer top level labels and was never called
 * issue: Record the WMI query start time from the database clock so devices are not polled again every cycle when PHP and the database use different time zones
 * security: Run the WMI Query Tool query action only for POST requests so credentials stay out of URLs, access logs and browser history
 * dev: Cover the Authentication, Queries and Query Tool pages and the Device Template hook with real-request end-to-end tests and replace the source-text checks
@@ -19,6 +24,7 @@
 * issue: Index cached WMI rows by every column of a comma separated primary key so multi-row results are no longer overwritten
 * issue: Purge WMI cache rows the device stopped returning, and keep the existing rows when a query fails
 * issue: Expire stale wmi_processes lock rows and stop the master poller waiting indefinitely for collectors that died
+||||||| parent of 7c3e181 (fix(tools): validate the host as an IP address or an RFC 1123 name)
 * security: Replace the confirmation pages' inline `onClick='cactiReturnTo()'` Cancel buttons with the CSP-safe `cactiReturnTo` class so the pages no longer trip Cacti's Content-Security-Policy script-src-attr directive
 * security: Move the WMI Accounts and Queries filter rows selects' inline onChange handlers into the ready block so the pages no longer trip Cacti's Content-Security-Policy script-src-attr directive
 * dev: Measure CI coverage with xdebug instead of pcov so the plugin's own sources are instrumented (pcov auto-scopes to the Composer root and skipped cacti/plugins/, leaving the patch-coverage gate with nothing to measure)

includes/functions.php

@@ -331,7 +331,7 @@ function plugin_wmi_create_resource_xml($id) {
 	if (isset($wmic['id'])) {
 		$data = "<WMIQuery>\n";
 		$data .= '	<name>' . $wmic['name'] . "</name>\n";
-		$data .= "	<script_path>|path_cacti|/scripts/wmi-script.php</script_path>\n";
+		$data .= "	<script_path>|path_cacti|/plugins/wmi/script/wmi-script.php</script_path>\n";
 		$data .= "	<script_function>wmi_script</script_function>\n";
 		$data .= '	<description>WMI Query for ' . $wmic['name'] . "</description>\n";
 		$data .= "	<script_server>php</script_server>\n";
@@ -646,6 +646,55 @@ function wmi_collectors_running($seed, $began, $max_age = 3600) {
 	return (int) db_fetch_cell_prepared('SELECT COUNT(*) FROM wmi_processes WHERE taskid = ?', [$seed]);
 }
 
+/**
+ * Validates a host as typed into the Query Tool: an IPv4 address, an IPv6
+ * address (optionally in brackets), or a host name under RFC 1123 (labels of
+ * 1 to 63 letters, digits and hyphens that neither start nor end with a
+ * hyphen, at most 253 characters, optional trailing dot). IPv6 zone ids
+ * (fe80::1%eth0) are refused, as is a name whose last label is all digits.
+ *
+ * @param string $host The host as entered.
+ *
+ * @return string|false The host to use (brackets removed), or false when it is
+ *                      not a valid address or name.
+ */
+function wmi_valid_host($host) {
+	if ($host === '') {
+		return false;
+	}
+
+	if ($host[0] === '[' && substr($host, -1) === ']') {
+		$inner = substr($host, 1, -1);
+
+		return filter_var($inner, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) !== false ? $inner : false;
+	}
+
+	if (filter_var($host, FILTER_VALIDATE_IP) !== false) {
+		return $host;
+	}
+
+	$name = substr($host, -1) === '.' ? substr($host, 0, -1) : $host;
+
+	if ($name === '' || strlen($name) > 253) {
+		return false;
+	}
+
+	$labels = explode('.', $name);
+
+	foreach ($labels as $label) {
+		if (!preg_match('/^[A-Za-z0-9]([A-Za-z0-9-]{0,61}[A-Za-z0-9])?$/D', $label)) {
+			return false;
+		}
+	}
+
+	// Not a number: 999.1.1.1 is a malformed address, not a host name.
+	if (ctype_digit((string) end($labels))) {
+		return false;
+	}
+
+	return $host;
+}
+
 /**
  * Returns the current unique hash for a WMI query, or generates a new
  * one if the stored value doesn't already look like a valid 128-bit hex

includes/linux_wmi.php

@@ -66,32 +66,73 @@ public function error(): ?string;
 }
 
 /**
- * Default transport: the Linux `wmic` client invoked through the shell.
+ * Default transport: the Linux `wmic` client.
+ *
+ * The credentials go to wmic in a private authentication file (-A) so they
+ * never appear in the process list. stdout is the result; stderr is kept
+ * apart and only added to the error text.
  */
 class Wmic_Shell_Transport implements Wmi_Transport {
 	private ?string $error = null;
 
+	/**
+	 * @param int         $timeout  Seconds to wait for wmic before it is stopped.
+	 * @param string|null $temp_dir Directory for the authentication file; the
+	 *                              system temporary directory when null.
+	 */
+	public function __construct(
+		private int $timeout = 120,
+		private ?string $temp_dir = null
+	) {
+	}
+
 	public function query(Wmi_Request $request): array|false {
 		$this->error = null;
 
-		$output = [];
-		$status = 0;
+		if (preg_match('/[\r\n\0]/', $request->username . $request->password)) {
+			$this->error = 'ERROR: WMI credentials must not contain line breaks';
+
+			return false;
+		}
+
+		$auth = $this->create_auth_file($request);
+
+		try {
+			$result = $this->run(
+				(DIRECTORY_SEPARATOR === '/' ? 'exec ' : '') . $this->build_command($request, $auth['file'] ?? null)
+			);
+		} finally {
+			if ($auth !== null) {
+				@unlink($auth['file']);
+				@rmdir($auth['dir']);
+			}
+		}
+
+		if ($result === null) {
+			$this->error = 'ERROR: Unable to start ' . $request->binary;
+
+			return false;
+		}
+
+		if ($result['timed_out']) {
+			$this->error = $this->with_stderr('ERROR: wmic did not finish within ' . $this->timeout . ' seconds', $result['stderr']);
 
-		exec($this->build_command($request), $output, $status);
+			return false;
+		}
 
-		if ($status !== 0) {
-			$this->error = 'ERROR: ' . implode("\n", $output);
+		if ($result['status'] !== 0) {
+			$this->error = $this->with_stderr('ERROR: ' . implode("\n", $result['lines']), $result['stderr']);
 
 			return false;
 		}
 
-		if (count($output) === 0) {
-			$this->error = 'ERROR: WMI Returned no Data';
+		if (count($result['lines']) === 0) {
+			$this->error = $this->with_stderr('ERROR: WMI Returned no Data', $result['stderr']);
 
 			return false;
 		}
 
-		return $output;
+		return $result['lines'];
 	}
 
 	public function error(): ?string {
@@ -102,17 +143,26 @@ public function error(): ?string {
 	 * Build the wmic command line. Every device-influenced value is shell
 	 * escaped; the hostname and namespace are additionally stripped of cmd.exe
 	 * metacharacters on win32. The delimiter is quoted so its pipe characters
-	 * are not read as a shell pipeline.
+	 * are not read as a shell pipeline. The hostname follows a literal `//`, so
+	 * it can never be read as an option.
+	 *
+	 * @param string|null $auth_file Credentials file for --authentication-file;
+	 *                               without it the credentials are passed as
+	 *                               arguments, which any local user can read.
 	 */
-	public function build_command(Wmi_Request $request): string {
+	public function build_command(Wmi_Request $request, ?string $auth_file = null): string {
 		$namespace = $request->namespace !== ''
 			? ' --namespace=' . cacti_escapeshellarg($this->single_backslashes($this->strip_metachars($request->namespace)))
 			: '';
 
+		$credentials = $auth_file !== null
+			? ' --authentication-file=' . cacti_escapeshellarg($auth_file)
+			: ' --user=' . cacti_escapeshellarg($request->username) .
+				' --password=' . cacti_escapeshellarg($request->password);
+
 		return cacti_escapeshellarg($request->binary) .
 			' --delimiter=' . cacti_escapeshellarg($request->separator) .
-			' --user=' . cacti_escapeshellarg($request->username) .
-			' --password=' . cacti_escapeshellarg($request->password) .
+			$credentials .
 			$namespace .
 			' //' . cacti_escapeshellarg($this->strip_metachars(trim($request->hostname))) .
 			' ' . cacti_escapeshellarg($request->query);
@@ -127,6 +177,163 @@ private function single_backslashes(string $value): string {
 		return preg_replace('/\\\\+/', '\\\\', $value);
 	}
 
+	/**
+	 * Write the credentials to a file only this user can read, inside a
+	 * directory with a random name. Returns null, after logging a warning,
+	 * when that is not possible; the caller then falls back to arguments.
+	 *
+	 * @return array{dir: string, file: string}|null
+	 */
+	private function create_auth_file(Wmi_Request $request): ?array {
+		$base = $this->temp_dir ?? sys_get_temp_dir();
+		$dir  = rtrim($base, '/') . '/wmi-' . bin2hex(random_bytes(8));
+
+		if (!is_dir($base) || !is_writable($base) || !@mkdir($dir, 0700)) {
+			cacti_log('WARNING: Unable to create a private directory for the wmic credentials; passing them as arguments', false, 'WMI');
+
+			return null;
+		}
+
+		$file    = $dir . '/' . bin2hex(random_bytes(8));
+		$handle  = $this->open_new_file($file);
+		$written = false;
+
+		if ($handle !== false) {
+			$content = $this->auth_file_content($request);
+			$written = fwrite($handle, $content) === strlen($content);
+
+			fclose($handle);
+		}
+
+		if (!$written || !@chmod($file, 0600)) {
+			if (is_file($file)) {
+				unlink($file);
+			}
+
+			rmdir($dir);
+
+			cacti_log('WARNING: Unable to write the wmic credentials file; passing the credentials as arguments', false, 'WMI');
+
+			return null;
+		}
+
+		return ['dir' => $dir, 'file' => $file];
+	}
+
+	/**
+	 * Create a file that must not exist yet, readable only by this user.
+	 *
+	 * @return resource|false
+	 */
+	protected function open_new_file(string $file) {
+		$old    = umask(0077);
+		$handle = @fopen($file, 'x');
+
+		umask($old);
+
+		return $handle;
+	}
+
+	/**
+	 * Start a process with its stdout and stderr as pipes.
+	 *
+	 * @param array<int, resource> $pipes
+	 *
+	 * @return resource|false
+	 */
+	protected function start_process(string $command, ?array &$pipes) {
+		return proc_open($command, [0 => ['file', '/dev/null', 'r'], 1 => ['pipe', 'w'], 2 => ['pipe', 'w']], $pipes);
+	}
+
+	/**
+	 * The samba credentials file format. A DOMAIN\user or DOMAIN/user name is
+	 * split the way wmic splits --user.
+	 */
+	private function auth_file_content(Wmi_Request $request): string {
+		$content = '';
+		$user    = $request->username;
+
+		if (preg_match('#^([^\\\\/]+)[\\\\/](.+)$#', $user, $match)) {
+			$content = 'domain=' . $match[1] . "\n";
+			$user    = $match[2];
+		}
+
+		return 'username=' . $user . "\n" . 'password=' . $request->password . "\n" . $content;
+	}
+
+	/**
+	 * Run a command with stdout and stderr read separately, stopping it when
+	 * it takes longer than the timeout.
+	 *
+	 * @return array{status: int, lines: array<int, string>, stderr: string, timed_out: bool}|null
+	 */
+	private function run(string $command): ?array {
+		$process = $this->start_process($command, $pipes);
+
+		if (!is_resource($process)) {
+			return null;
+		}
+
+		$open     = [1 => $pipes[1], 2 => $pipes[2]];
+		$buffer   = [1 => '', 2 => ''];
+		$deadline = microtime(true) + $this->timeout;
+		$expired  = false;
+
+		foreach ($open as $stream) {
+			stream_set_blocking($stream, false);
+		}
+
+		while ($open !== []) {
+			$left = $deadline - microtime(true);
+
+			if ($left <= 0) {
+				$expired = true;
+
+				break;
+			}
+
+			$read   = $open;
+			$write  = null;
+			$except = null;
+
+			@stream_select($read, $write, $except, 0, (int) min(200000, $left * 1000000));
+
+			foreach ($read as $key => $stream) {
+				$chunk = fread($stream, 8192);
+
+				if ($chunk === false || ($chunk === '' && feof($stream))) {
+					unset($open[$key]);
+				} else {
+					$buffer[$key] .= $chunk;
+				}
+			}
+		}
+
+		if ($expired) {
+			proc_terminate($process, 9);
+		}
+
+		fclose($pipes[1]);
+		fclose($pipes[2]);
+
+		$status = proc_close($process);
+
+		if (substr($buffer[1], -1) === "\n") {
+			$buffer[1] = substr($buffer[1], 0, -1);
+		}
+
+		return [
+			'status'    => $expired ? -1 : $status,
+			'lines'     => $buffer[1] === '' ? [] : array_map('rtrim', explode("\n", $buffer[1])),
+			'stderr'    => substr(trim($buffer[2]), 0, 2000),
+			'timed_out' => $expired,
+		];
+	}
+
+	private function with_stderr(string $message, string $stderr): string {
+		return $stderr === '' ? $message : ($message === 'ERROR: ' ? 'ERROR: ' : $message . "\n") . $stderr;
+	}
+
 	/**
 	 * A hostname or namespace never legitimately contains shell or cmd.exe
 	 * metacharacters. cmd.exe ignores \" , toggles quoting on every " , and

locales/po/cacti.pot

@@ -566,3 +566,11 @@ msgstr ""
 #: wmi_tools.php
 msgid "Showing columns and first one or two rows of data."
 msgstr ""
+
+#: setup.php
+msgid "WMI Client (wmic) Path"
+msgstr ""
+
+#: setup.php
+msgid "The path to the wmic binary used to query Windows devices from a Linux Cacti server."
+msgstr ""