Use a more secure command line building method

Previously, a command is built by string concatenation. Here, the
distinction between a value and multiple params got lost. Solve this
by using an array for shell arguments. As the escaping is now removed
from the `rrd_gen_graph` function, the canvas style needs to manually
add those quotes to make the JS code still work. That only supports
double-quotes, so hopefully nobody creates a name with a double quote
as that would break the fragile JS command line parser.

Separate the rrdtool options from the rrdtool graph command to make the
`$graph_type == 'canvas'` option work (it would otherwise not understand
the `rrdtool graph - -a PNG` option).

Merge the SVG and PNG cases as they are the same except for the
Content-Type header.

Fix a missing html escape in a debug style.

8 files changed, 100 insertions, 73 deletions

diff --git a/conf/config.php b/conf/config.php
index 30eaaf9..57a0188 100644
--- a/conf/config.php
+++ b/conf/config.php
@@ -12,8 +12,8 @@ $CONFIG['typesdb'][] = '/usr/share/collectd/types.db';
 # rrdtool executable
 $CONFIG['rrdtool'] = '/usr/bin/rrdtool';
 
-# rrdtool special options
-$CONFIG['rrdtool_opts'] = '';
+# rrdtool special command-line options
+$CONFIG['rrdtool_opts'] = [];
 
 # category of hosts to show on main page
 #$CONFIG['cat']['category1'] = array('host1', 'host2');
diff --git a/graph.php b/graph.php
index 1727c1b..3e33b92 100644
--- a/graph.php
+++ b/graph.php
@@ -86,7 +86,16 @@ if (isset($plugin_json[$type]['vertical'])) {
 }
 
 if (isset($plugin_json[$type]['rrdtool_opts'])) {
-        $obj->rrdtool_opts[] = $plugin_json[$type]['rrdtool_opts'];
+        $rrdtool_extra_opts = $plugin_json[$type]['rrdtool_opts'];
+        # compatibility with plugins which specify arguments as string
+        if (is_string($rrdtool_extra_opts)) {
+                $rrdtool_extra_opts = explode(' ', $rrdtool_extra_opts);
+        }
+
+        $obj->rrdtool_opts = array_merge(
+                $obj->rrdtool_opts,
+                $rrdtool_extra_opts
+        );
 }
 
 if (isset($plugin_json[$type]['datasize']) and $plugin_json[$type]['datasize'])
