Cacti / plugin_reportit

ReportIt Plugin for Cacti
GNU General Public License v2.0
7 stars 9 forks source link

Input Validation Not Performed #28

Closed gmourani closed 5 years ago

gmourani commented 6 years ago

Hello,

Another problem with version 1.0.0. I'm trying to run the report and get REPORTIT ERROR: PHP modul for RRDtool is not available even if php-rrd is installed! Also, the following appears in the log file.

CMDPHP Input Validation Not Performed for 'drp_action' Backtrace: (/plugins/reportit/reports.php: 39 form_actions)(/plugins/reportit/reports.php: 1260 get_request_var)(/lib/html_utility.php: 328 html_log_input_error)(/lib/html_validate.php: 44 cacti_debug_backtrace)

CMDPHP Input Validation Not Performed for 'id' Backtrace: (/plugins/reportit/run.php: 50 calculation)(/plugins/reportit/run.php: 62 get_request_var)(/lib/html_utility.php: 328 html_log_input_error)(/lib/html_validate.php: 44 cacti_debug_backtrace)

Regards,

netniV commented 6 years ago

You will only see this error when you have the cacti debugging at the developer level. This error comes when a piece of code uses get_request_var() before calling one of the validate routines on the variable.

Can you open a separate issue with your RRD issue as that will be a different problem?

gmourani commented 6 years ago

Ok, will do, thanks.

netniV commented 6 years ago

I'm reopening this issue as the validation error should not be there. If there are fields not being validated, they could lead to other security issues.

netniV commented 6 years ago

The following is a basic patch that I've started to produce for this, there is more to come:

diff --git a/reports.php b/reports.php
index 0bc9102..e381618 100644
--- a/reports.php
+++ b/reports.php
@@ -34,6 +34,97 @@ include_once(REPORTIT_BASE_PATH . '/lib/funct_html.php');

 set_default_action();

