Skip to content

Commit 9b3441c

Browse files
Thomas Vincentoz-agent
authored andcommitted
security(mactrack): remediate SQL injection in non-view files
mactrack_macauth.php: api_mactrack_maca_save() interpolated the user-supplied MAC address into an UPDATE ... LIKE "...%" (first-order SQLi); now bound via db_execute_prepared() with a ? placeholder. api_mactrack_maca_remove() called db_execute_prepared() while concatenating the (stored) mac_address into the SQL with no bound params (second-order SQLi + misuse of the prepared helper); the three DELETEs now bind mac_address as a parameter. mactrack_devices.php: Copy SNMP Settings (drp_action==7) built its UPDATE SET clause by interpolating host-table values with bare quotes; values are now bound with placeholders (column name stays interpolated as a trusted static-array key). Mirrors the fix already applied on the Cacti#334 branch. mactrack_device_types.php, mactrack_sites.php, mactrack_snmp.php: (int)-cast the remaining raw numeric request vars (type_id, site_id, and id used in snmp_id= fragments for move_item_up/down and get_sequence) — defense-in-depth over the FILTER_VALIDATE_INT request validation. 1.2.31-idiomatic, PHP 7.4-safe. Co-Authored-By: Oz <oz-agent@warp.dev>
1 parent 88e46a8 commit 9b3441c

5 files changed

Lines changed: 27 additions & 16 deletions

File tree

mactrack_device_types.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -915,7 +915,7 @@ function mactrack_get_device_types(&$sql_where, $rows, $apply_limits = true) {
915915
if (get_request_var('type_id') == '-1') {
916916
// Show all items
917917
} else {
918-
$sql_where .= ($sql_where != '' ? ' AND ' : ' WHERE ') . '(mtdt.device_type=' . get_request_var('type_id') . ')';
918+
$sql_where .= ($sql_where != '' ? ' AND ' : ' WHERE ') . '(mtdt.device_type=' . (int) get_request_var('type_id') . ')';
919919
}
920920

921921
if (get_request_var('enabled') == '-1') {

mactrack_devices.php

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -202,19 +202,26 @@ function form_mactrack_actions() {
202202

203203
if (isset($cacti_host['id'])) {
204204
reset($fields_mactrack_snmp_item);
205-
$updates = '';
205+
$set_clauses = [];
206+
$set_params = [];
206207

207208
foreach ($fields_mactrack_snmp_item as $field_name => $field_array) {
208209
if (isset($cacti_host[$field_name])) {
209-
$updates .= ($updates != '' ? ', ' : '') . $field_name . "='" . $cacti_host[$field_name] . "'";
210+
// $field_name is a trusted key from the static $fields_mactrack_snmp_item
211+
// definition; the host-table value is bound as a parameter to avoid
212+
// second-order SQL injection when it contains a quote.
213+
$set_clauses[] = $field_name . ' = ?';
214+
$set_params[] = $cacti_host[$field_name];
210215
}
211216
}
212217

213-
if ($updates != '') {
218+
if (cacti_sizeof($set_clauses)) {
219+
$set_params[] = $selected_items[$i];
220+
214221
db_execute_prepared('UPDATE mac_track_devices
215-
SET ' . $updates . '
216-
WHERE device_id=?',
217-
[$selected_items[$i]]);
222+
SET ' . implode(', ', $set_clauses) . '
223+
WHERE device_id = ?',
224+
$set_params);
218225
}
219226
} else {
220227
// skip silently; possible enhancement: tell the user what we did

mactrack_macauth.php

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -174,9 +174,10 @@ function api_mactrack_maca_save($mac_id, $mac_address, $description) {
174174
$mac_id = sql_save($save, 'mac_track_macauth', 'mac_address', false);
175175

176176
if ($mac_id) {
177-
db_execute('UPDATE mac_track_ports
177+
db_execute_prepared('UPDATE mac_track_ports
178178
SET authorized=1
179-
WHERE mac_address LIKE "' . $mac_address . '%"');
179+
WHERE mac_address LIKE ?',
180+
[$mac_address . '%']);
180181

181182
raise_message(1);
182183
} else {
@@ -198,13 +199,16 @@ function api_mactrack_maca_remove($mac_id) {
198199
[$mac_id]);
199200

200201
db_execute_prepared('DELETE FROM mac_track_ips
201-
WHERE mac_address="' . $mac_address . '"');
202+
WHERE mac_address = ?',
203+
[$mac_address]);
202204

203205
db_execute_prepared('DELETE FROM mac_track_ports
204-
WHERE mac_address="' . $mac_address . '"');
206+
WHERE mac_address = ?',
207+
[$mac_address]);
205208

206209
db_execute_prepared('DELETE FROM mac_track_aggregated_ports
207-
WHERE mac_address="' . $mac_address . '"');
210+
WHERE mac_address = ?',
211+
[$mac_address]);
208212

209213
cacti_log('AUDIT: MAC Address `' . $mac_address . '` is deleted from MacAuth by ' .
210214
db_fetch_cell_prepared('SELECT full_name FROM user_auth WHERE id = ?', [$_SESSION['sess_user_id']]), false, 'MACTRACK');

mactrack_sites.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ function mactrack_site_get_site_records(&$sql_where, $rows, $apply_limits = true
304304
}
305305

306306
if ((get_request_var('site_id') != '-1') && (get_request_var('detail'))) {
307-
$sql_where .= ($sql_where != '' ? ' AND ' : 'WHERE ') . '(mtd.site_id=' . get_request_var('site_id') . ')';
307+
$sql_where .= ($sql_where != '' ? ' AND ' : 'WHERE ') . '(mtd.site_id=' . (int) get_request_var('site_id') . ')';
308308
}
309309

310310
$sql_order = get_order_string();

mactrack_snmp.php

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -297,7 +297,7 @@ function mactrack_snmp_item_movedown() {
297297
get_filter_request_var('id');
298298
// ====================================================
299299

300-
move_item_down('mac_track_snmp_items', get_request_var('item_id'), 'snmp_id=' . get_request_var('id'));
300+
move_item_down('mac_track_snmp_items', get_request_var('item_id'), 'snmp_id=' . (int) get_request_var('id'));
301301
}
302302

303303
function mactrack_snmp_item_moveup() {
@@ -306,7 +306,7 @@ function mactrack_snmp_item_moveup() {
306306
get_filter_request_var('id');
307307
// ====================================================
308308

309-
move_item_up('mac_track_snmp_items', get_request_var('item_id'), 'snmp_id=' . get_request_var('id'));
309+
move_item_up('mac_track_snmp_items', get_request_var('item_id'), 'snmp_id=' . (int) get_request_var('id'));
310310
}
311311

312312
function mactrack_snmp_item_remove() {
@@ -347,7 +347,7 @@ function mactrack_snmp_item_edit() {
347347

348348
$mactrack_snmp_item = [];
349349
$mactrack_snmp_item['snmp_id'] = get_request_var('id');
350-
$mactrack_snmp_item['sequence'] = get_sequence('', 'sequence', 'mac_track_snmp_items', 'snmp_id=' . get_request_var('id'));
350+
$mactrack_snmp_item['sequence'] = get_sequence('', 'sequence', 'mac_track_snmp_items', 'snmp_id=' . (int) get_request_var('id'));
351351
}
352352

353353
form_start(get_current_page(), 'mactrack_item_edit');

0 commit comments

Comments
 (0)