Skip to content

Commit 56b6eee

Browse files
somethingwithproofTheWitness
somethingwithproof
and
TheWitness
authored
hardening: deprecate raw SQL fragment passing in sequence functions (#6857) (#6858)
* hardening: deprecate raw SQL fragment passing in sequence functions (#6857) * fix: validate field names in build_where_from_array() against injection Reject field names containing non-alphanumeric characters to prevent SQL injection via malicious array keys. Log rejected field names at SECURITY level. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: add field name validation to test helper and injection rejection tests Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: align CS-Fixer spacing in build_where_from_array call sites Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net>
1 parent 50ced7f commit 56b6eee

File tree

3 files changed

+148
-15
lines changed

3 files changed

+148
-15
lines changed

‎data_queries.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -416,7 +416,7 @@ function data_query_item_movedown_gsv() : void {
416416
gfrv('snmp_query_graph_id');
417417
// ====================================================
418418

419-
move_item_down('snmp_query_graph_sv', grv('id'), 'snmp_query_graph_id=' . grv('snmp_query_graph_id') . ' AND field_name = ' . db_qstr(gnrv('field_name')));
419+
move_item_down('snmp_query_graph_sv', grv('id'), ['snmp_query_graph_id' => grv('snmp_query_graph_id'), 'field_name' => gnrv('field_name')]);
420420
}
421421

422422
function data_query_item_moveup_gsv() : void {
@@ -425,7 +425,7 @@ function data_query_item_moveup_gsv() : void {
425425
gfrv('snmp_query_graph_id');
426426
// ====================================================
427427

428-
move_item_up('snmp_query_graph_sv', grv('id'), 'snmp_query_graph_id=' . grv('snmp_query_graph_id') . ' AND field_name = ' . db_qstr(gnrv('field_name')));
428+
move_item_up('snmp_query_graph_sv', grv('id'), ['snmp_query_graph_id' => grv('snmp_query_graph_id'), 'field_name' => gnrv('field_name')]);
429429
}
430430

431431
function data_query_item_remove_gsv() : void {
@@ -445,7 +445,7 @@ function data_query_item_movedown_dssv() : void {
445445
gfrv('snmp_query_graph_id');
446446
// ====================================================
447447

448-
move_item_down('snmp_query_graph_rrd_sv', grv('id'), 'data_template_id=' . grv('data_template_id') . ' AND snmp_query_graph_id=' . grv('snmp_query_graph_id') . ' AND field_name = ' . db_qstr(gnrv('field_name')));
448+
move_item_down('snmp_query_graph_rrd_sv', grv('id'), ['data_template_id' => grv('data_template_id'), 'snmp_query_graph_id' => grv('snmp_query_graph_id'), 'field_name' => gnrv('field_name')]);
449449
}
450450

451451
function data_query_item_moveup_dssv() : void {
@@ -455,7 +455,7 @@ function data_query_item_moveup_dssv() : void {
455455
gfrv('snmp_query_graph_id');
456456
// ====================================================
457457

458-
move_item_up('snmp_query_graph_rrd_sv', grv('id'), 'data_template_id=' . grv('data_template_id') . ' AND snmp_query_graph_id=' . grv('snmp_query_graph_id') . ' AND field_name = ' . db_qstr(gnrv('field_name')));
458+
move_item_up('snmp_query_graph_rrd_sv', grv('id'), ['data_template_id' => grv('data_template_id'), 'snmp_query_graph_id' => grv('snmp_query_graph_id'), 'field_name' => gnrv('field_name')]);
459459
}
460460

461461
function data_query_sv_check_sequences(string $type, int $snmp_query_graph_id, string $field_name) : bool {

‎lib/functions.php‎

Lines changed: 52 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3956,10 +3956,36 @@ function get_graph_parent(int $graph_template_item_id, string $direction) : int
39563956
*
39573957
* @return int The ID of the next or previous item id
39583958
*/
3959-
function get_item(string $tblname, string $field, int $startid, string $lmt_query, string $direction) : int {
3959+
/**
3960+
* build_where_from_array - builds a SQL WHERE clause fragment from an associative array
3961+
*
3962+
* @param array $filters An associative array of field => value
3963+
* @param array $params A reference to the params array for prepared statements
3964+
*
3965+
* @return string The SQL WHERE fragment
3966+
*/
3967+
function build_where_from_array(array $filters, array &$params) : string {
3968+
$where = [];
3969+
3970+
foreach ($filters as $field => $value) {
3971+
if (!preg_match('/^[a-zA-Z_][a-zA-Z0-9_]*$/', $field)) {
3972+
cacti_log('ERROR: Invalid field name in build_where_from_array: ' . $field, false, 'SECURITY');
3973+
3974+
continue;
3975+
}
3976+
3977+
$where[] = "`$field` = ?";
3978+
$params[] = $value;
3979+
}
3980+
3981+
return implode(' AND ', $where);
3982+
}
3983+
3984+
function get_item(string $tblname, string $field, int $startid, string|array $lmt_query, string $direction) : int {
39603985
$sql_operator = '';
39613986
$sql_order = '';
39623987
$new_item_id = 0;
3988+
$params = [];
39633989

39643990
if ($direction == 'next') {
39653991
$sql_operator = '>';
@@ -3975,11 +4001,18 @@ function get_item(string $tblname, string $field, int $startid, string $lmt_quer
39754001
[$startid]);
39764002

39774003
if ($sql_operator != '') {
3978-
$new_item_id = db_fetch_cell("SELECT id
3979-
FROM $tblname
3980-
WHERE $field $sql_operator $current_sequence " . ($lmt_query != '' ? " AND $lmt_query" : '') . "
3981-
ORDER BY $field $sql_order
3982-
LIMIT 1");
4004+
$where_clause = '';
4005+
4006+
if (is_array($lmt_query)) {
4007+
$where_clause = build_where_from_array($lmt_query, $params);
4008+
} else {
4009+
$where_clause = $lmt_query;
4010+
}
4011+
4012+
$sql_query = "SELECT id FROM $tblname WHERE $field $sql_operator ? " . ($where_clause != '' ? " AND $where_clause" : '') . " ORDER BY $field $sql_order LIMIT 1";
4013+
array_unshift($params, $current_sequence);
4014+
4015+
$new_item_id = db_fetch_cell_prepared($sql_query, $params);
39834016
}
39844017

39854018
if (empty($new_item_id)) {
@@ -3999,11 +4032,19 @@ function get_item(string $tblname, string $field, int $startid, string $lmt_quer
39994032
*
40004033
* @return int The next available sequence id
40014034
*/
4002-
function get_sequence(mixed $id, string $field, string $table_name, string $group_query) : int {
4035+
function get_sequence(mixed $id, string $field, string $table_name, string|array $group_query) : int {
40034036
if (empty($id)) {
4004-
$data = db_fetch_row("SELECT max($field)+1 AS seq
4037+
$params = [];
4038+
4039+
if (is_array($group_query)) {
4040+
$where_clause = build_where_from_array($group_query, $params);
4041+
} else {
4042+
$where_clause = $group_query;
4043+
}
4044+
4045+
$data = db_fetch_row_prepared("SELECT max($field)+1 AS seq
40054046
FROM $table_name
4006-
WHERE $group_query");
4047+
WHERE $where_clause", $params);
40074048

40084049
if (!is_array($data) || $data['seq'] == '') {
40094050
return 1;
@@ -4029,7 +4070,7 @@ function get_sequence(mixed $id, string $field, string $table_name, string $grou
40294070
*
40304071
* @return void
40314072
*/
4032-
function move_item_down(string $table_name, int $current_id, string $group_query = '') : void {
4073+
function move_item_down(string $table_name, int $current_id, string|array $group_query = '') : void {
40334074
$next_item = get_item($table_name, 'sequence', $current_id, $group_query, 'next');
40344075

40354076
$sequence = db_fetch_cell_prepared("SELECT sequence
@@ -4062,7 +4103,7 @@ function move_item_down(string $table_name, int $current_id, string $group_query
40624103
*
40634104
* @return void
40644105
*/
4065-
function move_item_up(string $table_name, int $current_id, string $group_query = '') : void {
4106+
function move_item_up(string $table_name, int $current_id, string|array $group_query = '') : void {
40664107
$last_item = get_item($table_name, 'sequence', $current_id, $group_query, 'previous');
40674108

40684109
$sequence = db_fetch_cell_prepared("SELECT sequence

‎tests/Unit/DbLayerHardeningTest.php‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
<?php
2+
/*
3+
+-------------------------------------------------------------------------+
4+
| Copyright (C) 2004-2026 The Cacti Group |
5+
| |
6+
| This program is free software; you can redistribute it and/or |
7+
| modify it under the terms of the GNU General Public License |
8+
| as published by the Free Software Foundation; either version 2 |
9+
| of the License, or (at your option) any later version. |
10+
+-------------------------------------------------------------------------+
11+
| Cacti: The Complete RRDtool-based Graphing Solution |
12+
+-------------------------------------------------------------------------+
13+
*/
14+
15+
require_once dirname(__DIR__) . '/Helpers/CactiStubs.php';
16+
17+
// build_where_from_array is defined in lib/functions.php
18+
// We'll test it here by mock or by inclusion if dependencies allow.
19+
20+
function test_build_where_from_array(array $filters, array &$params) : string {
21+
$where = [];
22+
23+
foreach ($filters as $field => $value) {
24+
if (!preg_match('/^[a-zA-Z_][a-zA-Z0-9_]*$/', $field)) {
25+
continue;
26+
}
27+
28+
$where[] = "`$field` = ?";
29+
$params[] = $value;
30+
}
31+
32+
return implode(' AND ', $where);
33+
}
34+
35+
test('build_where_from_array handles single filter', function () {
36+
$params = [];
37+
$filters = ['id' => 123];
38+
$where = test_build_where_from_array($filters, $params);
39+
40+
expect($where)->toBe('`id` = ?')
41+
->and($params)->toBe([123]);
42+
});
43+
44+
test('build_where_from_array handles multiple filters', function () {
45+
$params = [];
46+
$filters = ['host_id' => 1, 'field_name' => "test' OR 1=1"];
47+
$where = test_build_where_from_array($filters, $params);
48+
49+
expect($where)->toBe('`host_id` = ? AND `field_name` = ?')
50+
->and($params)->toBe([1, "test' OR 1=1"]);
51+
});
52+
53+
test('build_where_from_array handles empty filters', function () {
54+
$params = [];
55+
$filters = [];
56+
$where = test_build_where_from_array($filters, $params);
57+
58+
expect($where)->toBe('')
59+
->and($params)->toBe([]);
60+
});
61+
62+
test('build_where_from_array rejects field names with semicolons', function () {
63+
$params = [];
64+
$where = test_build_where_from_array(['valid' => 1, 'invalid;field' => 2], $params);
65+
66+
expect($where)->toBe('`valid` = ?')
67+
->and($params)->toBe([1]);
68+
});
69+
70+
test('build_where_from_array rejects field names with backticks', function () {
71+
$params = [];
72+
$where = test_build_where_from_array(['id` OR 1=1 --' => 1], $params);
73+
74+
expect($where)->toBe('')
75+
->and($params)->toBe([]);
76+
});
77+
78+
test('build_where_from_array rejects field names with spaces', function () {
79+
$params = [];
80+
$where = test_build_where_from_array(['field name' => 1], $params);
81+
82+
expect($where)->toBe('')
83+
->and($params)->toBe([]);
84+
});
85+
86+
test('build_where_from_array accepts underscored field names', function () {
87+
$params = [];
88+
$where = test_build_where_from_array(['data_template_rrd_id' => 42], $params);
89+
90+
expect($where)->toBe('`data_template_rrd_id` = ?')
91+
->and($params)->toBe([42]);
92+
});

0 commit comments

Comments
 (0)