Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Resolve issues with SQL injections in user_admin.php
  • Loading branch information
cigamit committed Nov 29, 2021
1 parent 0ee3f7b commit 33b894c
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 6 deletions.
1 change: 1 addition & 0 deletions CHANGELOG
@@ -1,6 +1,7 @@
Cacti CHANGELOG

1.2.20
-security: Resolve issues with SQL injections in user_admin.php
-issue#4363: Duplicate entries in graph_templates_item - mabye an aftermath of the template edit bug
-issue#4435: Unable to Save Graph Settings from the Graphs pages
-issue#4449: Script Server can crash if an OID is missing from device
Expand Down
12 changes: 6 additions & 6 deletions user_admin.php
Expand Up @@ -84,16 +84,16 @@
function update_policies() {
$set = '';

$set .= isset_request_var('policy_graphs') ? 'policy_graphs=' . get_nfilter_request_var('policy_graphs'):'';
$set .= isset_request_var('policy_trees') ? ($set != '' ? ',':'') . 'policy_trees=' . get_nfilter_request_var('policy_trees'):'';
$set .= isset_request_var('policy_hosts') ? ($set != '' ? ',':'') . 'policy_hosts=' . get_nfilter_request_var('policy_hosts'):'';
$set .= isset_request_var('policy_graph_templates') ? ($set != '' ? ',':'') . 'policy_graph_templates=' . get_nfilter_request_var('policy_graph_templates'):'';
$set .= isset_request_var('policy_graphs') ? 'policy_graphs=' . get_filter_request_var('policy_graphs'):'';
$set .= isset_request_var('policy_trees') ? ($set != '' ? ',':'') . 'policy_trees=' . get_filter_request_var('policy_trees'):'';
$set .= isset_request_var('policy_hosts') ? ($set != '' ? ',':'') . 'policy_hosts=' . get_filter_request_var('policy_hosts'):'';
$set .= isset_request_var('policy_graph_templates') ? ($set != '' ? ',':'') . 'policy_graph_templates=' . get_filter_request_var('policy_graph_templates'):'';

if ($set != '') {
db_execute_prepared("UPDATE user_auth SET $set WHERE id = ?", array(get_nfilter_request_var('id')));
db_execute_prepared("UPDATE user_auth SET $set WHERE id = ?", array(get_filter_request_var('id')));
}

header('Location: user_admin.php?action=user_edit&header=false&tab=' . get_nfilter_request_var('tab') . '&id=' . get_nfilter_request_var('id'));
header('Location: user_admin.php?action=user_edit&header=false&tab=' . get_nfilter_request_var('tab') . '&id=' . get_filter_request_var('id'));
exit;
}

Expand Down

5 comments on commit 33b894c

@TheWitness
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should check user group administration page too.

@cigamit
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I wonder if we should rewrite these to use prepared statements instead.

@cigamit
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate having to break them out into their own queries, but this works

	if (isset_request_var('policy_graphs')) {
		db_execute_prepared("UPDATE user_auth_group SET `policy_graphs` = ? WHERE id = ?", array(get_filter_request_var('policy_graphs'), get_filter_request_var('id')));
	}
	if (isset_request_var('policy_trees')) {
		db_execute_prepared("UPDATE user_auth_group SET `policy_trees` = ? WHERE id = ?", array(get_filter_request_var('policy_trees'), get_filter_request_var('id')));
	}
	if (isset_request_var('policy_hosts')) {
		db_execute_prepared("UPDATE user_auth_group SET `policy_hosts` = ? WHERE id = ?", array(get_filter_request_var('policy_hosts'), get_filter_request_var('id')));
	}
	if (isset_request_var('policy_graph_templates')) {
		db_execute_prepared("UPDATE user_auth_group SET `policy_graph_templates` = ? WHERE id = ?", array(get_filter_request_var('policy_graph_templates'), get_filter_request_var('id')));
	}

@netniV
Copy link
Member

@netniV netniV commented on 33b894c Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is to use a loop and add the set field names to an array, then the values to another array, then implode the list, then it's just one prepared statement. But that could over complicate things without providing much benefit.

@cigamit
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, I like that option better and its smaller. I will push a commit for that here in a sec.

Please sign in to comment.