+/* ================= input validation and session storage ================= */
+
+/* The following is a full list of request vars used in this file called by
+   get_request_var(), it does not cover request vars called via other methods
+   which include but are not limited to get_nfilter_request_var():
+
+       action, data_source_filter, drp_action, filter, host_template_id, id,
+       owner, page, preset_timespan, rec, report_addition, report_autoarchive,
+       report_autoexport, report_autoexport_max_records, report_description,
+       report_email_address, report_email_body, report_email_format,
+       report_email_recipient, report_email_subject, report_end_date,
+       report_owner, report_schedule_frequency, report_start_date,
+       report_timespan, rows, rrdlist_shifttime_end, rrdlist_shifttime_start,
+       rrdlist_subhead, rrdlist_timezone, rrdlist_weekday_end,
+       rrdlist_weekday_start, selected_items, sort_column, tab, template,
+       template_id
+
+   Some of these are now pre-filtered using standard Cacti methods.  Others
+   are using the reportit specific input_validate_xxx functions to validate
+   the values and can be requested unfiltered into those functions as long
+   as they are properly validated and use get_nfilter_request_var() after
+   to make sure no erroneous reports are made about the attempt to read the
+   variables.
+*/
+
+$filters = array(
+       'rows' => array(
+               'filter' => FILTER_VALIDATE_INT,
+               'pageset' => true,
+               'default' => '-1'
+               ),
+       'page' => array(
+               'filter' => FILTER_VALIDATE_INT,
+               'default' => '1'
+               ),
+       'host_template_id' => array(
+               'filter' => FILTER_VALIDATE_INT,
+               'default' => ''
+               ),
+       'id' => array(
+               'filter' => FILTER_VALIDATE_INT,
+               'default' => ''
+               ),
+       'filter' => array(
+               'filter' => FILTER_CALLBACK,
+               'pageset' => true,
+               'default' => '',
+               'options' => array('options' => 'sanitize_search_string')
+               ),
+       'data_source_filter' => array(
+               'filter' => FILTER_CALLBACK,
+               'default' => '',
+               'options' => array('options' => 'sanitize_search_string')
+               ),
+       'rrdlist_subhead' => array(
+               'filter' => FILTER_CALLBACK,
+               'default' => '',
+               'options' => array('options' => 'sanitize_search_string')
+               ),
+       'report_description' => array(
+               'filter' => FILTER_CALLBACK,
+               'default' => '',
+               'options' => array('options' => 'sanitize_search_string')
+               ),
+       'report_email_subject' => array(
+               'filter' => FILTER_CALLBACK,
+               'default' => '',
+               'options' => array('options' => 'sanitize_search_string')
+               ),
+       'report_email_body' => array(
+               'filter' => FILTER_CALLBACK,
+               'default' => '',
+               'options' => array('options' => 'sanitize_search_string')
+               ),
+       'report_email_address' => array(
+               'filter' => FILTER_VALIDATE_EMAIL,
+               'default' => ''
+               ),
+       'owner' => array(
+               'filter' => FILTER_VALIDATE_INT,
+               'default' => '-1'
+               ),
+       'template' => array(
+               'filter' => FILTER_VALIDATE_INT,
+               'default' => '-1'
+               ),
+);
+
+validate_store_request_vars($filters, 'sess_reports');
+/* ================= Input validation ================= */
+
 switch (get_request_var('action')) {
        case 'actions':
                form_actions();
@@ -504,11 +595,11 @@ function form_save() {
        switch(get_request_var('tab')) {
        case 'presets':
                input_validate_input_blacklist(get_request_var('id'),array(0));
-               input_validate_input_key(get_request_var('rrdlist_timezone'), $timezone, true);
-               input_validate_input_key(get_request_var('rrdlist_shifttime_start'), $shifttime);
-               input_validate_input_key(get_request_var('rrdlist_shifttime_end'), $shifttime2);
-               input_validate_input_key(get_request_var('rrdlist_weekday_start'), $weekday);
-               input_validate_input_key(get_request_var('rrdlist_weekday_end'), $weekday);
+               input_validate_input_key(get_nfilter_request_var('rrdlist_timezone'), $timezone, true);
+               input_validate_input_key(get_nfilter_request_var('rrdlist_shifttime_start'), $shifttime);
+               input_validate_input_key(get_nfilter_request_var('rrdlist_shifttime_end'), $shifttime2);
+               input_validate_input_key(get_nfilter_request_var('rrdlist_weekday_start'), $weekday);
+               input_validate_input_key(get_nfilter_request_var('rrdlist_weekday_end'), $weekday);

                form_input_validate(get_request_var('rrdlist_subhead'), 'rrdlist_subhead', '' ,true,3);

@@ -521,12 +612,12 @@ function form_save() {
                input_validate_input_key(get_request_var('report_owner'), $owner);

                if (read_config_option('reportit_operator')) {
-                       input_validate_input_key(get_request_var('report_schedule_frequency'), $frequency, true);
-                       input_validate_input_limits(get_request_var('report_autoarchive'),0,1000);
+                       input_validate_input_key(get_nfilter_request_var('report_schedule_frequency'), $frequency, true);
+                       input_validate_input_limits(get_nfilter_request_var('report_autoarchive'),0,1000);

                        if (read_config_option('reportit_auto_export')) {
-                               input_validate_input_limits(get_request_var('report_autoexport_max_records'),0,1000);
-                               input_validate_input_key(get_request_var('report_autoexport'), $format, true);
+                               input_validate_input_limits(get_nfilter_request_var('report_autoexport_max_records'),0,1000);
+                               input_validate_input_key(get_nfilter_request_var('report_autoexport'), $format, true);
                        }
                }

@@ -549,21 +640,21 @@ function form_save() {
                /* if template is locked we don't know if the variables have been changed */
                locked(get_request_var('template_id'));

-               form_input_validate(get_request_var('report_description'), 'report_description', '' ,false,3);
+               form_input_validate(get_nfilter_request_var('report_description'), 'report_description', '' ,false,3);

                /* validate start- and end date if sliding time should not be used */
                if (!isset_request_var('report_dynamic')) {
-                       if (!preg_match('/^\d{4}\-\d{2}\-\d{2}$/', get_request_var('report_start_date'))) {
+                       if (!preg_match('/^\d{4}\-\d{2}\-\d{2}$/', get_nfilter_request_var('report_start_date'))) {
                                session_custom_error_message('report_start_date', 'Invalid date');
                        }

-                       if (!preg_match('/^\d{4}\-\d{2}\-\d{2}$/', get_request_var('report_end_date'))) {
+                       if (!preg_match('/^\d{4}\-\d{2}\-\d{2}$/', get_nftiler_request_var('report_end_date'))) {
                                session_custom_error_message('report_end_date', 'Invalid date');
                        }

                        if (!is_error_message()) {
-                               list($ys, $ms, $ds) = explode('-', get_request_var('report_start_date'));
-                               list($ye, $me, $de) = explode('-', get_request_var('report_end_date'));
+                               list($ys, $ms, $ds) = explode('-', get_nfilter_request_var('report_start_date'));
+                               list($ye, $me, $de) = explode('-', get_nfilter_request_var('report_end_date'));

                                if (!checkdate($ms, $ds, $ys)) session_custom_error_message('report_start_date', 'Invalid date');
                                if (!checkdate($me, $de, $ye)) session_custom_error_message('report_end_date', 'Invalid date');
@@ -578,13 +669,13 @@ function form_save() {
                }

                if (!read_config_option('reportit_operator')) {
-                       input_validate_input_key(get_request_var('report_schedule_frequency'), $frequency, true);
-                   input_validate_input_limits(get_request_var('report_autoarchive'),0,1000);
+                       input_validate_input_key(get_nfilter_request_var('report_schedule_frequency'), $frequency, true);
+                       input_validate_input_limits(get_nfilter_request_var('report_autoarchive'),0,1000);

-                   if (read_config_option('reportit_auto_export')) {
-                               input_validate_input_limits(get_request_var('report_autoexport_max_records'),0,1000);
-                               input_validate_input_key(get_request_var('report_autoexport'), $format, true);
-                   }
+                       if (read_config_option('reportit_auto_export')) {
+                               input_validate_input_limits(get_nfilter_request_var('report_autoexport_max_records'),0,1000);
+                               input_validate_input_key(get_nfilter_request_var('report_autoexport'), $format, true);
+                       }
                }
        }
        /* ==================================================== */
diff --git a/run.php b/run.php
index 1cdc60b..b88cb20 100644
--- a/run.php
+++ b/run.php
@@ -44,6 +44,22 @@ if (isset($_SESSION['run']) && ($_SESSION['run'] == '0')) {
        exit;
 }

+/* ================= input validation and session storage ================= */
+$filters = array(
+       'action' => array(
+               'filter' => FILTER_CALLBACK,
+               'default' => '',
+               'options' => array('options' => 'sanitize_search_string')
+               ),
+       'id' => array(
+               'filter' => FILTER_VALIDATE_INT,
+               'default' => '1'
+               ),
+);
+
+validate_store_request_vars($filters, 'sess_reportit_run');
+/* ================= Input validation ================= */
+
 switch (get_request_var('action')) {
        case 'calculation':
                #top_header();
diff --git a/setup.php b/setup.php
index 8f14c04..26eefbc 100644
--- a/setup.php
+++ b/setup.php
@@ -80,6 +80,15 @@ function reportit_check_upgrade() {
        $old     = db_fetch_row("SELECT * FROM plugin_config WHERE directory='reportit'");
        $tables  = db_fetch_assoc("SHOW TABLE STATUS WHERE `Name` LIKE 'reportit%'");

+       $old_version = "#.#";
+       $old_status = "#";
+
+       if (sizeof($old)) {
+               $old_version = $old['version'];
+               $old_status = $old['status'];
+       }
+       cacti_log("REPORTIT: v$current replacing $old_version ($old_status)");
+
        if (sizeof($old) && $current == $old['version']){
                /* ReportIT is up to date */
                return true;
gmourani commented 6 years ago

I've applied the patches. Here what I can see in the Apache log file now related to this issue.

[Mon Jul 09 09:52:47 2018] [error] [client 192.168.2.147] PHP Notice: Undefined index: reportit_tWizard in /var/www/html/cacti/plugins/reportit/templates.php on line 734, referer: https://192.168.2.227/cacti/plugins/reportit/templates.php?action=template_export_wizard

[Mon Jul 09 09:52:47 2018] [error] [client 192.168.2.147] PHP Notice: Undefined index: in /var/www/html/cacti/plugins/reportit/templates.php on line 737, referer: https://192.168.2.227/cacti/plugins/reportit/templates.php?action=template_export_wizard

Regards,

netniV commented 6 years ago

Oh, wasn't expecting anyone to actually apply the patches yet, they are still a work in progress so there will be more ;-)

Could you open a separate issue for the indexing problem? I thought someone already had but I can't actually see one open.

gmourani commented 6 years ago

Done.

browniebraun commented 6 years ago

The different options how ReportIt can establish a connection to RRDtool will be removed. This plugin is more than a decade old - coming from times when RRDtool itself did not support pipelining.

netniV commented 5 years ago

I believe this has now been resolved.