Security hardening: check.php auth, output escaping, output-file confinement, group visibility, format-string guard

CHANGELOG.md

@@ -14,8 +14,16 @@ IMPORTANT NOTE: This version only works on CACTI 1.x++!
 
 ## Changes
 
---- develop ---
-
+--- 2.1 ---
+
+* security: Require the Manage Weathermap realm to view check.php, which previously disclosed the PHP version, host name, kernel, ini paths and loaded extensions to any anonymous requester
+* security: Escape map titles, info URLs and config filenames before they are rendered on the management pages, in session messages and in the RSS feed
+* security: Confine the OUTPUTFILE, HTMLOUTPUTFILE, IMAGEOUTPUTFILE and DATAOUTPUTFILE map directives to the plugin output directory so a map config cannot make the poller write or overwrite an arbitrary file; a file name or a path into output/ is accepted, and the accepted names are listed under HTMLOUTPUTFILE in the configuration reference
+* security: Restrict each output-file directive to its own extensions so DATAOUTPUTFILE, whose contents come from the map, can no longer be written as an includable HTML page that the graph tree executes
+* security: Reject executable-scheme info URLs (javascript:, data:, vbscript:), including HTML-entity-encoded forms, in the generated image map, emitting nohref so a map link cannot run script in a viewer's session
+* security: Reject an over-length duplicated-map title (more than 4088 bytes after cleaning) so it cannot inject map-config directives across the poller's config-line read boundary, matching the new-map check
+* security: Hide maps from members of a disabled user group in the list and cycle views, matching the per-map permission check
+* security: Treat an invalid format string in a map label, title or note as literal text so it can no longer abort the poller cycle on PHP 8
 * issue#346: Split js/editor.js into dialog, picker, compact-editor and asset-library scripts so browser tests load whole files
 * issue#343: Run engine unit tests in-process where they no longer need isolation
 * issue#342: Split the engine base classes and constants out of WeatherMap.class.php so tests load only what they need

INFO

@@ -1,6 +1,6 @@
 [info]
 name = weathermap
-version = 2.0
+version = 2.1
 longname = Weathermap Plugin
 author = The Cacti Group, Howard Jones
 email = developers@cacti.net

check.php

@@ -40,6 +40,24 @@
  +-------------------------------------------------------------------------+
 */
 
+/* This report names the host, the kernel, the PHP build and the ini paths, so over
+ * the web it is gated behind the Manage Weathermap realm.  The CLI run is left open:
+ * the point of running it both ways is to compare the two PHP configurations.
+ *
+ * The realm is checked here rather than left to the plugin_realms row alone.  That
+ * row is written by the installer and by the upgrade step, and the upgrade step only
+ * runs when the recorded version differs from INFO, so an install already sitting on
+ * this version would otherwise get authentication without authorisation. */
+if (PHP_SAPI !== 'cli') {
+	include_once(__DIR__ . '/../../include/auth.php');
+
+	if (!api_plugin_user_realm_auth('weathermap-cacti-plugin-mgmt.php')) {
+		header('Location: ' . $config['url_path'] . 'permission_denied.php');
+
+		exit;
+	}
+}
+
 // get some basics...
 $php_version = phpversion();
 $mem_allowed = ini_get('memory_limit');

docs/pages/config-reference.html

@@ -2506,6 +2506,25 @@ <h3 id="GLOBAL_HTMLOUTPUTFILE">HTMLOUTPUTFILE</h3>
           <p xmlns="http://www.w3.org/1999/xhtml" xmlns:xi="http://www.w3.org/2001/XInclude">The
           equivalent command-line option takes precedence over this configuration line, if both are
           used.</p>