diff --git a/plugin/cpu.json b/plugin/cpu.json
index 50254f8..34e021d 100644
--- a/plugin/cpu.json
+++ b/plugin/cpu.json
@@ -2,7 +2,7 @@
         "cpu": {
                 "title": "CPU-{{PI}} usage",
                 "vertical": "Jiffies",
-                "rrdtool_opts": "-u 100",
+                "rrdtool_opts": ["-u", "100"],
                 "type": "stacked",
                 "legend": {
                         "idle": {
diff --git a/type/Base.class.php b/type/Base.class.php
index 1f42f4f..72ce437 100644
--- a/type/Base.class.php
+++ b/type/Base.class.php
@@ -36,8 +36,14 @@ class Type_Base {
         function __construct($config, $_get) {
                 $this->datadir = $config['datadir'];
                 $this->rrdtool = $config['rrdtool'];
-                if (!empty($config['rrdtool_opts']))
-                        $this->rrdtool_opts[] = $config['rrdtool_opts'];
+                if (!empty($config['rrdtool_opts'])) {
+                        if (is_array($config['rrdtool_opts'])) {
+                                $this->rrdtool_opts = $config['rrdtool_opts'];
+                        } else {
+                                $this->rrdtool_opts = explode(' ',
+                                                $config['rrdtool_opts']);
+                        }
+                }
                 $this->cache = $config['cache'];
                 $this->parse_get($_get);
                 $this->rrd_title = sprintf(
@@ -200,78 +206,90 @@ class Type_Base {
                 $this->rainbow_colors();
                 $this->colors = $colors + $this->colors;
 
-                $graphdata = $this->rrd_gen_graph();
+                $graphoptions = $this->rrd_gen_graph();
+                # $shellcmd contains escaped rrdtool arguments
+                $shellcmd = array();
+                foreach ($graphoptions as $arg)
+                        $shellcmd[] = escapeshellarg($arg);
 
                 $style = $debug !== false ? $debug : $this->graph_type;
                 switch ($style) {
                         case 'cmd':
                                 print '<pre>';
-                                foreach ($graphdata as $d) {
+                                foreach ($shellcmd as $d) {
                                         printf("%s \\\n", htmlentities($d));
                                 }
                                 print '</pre>';
                         break;
                         case 'canvas':
-                                printf('<canvas id="%s" class="rrd">', sha1(serialize($graphdata)));
-                                foreach ($graphdata as $d) {
-                                        printf("%s\n", htmlentities($d));
+                                printf('<canvas id="%s" class="rrd">', sha1(serialize($graphoptions)));
+                                foreach ($graphoptions as $d) {
+                                        printf("\"%s\"\n", htmlentities($d));
                                 }
                                 print '</canvas>';
                         break;
                         case 'debug':
                         case 1:
                                 print '<pre>';
-                                print_r($graphdata);
+                                htmlentities(print_r($shellcmd, TRUE));
                                 print '</pre>';
                         break;
                         case 'svg':
-                                # caching
-                                if (is_numeric($this->cache) && $this->cache > 0)
-                                        header("Expires: " . date(DATE_RFC822,strtotime($this->cache." seconds")));
-                                header("content-type: image/svg+xml");
-                                $graphdata = implode(' ', $graphdata);
-                                echo `$graphdata`;
-                        break;
                         case 'png':
                         default:
                                 # caching
                                 if (is_numeric($this->cache) && $this->cache > 0)
                                         header("Expires: " . date(DATE_RFC822,strtotime($this->cache." seconds")));
-                                header("content-type: image/png");
-                                $graphdata = implode(' ', $graphdata);
-                                echo `$graphdata`;
+
+                                if ($style === 'svg')
+                                        header("content-type: image/svg+xml");
+                                else
+                                        header("content-type: image/png");
+
+                                $shellcmd = array_merge(
+                                        $this->rrd_graph_command($style),
+                                        $shellcmd
+                                );
+                                $shellcmd = implode(' ', $shellcmd);
+                                passthru($shellcmd);
                         break;
                 }
         }
 
+        function rrd_graph_command($imgformat) {
+                if (!in_array($imgformat, array('png', 'svg')))
+                        $imgformat = 'png';
+
+                return array(
+                        $this->rrdtool,
+                        'graph',
+                        '-',
+                        '-a', strtoupper($imgformat)
+                );
+        }
+
         function rrd_options() {
-                switch ($this->graph_type) {
-                        case 'png':
-                        case 'hybrid':
-                                $rrdgraph[] = $this->rrdtool;
-                                $rrdgraph[] = 'graph - -a PNG';
-                        break;
-                        case 'svg':
-                                $rrdgraph[] = $this->rrdtool;
-                                $rrdgraph[] = 'graph - -a SVG';
-                        break;
-                        default:
-                        break;
-                }
-                if (!empty($this->rrdtool_opts))
-                        foreach($this->rrdtool_opts as $opt)
-                                $rrdgraph[] = $opt;
+                $rrdgraph = array();
+                foreach($this->rrdtool_opts as $opt)
+                        $rrdgraph[] = $opt;
                 if ($this->graph_smooth)
                         $rrdgraph[] = '-E';
-                if ($this->base)
-                        $rrdgraph[] = '--base '.$this->base;
-                $rrdgraph[] = sprintf('-w %d', is_numeric($this->width) ? $this->width : 400);
-                $rrdgraph[] = sprintf('-h %d', is_numeric($this->height) ? $this->height : 175);
-                $rrdgraph[] = '-l 0';
-                $rrdgraph[] = sprintf('-t "%s on %s"', $this->rrd_title, $this->args['host']);
-                if ($this->rrd_vertical)
-                        $rrdgraph[] = sprintf('-v "%s"', $this->rrd_vertical);
-                $rrdgraph[] = sprintf('-s e-%d', is_numeric($this->seconds) ? $this->seconds : 86400);
+                if ($this->base) {
+                        $rrdgraph[] = '--base';
+                        $rrdgraph[] = $this->base;
+                }
+                $rrdgraph = array_merge($rrdgraph, array(
+                        '-w', is_numeric($this->width) ? $this->width : 400,
+                        '-h', is_numeric($this->height) ? $this->height : 175,
+                        '-l', '0',
+                        '-t', "{$this->rrd_title} on {$this->args['host']}"
+                ));
+                if ($this->rrd_vertical) {
+                        $rrdgraph[] = '-v';
+                        $rrdgraph[] = $this->rrd_vertical;
+                }
+                $rrdgraph[] = '-s';
+                $rrdgraph[] = sprintf('e-%d', is_numeric($this->seconds) ? $this->seconds : 86400);
 
                 return $rrdgraph;
         }
diff --git a/type/Default.class.php b/type/Default.class.php
index 6e7efd9..4cb9ff7 100644
--- a/type/Default.class.php
+++ b/type/Default.class.php
@@ -48,11 +48,11 @@ class Type_Default extends Type_Base {
                 foreach ($sources as $source) {
                         $legend = empty($this->legend[$source]) ? $source : $this->legend[$source];
                         $color = is_array($this->colors) ? (isset($this->colors[$source])?$this->colors[$source]:$this->colors[$c++]): $this->colors;
-                        $rrdgraph[] = sprintf('"LINE1:avg_%s#%s:%s"', crc32hex($source), $this->validate_color($color), $this->rrd_escape($legend));
-                        $rrdgraph[] = sprintf('"GPRINT:min_%s:MIN:%s Min,"', crc32hex($source), $this->rrd_format);
-                        $rrdgraph[] = sprintf('"GPRINT:avg_%s:AVERAGE:%s Avg,"', crc32hex($source), $this->rrd_format);
-                        $rrdgraph[] = sprintf('"GPRINT:max_%s:MAX:%s Max,"', crc32hex($source), $this->rrd_format);
-                        $rrdgraph[] = sprintf('"GPRINT:avg_%s:LAST:%s Last\\l"', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('LINE1:avg_%s#%s:%s', crc32hex($source), $this->validate_color($color), $this->rrd_escape($legend));
+                        $rrdgraph[] = sprintf('GPRINT:min_%s:MIN:%s Min,', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('GPRINT:avg_%s:AVERAGE:%s Avg,', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('GPRINT:max_%s:MAX:%s Max,', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('GPRINT:avg_%s:LAST:%s Last\\l', crc32hex($source), $this->rrd_format);
                 }
 
                 return $rrdgraph;
diff --git a/type/GenericIO.class.php b/type/GenericIO.class.php
index cab5220..20029a1 100644
--- a/type/GenericIO.class.php
+++ b/type/GenericIO.class.php
@@ -59,21 +59,21 @@ class Type_GenericIO extends Type_Base {
                 $i = 0;
                 foreach($sources as $source) {
                         $legend = empty($this->legend[$source]) ? $source : $this->legend[$source];
-                        $rrdgraph[] = sprintf('"LINE1:avg_%s%s#%s:%s"', crc32hex($source), $i == 1 ? '_neg' : '', $this->colors[$source], $this->rrd_escape($legend));
-                        $rrdgraph[] = sprintf('"GPRINT:min_%s:MIN:%s Min,"', crc32hex($source), $this->rrd_format);
-                        $rrdgraph[] = sprintf('"GPRINT:avg_%s:AVERAGE:%s Avg,"', crc32hex($source), $this->rrd_format);
-                        $rrdgraph[] = sprintf('"GPRINT:max_%s:MAX:%s Max,"', crc32hex($source), $this->rrd_format);
-                        $rrdgraph[] = sprintf('"GPRINT:avg_%s:LAST:%s Last"', crc32hex($source), $this->rrd_format);
-                        $rrdgraph[] = sprintf('"GPRINT:tot_%s:%s Total\l"',crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('LINE1:avg_%s%s#%s:%s', crc32hex($source), $i == 1 ? '_neg' : '', $this->colors[$source], $this->rrd_escape($legend));
+                        $rrdgraph[] = sprintf('GPRINT:min_%s:MIN:%s Min,', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('GPRINT:avg_%s:AVERAGE:%s Avg,', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('GPRINT:max_%s:MAX:%s Max,', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('GPRINT:avg_%s:LAST:%s Last', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('GPRINT:tot_%s:%s Total\l',crc32hex($source), $this->rrd_format);
                         $i++;
                 }
-                
+
                 if ($this->percentile) {
-                        $rrdgraph[] = sprintf('"COMMENT: \l"');
+                        $rrdgraph[] = 'COMMENT: \l';
                         foreach($sources as $source) {
                                 $legend = empty($this->legend[$source]) ? $source : $this->legend[$source];
-                                $rrdgraph[] = sprintf('"HRULE:pct_%s#%s:%sth Percentile %s"', crc32hex($source), $this->get_faded_color($this->colors[$source], '000000', 0.6), $this->percentile, $this->rrd_escape($legend));
-                                $rrdgraph[] = sprintf('"GPRINT:pct_%s:%s\l"', crc32hex($source), $this->rrd_format);
+                                $rrdgraph[] = sprintf('HRULE:pct_%s#%s:%sth Percentile %s', crc32hex($source), $this->get_faded_color($this->colors[$source], '000000', 0.6), $this->percentile, $this->rrd_escape($legend));
+                                $rrdgraph[] = sprintf('GPRINT:pct_%s:%s\l', crc32hex($source), $this->rrd_format);
                         }
                 }
 
diff --git a/type/GenericStacked.class.php b/type/GenericStacked.class.php
index a74ff18..d41c2ab 100644
--- a/type/GenericStacked.class.php
+++ b/type/GenericStacked.class.php
@@ -51,11 +51,11 @@ class Type_GenericStacked extends Type_Base {
                 foreach ($sources as $source) {
                         $legend = empty($this->legend[$source]) ? $source : $this->legend[$source];
                         $color = is_array($this->colors) ? (isset($this->colors[$source])?$this->colors[$source]:$this->colors[$c++]) : $this->colors;
-                        $rrdgraph[] = sprintf('"LINE1:area_%s#%s:%s"', crc32hex($source), $this->validate_color($color), $this->rrd_escape($legend));
-                        $rrdgraph[] = sprintf('"GPRINT:min_%s:MIN:%s Min,"', crc32hex($source), $this->rrd_format);
-                        $rrdgraph[] = sprintf('"GPRINT:avg_%s:AVERAGE:%s Avg,"', crc32hex($source), $this->rrd_format);
-                        $rrdgraph[] = sprintf('"GPRINT:max_%s:MAX:%s Max,"', crc32hex($source), $this->rrd_format);
-                        $rrdgraph[] = sprintf('"GPRINT:avg_%s:LAST:%s Last\\l"', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('LINE1:area_%s#%s:%s', crc32hex($source), $this->validate_color($color), $this->rrd_escape($legend));
+                        $rrdgraph[] = sprintf('GPRINT:min_%s:MIN:%s Min,', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('GPRINT:avg_%s:AVERAGE:%s Avg,', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('GPRINT:max_%s:MAX:%s Max,', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('GPRINT:avg_%s:LAST:%s Last\\l', crc32hex($source), $this->rrd_format);
                 }
 
                 return $rrdgraph;
diff --git a/type/Uptime.class.php b/type/Uptime.class.php
index e57df0b..52af3a4 100644
--- a/type/Uptime.class.php
+++ b/type/Uptime.class.php
@@ -45,16 +45,16 @@ class Type_Uptime extends Type_Base {
                         $color = is_array($this->colors) ? (isset($this->colors[$source])?$this->colors[$source]:$this->colors[$c++]) : $this->colors;
 
                         //current value
-                        $rrdgraph[] = sprintf('"LINE1:area_%s#%s:%s"', crc32hex($source), $this->validate_color($color), $this->rrd_escape($legend));
-                        $rrdgraph[] = sprintf('"GPRINT:c_avg_%s:LAST:%s days\\l"', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('LINE1:area_%s#%s:%s', crc32hex($source), $this->validate_color($color), $this->rrd_escape($legend));
+                        $rrdgraph[] = sprintf('GPRINT:c_avg_%s:LAST:%s days\\l', crc32hex($source), $this->rrd_format);
 
                         //max value
-                        $rrdgraph[] = sprintf('"LINE1:v_max_%s#FF0000:Maximum:dashes"', crc32hex($source));
-                        $rrdgraph[] = sprintf('"GPRINT:v_max_%s:%s days\\l"', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('LINE1:v_max_%s#FF0000:Maximum:dashes', crc32hex($source));
+                        $rrdgraph[] = sprintf('GPRINT:v_max_%s:%s days\\l', crc32hex($source), $this->rrd_format);
 
                         //avg value
-                        $rrdgraph[] = sprintf('"LINE1:v_avg_%s#0000FF:Average:dashes"', crc32hex($source));
-                        $rrdgraph[] = sprintf('"GPRINT:v_avg_%s:%s days\\l"', crc32hex($source), $this->rrd_format);
+                        $rrdgraph[] = sprintf('LINE1:v_avg_%s#0000FF:Average:dashes', crc32hex($source));
+                        $rrdgraph[] = sprintf('GPRINT:v_avg_%s:%s days\\l', crc32hex($source), $this->rrd_format);
                 }
 
                 return $rrdgraph;