v0.19b

Seems like if REQUEST_METHOD is set to GET, then we can drop two redundant ways the query string is given. munin-cgi-graph itself reads the environment variables only, but its calls to Perl's CGI::param will look at the command line if REQUEST_METHOD is not used, otherwise it uses environment variables like CGI used to work.

Since this is all behind admin auth anyway, there isn't a public vulnerability. #914 was opened without comment which lead me to notice the redundancy and worry about a vulnerability, before I realized this is admin-only anyway.

The vulnerability was created by 6d6f3ea391.

See #914.

This is the v0.19b hotfix commit.

CHANGELOG.md

@@ -1,6 +1,13 @@
 CHANGELOG
 =========
 
+v0.19b (August 20, 2016)
+------------------------
+
+This update corrects a security issue introduced in v0.18.
+
+A remote code execution vulnerability is corrected in how the munin system monitoring graphs are generated for the control panel. The vulnerability involves an administrative user visiting a carefully crafted URL.
+
 v0.19a (August 18, 2016)
 ------------------------
 

README.md

@@ -59,7 +59,7 @@ by me:
 	$ curl -s https://keybase.io/joshdata/key.asc | gpg --import
 	gpg: key C10BDD81: public key "Joshua Tauberer <jt@occams.info>" imported
 
-	$ git verify-tag v0.19a
+	$ git verify-tag v0.19b
 	gpg: Signature made ..... using RSA key ID C10BDD81
 	gpg: Good signature from "Joshua Tauberer <jt@occams.info>"
 	gpg: WARNING: This key is not certified with a trusted signature!
@@ -72,7 +72,7 @@ and on my [personal homepage](https://razor.occams.info/). (Of course, if this r
 
 Checkout the tag corresponding to the most recent release:
 
-	$ git checkout v0.19a
+	$ git checkout v0.19b
 
 Begin the installation.
 

management/daemon.py

@@ -541,10 +541,9 @@ def munin_cgi(filename):
 	headers based on parameters in the requesting URL. All output is written
 	to stdout which munin_cgi splits into response headers and binary response
 	data.
-	munin-cgi-graph reads environment variables as well as passed input to determine
+	munin-cgi-graph reads environment variables to determine
 	what it should do. It expects a path to be in the env-var PATH_INFO, and a
-	querystring to be in the env-var QUERY_STRING as well as passed as input to the
+	querystring to be in the env-var QUERY_STRING.
-	command.
 	munin-cgi-graph has several failure modes. Some write HTTP Status headers and
 	others return nonzero exit codes.
 	Situating munin_cgi between the user-agent and munin-cgi-graph enables keeping
@@ -552,7 +551,7 @@ def munin_cgi(filename):
 	support infrastructure like spawn-fcgi.
 	"""
 
-	COMMAND = 'su - munin --preserve-environment --shell=/bin/bash -c /usr/lib/munin/cgi/munin-cgi-graph "%s"'
+	COMMAND = 'su - munin --preserve-environment --shell=/bin/bash -c /usr/lib/munin/cgi/munin-cgi-graph'
 	# su changes user, we use the munin user here
 	# --preserve-environment retains the environment, which is where Popen's `env` data is
 	# --shell=/bin/bash ensures the shell used is bash
@@ -564,12 +563,10 @@ def munin_cgi(filename):
 
 	query_str = request.query_string.decode("utf-8", 'ignore')
 
-	env = {'PATH_INFO': '/%s/' % filename, 'QUERY_STRING': query_str}
+	env = {'PATH_INFO': '/%s/' % filename, 'REQUEST_METHOD': 'GET', 'QUERY_STRING': query_str}
-	cmd = COMMAND % query_str
 	code, binout = utils.shell('check_output',
-							   cmd.split(' ', 5),
+							   COMMAND.split(" ", 5),
-							   # Using a maxsplit of 5 keeps the last 2 arguments together
+							   # Using a maxsplit of 5 keeps the last arguments together
-							   input=query_str.encode('UTF-8'),
 							   env=env,
 							   return_bytes=True,
 							   trap=True)

setup/bootstrap.sh

@@ -7,7 +7,7 @@
 #########################################################
 
 if [ -z "$TAG" ]; then
-	TAG=v0.19a
+	TAG=v0.19b
 fi
 
 # Are we running as root?