+          <p xmlns="http://www.w3.org/1999/xhtml" xmlns:xi="http://www.w3.org/2001/XInclude">
+          <a id="CACTI_OUTPUT_FILE_NAMES"></a>When the Cacti plugin writes this file, it writes only into the
+          plugin's <code>output/</code> directory. Give a file name such as <code>network.prod.png</code> or
+          <code>Core Links.html</code>, or a path whose directory is the output directory, such as
+          <code>output/network.png</code> (relative to the plugin directory) or the absolute path of the
+          output directory. A file name starts with a letter or digit, may contain letters, digits,
+          spaces, dots, underscores and hyphens, and ends in an extension appropriate to the directive,
+          in any letter case: <code>HTMLOUTPUTFILE</code> accepts <code>.html</code> or <code>.htm</code>;
+          <code>IMAGEOUTPUTFILE</code> accepts <code>.png</code>, <code>.jpg</code>, <code>.jpeg</code> or
+          <code>.gif</code>; and <code>DATAOUTPUTFILE</code> accepts only the data extensions
+          <code>.txt</code>, <code>.dat</code> or <code>.tsv</code> (so a data file, whose contents come
+          from the map, cannot be written as an includable HTML page). A name with a script extension in the
+          middle, such as <code>map.php.png</code>, is refused, as is an existing file in the output
+          directory that is a symbolic link or has a second hard link.</p>
+          <p xmlns="http://www.w3.org/1999/xhtml" xmlns:xi="http://www.w3.org/2001/XInclude">Any other value, such as a path into a
+          subdirectory or outside <code>output/</code>, is ignored and the poller logs a
+          <code>WMPOLL08</code> warning; the map itself is still drawn into <code>output/</code>. To keep a
+          second copy elsewhere, change the directive to a file name and copy or serve the file from
+          <code>output/</code>.</p>
         </div>
         <div class="changes">
           <h4>Change History</h4>
@@ -2532,8 +2551,10 @@ <h3 id="GLOBAL_DATAOUTPUTFILE">DATAOUTPUTFILE</h3>
           "targets.html#wmdata">wmdata datasource plugin</a>, which allows one map to refer to data
           in another map.</p>
           <p xmlns="http://www.w3.org/1999/xhtml" xmlns:xi="http://www.w3.org/2001/XInclude">If you
-          are using the Cacti plugin, this data is always saved in the output/ directory, in
-          addition to anywhere you specify with this command.</p>
+          are using the Cacti plugin, this data is always saved in the output/ directory.</p>
+          <p xmlns="http://www.w3.org/1999/xhtml" xmlns:xi="http://www.w3.org/2001/XInclude">The Cacti plugin writes this
+          file into its output directory only; see <a href="#CACTI_OUTPUT_FILE_NAMES">the accepted names</a>
+          under HTMLOUTPUTFILE.</p>
         </div>
         <div class="changes">
           <h4>Change History</h4>
@@ -2574,6 +2595,9 @@ <h3 id="GLOBAL_IMAGEOUTPUTFILE">IMAGEOUTPUTFILE</h3>
           <p xmlns="http://www.w3.org/1999/xhtml" xmlns:xi="http://www.w3.org/2001/XInclude">The
           equivalent command-line option takes precedence over this configuration line, if both are
           used.</p>
+          <p xmlns="http://www.w3.org/1999/xhtml" xmlns:xi="http://www.w3.org/2001/XInclude">The Cacti plugin writes this
+          file into its output directory only; see <a href="#CACTI_OUTPUT_FILE_NAMES">the accepted names</a>
+          under HTMLOUTPUTFILE.</p>
         </div>
         <div class="changes">
           <h4>Change History</h4>

lib/HTML_ImageMap.class.php

@@ -40,6 +40,58 @@
  +-------------------------------------------------------------------------+
 */
 
+/**
+ * Decides whether an <area> href from a map config is safe to emit.
+ *
+ * INFOURL reaches this straight from the map config, which Manage and Edit
+ * Weathermap users author.  html_escape() keeps the value inside the quoted
+ * attribute, but an executable-scheme URL such as "javascript:alert(1)" is a
+ * valid attribute value that still runs script when a viewer clicks the
+ * hotspot.  Only http(s) and scheme-less links (ordinary relative Cacti URLs,
+ * absolute paths, anchors and protocol-relative links) are allowed; anything
+ * carrying another scheme is rejected so the caller can emit nohref instead.
+ *
+ * The value is checked after decoding HTML entities because Cacti 1.2.x's
+ * html_escape() calls htmlspecialchars() with double_encode = false and so
+ * passes entities in the value through unchanged; the browser then decodes
+ * them once, turning "javascript:" or "javascript:" back into an
+ * executable javascript: link.  Decoding here the same single way the browser
+ * will keeps those out.
+ *
+ * @param  string $url The raw href from the map config.
+ * @return bool   True when the URL may be used as an href.
+ */
+function wm_href_is_safe($url) {
+	if (!is_string($url)) {
+		return false;
+	}
+
+	// Decode entities the one time the browser will, so an entity-encoded
+	// scheme such as "javascript:" or "javascript:" is tested as
+	// the "javascript:" it becomes in the rendered attribute.
+	$charset = ini_get('default_charset') ?: 'UTF-8';
+
+	$decoded = html_entity_decode($url, ENT_QUOTES | ENT_HTML5, $charset);
+
+	// Strip whitespace and control characters so an obfuscated scheme such
+	// as "java\tscript:" (including its entity form "java	script:") cannot
+	// slip the scheme test below.
+	$candidate = preg_replace('/[\x00-\x20\x7f]+/', '', $decoded);
+
+	if ($candidate === '' || $candidate === null) {
+		return false;
+	}
+
+	// A scheme is the run of scheme characters before the first colon, taken
+	// only when no "/", "?" or "#" has appeared first (otherwise the colon
+	// belongs to a path or query, e.g. graph_view.php?x=a:b, which is relative).
+	if (preg_match('#^([A-Za-z][A-Za-z0-9+.\-]*):#', $candidate, $m)) {
+		return in_array(strtolower($m[1]), ['http', 'https'], true);
+	}
+
+	return true;
+}
+
 /**
  * A simple port of the guts of Apache's mod_imap
  * - if you have an image control in a form, it's not really defined what happens to USEMAP
@@ -83,8 +135,13 @@ function common_html() {
 			$h .= 'class="node" ';
 		}
 
-		if ($this->href != '') {
-			$h .= 'href="' . $this->href . '" ';
+		if ($this->href != '' && wm_href_is_safe($this->href)) {
+			/* href carries INFOURL straight from the map config.  The poller writes this
+			 * markup to output/<hash>.html, which the plugin pages later include() as PHP,
+			 * so an unescaped value is executable and not merely injectable.  wm_href_is_safe()
+			 * also rejects executable-scheme URLs such as javascript: that html_escape() would
+			 * otherwise leave clickable, falling through to nohref for them. */
+			$h .= 'href="' . html_escape($this->href) . '" ';
 		} else {
 			$h .= 'nohref ';
 		}

lib/WeatherMap.functions.php

