Commit a14b1779 authored by Joshua Tauberer's avatar Joshua Tauberer

simplify how munin-cgi-graph is called to reduce the attack surface area

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 6d6f3ea3.

See #914.

This is the v0.19b hotfix commit.
parent 7c9f3e0b
CHANGELOG CHANGELOG
========= =========
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) v0.19a (August 18, 2016)
------------------------ ------------------------
......
...@@ -541,10 +541,9 @@ def munin_cgi(filename): ...@@ -541,10 +541,9 @@ def munin_cgi(filename):
headers based on parameters in the requesting URL. All output is written headers based on parameters in the requesting URL. All output is written
to stdout which munin_cgi splits into response headers and binary response to stdout which munin_cgi splits into response headers and binary response
data. 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 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 munin-cgi-graph has several failure modes. Some write HTTP Status headers and
others return nonzero exit codes. others return nonzero exit codes.
Situating munin_cgi between the user-agent and munin-cgi-graph enables keeping Situating munin_cgi between the user-agent and munin-cgi-graph enables keeping
...@@ -552,7 +551,7 @@ def munin_cgi(filename): ...@@ -552,7 +551,7 @@ def munin_cgi(filename):
support infrastructure like spawn-fcgi. 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 # su changes user, we use the munin user here
# --preserve-environment retains the environment, which is where Popen's `env` data is # --preserve-environment retains the environment, which is where Popen's `env` data is
# --shell=/bin/bash ensures the shell used is bash # --shell=/bin/bash ensures the shell used is bash
...@@ -564,12 +563,10 @@ def munin_cgi(filename): ...@@ -564,12 +563,10 @@ def munin_cgi(filename):
query_str = request.query_string.decode("utf-8", 'ignore') 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', 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, env=env,
return_bytes=True, return_bytes=True,
trap=True) trap=True)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment