From a14b17794bd9a02e07f9aff2f6213a6e7d26fc2d Mon Sep 17 00:00:00 2001 From: Joshua Tauberer Date: Fri, 19 Aug 2016 12:42:43 -0400 Subject: [PATCH] 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 6d6f3ea3919d81abfd1a6eddd8787c98b761e1bb. See #914. This is the v0.19b hotfix commit. --- CHANGELOG.md | 2 ++ management/daemon.py | 15 ++++++--------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9d6a19c..9c81343d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ 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) ------------------------ diff --git a/management/daemon.py b/management/daemon.py index 9bc6429b..3c712303 100755 --- a/management/daemon.py +++ b/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 - command. + querystring to be in the env-var QUERY_STRING. 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} - cmd = COMMAND % query_str + env = {'PATH_INFO': '/%s/' % filename, 'REQUEST_METHOD': 'GET', 'QUERY_STRING': query_str} code, binout = utils.shell('check_output', - cmd.split(' ', 5), - # Using a maxsplit of 5 keeps the last 2 arguments together - input=query_str.encode('UTF-8'), + COMMAND.split(" ", 5), + # Using a maxsplit of 5 keeps the last arguments together env=env, return_bytes=True, trap=True)