@@ -41,9 +41,19 @@
 */
 
 /**
- * @param mixed $userid
- * @param mixed $group_id
- * @return mixed
+ * Returns the active maps a user may view.
+ *
+ * A map is allowed when it is granted to everyone (weathermap_auth.userid = 0),
+ * directly to the user, or to an enabled user group the user belongs to.  The
+ * group branch gates on uag.enabled = 'on' so a member of a disabled group does
+ * not retain access, matching is_weathermap_allowed().
+ *
+ * @param int      $userid   The Cacti user id to evaluate.
+ * @param int|null $group_id Optional weathermap group id to limit the result to;
+ *                           callers pass a value validated by
+ *                           get_filter_request_var(), so it is an integer.
+ * @return array<int,array<string,mixed>> Rows from weathermap_maps the user may
+ *                           view, ordered by sort order.
  */
 function get_allowed_weathermaps($userid, $group_id = null) {
 	// Special Group Limiter
@@ -73,6 +83,7 @@ function get_allowed_weathermaps($userid, $group_id = null) {
 			INNER JOIN user_auth_group_members AS uagm
 			ON uag.id = uagm.group_id
 			AND uagm.user_id = ?
+			WHERE uag.enabled = 'on'
 		) AS rs
 		ON rs.mapid = wm.id
 		WHERE wm.active = 'on'
@@ -239,10 +250,18 @@ function js_escape($str, $wrap = true) {
 }
 
 /**
- * @param mixed $format
- * @param string $value
- * @param int $kilo
- * @return mixed
+ * Formats a value for a map label, title or note.
+ *
+ * Supports weathermap's own %k (SI-scaled) and %t/%T (duration) specifiers in
+ * addition to a plain printf format.  The format text comes from the map config,
+ * so a stray or unknown conversion specifier falls back to the literal text
+ * rather than raising the ArgumentCountError / ValueError that sprintf() throws
+ * under PHP 8, which would abort the poller's map loop.
+ *
+ * @param mixed  $format The format string from the map config.
+ * @param string $value  The value to format.
+ * @param int    $kilo   The divisor for %k scaling (1000 or 1024).
+ * @return string        The formatted text, or the literal format on failure.
  */
 function mysprintf($format, $value, $kilo = 1000) {
 	$output = '';
@@ -307,7 +326,22 @@ function mysprintf($format, $value, $kilo = 1000) {
 	} else {
 		wm_debug('Falling through to standard sprintf');
 
-		$output = sprintf($format, $value);
+		/* $format is label/notes/title text from the map config, so it can carry
+		 * stray or extra conversion specifiers.  Under PHP 8 sprintf() throws
+		 * (ArgumentCountError for too few arguments, ValueError for an unknown
+		 * specifier) rather than warning, and this runs inside the poller's map
+		 * loop with no handler above it, so one bad label would abort the whole
+		 * poller cycle.  Fall back to the literal text instead of crashing. */
+		try {
+			$output = @sprintf($format, $value);
+
+			if ($output === false) {
+				$output = $format;
+			}
+		} catch (\Throwable $e) {
+			wm_warn('Invalid format string in map text, using it literally: ' . $format . ' [WMFMT01]');
+			$output = $format;
+		}
 	}
 
 	return $output;

lib/map-title.php

@@ -0,0 +1,82 @@
+<?php
+/*
+ +-------------------------------------------------------------------------+
+ | Copyright (C) 2004-2026 The Cacti Group, Howard Jones                   |
+ |                                                                         |
+ | 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/                                                   |
+ +-------------------------------------------------------------------------+
+*/
+
+/**
+ * Flattens a map title so it cannot inject map-config directives.
+ *
+ * Map configs are line oriented, and a duplicated map has its title substituted
+ * into the copied config file's TITLE line.  A title carrying a newline would
+ * otherwise append arbitrary directives (such as NODE or INFOURL) to that
+ * config, so every control character is collapsed to a space and the result is
+ * trimmed.  A non-string value (for example an array from a `title[]=` request
+ * variable) yields an empty string rather than a TypeError.
+ *
+ * @param  mixed  $title The raw title, from a request variable or the stored map.
+ * @return string The single-line title, safe to write into a config file.
+ */
+function map_clean_title($title): string {
+	/* The title arrives from a request variable by way of str_replace(), so an
+	 * array valued parameter such as title[]= would otherwise reach the string
+	 * type declaration here and raise a TypeError. */
+	if (!is_string($title)) {
+		return '';
+	}
+
+	return trim(preg_replace('/[\x00-\x1f\x7f]+/', ' ', $title) ?? $title);
+}
+
+/**
+ * Reports whether substituting a duplicated title would overflow a config line.
+ *
+ * map_duplicate() copies a map by replacing "TITLE <old>" with "TITLE <new>"
+ * across the source config with a whole-file str_replace(), which also rewrites
+ * the string wherever else it appears (an indented TITLE line, or a comment or
+ * label that happens to contain it).  ReadConfig() parses the config with
+ * fgets($fd, 4096) and trims each chunk, so a rewritten line that reaches that
+ * boundary is split and its trimmed tail is parsed as a separate directive,
+ * even past a leading '#'.  A line is read whole only when its bytes plus the
+ * newline fit in one 4096-byte read, i.e. the line is at most 4094 bytes, so
+ * reject the substitution when any line it lengthens would exceed that.
+ *
+ * @param  mixed  $old_title The source map's cached title (the TITLE value).
+ * @param  mixed  $new_title The cleaned title the duplicate will carry.
+ * @param  mixed  $contents  The source config file's contents.
+ * @return bool   True when the substitution would overflow a config line.
+ */
+function map_duplicate_title_overflows_line($old_title, $new_title, $contents): bool {
+	if (!is_string($contents) || !is_string($old_title) || !is_string($new_title)) {
+		return false;
+	}
+
+	$marker = 'TITLE ' . $old_title;
+
+	foreach (explode("\n", $contents) as $line) {
+		if (strpos($line, $marker) !== false
+			&& strlen(str_replace($marker, 'TITLE ' . $new_title, $line)) > 4094) {
+			return true;
+		}
+	}
+
+	return false;
+}