Re: cacti-0.8.8b: Multiple COMMENT arguments in "rrdtool graph" ??

All,

please find my current solution to this issue and post your results. Diff is
made against 088b code
Reinhard

Index: rrd.php
===================================================================
--- rrd.php	(Revision 7407)
+++ rrd.php	(Arbeitskopie)
@@ -1343,20 +1343,20 @@
 		$need_rrd_nl = TRUE;

 		if ($graph_item_types{$graph_item["graph_type_id"]} == "COMMENT") {
+			# perform variable substitution first (in case this will yield an empty
results or brings command injection problems)
+			$comment_arg =
rrd_substitute_host_query_data($graph_variables["text_format"][$graph_item_id],
$graph, $graph_item);
+			# next, compute the argument of the COMMENT statement and perform injection
counter measures
+			if (trim($comment_arg) == '') { # an empty COMMENT must be treated with care
+				$comment_arg = cacti_escapeshellarg(' ' . $hardreturn[$graph_item_id]);
+			} else {
+				$comment_arg = cacti_escapeshellarg($comment_arg .
$hardreturn[$graph_item_id]);
+			}
+
+			# create rrdtool specific command line
 			if (read_config_option("rrdtool_version") != "rrd-1.0.x") {
-				$comment_string = $graph_item_types{$graph_item["graph_type_id"]} . ":" .
str_replace(":", "\:",
cacti_escapeshellarg($graph_variables["text_format"][$graph_item_id] .
$hardreturn[$graph_item_id])) . " ";
-				if (trim($comment_string) == 'COMMENT:"\n"') {
-					$txt_graph_items .= 'COMMENT:" \n"'; # rrdtool will skip a COMMENT that
holds a NL only; so add a blank to make NL work
-				} else if (trim($comment_string) != "COMMENT:\"\"") {
-					$txt_graph_items .= rrd_substitute_host_query_data($comment_string,
$graph, $graph_item);
-				}
+				$txt_graph_items .= $graph_item_types{$graph_item["graph_type_id"]} . ":" .
str_replace(":", "\:", $comment_arg) . " ";
 			}else {
-				$comment_string = $graph_item_types{$graph_item["graph_type_id"]} . ":" .
cacti_escapeshellarg($graph_variables["text_format"][$graph_item_id] .
$hardreturn[$graph_item_id]) . " ";
-				if (trim($comment_string) == 'COMMENT:"\n"') {
-					$txt_graph_items .= 'COMMENT:" \n"'; # rrdtool will skip a COMMENT that
holds a NL only; so add a blank to make NL work
-				} else if (trim($comment_string) != "COMMENT:\"\"") {
-					$txt_graph_items .= rrd_substitute_host_query_data($comment_string,
$graph, $graph_item);
-				}
+				$txt_graph_items .= $graph_item_types{$graph_item["graph_type_id"]} . ":" .
$comment_arg . " ";
 			}
 		}elseif (($graph_item_types{$graph_item["graph_type_id"]} == "GPRINT") &&
(!isset($graph_data_array["graph_nolegend"]))) {
 			$graph_variables["text_format"][$graph_item_id] = str_replace(":", "\:",
$graph_variables["text_format"][$graph_item_id]); /* escape colons */






On 08.08.2013 11:51, Gandalf wrote:
> On 08.08.2013 10:20, Marc-Christian Petersen wrote:
>> the problem code is this (diff 0.8.8a -> 0.8.8b):
>>
>> lib/rrd.php
>>
>> @@ -1344,14 +1344,14 @@ function rrdtool_function_graph($local_g
>>
>>                 if ($graph_item_types{$graph_item["graph_type_id"]} == "COMMENT") {
>>                         if (read_config_option("rrdtool_version") != "rrd-1.0.x") {
>> -                               $comment_string = $graph_item_types{$graph_item["graph_type_id"]} . ":\"" . str_replace(":", "\:", $graph_variables["text_format"][$graph_item_id]) . $hardreturn[$graph_item_id] . "\" ";
>> +                               $comment_string = $graph_item_types{$graph_item["graph_type_id"]} . ":" . str_replace(":", "\:", cacti_escapeshellarg($graph_variables["text_format"][$graph_item_id] . $hardreturn[$graph_item_id])) . " ";
>>                                 if (trim($comment_string) == 'COMMENT:"\n"') {
>>                                         $txt_graph_items .= 'COMMENT:" \n"'; # rrdtool will skip a COMMENT that holds a NL only; so add a blank to make NL work
>>                                 } else if (trim($comment_string) != "COMMENT:\"\"") {
>>                                         $txt_graph_items .= rrd_substitute_host_query_data($comment_string, $graph, $graph_item);
>>                                 }
>>                         }else {
>> -                               $comment_string = $graph_item_types{$graph_item["graph_type_id"]} . ":\"" . $graph_variables["text_format"][$graph_item_id] . $hardreturn[$graph_item_id] . "\" ";
>> +                               $comment_string = $graph_item_types{$graph_item["graph_type_id"]} . ":" . cacti_escapeshellarg($graph_variables["text_format"][$graph_item_id] . $hardreturn[$graph_item_id]) . " ";
>>                                 if (trim($comment_string) == 'COMMENT:"\n"') {
>>                                         $txt_graph_items .= 'COMMENT:" \n"'; # rrdtool will skip a COMMENT that holds a NL only; so add a blank to make NL work
>>                                 } else if (trim($comment_string) != "COMMENT:\"\"") {
>>
>> unapplying this fixes the "no empty comments" problem.
> 
> Thanks for pointing out the location of the error.
> It is not recommended to undo this change, due to security reasons (command line
> injection).
> I'm working out a different fix, that mostly will keep the old behavior. There's
> only one corner case:
> 
> Having an empty COMMENT with a HR should result in a "empty line in the legend".
> This requires the rrdtool statement to have sth like
> "COMMENT: '\n'" (an whitespace has to be added)
> But what should an empty COMMENT without a HR yield? Currently, this will go
> "COMMENT: ", which includes a whitespace even though not entered by the user.
> A different solution would be to skip an empty COMMENT without a HR completely.
> I suppose that this is the better solution, even if it does not mimic the old
> (wrong?) behavior.
> 
> Thoughts?
> Reinhard
> 
> ------------------------------------------------------------------------------
> Get 100% visibility into Java/.NET code with AppDynamics Lite!
> It's a free troubleshooting tool designed for production.
> Get down to code-level detail for bottlenecks, with <2% overhead. 
> Download for free and get started troubleshooting in minutes. 
> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
> _______________________________________________
> cacti-user mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/cacti-user
> 

------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk