From eb77d251ebd93534b3ecc1529520afeac7817327 Mon Sep 17 00:00:00 2001 From: Sean Mancini Date: Fri, 24 Jul 2026 08:46:21 -0400 Subject: [PATCH 01/10] code review sweep security: harden audit logging and management - prevent stored XSS in audit views - require CSRF-protected POST and management permission for purging - recursively redact web and CLI credentials - generate safe, standards-compliant CSV exports - record hook events explicitly as attempted actions - harden retention, replication, and external logging - add security regression checks - bump plugin version to 1.3 --- .github/workflows/plugin-ci-workflow.yml | 8 +- CHANGELOG.md | 6 + INFO | 2 +- README.md | 4 + audit.php | 391 ++++++++++++----------- audit_functions.php | 153 ++++++--- js/functions.js | 31 +- review.md | 318 ++++++++++++++++++ setup.php | 58 ++-- tests/controller_security_test.php | 32 ++ tests/security_functions_test.php | 52 +++ 11 files changed, 788 insertions(+), 267 deletions(-) create mode 100644 review.md create mode 100644 tests/controller_security_test.php create mode 100644 tests/security_functions_test.php diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index 87991cc..5c85de3 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -180,6 +180,12 @@ jobs: echo "Syntax errors found!" exit 1 fi + + - name: Run Audit Security Helper Tests + run: | + cd ${{ github.workspace }}/cacti/plugins/audit + php tests/security_functions_test.php + php tests/controller_security_test.php - name: Run Cacti Poller @@ -221,5 +227,3 @@ jobs: echo "No audit log entries found!" exit 1 fi - - diff --git a/CHANGELOG.md b/CHANGELOG.md index 69a51c0..e117d79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ --- develop --- +* security: Escape stored audit data before rendering to prevent stored XSS +* security: Require an authorized, CSRF-protected POST to purge the audit log +* security: Recursively redact sensitive web and CLI values +* security: Generate standards-compliant, spreadsheet-safe CSV exports +* feature: Mark hook-time records explicitly as attempted actions +* issue: Harden external file logging, retention, malformed records, and replication * issue#38: Graph Template table does not exist * issue: If the audit log does not exist or is not set, set it and create it * issue: Audit assumes that all selected_items are numeric resulting in fatal error diff --git a/INFO b/INFO index 31298db..7fda835 100644 --- a/INFO +++ b/INFO @@ -21,7 +21,7 @@ [info] name = audit -version = 1.2 +version = 1.3 longname = Audit Plugin for Cacti author = The Cacti Group email = diff --git a/README.md b/README.md index 064d75d..95c2bfb 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,10 @@ This plugin allows Cacti Administrators to log user change activity * Captures CLI commands and who ran them +Audit entries generated by the `config_insert` hook record requested actions as +`attempted`. They do not prove that the page-specific operation completed +successfully. + ## Installation Install just like any other plugin, just throw it in the plugin directory, and diff --git a/audit.php b/audit.php index dffed41..9f607ef 100644 --- a/audit.php +++ b/audit.php @@ -33,6 +33,17 @@ break; case 'purge': + if (!isset($_SERVER['REQUEST_METHOD']) || $_SERVER['REQUEST_METHOD'] !== 'POST') { + http_response_code(405); + header('Allow: POST'); + exit; + } + + if (!api_plugin_user_realm_auth('audit_manage.php') || !csrf_check(false)) { + http_response_code(403); + exit; + } + audit_purge(); top_header(); @@ -40,101 +51,99 @@ bottom_footer(); break; - case 'getdata': - $data = db_fetch_row_prepared('SELECT * +case 'getdata': + $data = db_fetch_row_prepared('SELECT * FROM audit_log WHERE id = ?', - array(get_filter_request_var('id'))); - - $output = ''; - - if ($data['action'] == 'cli') { - $width = 'wide'; - $output .= ''; - } - } else { - $output .= '
'; - $output .= '' . __('Page:', 'audit') . ' ' . $data['page'] . ''; - $output .= '
' . __('User:', 'audit') . ' ' . $data['user_agent'] . ''; - $output .= '
' . __('IP Address:', 'audit') . ' ' . $data['ip_address'] . ''; - $output .= '
' . __('Date:', 'audit') . ' ' . $data['event_time'] . ''; - $output .= '
' . __('Action:', 'audit') . ' ' . $data['action'] . ''; - $output .= '
'; - $output .= '' . __('Script:', 'audit') . ' ' . $data['post'] . ''; - } elseif (cacti_sizeof($data)) { - $attribs = json_decode($data['post']); - - $nattribs = array(); - foreach($attribs as $field => $content) { - $nattribs[$field] = $content; - } - ksort($nattribs); + array(get_filter_request_var('id'))); - if (cacti_sizeof($nattribs) > 16) { - $width = 'wide'; - } else { - $width = 'narrow'; - } + if (!cacti_sizeof($data)) { + http_response_code(404); + print html_escape(__('Audit event not found.', 'audit')); + break; + } - $output .= ''; - $output .= ''; +function audit_render_event_details($data) { + $width = 'wide'; + $output = '
'; - $output .= '' . __('Page:', 'audit') . ' ' . $data['page'] . ''; - $output .= '
' . __('User:', 'audit') . ' ' . get_username($data['user_id']) . ''; - $output .= '
' . __('IP Address:', 'audit') . ' ' . $data['ip_address'] . ''; - $output .= '
' . __('Date:', 'audit') . ' ' . $data['event_time'] . ''; - $output .= '
' . __('Action:', 'audit') . ' ' . $data['action'] . ''; - $output .= '
'; - $output .= ''; - - if (cacti_sizeof($nattribs) > 16) { - $columns = 2; - $output .= ''; - } else { - $columns = 1; - $output .= ''; - } + $output = audit_render_event_details($data); + echo $output; - $i = 0; - if (cacti_sizeof($nattribs)) { - foreach($nattribs as $field => $content) { - if ($i % $columns == 0) { - $output .= ($output != '' ? '':'') . ''; - } - - if (is_array($content)) { - $output .= '' . implode(',', $content) . ''; - } else { - $output .= ''; - } - - $i++; - } - - if ($i % $columns > 0) { - $output . ''; - } - } + break; +default: + top_header(); + audit_log(); + bottom_footer(); +} - // Display the Record Data under selected_items if it is not empty - $recordData = json_decode($data['object_data']); - if (!empty($recordData)) { - $output .= '
' . __('Attrib', 'audit') . '' . __('Value', 'audit') . '' . __('Attrib', 'audit') . '' . __('Value', 'audit') . '
' . __('Attrib', 'audit') . '' . __('Value', 'audit') . '
' . $field . '' . $field . '' . $content . '
'; - $output .= '

' . __('Record Data:', 'audit') . '
'; + $output .= '' . __('Page:', 'audit') . ' ' . html_escape($data['page']) . ''; + $output .= '
' . __('User:', 'audit') . ' ' . html_escape($data['action'] == 'cli' ? $data['user_agent'] : get_username($data['user_id'])) . ''; + $output .= '
' . __('IP Address:', 'audit') . ' ' . html_escape($data['ip_address']) . ''; + $output .= '
' . __('Date:', 'audit') . ' ' . html_escape($data['event_time']) . ''; + $output .= '
' . __('Action:', 'audit') . ' ' . html_escape($data['action']) . ''; + $output .= '
' . __('Outcome:', 'audit') . ' ' . html_escape($data['outcome']) . ''; + $output .= '
'; + + if ($data['action'] == 'cli') { + $output .= '' . __('Script:', 'audit') . ' ' . html_escape($data['post']) . ''; + return $output . '
'; + } - foreach ($recordData as $record) { - $output .= '
' . json_encode($record, JSON_PRETTY_PRINT) . '
'; - } + $attribs = json_decode($data['post'], true); + $attribs = is_array($attribs) ? $attribs : array( + __('Stored Data', 'audit') => __('The stored request data is not valid JSON.', 'audit') + ); + ksort($attribs); + + $columns = cacti_sizeof($attribs) > 16 ? 2 : 1; + $output .= ''; + $output .= $columns == 2 + ? '' + : ''; + + $i = 0; + foreach ($attribs as $field => $content) { + if ($i % $columns == 0) { + $output .= ''; } - // Output the final result - echo $output; + $output .= ''; + $i++; + + if ($i % $columns == 0) { + $output .= ''; + } + } + if ($i % $columns > 0) { + $output .= ''; + } - break; -default: - top_header(); - audit_log(); - bottom_footer(); + $record_data = json_decode($data['object_data'], true); + if (is_array($record_data) && !empty($record_data)) { + $output .= ''; + $output .= ''; + + foreach ($record_data as $record) { + $output .= ''; + } + } + + return $output . '
' . __('Attrib', 'audit') . '' . __('Value', 'audit') . '' . __('Attrib', 'audit') . '' . __('Value', 'audit') . '
' . __('Attrib', 'audit') . '' . __('Value', 'audit') . '
' . html_escape($field) . '' . audit_render_value($content) . '

' . __('Record Data:', 'audit') . '
' . html_escape(json_encode($record, JSON_PRETTY_PRINT | JSON_INVALID_UTF8_SUBSTITUTE)) . '
'; +} + +function audit_render_value($value) { + if (is_array($value) || is_object($value)) { + return '
' . html_escape(json_encode($value, JSON_PRETTY_PRINT | JSON_INVALID_UTF8_SUBSTITUTE)) . '
'; + } + + if (is_bool($value)) { + $value = $value ? 'true' : 'false'; + } elseif ($value === null) { + $value = 'null'; + } + + return html_escape((string) $value); } function audit_purge() { @@ -148,67 +157,71 @@ function audit_purge() { } function audit_export_rows() { - process_request_vars(); + audit_process_request_vars(); /* form the 'where' clause for our main sql query */ + $sql_clauses = array(); + $sql_params = array(); + if (get_request_var('filter') != '') { - $sql_where = 'WHERE ( - page LIKE ' . db_qstr('%' . get_request_var('filter') . '%') . ' - OR post LIKE ' . db_qstr('%' . get_request_var('filter') . '%') . ')'; - } else { - $sql_where = ''; + $sql_clauses[] = '(page LIKE ? OR post LIKE ?)'; + $sql_params[] = '%' . get_request_var('filter') . '%'; + $sql_params[] = '%' . get_request_var('filter') . '%'; } if (get_request_var('event_page') != '-1') { - $sql_where .= ($sql_where != '' ? ' AND ':'WHERE ') . ' page = ' . db_qstr(get_request_var('event_page')); + $sql_clauses[] = 'page = ?'; + $sql_params[] = get_request_var('event_page'); } if (!isempty_request_var('user_id') && get_request_var('user_id') > '-1') { - $sql_where .= ($sql_where != '' ? ' AND ':'WHERE ') . ' user_id = ' . get_request_var('user_id'); + $sql_clauses[] = 'user_id = ?'; + $sql_params[] = get_request_var('user_id'); } - $events = db_fetch_assoc("SELECT audit_log.*, user_auth.username - FROM audit_log - LEFT JOIN user_auth - ON audit_log.user_id=user_auth.id - $sql_where"); + $sql_where = cacti_sizeof($sql_clauses) ? 'WHERE ' . implode(' AND ', $sql_clauses) : ''; + + $events = db_fetch_assoc_prepared("SELECT audit_log.*, user_auth.username + FROM audit_log + LEFT JOIN user_auth + ON audit_log.user_id=user_auth.id + $sql_where", + $sql_params); if (cacti_sizeof($events)) { header('Content-Disposition: attachment; filename=audit_export.csv'); + header('Content-Type: text/csv; charset=UTF-8'); + header('X-Content-Type-Options: nosniff'); - print __x('Column Header used for CSV log export. Ensure that you do NOT(!) remove one of the commas. The output needs to be CSV compliant.','page, user_id, username, action, ip_address, user_agent, event_time, post', 'audit') . "\n"; + $output = fopen('php://output', 'w'); + fputcsv($output, array('page', 'user_id', 'username', 'action', 'outcome', 'ip_address', 'user_agent', 'event_time', 'post')); foreach($events as $event) { - $post = json_decode($event['post']); - $poster = ''; - foreach($post as $var => $value) { - if (is_array($value)) { - $poster .= ($poster != '' ? '|':'') . $var . ':' . implode('%', $value); - } else { - $poster .= ($poster != '' ? '|':'') . $var . ':' . $value; - } + if ($event['action'] == 'cli') { + $poster = $event['post']; + } else { + $post = json_decode($event['post'], true); + $poster = is_array($post) ? json_encode($post, JSON_INVALID_UTF8_SUBSTITUTE) : $event['post']; } - print - $event['page'] . ', ' . - $event['user_id'] . ', ' . - get_username($event['user_id']) . ', ' . - $event['action'] . ', ' . - $event['ip_address'] . ', ' . - $event['user_agent'] . ', ' . - $event['event_time'] . ', ' . - $poster . "\n"; + fputcsv($output, array_map('audit_csv_safe_cell', array( + $event['page'], + $event['user_id'], + get_username($event['user_id']), + $event['action'], + $event['outcome'], + $event['ip_address'], + $event['user_agent'], + $event['event_time'], + $poster + ))); } - } -} -function audit_csv_escape($string) { - $string = str_replace('"', '', $string); - $string = str_replace(',', '|', $string); - return $string; + fclose($output); + } } -function process_request_vars() { +function audit_process_request_vars() { /* ================= input validation and session storage ================= */ $filters = array( 'rows' => array( @@ -255,7 +268,7 @@ function process_request_vars() { function audit_log() { global $item_rows; - process_request_vars(); + audit_process_request_vars(); if (get_request_var('rows') == '-1') { $rows = read_config_option('num_rows_table'); @@ -284,10 +297,10 @@ function audit_log() { + @@ -319,7 +333,7 @@ function audit_log() { $value) { - print "\n"; + print "\n"; } } ?> @@ -330,13 +344,14 @@ function audit_log() { - + + + - - - - '> + + + '> @@ -345,43 +360,50 @@ function audit_log() { html_end_box(); /* form the 'where' clause for our main sql query */ + $sql_clauses = array(); + $sql_params = array(); + if (get_request_var('filter') != '') { - $sql_where = 'WHERE ( - page LIKE ' . db_qstr('%' . get_request_var('filter') . '%') . ' - OR post LIKE ' . db_qstr('%' . get_request_var('filter') . '%') . ')'; - } else { - $sql_where = ''; + $sql_clauses[] = '(page LIKE ? OR post LIKE ?)'; + $sql_params[] = '%' . get_request_var('filter') . '%'; + $sql_params[] = '%' . get_request_var('filter') . '%'; } if (get_request_var('event_page') != '-1') { - $sql_where .= ($sql_where != '' ? ' AND ':'WHERE ') . ' page = ' . db_qstr(get_request_var('event_page')); + $sql_clauses[] = 'page = ?'; + $sql_params[] = get_request_var('event_page'); } if (!isempty_request_var('user_id') && get_request_var('user_id') > '-1') { - $sql_where .= ($sql_where != '' ? ' AND ':'WHERE ') . ' user_id = ' . get_request_var('user_id'); + $sql_clauses[] = 'user_id = ?'; + $sql_params[] = get_request_var('user_id'); } - $total_rows = db_fetch_cell("SELECT - COUNT(*) - FROM audit_log - LEFT JOIN user_auth - ON audit_log.user_id=user_auth.id - $sql_where"); + $sql_where = cacti_sizeof($sql_clauses) ? 'WHERE ' . implode(' AND ', $sql_clauses) : ''; + + $total_rows = db_fetch_cell_prepared("SELECT + COUNT(*) + FROM audit_log + LEFT JOIN user_auth + ON audit_log.user_id=user_auth.id + $sql_where", + $sql_params); $sql_order = get_order_string(); $sql_limit = ' LIMIT ' . ($rows*(get_request_var('page')-1)) . ',' . $rows; - $events = db_fetch_assoc("SELECT audit_log.*, user_auth.username - FROM audit_log - LEFT JOIN user_auth - ON audit_log.user_id=user_auth.id - $sql_where - $sql_order - $sql_limit"); + $events = db_fetch_assoc_prepared("SELECT audit_log.*, user_auth.username + FROM audit_log + LEFT JOIN user_auth + ON audit_log.user_id=user_auth.id + $sql_where + $sql_order + $sql_limit", + $sql_params); - $nav = html_nav_bar('audit.php?filter=' . get_request_var('filter'), MAX_DISPLAY_PAGES, get_request_var('page'), $rows, $total_rows, 5, __('Audit Events', 'audit'), 'page', 'main'); + $nav = html_nav_bar('audit.php?filter=' . rawurlencode(get_request_var('filter')), MAX_DISPLAY_PAGES, get_request_var('page'), $rows, $total_rows, 5, __('Audit Events', 'audit'), 'page', 'main'); - print $nav; + print $nav; html_start_box('', '100%', '', '3', 'center', ''); @@ -398,11 +420,17 @@ function audit_log() { 'sort' => 'ASC', 'tip' => __('The user who generated the event.', 'audit') ), - 'action' => array( - 'display' => __('Action', 'audit'), - 'align' => 'left', - 'sort' => 'ASC', - 'tip' => __('The Cacti Action requested. Hover over action to see $_POST data.', 'audit') + 'action' => array( + 'display' => __('Requested Action', 'audit'), + 'align' => 'left', + 'sort' => 'ASC', + 'tip' => __('The requested Cacti action. Hover over the action to see request data.', 'audit') + ), + 'outcome' => array( + 'display' => __('Outcome', 'audit'), + 'align' => 'left', + 'sort' => 'ASC', + 'tip' => __('Whether this event records an attempted or confirmed action.', 'audit') ), 'user_agent' => array( 'display' => __('User Agent', 'audit'), @@ -428,29 +456,31 @@ function audit_log() { $i = 0; if (cacti_sizeof($events)) { - foreach ($events as $e) { - if ($e['action'] == 'cli') { - form_alternate_row('line' . $e['id'], false); - form_selectable_cell($e['page'], $e['id']); - form_selectable_cell($e['user_agent'], $e['id']); - form_selectable_cell('' . ucfirst($e['action']) . '', $e['id']); - form_selectable_cell(__('N/A', 'audit'), $e['id']); - form_selectable_cell($e['ip_address'], $e['id'], '', 'right'); - form_selectable_cell($e['event_time'], $e['id'], '', 'right'); - form_end_row(); - } else { - form_alternate_row('line' . $e['id'], false); - form_selectable_cell(filter_value($e['page'], get_request_var('filter')), $e['id']); - form_selectable_cell($e['username'], $e['id']); - form_selectable_cell('' . ucfirst($e['action']) . '', $e['id']); - form_selectable_cell($e['user_agent'], $e['id']); - form_selectable_cell($e['ip_address'], $e['id'], '', 'right'); - form_selectable_cell($e['event_time'], $e['id'], '', 'right'); + foreach ($events as $e) { + if ($e['action'] == 'cli') { + form_alternate_row('line' . $e['id'], false); + form_selectable_ecell($e['page'], $e['id']); + form_selectable_ecell($e['user_agent'], $e['id']); + form_selectable_cell('' . html_escape(ucfirst($e['action'])) . '', $e['id']); + form_selectable_ecell($e['outcome'], $e['id']); + form_selectable_cell(__('N/A', 'audit'), $e['id']); + form_selectable_ecell($e['ip_address'], $e['id'], '', 'right'); + form_selectable_ecell($e['event_time'], $e['id'], '', 'right'); + form_end_row(); + } else { + form_alternate_row('line' . $e['id'], false); + form_selectable_cell(filter_value($e['page'], get_request_var('filter')), $e['id']); + form_selectable_ecell($e['username'], $e['id']); + form_selectable_cell('' . html_escape(ucfirst($e['action'])) . '', $e['id']); + form_selectable_ecell($e['outcome'], $e['id']); + form_selectable_ecell($e['user_agent'], $e['id']); + form_selectable_ecell($e['ip_address'], $e['id'], '', 'right'); + form_selectable_ecell($e['event_time'], $e['id'], '', 'right'); form_end_row(); } } } else { - print "" . __('No Audit Log Events Found', 'audit') . "\n"; + print "" . __('No Audit Log Events Found', 'audit') . "\n"; } html_end_box(false); @@ -463,4 +493,3 @@ function audit_log() { $value) { + if (audit_is_sensitive_key($key)) { + $redacted[$key] = '[REDACTED]'; + } elseif (is_array($value)) { + $redacted[$key] = audit_redact_sensitive_data($value); + } else { + $redacted[$key] = $value; + } + } + + return $redacted; +} + +function audit_redact_cli_arguments($arguments) { + $redacted = array(); + $redact_next = false; + + foreach ($arguments as $argument) { + if ($redact_next) { + $redacted[] = '[REDACTED]'; + $redact_next = false; + continue; + } + + if (preg_match('/^(--?[^=]*(?:pass(?:word)?|phrase|token|secret|api[_-]?key|private[_-]?key|community|credential|authorization|authentication)[^=]*)=(.*)$/i', $argument, $matches)) { + $redacted[] = $matches[1] . '=[REDACTED]'; + continue; + } + + if (preg_match('/^--?[^=]*(?:pass(?:word)?|phrase|token|secret|api[_-]?key|private[_-]?key|community|credential|authorization|authentication)/i', $argument)) { + $redacted[] = $argument; + $redact_next = true; + continue; + } + + $redacted[] = preg_replace('#^([a-z][a-z0-9+.-]*://[^:/@\s]+):[^@\s]+@#i', '$1:[REDACTED]@', $argument); + } + + return $redacted; +} + +function audit_json_encode($data) { + $json = json_encode($data, JSON_INVALID_UTF8_SUBSTITUTE); + + if ($json === false) { + return json_encode(array('audit_encoding_error' => json_last_error_msg())); + } + + return $json; +} + +function audit_csv_safe_cell($value) { + $value = (string) $value; + + if (preg_match('/^[=+\-@]/', ltrim($value))) { + return "'" . $value; + } + + return $value; } @@ -128,16 +200,13 @@ function audit_config_insert() { if (audit_log_valid_event()) { /* prepare post */ - $post = $_REQUEST; + $post = filter_input_array(INPUT_POST, FILTER_UNSAFE_RAW); + $post = is_array($post) ? $post : array(); /* remove unsafe variables */ unset($post['__csrf_magic']); unset($post['header']); - foreach ($post as $key => $value) { - if (preg_match('/pass|phrase/i', $key)) { - unset($post[$key]); - } - } + $post = audit_redact_sensitive_data($post); /* check if drp_action is present and update action accordingly */ if (isset($post['drp_action']) && $post['drp_action'] == 1) { @@ -147,15 +216,16 @@ function audit_config_insert() { } /* sanitize and serialize selected items */ - if (isset($post['selected_items'])) { - $selected_items = unserialize(stripslashes($post['selected_items']), array('allowed_classes' => false)); - $drop_action = $post['drp_action']; + if (isset($post['selected_items']) && is_string($post['selected_items'])) { + $selected_items = @unserialize(stripslashes($post['selected_items']), array('allowed_classes' => false)); + $selected_items = is_array($selected_items) ? $selected_items : array(); + $drop_action = $post['drp_action'] ?? false; } else { $selected_items = array(); $drop_action = false; } - $post = json_encode($post); + $post = audit_json_encode($post); $page = basename($_SERVER['SCRIPT_NAME']); $user_id = (isset($_SESSION['sess_user_id']) ? $_SESSION['sess_user_id'] : 0); $event_time = date('Y-m-d H:i:s'); @@ -164,7 +234,7 @@ function audit_config_insert() { $ip_address = get_client_addr(); /* Get the User Agent */ - $user_agent = $_SERVER['HTTP_USER_AGENT']; + $user_agent = $_SERVER['HTTP_USER_AGENT'] ?? ''; if (empty($action) && isset_request_var('action')) { $action = get_nfilter_request_var('action'); @@ -207,29 +277,36 @@ function audit_config_insert() { $base = CACTI_PATH_BASE; } - db_execute_prepared('INSERT INTO audit_log (page, user_id, action, ip_address, user_agent, event_time, post, object_data) - VALUES (?, ?, ?, ?, ?, ?, ?, ?)', - array($page, $user_id, $action, $ip_address, $user_agent, $event_time, $post, $object_data)); + db_execute_prepared('INSERT INTO audit_log (page, user_id, action, outcome, ip_address, user_agent, event_time, post, object_data) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)', + array($page, $user_id, $action, 'attempted', $ip_address, $user_agent, $event_time, $post, $object_data)); + + $external_logging = read_config_option('audit_log_external') == 'on'; - if ($audit_log == '') { + if ($external_logging && $audit_log == '') { set_config_option('audit_log_external_path', $base . '/log/audit.log'); $audit_log = $base . '/log/audit.log'; } - if ($audit_log != '' && !file_exists($audit_log)) { + if ($external_logging && $audit_log != '' && !file_exists($audit_log)) { if (is_writable(dirname($audit_log))) { cacti_log(sprintf('NOTE: The Audit Log file \'%s\' does not exist. Creating it.', $audit_log), false, 'AUDIT'); - touch($audit_log); + if (!touch($audit_log)) { + cacti_log(sprintf('ERROR: Unable to create Audit Log file \'%s\'.', $audit_log), false, 'AUDIT'); + } else { + @chmod($audit_log, 0600); + } } else { cacti_log(sprintf('ERROR: Audit Log file path \'%s\' does not exist and the path is not writeable.', $audit_log), false, 'AUDIT'); } } - if (read_config_option('audit_log_external') == 'on' && $audit_log != '' && file_exists($audit_log)) { + if ($external_logging && $audit_log != '' && is_file($audit_log) && !is_link($audit_log)) { $log_data = array( 'page' => $page, 'user_id' => $user_id, 'action' => $action, + 'outcome' => 'attempted', 'ip_address' => $ip_address, 'user_agent' => $user_agent, 'event_time' => $event_time, @@ -237,33 +314,35 @@ function audit_config_insert() { 'object_data' => $object_data ); - $log_msg = json_encode($log_data) . "\n"; - $file = fopen($audit_log, 'a'); + $log_msg = audit_json_encode($log_data) . "\n"; + $written = file_put_contents($audit_log, $log_msg, FILE_APPEND | LOCK_EX); - if ($file) { - fwrite($file, $log_msg); - fclose($file); + if ($written !== strlen($log_msg)) { + cacti_log(sprintf('ERROR: Unable to append a complete record to Audit Log file \'%s\'.', $audit_log), false, 'AUDIT'); } + } elseif ($external_logging && $audit_log != '') { + cacti_log(sprintf('ERROR: Audit Log file \'%s\' is not a regular file or is a symbolic link.', $audit_log), false, 'AUDIT'); } - } elseif (isset($_SERVER['argv'])) { - $page = basename($_SERVER['argv'][0]); + } elseif (isset($_SERVER['argv']) && cacti_sizeof($_SERVER['argv'])) { + $arguments = audit_redact_cli_arguments($_SERVER['argv']); + $page = basename($arguments[0]); $user_id = 0; $action = 'cli'; $ip_address = getHostByName(php_uname('n')); $user_agent = get_current_user(); $event_time = date('Y-m-d H:i:s'); - $post = implode(' ', $_SERVER['argv']); + $post = implode(' ', $arguments); /* don't insert poller records */ - if (strpos($_SERVER['argv'][0], 'poller') === false && - strpos($_SERVER['argv'][0], 'cmd.php') === false && - strpos($_SERVER['argv'][0], '/scripts/') === false && - strpos($_SERVER['argv'][0], 'script_server.php') === false && - strpos($_SERVER['argv'][0], '_process.php') === false) { - - db_execute_prepared('INSERT INTO audit_log (page, user_id, action, ip_address, user_agent, event_time, post) - VALUES (?, ?, ?, ?, ?, ?, ?)', - array($page, $user_id, $action, $ip_address, $user_agent, $event_time, $post)); + if (strpos($arguments[0], 'poller') === false && + strpos($arguments[0], 'cmd.php') === false && + strpos($arguments[0], '/scripts/') === false && + strpos($arguments[0], 'script_server.php') === false && + strpos($arguments[0], '_process.php') === false) { + + db_execute_prepared('INSERT INTO audit_log (page, user_id, action, outcome, ip_address, user_agent, event_time, post) + VALUES (?, ?, ?, ?, ?, ?, ?, ?)', + array($page, $user_id, $action, 'attempted', $ip_address, $user_agent, $event_time, $post)); } } -} \ No newline at end of file +} diff --git a/js/functions.js b/js/functions.js index 693b7c3..41bf752 100644 --- a/js/functions.js +++ b/js/functions.js @@ -31,12 +31,12 @@ * Apply filter to audit log */ function audit_applyFilter() { - strURL = 'audit.php' + - '?filter='+$('#filter').val()+ - '&rows='+$('#rows').val()+ - '&page='+$('#page').val()+ - '&event_page='+$('#event_page').val()+ - '&user_id='+$('#user_id').val()+ + var strURL = 'audit.php' + + '?filter='+encodeURIComponent($('#filter').val())+ + '&rows='+encodeURIComponent($('#rows').val())+ + '&page='+encodeURIComponent($('#page').val())+ + '&event_page='+encodeURIComponent($('#event_page').val())+ + '&user_id='+encodeURIComponent($('#user_id').val())+ '&header=false'; loadPageNoHeader(strURL); } @@ -45,7 +45,7 @@ function audit_applyFilter() { * Clear all filters */ function audit_clearFilter() { - strURL = 'audit.php?clear=1&header=false'; + var strURL = 'audit.php?clear=1&header=false'; loadPageNoHeader(strURL); } @@ -108,15 +108,20 @@ $(function() { }); $('#purge').click(function() { - strURL = 'audit.php?action=purge&header=false'; - loadPageNoHeader(strURL); + if (!window.confirm($(this).data('confirm'))) { + return; + } + + loadPageUsingPost('audit.php?action=purge&header=false', { + __csrf_magic: csrfMagicToken + }); }); $('#export').click(function() { document.location = 'audit.php?action=export' + - '&filter='+$('#filter').val()+ - '&event_page='+$('#event_page').val()+ - '&user_id='+$('#user_id').val(); + '&filter='+encodeURIComponent($('#filter').val())+ + '&event_page='+encodeURIComponent($('#event_page').val())+ + '&user_id='+encodeURIComponent($('#user_id').val()); }); $('#form_audit').submit(function(event) { @@ -128,7 +133,7 @@ $(function() { $('span[id^="event"]').hover(function() { audit_close_dialog(); - id = $(this).attr('id').replace('event', ''); + var id = $(this).attr('id').replace('event', ''); if (auditTimer != null) { clearTimeout(auditTimer); diff --git a/review.md b/review.md new file mode 100644 index 0000000..d64aec5 --- /dev/null +++ b/review.md @@ -0,0 +1,318 @@ +# Audit Plugin Code Review + +> Remediation update (branch `code_audit`): the implementation now addresses +> AUD-001 through AUD-010, adds focused security-helper coverage for AUD-011, +> and bumps the plugin schema/version to 1.3. The findings below describe the +> pre-remediation code that was reviewed and are retained as the audit record. +> Full browser/database integration coverage is still recommended before release. + +## Executive summary + +The plugin is small and readable, uses prepared statements for its primary insert and object lookups, and passes PHP syntax checks. However, it should not be released in its current form. The audit found two high-severity security defects, several confidentiality and audit-integrity weaknesses, broken remote-replication code, and no meaningful automated coverage for the sensitive paths. + +The highest priorities are: + +1. Escape every value rendered from `audit_log`. +2. Make purge a CSRF-protected POST operation with explicit authorization. +3. Replace the current credential redaction with recursive, allowlist-oriented collection, including CLI arguments. +4. Produce exports with a real CSV writer and neutralize spreadsheet formulas. +5. Define whether records represent attempts or successful changes and record that outcome accurately. + +## Scope and methodology + +Reviewed: + +- `.github/copilot-instructions.md` +- `setup.php` +- `audit.php` +- `audit_functions.php` +- `js/functions.js` +- `INFO`, `README.md`, `CHANGELOG.md` +- `.github/workflows/plugin-ci-workflow.yml` +- localization build assets and repository layout + +The review traced install/upgrade/uninstall, hook registration, GUI and CLI event capture, database writes and reads, record-detail rendering, filtering, export, purge, retention, external-file logging, and remote replication. Cacti core helpers in the adjacent Cacti checkout were consulted to verify authorization, CSRF, sorting, validation, and HTML helper behavior. + +Validation performed: + +```text +find . -name '*.php' -print0 | xargs -0 -n1 php -l +``` + +Result: every PHP file passed syntax validation. + +No plugin unit or functional test suite is present. The CI workflow installs the plugin, checks syntax, runs the poller, and asserts that at least one CLI audit row exists; it does not exercise the web UI or any negative/security cases. + +## Findings + +### AUD-001 — High — Stored XSS in audit detail and list views + +**Evidence** + +- `audit.php:54-60` directly concatenates CLI record fields into HTML. +- `audit.php:77-81` directly concatenates page, username, IP address, date, and action into HTML. +- `audit.php:95-104` directly renders request field names and values. +- `audit.php:121-123` embeds JSON record data inside `
` without HTML escaping.
+- `audit.php:432-448` passes database values such as `user_agent`, username, page, IP, and action to `form_selectable_cell()`. Cacti's `form_selectable_cell()` does not escape its content; `form_selectable_ecell()` is the escaping variant.
+- `audit_functions.php:131-158` stores attacker-influenced request data, and `audit_functions.php:167` stores the request's `User-Agent`.
+
+**Impact**
+
+An authenticated user who can cause an audited request can persist HTML or JavaScript in a request value or user-agent string. When an administrator with the audit realm views the list or hovers over the event, that content is inserted into the DOM. This can execute script in the administrator's Cacti session and may permit administrative account compromise.
+
+**Recommendation**
+
+- Escape all scalar values at the final HTML output boundary with `html_escape()`/`htmlspecialchars(..., ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8')`.
+- Recursively render arrays and objects as escaped text, never as markup.
+- Use `form_selectable_ecell()` for plain database values. Keep `form_selectable_cell()` only where the plugin itself constructs trusted markup, and escape the interpolated action inside that markup.
+- Return detail data as JSON and construct text nodes client-side, or keep the HTML endpoint but apply centralized contextual escaping.
+- Add regression tests containing `'
+	)
+);
+
+$redacted = audit_redact_sensitive_data($request);
+
+audit_test_assert_same('[REDACTED]', $redacted['password'], 'Top-level passwords must be redacted.');
+audit_test_assert_same('[REDACTED]', $redacted['nested']['api_token'], 'Nested tokens must be redacted.');
+audit_test_assert_same(
+	'',
+	$redacted['nested']['description'],
+	'Non-secret data must remain available for later context-aware output escaping.'
+);
+
+$arguments = audit_redact_cli_arguments(array(
+	'cli/example.php',
+	'--password=secret-one',
+	'--api-token',
+	'secret-two',
+	'https://user:secret-three@example.com/path',
+	'--description=test'
+));
+
+audit_test_assert_same('--password=[REDACTED]', $arguments[1], 'Inline CLI passwords must be redacted.');
+audit_test_assert_same('[REDACTED]', $arguments[3], 'Separate CLI secret values must be redacted.');
+audit_test_assert_same(
+	'https://user:[REDACTED]@example.com/path',
+	$arguments[4],
+	'Credentials embedded in a URI must be redacted.'
+);
+
+audit_test_assert_same("'=1+1", audit_csv_safe_cell('=1+1'), 'Spreadsheet formulas must be neutralized.');
+
+print "Security helper tests passed.\n";

From 1dc34b24080cf10f4d6a5103c8fd7a1cb5b25f22 Mon Sep 17 00:00:00 2001
From: Sean Mancini 
Date: Fri, 24 Jul 2026 08:50:21 -0400
Subject: [PATCH 02/10] Update audit.php

---
 audit.php | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/audit.php b/audit.php
index 9f607ef..1c39858 100644
--- a/audit.php
+++ b/audit.php
@@ -194,7 +194,7 @@ function audit_export_rows() {
 		header('X-Content-Type-Options: nosniff');
 
 		$output = fopen('php://output', 'w');
-		fputcsv($output, array('page', 'user_id', 'username', 'action', 'outcome', 'ip_address', 'user_agent', 'event_time', 'post'));
+		fputcsv($output, array('page', 'user_id', 'username', 'action', 'outcome', 'ip_address', 'user_agent', 'event_time', 'post'), ',', '"', '');
 
 		foreach($events as $event) {
 			if ($event['action'] == 'cli') {
@@ -214,7 +214,7 @@ function audit_export_rows() {
 				$event['user_agent'],
 				$event['event_time'],
 				$poster
-			)));
+			)), ',', '"', '');
 		}
 
 		fclose($output);

From 98c88f7b4f3d5768dc773ea3dc20e5fd44d6746d Mon Sep 17 00:00:00 2001
From: Sean Mancini 
Date: Fri, 24 Jul 2026 09:03:49 -0400
Subject: [PATCH 03/10] additional sweep

security: complete audit logging hardening

- bound request depth, field counts, string sizes, and JSON parsing
- detect and redact additional secret-shaped values
- finalize request outcomes as completed or failed
- track and retry failed external log delivery
- expose delivery status in the UI and CSV exports
- upgrade existing remote-poller schemas
- add retention, schema, delivery, and security regression tests
- update documentation and translation template
- bump plugin version to 1.4
---
 .github/workflows/plugin-ci-workflow.yml |  20 +++
 CHANGELOG.md                             |   3 +
 INFO                                     |   2 +-
 README.md                                |  11 +-
 audit.php                                |  28 +++-
 audit_functions.php                      | 178 +++++++++++++++++++++--
 locales/po/cacti.pot                     |  53 +++++++
 review.md                                |   7 +-
 setup.php                                |  16 +-
 tests/controller_security_test.php       |  18 +++
 tests/security_functions_test.php        |  50 ++++++-
 11 files changed, 357 insertions(+), 29 deletions(-)

diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml
index 5c85de3..8e6131a 100644
--- a/.github/workflows/plugin-ci-workflow.yml
+++ b/.github/workflows/plugin-ci-workflow.yml
@@ -172,6 +172,20 @@ jobs:
       run: |
         cd ${{ github.workspace }}/cacti
         sudo php cli/plugin_manage.php --plugin=audit --install --enable
+
+    - name: Verify Audit Schema
+      run: |
+        COLUMN_COUNT=$(mysql -u cactiuser -p'cactiuser' -h 127.0.0.1 cacti -se "
+          SELECT COUNT(*)
+          FROM information_schema.columns
+          WHERE table_schema = 'cacti'
+            AND table_name = 'audit_log'
+            AND column_name IN ('outcome', 'external_status', 'external_error');
+        ")
+        if [ "$COLUMN_COUNT" -ne 3 ]; then
+          echo "Audit schema security columns are missing"
+          exit 1
+        fi
     
     - name: Check PHP Syntax for Plugin
       run: |
@@ -227,3 +241,9 @@ jobs:
           echo "No audit log entries found!"
           exit 1
         fi
+
+        CLI_OUTCOME=$(mysql -u cactiuser -p'cactiuser' -h 127.0.0.1 cacti -se "select outcome from audit_log where action = 'cli' order by id desc limit 1;")
+        if [ "$CLI_OUTCOME" != "attempted" ]; then
+          echo "Unexpected CLI audit outcome: $CLI_OUTCOME"
+          exit 1
+        fi
diff --git a/CHANGELOG.md b/CHANGELOG.md
index e117d79..0fd3986 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,11 +2,14 @@
 
 --- develop ---
 
+* feature: Track and retry failed external audit-log delivery
+* security: Bound nested request depth, field counts, string sizes, and JSON parsing
 * security: Escape stored audit data before rendering to prevent stored XSS
 * security: Require an authorized, CSRF-protected POST to purge the audit log
 * security: Recursively redact sensitive web and CLI values
 * security: Generate standards-compliant, spreadsheet-safe CSV exports
 * feature: Mark hook-time records explicitly as attempted actions
+* feature: Finalize request outcomes and expose external-log delivery status
 * issue: Harden external file logging, retention, malformed records, and replication
 * issue#38: Graph Template table does not exist
 * issue: If the audit log does not exist or is not set, set it and create it
diff --git a/INFO b/INFO
index 7fda835..6510278 100644
--- a/INFO
+++ b/INFO
@@ -21,7 +21,7 @@
 
 [info]
 name = audit
-version = 1.3
+version = 1.4
 longname = Audit Plugin for Cacti
 author = The Cacti Group
 email = 
diff --git a/README.md b/README.md
index 95c2bfb..f085034 100644
--- a/README.md
+++ b/README.md
@@ -15,9 +15,10 @@ This plugin allows Cacti Administrators to log user change activity
 
 * Captures CLI commands and who ran them
 
-Audit entries generated by the `config_insert` hook record requested actions as
-`attempted`. They do not prove that the page-specific operation completed
-successfully.
+Audit entries generated by the `config_insert` hook begin as `attempted` and are
+finalized as `request_completed` or `request_failed` when PHP shuts down. A
+completed request does not necessarily prove that every page-specific database
+operation succeeded.
 
 ## Installation
 
@@ -30,6 +31,10 @@ data retention and turn on auditing.
 
 You can also enable file based logging for ingestion by Siem or Log analysis tools such as splunk
 
+External file delivery is tracked on each database record. Failed appends are
+retried by the poller in batches and therefore have at-least-once delivery
+semantics; downstream ingestion should deduplicate when necessary.
+
 ## Possible Bugs
 
 If you figure out this problem, see the Cacti forums!
diff --git a/audit.php b/audit.php
index 1c39858..82d9be7 100644
--- a/audit.php
+++ b/audit.php
@@ -82,6 +82,10 @@ function audit_render_event_details($data) {
 	$output .= '
' . __('Date:', 'audit') . ' ' . html_escape($data['event_time']) . ''; $output .= '
' . __('Action:', 'audit') . ' ' . html_escape($data['action']) . ''; $output .= '
' . __('Outcome:', 'audit') . ' ' . html_escape($data['outcome']) . ''; + $output .= '
' . __('External Delivery:', 'audit') . ' ' . html_escape($data['external_status']) . ''; + if ($data['external_error'] != '') { + $output .= '
' . __('External Error:', 'audit') . ' ' . html_escape($data['external_error']) . ''; + } $output .= '
'; if ($data['action'] == 'cli') { @@ -89,9 +93,9 @@ function audit_render_event_details($data) { return $output . ''; } - $attribs = json_decode($data['post'], true); + $attribs = audit_json_decode($data['post'], $json_error); $attribs = is_array($attribs) ? $attribs : array( - __('Stored Data', 'audit') => __('The stored request data is not valid JSON.', 'audit') + __('Stored Data', 'audit') => __('The stored request data is not valid JSON: %s', $json_error, 'audit') ); ksort($attribs); @@ -119,7 +123,7 @@ function audit_render_event_details($data) { $output .= ''; } - $record_data = json_decode($data['object_data'], true); + $record_data = audit_json_decode($data['object_data'], $object_error); if (is_array($record_data) && !empty($record_data)) { $output .= '
'; $output .= '' . __('Record Data:', 'audit') . ''; @@ -194,13 +198,13 @@ function audit_export_rows() { header('X-Content-Type-Options: nosniff'); $output = fopen('php://output', 'w'); - fputcsv($output, array('page', 'user_id', 'username', 'action', 'outcome', 'ip_address', 'user_agent', 'event_time', 'post'), ',', '"', ''); + fputcsv($output, array('page', 'user_id', 'username', 'action', 'outcome', 'external_status', 'external_error', 'ip_address', 'user_agent', 'event_time', 'post'), ',', '"', ''); foreach($events as $event) { if ($event['action'] == 'cli') { $poster = $event['post']; } else { - $post = json_decode($event['post'], true); + $post = audit_json_decode($event['post'], $json_error); $poster = is_array($post) ? json_encode($post, JSON_INVALID_UTF8_SUBSTITUTE) : $event['post']; } @@ -210,6 +214,8 @@ function audit_export_rows() { get_username($event['user_id']), $event['action'], $event['outcome'], + $event['external_status'], + $event['external_error'], $event['ip_address'], $event['user_agent'], $event['event_time'], @@ -430,7 +436,13 @@ function audit_log() { 'display' => __('Outcome', 'audit'), 'align' => 'left', 'sort' => 'ASC', - 'tip' => __('Whether this event records an attempted or confirmed action.', 'audit') + 'tip' => __('Request processing state; completion does not guarantee that every operation succeeded.', 'audit') + ), + 'external_status' => array( + 'display' => __('External Delivery', 'audit'), + 'align' => 'left', + 'sort' => 'ASC', + 'tip' => __('Delivery state for the optional external audit log.', 'audit') ), 'user_agent' => array( 'display' => __('User Agent', 'audit'), @@ -463,6 +475,7 @@ function audit_log() { form_selectable_ecell($e['user_agent'], $e['id']); form_selectable_cell('' . html_escape(ucfirst($e['action'])) . '', $e['id']); form_selectable_ecell($e['outcome'], $e['id']); + form_selectable_ecell($e['external_status'], $e['id']); form_selectable_cell(__('N/A', 'audit'), $e['id']); form_selectable_ecell($e['ip_address'], $e['id'], '', 'right'); form_selectable_ecell($e['event_time'], $e['id'], '', 'right'); @@ -473,6 +486,7 @@ function audit_log() { form_selectable_ecell($e['username'], $e['id']); form_selectable_cell('' . html_escape(ucfirst($e['action'])) . '', $e['id']); form_selectable_ecell($e['outcome'], $e['id']); + form_selectable_ecell($e['external_status'], $e['id']); form_selectable_ecell($e['user_agent'], $e['id']); form_selectable_ecell($e['ip_address'], $e['id'], '', 'right'); form_selectable_ecell($e['event_time'], $e['id'], '', 'right'); @@ -480,7 +494,7 @@ function audit_log() { } } } else { - print "" . __('No Audit Log Events Found', 'audit') . "\n"; + print "" . __('No Audit Log Events Found', 'audit') . "\n"; } html_end_box(false); diff --git a/audit_functions.php b/audit_functions.php index 7b5b1dc..89c9848 100644 --- a/audit_functions.php +++ b/audit_functions.php @@ -138,13 +138,59 @@ function audit_redact_sensitive_data($data) { } elseif (is_array($value)) { $redacted[$key] = audit_redact_sensitive_data($value); } else { - $redacted[$key] = $value; + $redacted[$key] = audit_redact_sensitive_value($value); } } return $redacted; } +function audit_redact_sensitive_value($value) { + if (!is_string($value)) { + return $value; + } + + if (preg_match('/-----BEGIN (?:[A-Z ]+ )?PRIVATE KEY-----/', $value) || + preg_match('/^(?:Bearer|Basic)\s+[A-Za-z0-9+\/_=.-]+$/i', trim($value)) || + preg_match('/^[A-Za-z0-9_-]{20,}\.[A-Za-z0-9_-]{20,}\.[A-Za-z0-9_-]{20,}$/', trim($value))) { + return '[REDACTED]'; + } + + return preg_replace('#^([a-z][a-z0-9+.-]*://[^:/@\s]+):[^@\s]+@#i', '$1:[REDACTED]@', $value); +} + +function audit_bound_log_data($data, $depth = 0, $state = null) { + if ($state === null) { + $state = (object) array('fields' => 0); + } + + if ($depth >= 12) { + return '[MAXIMUM DEPTH REACHED]'; + } + + if (is_array($data)) { + $bounded = array(); + + foreach ($data as $key => $value) { + if ($state->fields >= 1000) { + $bounded['audit_truncated'] = 'Additional fields were omitted.'; + break; + } + + $state->fields++; + $bounded[$key] = audit_bound_log_data($value, $depth + 1, $state); + } + + return $bounded; + } + + if (is_string($data) && strlen($data) > 65536) { + return substr($data, 0, 65536) . '[TRUNCATED]'; + } + + return $data; +} + function audit_redact_cli_arguments($arguments) { $redacted = array(); $redact_next = false; @@ -174,7 +220,7 @@ function audit_redact_cli_arguments($arguments) { } function audit_json_encode($data) { - $json = json_encode($data, JSON_INVALID_UTF8_SUBSTITUTE); + $json = json_encode(audit_bound_log_data($data), JSON_INVALID_UTF8_SUBSTITUTE, 16); if ($json === false) { return json_encode(array('audit_encoding_error' => json_last_error_msg())); @@ -183,6 +229,17 @@ function audit_json_encode($data) { return $json; } +function audit_json_decode($json, &$error = null) { + $error = null; + + try { + return json_decode($json, true, 16, JSON_THROW_ON_ERROR); + } catch (Throwable $exception) { + $error = $exception->getMessage(); + return null; + } +} + function audit_csv_safe_cell($value) { $value = (string) $value; @@ -193,6 +250,96 @@ function audit_csv_safe_cell($value) { return $value; } +function audit_retention_cutoff($retention, $now = null) { + $now = $now instanceof DateTimeImmutable + ? $now->setTimezone(new DateTimeZone('UTC')) + : new DateTimeImmutable('now', new DateTimeZone('UTC')); + + return $now->sub(new DateInterval('P' . max(0, (int) $retention) . 'D')); +} + +function audit_append_external_log($path, $message) { + if ($path == '' || !is_file($path) || is_link($path)) { + return array('status' => 'failed', 'error' => 'Destination is not a regular file or is a symbolic link.'); + } + + $written = file_put_contents($path, $message, FILE_APPEND | LOCK_EX); + if ($written !== strlen($message)) { + return array('status' => 'failed', 'error' => 'Unable to append a complete record.'); + } + + return array('status' => 'delivered', 'error' => ''); +} + +function audit_set_external_status($id, $status, $error = '') { + db_execute_prepared('UPDATE audit_log + SET external_status = ?, external_error = ? + WHERE id = ?', + array($status, $error, $id)); +} + +function audit_retry_external_logs() { + if (read_config_option('audit_log_external') != 'on') { + return; + } + + $path = read_config_option('audit_log_external_path'); + if ($path == '' || !is_file($path) || is_link($path)) { + return; + } + + $events = db_fetch_assoc("SELECT * + FROM audit_log + WHERE external_status = 'failed' + ORDER BY id + LIMIT 100"); + + foreach ($events as $event) { + $log_data = array( + 'page' => $event['page'], + 'user_id' => $event['user_id'], + 'action' => $event['action'], + 'outcome' => $event['outcome'], + 'ip_address' => $event['ip_address'], + 'user_agent' => $event['user_agent'], + 'event_time' => $event['event_time'], + 'post' => $event['post'], + 'object_data' => $event['object_data'] + ); + + $message = audit_json_encode($log_data) . "\n"; + $delivery = audit_append_external_log($path, $message); + audit_set_external_status($event['id'], $delivery['status'], $delivery['error']); + + if ($delivery['status'] != 'delivered') { + break; + } + } +} + +function audit_request_outcome($error = null, $status_code = 200) { + $fatal_types = array(E_ERROR, E_PARSE, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR, E_RECOVERABLE_ERROR); + + if ((is_array($error) && in_array($error['type'] ?? null, $fatal_types, true)) || + $status_code >= 400) { + return 'request_failed'; + } + + return 'request_completed'; +} + +function audit_finalize_request($id) { + $status_code = http_response_code(); + $status_code = is_int($status_code) ? $status_code : 200; + $outcome = audit_request_outcome(error_get_last(), $status_code); + + db_execute_prepared("UPDATE audit_log + SET outcome = ? + WHERE id = ? + AND outcome = 'attempted'", + array($outcome, $id)); +} + function audit_config_insert() { @@ -270,6 +417,8 @@ function audit_config_insert() { } $audit_log = read_config_option('audit_log_external_path'); + $external_logging = read_config_option('audit_log_external') == 'on'; + $external_status = $external_logging ? 'pending' : 'disabled'; if (!defined('CACTI_PATH_BASE')) { $base = $config['base_path']; @@ -277,11 +426,11 @@ function audit_config_insert() { $base = CACTI_PATH_BASE; } - db_execute_prepared('INSERT INTO audit_log (page, user_id, action, outcome, ip_address, user_agent, event_time, post, object_data) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)', - array($page, $user_id, $action, 'attempted', $ip_address, $user_agent, $event_time, $post, $object_data)); - - $external_logging = read_config_option('audit_log_external') == 'on'; + db_execute_prepared('INSERT INTO audit_log (page, user_id, action, outcome, ip_address, user_agent, event_time, post, object_data, external_status) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)', + array($page, $user_id, $action, 'attempted', $ip_address, $user_agent, $event_time, $post, $object_data, $external_status)); + $audit_id = db_fetch_insert_id(); + register_shutdown_function('audit_finalize_request', $audit_id); if ($external_logging && $audit_log == '') { set_config_option('audit_log_external_path', $base . '/log/audit.log'); @@ -315,12 +464,15 @@ function audit_config_insert() { ); $log_msg = audit_json_encode($log_data) . "\n"; - $written = file_put_contents($audit_log, $log_msg, FILE_APPEND | LOCK_EX); + $delivery = audit_append_external_log($audit_log, $log_msg); + audit_set_external_status($audit_id, $delivery['status'], $delivery['error']); - if ($written !== strlen($log_msg)) { - cacti_log(sprintf('ERROR: Unable to append a complete record to Audit Log file \'%s\'.', $audit_log), false, 'AUDIT'); + if ($delivery['status'] != 'delivered') { + cacti_log(sprintf('ERROR: Unable to append a complete record to Audit Log file \'%s\': %s', $audit_log, $delivery['error']), false, 'AUDIT'); } } elseif ($external_logging && $audit_log != '') { + $error = 'Destination is not a regular file or is a symbolic link.'; + audit_set_external_status($audit_id, 'failed', $error); cacti_log(sprintf('ERROR: Audit Log file \'%s\' is not a regular file or is a symbolic link.', $audit_log), false, 'AUDIT'); } } elseif (isset($_SERVER['argv']) && cacti_sizeof($_SERVER['argv'])) { @@ -340,9 +492,9 @@ function audit_config_insert() { strpos($arguments[0], 'script_server.php') === false && strpos($arguments[0], '_process.php') === false) { - db_execute_prepared('INSERT INTO audit_log (page, user_id, action, outcome, ip_address, user_agent, event_time, post) - VALUES (?, ?, ?, ?, ?, ?, ?, ?)', - array($page, $user_id, $action, 'attempted', $ip_address, $user_agent, $event_time, $post)); + db_execute_prepared('INSERT INTO audit_log (page, user_id, action, outcome, ip_address, user_agent, event_time, post, external_status) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)', + array($page, $user_id, $action, 'attempted', $ip_address, $user_agent, $event_time, $post, 'not_applicable')); } } } diff --git a/locales/po/cacti.pot b/locales/po/cacti.pot index 556ffbc..0d57f0c 100644 --- a/locales/po/cacti.pot +++ b/locales/po/cacti.pot @@ -283,3 +283,56 @@ msgstr "" #: setup.php:316 msgid "Audit Event Log" msgstr "" + +#: audit.php +msgid "Audit event not found." +msgstr "" + +#: audit.php +msgid "External Delivery:" +msgstr "" + +#: audit.php +msgid "External Error:" +msgstr "" + +#: audit.php +msgid "Stored Data" +msgstr "" + +#: audit.php +#, php-format +msgid "The stored request data is not valid JSON: %s" +msgstr "" + +#: audit.php +msgid "Requested Action" +msgstr "" + +#: audit.php +msgid "The requested Cacti action. Hover over the action to see request data." +msgstr "" + +#: audit.php +msgid "Outcome" +msgstr "" + +#: audit.php +msgid "Request processing state; completion does not guarantee that every operation succeeded." +msgstr "" + +#: audit.php +msgid "External Delivery" +msgstr "" + +#: audit.php +msgid "Delivery state for the optional external audit log." +msgstr "" + +#: audit.php +msgid "Permanently purge all audit log events?" +msgstr "" + +#: setup.php +msgid "Manage Cacti Audit Log" +msgstr "" diff --git a/review.md b/review.md index d64aec5..f35b4cf 100644 --- a/review.md +++ b/review.md @@ -2,9 +2,14 @@ > Remediation update (branch `code_audit`): the implementation now addresses > AUD-001 through AUD-010, adds focused security-helper coverage for AUD-011, -> and bumps the plugin schema/version to 1.3. The findings below describe the +> and bumps the plugin schema/version to 1.4. The findings below describe the > pre-remediation code that was reviewed and are retained as the audit record. > Full browser/database integration coverage is still recommended before release. +> Generic requests are finalized as completed or failed, but operation-specific +> success cannot be proven without corresponding completion hooks in each Cacti +> page. Request collection uses recursive redaction and strict size/depth bounds; +> converting every supported page to a field allowlist remains a future, +> compatibility-sensitive change. ## Executive summary diff --git a/setup.php b/setup.php index cf563f0..432aa9b 100644 --- a/setup.php +++ b/setup.php @@ -84,6 +84,8 @@ function audit_check_upgrade() { db_execute('ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS object_data LONGBLOB'); db_execute("ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS outcome varchar(20) NOT NULL DEFAULT 'unknown' AFTER action"); + db_execute("ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS external_status varchar(20) NOT NULL DEFAULT 'unknown' AFTER object_data"); + db_execute('ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS external_error varchar(1024) DEFAULT NULL AFTER external_status'); db_execute_prepared('UPDATE plugin_config SET version = ? @@ -126,14 +128,21 @@ function audit_replicate_out($data) { } } } else { - cacti_log('INFO: Audit Log table exists skipping', false, 'REPLICATE'); + cacti_log('INFO: Audit Log table exists, checking schema', false, 'REPLICATE'); } + + db_execute('ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS object_data LONGBLOB', true, $rcnn_id); + db_execute("ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS outcome varchar(20) NOT NULL DEFAULT 'unknown' AFTER action", true, $rcnn_id); + db_execute("ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS external_status varchar(20) NOT NULL DEFAULT 'unknown' AFTER object_data", true, $rcnn_id); + db_execute('ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS external_error varchar(1024) DEFAULT NULL AFTER external_status', true, $rcnn_id); } return $data; } function audit_poller_bottom() { + audit_retry_external_logs(); + $last_check = read_config_option('audit_last_check'); $now = gmdate('Y-m-d'); @@ -141,8 +150,7 @@ function audit_poller_bottom() { $retention = read_config_option('audit_retention'); if ($retention > 0) { - $cutoff = new DateTimeImmutable('now', new DateTimeZone('UTC')); - $cutoff = $cutoff->sub(new DateInterval('P' . (int) $retention . 'D')); + $cutoff = audit_retention_cutoff($retention); db_execute_prepared('DELETE FROM audit_log WHERE event_time < ?', array($cutoff->format('Y-m-d H:i:s'))); $rows = db_affected_rows(); @@ -168,6 +176,8 @@ function audit_setup_table() { `event_time` timestamp DEFAULT CURRENT_TIMESTAMP, `post` longblob, `object_data` longblob, + `external_status` varchar(20) NOT NULL DEFAULT 'unknown', + `external_error` varchar(1024) DEFAULT NULL, PRIMARY KEY (`id`), KEY `user_id` (`user_id`), KEY `page` (`page`), diff --git a/tests/controller_security_test.php b/tests/controller_security_test.php index 5aa16a2..2b549b4 100644 --- a/tests/controller_security_test.php +++ b/tests/controller_security_test.php @@ -2,6 +2,7 @@ $controller = file_get_contents(dirname(__DIR__) . '/audit.php'); $javascript = file_get_contents(dirname(__DIR__) . '/js/functions.js'); +$setup = file_get_contents(dirname(__DIR__) . '/setup.php'); $required_controller_guards = array( "\$_SERVER['REQUEST_METHOD'] !== 'POST'", @@ -19,6 +20,23 @@ } } +$required_schema_fragments = array( + 'api_plugin_register_realm(\'audit\', \'audit_manage.php\'', + 'api_plugin_register_hook(\'audit\', \'replicate_out\'', + 'ADD COLUMN IF NOT EXISTS outcome', + 'ADD COLUMN IF NOT EXISTS external_status', + 'ADD COLUMN IF NOT EXISTS external_error', + 'SHOW CREATE TABLE $table', + 'audit_retry_external_logs()' +); + +foreach ($required_schema_fragments as $fragment) { + if (strpos($setup, $fragment) === false) { + fwrite(STDERR, 'Missing schema or replication requirement: ' . $fragment . PHP_EOL); + exit(1); + } +} + if (strpos($javascript, "loadPageNoHeader('audit.php?action=purge") !== false) { fwrite(STDERR, 'Purge must not use the legacy GET request path.' . PHP_EOL); exit(1); diff --git a/tests/security_functions_test.php b/tests/security_functions_test.php index fdd6578..57a222d 100644 --- a/tests/security_functions_test.php +++ b/tests/security_functions_test.php @@ -16,7 +16,8 @@ function audit_test_assert_same($expected, $actual, $message) { 'password' => 'top-secret', 'nested' => array( 'api_token' => 'nested-secret', - 'description' => '' + 'description' => '', + 'opaque_value' => 'Bearer abcdefghijklmnopqrstuvwxyz' ) ); @@ -24,6 +25,7 @@ function audit_test_assert_same($expected, $actual, $message) { audit_test_assert_same('[REDACTED]', $redacted['password'], 'Top-level passwords must be redacted.'); audit_test_assert_same('[REDACTED]', $redacted['nested']['api_token'], 'Nested tokens must be redacted.'); +audit_test_assert_same('[REDACTED]', $redacted['nested']['opaque_value'], 'Secret-shaped values must be redacted even when their key is unknown.'); audit_test_assert_same( '', $redacted['nested']['description'], @@ -49,4 +51,50 @@ function audit_test_assert_same($expected, $actual, $message) { audit_test_assert_same("'=1+1", audit_csv_safe_cell('=1+1'), 'Spreadsheet formulas must be neutralized.'); +$deep = array(); +$cursor = &$deep; +for ($i = 0; $i < 15; $i++) { + $cursor['nested'] = array(); + $cursor = &$cursor['nested']; +} +unset($cursor); + +$bounded_json = audit_json_encode($deep); +if (strpos($bounded_json, 'MAXIMUM DEPTH REACHED') === false) { + fwrite(STDERR, 'Deep request structures must be bounded.' . PHP_EOL); + exit(1); +} + +$invalid_json = audit_json_decode('{invalid', $json_error); +audit_test_assert_same(null, $invalid_json, 'Malformed JSON must return a controlled null result.'); +if ($json_error === null || $json_error === '') { + fwrite(STDERR, 'Malformed JSON must return a useful error.' . PHP_EOL); + exit(1); +} + +$now = new DateTimeImmutable('2026-03-09 12:00:00', new DateTimeZone('America/New_York')); +$cutoff = audit_retention_cutoff(1, $now); +audit_test_assert_same( + '2026-03-08 16:00:00', + $cutoff->format('Y-m-d H:i:s'), + 'Retention must subtract full UTC days consistently across DST.' +); + +audit_test_assert_same( + 'request_completed', + audit_request_outcome(null, 302), + 'Successful redirects must finalize as completed requests.' +); +audit_test_assert_same( + 'request_failed', + audit_request_outcome(array('type' => E_ERROR), 200), + 'Fatal errors must finalize as failed requests.' +); + +$temporary_log = tempnam(sys_get_temp_dir(), 'audit-test-'); +$delivery = audit_append_external_log($temporary_log, "test-record\n"); +audit_test_assert_same('delivered', $delivery['status'], 'A complete external log write must report delivery.'); +audit_test_assert_same("test-record\n", file_get_contents($temporary_log), 'External log content must be complete.'); +unlink($temporary_log); + print "Security helper tests passed.\n"; From 46606809715c81f63defca548ef6101a43275879 Mon Sep 17 00:00:00 2001 From: Sean Mancini Date: Fri, 24 Jul 2026 09:12:21 -0400 Subject: [PATCH 04/10] rename some columns to be more specific --- .github/workflows/plugin-ci-workflow.yml | 8 +++--- CHANGELOG.md | 1 + INFO | 2 +- README.md | 4 +-- audit.php | 14 ++++----- audit_functions.php | 26 ++++++++--------- locales/po/cacti.pot | 6 +++- review.md | 5 +++- setup.php | 36 ++++++++++++++++++++++-- tests/controller_security_test.php | 2 +- tests/security_functions_test.php | 8 +++--- 11 files changed, 75 insertions(+), 37 deletions(-) diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index 8e6131a..a5b0a9d 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -180,7 +180,7 @@ jobs: FROM information_schema.columns WHERE table_schema = 'cacti' AND table_name = 'audit_log' - AND column_name IN ('outcome', 'external_status', 'external_error'); + AND column_name IN ('request_status', 'external_status', 'external_error'); ") if [ "$COLUMN_COUNT" -ne 3 ]; then echo "Audit schema security columns are missing" @@ -242,8 +242,8 @@ jobs: exit 1 fi - CLI_OUTCOME=$(mysql -u cactiuser -p'cactiuser' -h 127.0.0.1 cacti -se "select outcome from audit_log where action = 'cli' order by id desc limit 1;") - if [ "$CLI_OUTCOME" != "attempted" ]; then - echo "Unexpected CLI audit outcome: $CLI_OUTCOME" + CLI_STATUS=$(mysql -u cactiuser -p'cactiuser' -h 127.0.0.1 cacti -se "select request_status from audit_log where action = 'cli' order by id desc limit 1;") + if [ "$CLI_STATUS" != "started" ]; then + echo "Unexpected CLI request status: $CLI_STATUS" exit 1 fi diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fd3986..d24dcf6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ --- develop --- +* feature: Rename outcome to request_status with started/completed/failed values * feature: Track and retry failed external audit-log delivery * security: Bound nested request depth, field counts, string sizes, and JSON parsing * security: Escape stored audit data before rendering to prevent stored XSS diff --git a/INFO b/INFO index 6510278..7fda835 100644 --- a/INFO +++ b/INFO @@ -21,7 +21,7 @@ [info] name = audit -version = 1.4 +version = 1.3 longname = Audit Plugin for Cacti author = The Cacti Group email = diff --git a/README.md b/README.md index f085034..0de354d 100644 --- a/README.md +++ b/README.md @@ -15,8 +15,8 @@ This plugin allows Cacti Administrators to log user change activity * Captures CLI commands and who ran them -Audit entries generated by the `config_insert` hook begin as `attempted` and are -finalized as `request_completed` or `request_failed` when PHP shuts down. A +Audit entries generated by the `config_insert` hook begin with request status +`started` and are finalized as `completed` or `failed` when PHP shuts down. A completed request does not necessarily prove that every page-specific database operation succeeded. diff --git a/audit.php b/audit.php index 82d9be7..49afa4d 100644 --- a/audit.php +++ b/audit.php @@ -81,7 +81,7 @@ function audit_render_event_details($data) { $output .= '
' . __('IP Address:', 'audit') . ' ' . html_escape($data['ip_address']) . ''; $output .= '
' . __('Date:', 'audit') . ' ' . html_escape($data['event_time']) . ''; $output .= '
' . __('Action:', 'audit') . ' ' . html_escape($data['action']) . ''; - $output .= '
' . __('Outcome:', 'audit') . ' ' . html_escape($data['outcome']) . ''; + $output .= '
' . __('Request Status:', 'audit') . ' ' . html_escape($data['request_status']) . ''; $output .= '
' . __('External Delivery:', 'audit') . ' ' . html_escape($data['external_status']) . ''; if ($data['external_error'] != '') { $output .= '
' . __('External Error:', 'audit') . ' ' . html_escape($data['external_error']) . ''; @@ -198,7 +198,7 @@ function audit_export_rows() { header('X-Content-Type-Options: nosniff'); $output = fopen('php://output', 'w'); - fputcsv($output, array('page', 'user_id', 'username', 'action', 'outcome', 'external_status', 'external_error', 'ip_address', 'user_agent', 'event_time', 'post'), ',', '"', ''); + fputcsv($output, array('page', 'user_id', 'username', 'action', 'request_status', 'external_status', 'external_error', 'ip_address', 'user_agent', 'event_time', 'post'), ',', '"', ''); foreach($events as $event) { if ($event['action'] == 'cli') { @@ -213,7 +213,7 @@ function audit_export_rows() { $event['user_id'], get_username($event['user_id']), $event['action'], - $event['outcome'], + $event['request_status'], $event['external_status'], $event['external_error'], $event['ip_address'], @@ -432,8 +432,8 @@ function audit_log() { 'sort' => 'ASC', 'tip' => __('The requested Cacti action. Hover over the action to see request data.', 'audit') ), - 'outcome' => array( - 'display' => __('Outcome', 'audit'), + 'request_status' => array( + 'display' => __('Request Status', 'audit'), 'align' => 'left', 'sort' => 'ASC', 'tip' => __('Request processing state; completion does not guarantee that every operation succeeded.', 'audit') @@ -474,7 +474,7 @@ function audit_log() { form_selectable_ecell($e['page'], $e['id']); form_selectable_ecell($e['user_agent'], $e['id']); form_selectable_cell('' . html_escape(ucfirst($e['action'])) . '', $e['id']); - form_selectable_ecell($e['outcome'], $e['id']); + form_selectable_ecell($e['request_status'], $e['id']); form_selectable_ecell($e['external_status'], $e['id']); form_selectable_cell(__('N/A', 'audit'), $e['id']); form_selectable_ecell($e['ip_address'], $e['id'], '', 'right'); @@ -485,7 +485,7 @@ function audit_log() { form_selectable_cell(filter_value($e['page'], get_request_var('filter')), $e['id']); form_selectable_ecell($e['username'], $e['id']); form_selectable_cell('' . html_escape(ucfirst($e['action'])) . '', $e['id']); - form_selectable_ecell($e['outcome'], $e['id']); + form_selectable_ecell($e['request_status'], $e['id']); form_selectable_ecell($e['external_status'], $e['id']); form_selectable_ecell($e['user_agent'], $e['id']); form_selectable_ecell($e['ip_address'], $e['id'], '', 'right'); diff --git a/audit_functions.php b/audit_functions.php index 89c9848..78c4739 100644 --- a/audit_functions.php +++ b/audit_functions.php @@ -299,7 +299,7 @@ function audit_retry_external_logs() { 'page' => $event['page'], 'user_id' => $event['user_id'], 'action' => $event['action'], - 'outcome' => $event['outcome'], + 'request_status' => $event['request_status'], 'ip_address' => $event['ip_address'], 'user_agent' => $event['user_agent'], 'event_time' => $event['event_time'], @@ -317,27 +317,27 @@ function audit_retry_external_logs() { } } -function audit_request_outcome($error = null, $status_code = 200) { +function audit_request_status($error = null, $status_code = 200) { $fatal_types = array(E_ERROR, E_PARSE, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR, E_RECOVERABLE_ERROR); if ((is_array($error) && in_array($error['type'] ?? null, $fatal_types, true)) || $status_code >= 400) { - return 'request_failed'; + return 'failed'; } - return 'request_completed'; + return 'completed'; } function audit_finalize_request($id) { $status_code = http_response_code(); $status_code = is_int($status_code) ? $status_code : 200; - $outcome = audit_request_outcome(error_get_last(), $status_code); + $request_status = audit_request_status(error_get_last(), $status_code); db_execute_prepared("UPDATE audit_log - SET outcome = ? + SET request_status = ? WHERE id = ? - AND outcome = 'attempted'", - array($outcome, $id)); + AND request_status = 'started'", + array($request_status, $id)); } @@ -426,9 +426,9 @@ function audit_config_insert() { $base = CACTI_PATH_BASE; } - db_execute_prepared('INSERT INTO audit_log (page, user_id, action, outcome, ip_address, user_agent, event_time, post, object_data, external_status) + db_execute_prepared('INSERT INTO audit_log (page, user_id, action, request_status, ip_address, user_agent, event_time, post, object_data, external_status) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)', - array($page, $user_id, $action, 'attempted', $ip_address, $user_agent, $event_time, $post, $object_data, $external_status)); + array($page, $user_id, $action, 'started', $ip_address, $user_agent, $event_time, $post, $object_data, $external_status)); $audit_id = db_fetch_insert_id(); register_shutdown_function('audit_finalize_request', $audit_id); @@ -455,7 +455,7 @@ function audit_config_insert() { 'page' => $page, 'user_id' => $user_id, 'action' => $action, - 'outcome' => 'attempted', + 'request_status' => 'started', 'ip_address' => $ip_address, 'user_agent' => $user_agent, 'event_time' => $event_time, @@ -492,9 +492,9 @@ function audit_config_insert() { strpos($arguments[0], 'script_server.php') === false && strpos($arguments[0], '_process.php') === false) { - db_execute_prepared('INSERT INTO audit_log (page, user_id, action, outcome, ip_address, user_agent, event_time, post, external_status) + db_execute_prepared('INSERT INTO audit_log (page, user_id, action, request_status, ip_address, user_agent, event_time, post, external_status) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)', - array($page, $user_id, $action, 'attempted', $ip_address, $user_agent, $event_time, $post, 'not_applicable')); + array($page, $user_id, $action, 'started', $ip_address, $user_agent, $event_time, $post, 'not_applicable')); } } } diff --git a/locales/po/cacti.pot b/locales/po/cacti.pot index 0d57f0c..ad15bf4 100644 --- a/locales/po/cacti.pot +++ b/locales/po/cacti.pot @@ -314,7 +314,11 @@ msgid "The requested Cacti action. Hover over the action to see request data." msgstr "" #: audit.php -msgid "Outcome" +msgid "Request Status" +msgstr "" + +#: audit.php +msgid "Request Status:" msgstr "" #: audit.php diff --git a/review.md b/review.md index f35b4cf..a24331c 100644 --- a/review.md +++ b/review.md @@ -2,7 +2,7 @@ > Remediation update (branch `code_audit`): the implementation now addresses > AUD-001 through AUD-010, adds focused security-helper coverage for AUD-011, -> and bumps the plugin schema/version to 1.4. The findings below describe the +> and bumps the plugin schema/version to 1.3. The findings below describe the > pre-remediation code that was reviewed and are retained as the audit record. > Full browser/database integration coverage is still recommended before release. > Generic requests are finalized as completed or failed, but operation-specific @@ -10,6 +10,9 @@ > page. Request collection uses recursive redaction and strict size/depth bounds; > converting every supported page to a field allowlist remains a future, > compatibility-sensitive change. +> Request lifecycle state is stored in `request_status` using `started`, +> `completed`, and `failed`; it is intentionally distinct from operation-level +> success. ## Executive summary diff --git a/setup.php b/setup.php index 432aa9b..99e6082 100644 --- a/setup.php +++ b/setup.php @@ -83,7 +83,22 @@ function audit_check_upgrade() { } db_execute('ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS object_data LONGBLOB'); - db_execute("ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS outcome varchar(20) NOT NULL DEFAULT 'unknown' AFTER action"); + if (db_column_exists('audit_log', 'outcome')) { + if (!db_column_exists('audit_log', 'request_status')) { + db_execute("ALTER TABLE audit_log CHANGE COLUMN outcome request_status varchar(20) NOT NULL DEFAULT 'unknown'"); + } else { + db_execute("UPDATE audit_log SET request_status = outcome WHERE request_status = 'unknown'"); + db_execute('ALTER TABLE audit_log DROP COLUMN outcome'); + } + } elseif (!db_column_exists('audit_log', 'request_status')) { + db_execute("ALTER TABLE audit_log ADD COLUMN request_status varchar(20) NOT NULL DEFAULT 'unknown' AFTER action"); + } + + db_execute("UPDATE audit_log SET request_status = CASE request_status + WHEN 'attempted' THEN 'started' + WHEN 'request_completed' THEN 'completed' + WHEN 'request_failed' THEN 'failed' + ELSE request_status END"); db_execute("ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS external_status varchar(20) NOT NULL DEFAULT 'unknown' AFTER object_data"); db_execute('ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS external_error varchar(1024) DEFAULT NULL AFTER external_status'); @@ -132,7 +147,22 @@ function audit_replicate_out($data) { } db_execute('ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS object_data LONGBLOB', true, $rcnn_id); - db_execute("ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS outcome varchar(20) NOT NULL DEFAULT 'unknown' AFTER action", true, $rcnn_id); + if (db_column_exists('audit_log', 'outcome', false, $rcnn_id)) { + if (!db_column_exists('audit_log', 'request_status', false, $rcnn_id)) { + db_execute("ALTER TABLE audit_log CHANGE COLUMN outcome request_status varchar(20) NOT NULL DEFAULT 'unknown'", true, $rcnn_id); + } else { + db_execute("UPDATE audit_log SET request_status = outcome WHERE request_status = 'unknown'", true, $rcnn_id); + db_execute('ALTER TABLE audit_log DROP COLUMN outcome', true, $rcnn_id); + } + } elseif (!db_column_exists('audit_log', 'request_status', false, $rcnn_id)) { + db_execute("ALTER TABLE audit_log ADD COLUMN request_status varchar(20) NOT NULL DEFAULT 'unknown' AFTER action", true, $rcnn_id); + } + + db_execute("UPDATE audit_log SET request_status = CASE request_status + WHEN 'attempted' THEN 'started' + WHEN 'request_completed' THEN 'completed' + WHEN 'request_failed' THEN 'failed' + ELSE request_status END", true, $rcnn_id); db_execute("ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS external_status varchar(20) NOT NULL DEFAULT 'unknown' AFTER object_data", true, $rcnn_id); db_execute('ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS external_error varchar(1024) DEFAULT NULL AFTER external_status', true, $rcnn_id); } @@ -170,7 +200,7 @@ function audit_setup_table() { `page` varchar(40) DEFAULT NULL, `user_id` int(10) unsigned DEFAULT NULL, `action` varchar(20) DEFAULT NULL, - `outcome` varchar(20) NOT NULL DEFAULT 'unknown', + `request_status` varchar(20) NOT NULL DEFAULT 'unknown', `ip_address` varchar(40) DEFAULT NULL, `user_agent` varchar(256) DEFAULT NULL, `event_time` timestamp DEFAULT CURRENT_TIMESTAMP, diff --git a/tests/controller_security_test.php b/tests/controller_security_test.php index 2b549b4..5d9e382 100644 --- a/tests/controller_security_test.php +++ b/tests/controller_security_test.php @@ -23,7 +23,7 @@ $required_schema_fragments = array( 'api_plugin_register_realm(\'audit\', \'audit_manage.php\'', 'api_plugin_register_hook(\'audit\', \'replicate_out\'', - 'ADD COLUMN IF NOT EXISTS outcome', + 'request_status', 'ADD COLUMN IF NOT EXISTS external_status', 'ADD COLUMN IF NOT EXISTS external_error', 'SHOW CREATE TABLE $table', diff --git a/tests/security_functions_test.php b/tests/security_functions_test.php index 57a222d..89c1121 100644 --- a/tests/security_functions_test.php +++ b/tests/security_functions_test.php @@ -81,13 +81,13 @@ function audit_test_assert_same($expected, $actual, $message) { ); audit_test_assert_same( - 'request_completed', - audit_request_outcome(null, 302), + 'completed', + audit_request_status(null, 302), 'Successful redirects must finalize as completed requests.' ); audit_test_assert_same( - 'request_failed', - audit_request_outcome(array('type' => E_ERROR), 200), + 'failed', + audit_request_status(array('type' => E_ERROR), 200), 'Fatal errors must finalize as failed requests.' ); From 6d3afb2f30f80cba867e15b5cb2afd1bde642cd1 Mon Sep 17 00:00:00 2001 From: Sean Mancini Date: Fri, 24 Jul 2026 09:21:51 -0400 Subject: [PATCH 05/10] Support json for external logging This will make it easier for some SIEMS like splunk to ingest the log file --- CHANGELOG.md | 1 + README.md | 6 +++++- audit_functions.php | 36 +++++++++++++++++++++++++++++-- setup.php | 10 +++++++++ tests/security_functions_test.php | 23 ++++++++++++++++++++ 5 files changed, 73 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d24dcf6..5c62f10 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ --- develop --- +* feature: Add selectable text or JSON formats for external audit logging * feature: Rename outcome to request_status with started/completed/failed values * feature: Track and retry failed external audit-log delivery * security: Bound nested request depth, field counts, string sizes, and JSON parsing diff --git a/README.md b/README.md index 0de354d..4713134 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,11 @@ directory is named 'audit' and not 'plugin_audit'. Once this is done, you have to goto Configuration -> Settings -> Audit and define data retention and turn on auditing. -You can also enable file based logging for ingestion by Siem or Log analysis tools such as splunk +You can also enable file-based logging for ingestion by SIEM or log-analysis +tools such as Splunk. External records can be written as newline-delimited JSON +(one JSON object per line) or as single-line text using quoted `key="value"` +fields. Control characters in text values are escaped so every event remains on +one line. JSON is the default to preserve the format used by earlier releases. External file delivery is tracked on each database record. Failed appends are retried by the poller in batches and therefore have at-least-once delivery diff --git a/audit_functions.php b/audit_functions.php index 78c4739..3554098 100644 --- a/audit_functions.php +++ b/audit_functions.php @@ -240,6 +240,33 @@ function audit_json_decode($json, &$error = null) { } } +function audit_external_log_format($data, $format = 'json') { + if ($format === 'text') { + $fields = array(); + + foreach ($data as $name => $value) { + if (is_array($value) || is_object($value)) { + $value = audit_json_encode($value); + } elseif ($value === null) { + $value = ''; + } elseif (is_bool($value)) { + $value = $value ? 'true' : 'false'; + } + + $value = str_replace( + array('\\', "\r", "\n", "\t", '"'), + array('\\\\', '\r', '\n', '\t', '\"'), + (string) $value + ); + $fields[] = $name . '="' . $value . '"'; + } + + return implode(' ', $fields) . "\n"; + } + + return audit_json_encode($data) . "\n"; +} + function audit_csv_safe_cell($value) { $value = (string) $value; @@ -288,6 +315,9 @@ function audit_retry_external_logs() { return; } + $format = read_config_option('audit_log_external_format'); + $format = $format === 'text' ? 'text' : 'json'; + $events = db_fetch_assoc("SELECT * FROM audit_log WHERE external_status = 'failed' @@ -307,7 +337,7 @@ function audit_retry_external_logs() { 'object_data' => $event['object_data'] ); - $message = audit_json_encode($log_data) . "\n"; + $message = audit_external_log_format($log_data, $format); $delivery = audit_append_external_log($path, $message); audit_set_external_status($event['id'], $delivery['status'], $delivery['error']); @@ -419,6 +449,8 @@ function audit_config_insert() { $audit_log = read_config_option('audit_log_external_path'); $external_logging = read_config_option('audit_log_external') == 'on'; $external_status = $external_logging ? 'pending' : 'disabled'; + $external_format = read_config_option('audit_log_external_format'); + $external_format = $external_format === 'text' ? 'text' : 'json'; if (!defined('CACTI_PATH_BASE')) { $base = $config['base_path']; @@ -463,7 +495,7 @@ function audit_config_insert() { 'object_data' => $object_data ); - $log_msg = audit_json_encode($log_data) . "\n"; + $log_msg = audit_external_log_format($log_data, $external_format); $delivery = audit_append_external_log($audit_log, $log_msg); audit_set_external_status($audit_id, $delivery['status'], $delivery['error']); diff --git a/setup.php b/setup.php index 99e6082..537a0f3 100644 --- a/setup.php +++ b/setup.php @@ -334,6 +334,16 @@ function audit_config_settings() { 'method' => 'checkbox', 'default' => 'off' ), + 'audit_log_external_format' => array( + 'friendly_name' => __('External Audit Log Format', 'audit'), + 'description' => __('Select the output format for external audit log records.', 'audit'), + 'method' => 'drop_array', + 'default' => 'json', + 'array' => array( + 'text' => __('Text', 'audit'), + 'json' => __('JSON', 'audit') + ) + ), 'audit_log_external_path' => array( 'friendly_name' => __('External Audit Log Log file Path', 'audit'), 'description' => __('Enter the path to the external audit log file.', 'audit'), diff --git a/tests/security_functions_test.php b/tests/security_functions_test.php index 89c1121..bc879f9 100644 --- a/tests/security_functions_test.php +++ b/tests/security_functions_test.php @@ -91,6 +91,29 @@ function audit_test_assert_same($expected, $actual, $message) { 'Fatal errors must finalize as failed requests.' ); +$external_record = array( + 'event_time' => '2026-07-24 10:00:00', + 'action' => "Update\nDevice", + 'post' => array('id' => 42) +); +$json_record = audit_external_log_format($external_record, 'json'); +audit_test_assert_same("\n", substr($json_record, -1), 'JSON external records must end with a newline.'); +audit_test_assert_same( + $external_record, + json_decode(trim($json_record), true), + 'JSON external records must contain the complete event.' +); +audit_test_assert_same( + 'event_time="2026-07-24 10:00:00" action="Update\nDevice" post="{\"id\":42}"' . "\n", + audit_external_log_format($external_record, 'text'), + 'Text external records must be single-line key/value data with escaped values.' +); +audit_test_assert_same( + $json_record, + audit_external_log_format($external_record, 'unsupported'), + 'Unknown external formats must safely fall back to JSON.' +); + $temporary_log = tempnam(sys_get_temp_dir(), 'audit-test-'); $delivery = audit_append_external_log($temporary_log, "test-record\n"); audit_test_assert_same('delivered', $delivery['status'], 'A complete external log write must report delivery.'); From ab887ac6c87293cd878133392b9a459d69bfb230 Mon Sep 17 00:00:00 2001 From: Sean Mancini Date: Fri, 24 Jul 2026 09:25:21 -0400 Subject: [PATCH 06/10] fix json formatting --- README.md | 4 +++- audit_functions.php | 16 +++++++++++++--- tests/security_functions_test.php | 19 +++++++++++++++---- 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 4713134..a1f0a5a 100644 --- a/README.md +++ b/README.md @@ -33,7 +33,9 @@ You can also enable file-based logging for ingestion by SIEM or log-analysis tools such as Splunk. External records can be written as newline-delimited JSON (one JSON object per line) or as single-line text using quoted `key="value"` fields. Control characters in text values are escaped so every event remains on -one line. JSON is the default to preserve the format used by earlier releases. +one line. In JSON output, `post` and `object_data` are native nested structures, +not JSON-encoded strings. JSON is the default to preserve the format used by +earlier releases. External file delivery is tracked on each database record. Failed appends are retried by the poller in batches and therefore have at-least-once delivery diff --git a/audit_functions.php b/audit_functions.php index 3554098..73a12d6 100644 --- a/audit_functions.php +++ b/audit_functions.php @@ -219,8 +219,8 @@ function audit_redact_cli_arguments($arguments) { return $redacted; } -function audit_json_encode($data) { - $json = json_encode(audit_bound_log_data($data), JSON_INVALID_UTF8_SUBSTITUTE, 16); +function audit_json_encode($data, $options = 0) { + $json = json_encode(audit_bound_log_data($data), JSON_INVALID_UTF8_SUBSTITUTE | $options, 16); if ($json === false) { return json_encode(array('audit_encoding_error' => json_last_error_msg())); @@ -264,7 +264,17 @@ function audit_external_log_format($data, $format = 'json') { return implode(' ', $fields) . "\n"; } - return audit_json_encode($data) . "\n"; + foreach (array('post', 'object_data') as $name) { + if (isset($data[$name]) && is_string($data[$name])) { + $decoded = audit_json_decode($data[$name], $error); + + if ($error === null) { + $data[$name] = $decoded; + } + } + } + + return audit_json_encode($data, JSON_UNESCAPED_SLASHES) . "\n"; } function audit_csv_safe_cell($value) { diff --git a/tests/security_functions_test.php b/tests/security_functions_test.php index bc879f9..7bc3dea 100644 --- a/tests/security_functions_test.php +++ b/tests/security_functions_test.php @@ -94,20 +94,31 @@ function audit_test_assert_same($expected, $actual, $message) { $external_record = array( 'event_time' => '2026-07-24 10:00:00', 'action' => "Update\nDevice", - 'post' => array('id' => 42) + 'post' => '{"id":42}', + 'object_data' => '[]' ); $json_record = audit_external_log_format($external_record, 'json'); audit_test_assert_same("\n", substr($json_record, -1), 'JSON external records must end with a newline.'); audit_test_assert_same( - $external_record, + array( + 'event_time' => '2026-07-24 10:00:00', + 'action' => "Update\nDevice", + 'post' => array('id' => 42), + 'object_data' => array() + ), json_decode(trim($json_record), true), - 'JSON external records must contain the complete event.' + 'JSON external records must expose stored JSON fields as native structures.' ); audit_test_assert_same( - 'event_time="2026-07-24 10:00:00" action="Update\nDevice" post="{\"id\":42}"' . "\n", + 'event_time="2026-07-24 10:00:00" action="Update\nDevice" post="{\"id\":42}" object_data="[]"' . "\n", audit_external_log_format($external_record, 'text'), 'Text external records must be single-line key/value data with escaped values.' ); +audit_test_assert_same( + '{"post":"not-json"}' . "\n", + audit_external_log_format(array('post' => 'not-json'), 'json'), + 'Malformed stored JSON fields must remain available as strings.' +); audit_test_assert_same( $json_record, audit_external_log_format($external_record, 'unsupported'), From 6e27cc33a6524b3b81173d44f2f8ffe19d0658b3 Mon Sep 17 00:00:00 2001 From: Sean Mancini Date: Fri, 24 Jul 2026 10:12:01 -0400 Subject: [PATCH 07/10] expand auditing coverage feat: add normalized compliance audit event capture - add event and correlation UUIDs with integrity metadata - record actor, target, outcome, timing, and event classification - deliver finalized request outcomes to external log consumers - audit log views, searches, details, exports, and purges - capture logout and session-timeout events on Cacti 1.2.x - finalize CLI audit events and expand CSV exports - add schema migration, documentation, and tests --- CHANGELOG.md | 5 + INFO | 2 +- README.md | 17 ++ audit.php | 75 +++++++- audit_functions.php | 277 +++++++++++++++++++++++------ setup.php | 79 +++++++- tests/controller_security_test.php | 6 +- tests/security_functions_test.php | 34 ++++ 8 files changed, 431 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c62f10..8954bc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ --- develop --- +* feature: Add normalized compliance event identifiers, categories, actors, targets, outcomes, timing, and integrity metadata +* feature: Deliver finalized request outcomes to external log consumers +* feature: Audit audit-log views, searches, event detail access, exports, and purges +* feature: Capture Cacti 1.2.x logout and session-timeout events through the supported logout hook +* feature: Finalize captured CLI activity and make it available to external log delivery * feature: Add selectable text or JSON formats for external audit logging * feature: Rename outcome to request_status with started/completed/failed values * feature: Track and retry failed external audit-log delivery diff --git a/INFO b/INFO index 7fda835..6510278 100644 --- a/INFO +++ b/INFO @@ -21,7 +21,7 @@ [info] name = audit -version = 1.3 +version = 1.4 longname = Audit Plugin for Cacti author = The Cacti Group email = diff --git a/README.md b/README.md index a1f0a5a..b2bd0d8 100644 --- a/README.md +++ b/README.md @@ -41,6 +41,23 @@ External file delivery is tracked on each database record. Failed appends are retried by the poller in batches and therefore have at-least-once delivery semantics; downstream ingestion should deduplicate when necessary. +Version 1.4 records a stable event UUID and request correlation UUID on new +events. External records are written only after request finalization, so SIEM +consumers receive the final request status instead of the earlier transient +`started` state. Consumers should deduplicate on `event_uuid`. + +The normalized fields distinguish request processing from the result of the +requested Cacti operation. `request_status=completed` means that PHP request +processing completed without a fatal error or an HTTP error response. It does +not by itself prove that page-specific validation or database work succeeded. +`operation_outcome` remains `unknown` unless an authoritative Cacti 1.2.x hook +or plugin-owned operation supplies the result. + +The plugin also audits access to its own event list, searches, event details, +exports and purge operations. Logout and session-timeout events are captured +through Cacti's supported `logout_pre_session_destroy` hook. Database-level +changes, API activity and MFA events are outside the current Cacti 1.2.x scope. + ## Possible Bugs If you figure out this problem, see the Cacti forums! diff --git a/audit.php b/audit.php index 49afa4d..67e7efd 100644 --- a/audit.php +++ b/audit.php @@ -63,6 +63,13 @@ break; } + audit_record_event('audit.event.viewed', array( + 'event_category' => 'audit', + 'target_type' => 'audit_event', + 'target_id' => $data['event_uuid'] != '' ? $data['event_uuid'] : $data['id'], + 'details' => array('record_id' => $data['id']) + )); + $output = audit_render_event_details($data); echo $output; @@ -81,7 +88,10 @@ function audit_render_event_details($data) { $output .= '
' . __('IP Address:', 'audit') . ' ' . html_escape($data['ip_address']) . ''; $output .= '
' . __('Date:', 'audit') . ' ' . html_escape($data['event_time']) . ''; $output .= '
' . __('Action:', 'audit') . ' ' . html_escape($data['action']) . ''; + $output .= '
' . __('Event Type:', 'audit') . ' ' . html_escape($data['event_type']) . ''; + $output .= '
' . __('Event ID:', 'audit') . ' ' . html_escape($data['event_uuid']) . ''; $output .= '
' . __('Request Status:', 'audit') . ' ' . html_escape($data['request_status']) . ''; + $output .= '
' . __('Operation Outcome:', 'audit') . ' ' . html_escape($data['operation_outcome']) . ''; $output .= '
' . __('External Delivery:', 'audit') . ' ' . html_escape($data['external_status']) . ''; if ($data['external_error'] != '') { $output .= '
' . __('External Error:', 'audit') . ' ' . html_escape($data['external_error']) . ''; @@ -153,6 +163,13 @@ function audit_render_value($value) { function audit_purge() { db_execute('TRUNCATE TABLE audit_log'); + audit_record_event('audit.log.purged', array( + 'event_category' => 'audit', + 'severity' => 'warning', + 'action' => 'purge', + 'target_type' => 'audit_log' + )); + $_SESSION['audit_message'] = __('Audit Log Purged by %s', get_username($_SESSION['sess_user_id']), 'audit'); cacti_log('NOTE: Audit Log Purged by ' . get_username($_SESSION['sess_user_id']), false, 'WEBUI'); @@ -192,13 +209,25 @@ function audit_export_rows() { $sql_where", $sql_params); + audit_record_event('audit.log.exported', array( + 'event_category' => 'audit', + 'action' => 'export', + 'target_type' => 'audit_log', + 'details' => array( + 'row_count' => cacti_sizeof($events), + 'filter' => get_request_var('filter'), + 'event_page' => get_request_var('event_page'), + 'user_id' => get_request_var('user_id') + ) + )); + if (cacti_sizeof($events)) { header('Content-Disposition: attachment; filename=audit_export.csv'); header('Content-Type: text/csv; charset=UTF-8'); header('X-Content-Type-Options: nosniff'); $output = fopen('php://output', 'w'); - fputcsv($output, array('page', 'user_id', 'username', 'action', 'request_status', 'external_status', 'external_error', 'ip_address', 'user_agent', 'event_time', 'post'), ',', '"', ''); + fputcsv($output, array('event_uuid', 'correlation_id', 'event_type', 'event_category', 'severity', 'page', 'user_id', 'username', 'action', 'request_status', 'operation_outcome', 'outcome_reason', 'target_type', 'target_id', 'external_status', 'external_error', 'ip_address', 'user_agent', 'http_method', 'http_status', 'event_time', 'completed_time', 'duration_ms', 'integrity_hash', 'post', 'details'), ',', '"', ''); foreach($events as $event) { if ($event['action'] == 'cli') { @@ -208,19 +237,34 @@ function audit_export_rows() { $poster = is_array($post) ? json_encode($post, JSON_INVALID_UTF8_SUBSTITUTE) : $event['post']; } - fputcsv($output, array_map('audit_csv_safe_cell', array( - $event['page'], + fputcsv($output, array_map('audit_csv_safe_cell', array( + $event['event_uuid'], + $event['correlation_id'], + $event['event_type'], + $event['event_category'], + $event['severity'], + $event['page'], $event['user_id'], get_username($event['user_id']), $event['action'], - $event['request_status'], - $event['external_status'], + $event['request_status'], + $event['operation_outcome'], + $event['outcome_reason'], + $event['target_type'], + $event['target_id'], + $event['external_status'], $event['external_error'], $event['ip_address'], - $event['user_agent'], - $event['event_time'], - $poster - )), ',', '"', ''); + $event['user_agent'], + $event['http_method'], + $event['http_status'], + $event['event_time'], + $event['completed_time'], + $event['duration_ms'], + $event['integrity_hash'], + $poster, + $event['details'] + )), ',', '"', ''); } fclose($output); @@ -275,6 +319,19 @@ function audit_log() { global $item_rows; audit_process_request_vars(); + $has_filters = get_request_var('filter') != '' || + get_request_var('event_page') != '-1' || + (!isempty_request_var('user_id') && get_request_var('user_id') > '-1'); + audit_record_event($has_filters ? 'audit.log.searched' : 'audit.log.viewed', array( + 'event_category' => 'audit', + 'action' => $has_filters ? 'search' : 'view', + 'target_type' => 'audit_log', + 'details' => array( + 'filter' => get_request_var('filter'), + 'event_page' => get_request_var('event_page'), + 'user_id' => get_request_var('user_id') + ) + )); if (get_request_var('rows') == '-1') { $rows = read_config_option('num_rows_table'); diff --git a/audit_functions.php b/audit_functions.php index 73a12d6..882f50c 100644 --- a/audit_functions.php +++ b/audit_functions.php @@ -240,6 +240,84 @@ function audit_json_decode($json, &$error = null) { } } +function audit_uuid_v4() { + $bytes = random_bytes(16); + $bytes[6] = chr((ord($bytes[6]) & 0x0f) | 0x40); + $bytes[8] = chr((ord($bytes[8]) & 0x3f) | 0x80); + $hex = bin2hex($bytes); + + return substr($hex, 0, 8) . '-' . substr($hex, 8, 4) . '-' . + substr($hex, 12, 4) . '-' . substr($hex, 16, 4) . '-' . substr($hex, 20); +} + +function audit_request_correlation_id() { + static $correlation_id; + + if ($correlation_id === null) { + $correlation_id = audit_uuid_v4(); + } + + return $correlation_id; +} + +function audit_utc_time($microtime = null) { + $microtime = $microtime === null ? microtime(true) : $microtime; + $seconds = (int) $microtime; + $micros = (int) round(($microtime - $seconds) * 1000000); + + if ($micros >= 1000000) { + $seconds++; + $micros = 0; + } + + return gmdate('Y-m-d H:i:s', $seconds) . '.' . sprintf('%06d', $micros); +} + +function audit_event_integrity_hash($event) { + $material = array( + 'event_uuid' => $event['event_uuid'] ?? '', + 'correlation_id' => $event['correlation_id'] ?? '', + 'event_type' => $event['event_type'] ?? '', + 'user_id' => $event['user_id'] ?? 0, + 'action' => $event['action'] ?? '', + 'event_time' => $event['event_time'] ?? '', + 'operation_outcome'=> $event['operation_outcome'] ?? '', + 'target_type' => $event['target_type'] ?? '', + 'target_id' => $event['target_id'] ?? '', + 'details' => $event['details'] ?? '' + ); + + return hash('sha256', audit_json_encode($material, JSON_UNESCAPED_SLASHES)); +} + +function audit_event_type_for_request($page, $action) { + $page_name = preg_replace('/\.php$/', '', (string) $page); + $page_name = preg_replace('/[^a-z0-9_]+/i', '_', $page_name); + $verb = preg_replace('/[^a-z0-9_]+/i', '_', strtolower((string) $action)); + $verb = trim($verb, '_'); + + return 'cacti.' . ($page_name !== '' ? $page_name : 'request') . '.' . + ($verb !== '' && $verb !== 'none' ? $verb : 'submitted'); +} + +function audit_external_event_data($event) { + $fields = array( + 'id', 'event_uuid', 'correlation_id', 'event_type', 'event_category', + 'severity', 'actor_type', 'page', 'user_id', 'action', 'request_status', + 'operation_outcome', 'outcome_reason', 'target_type', 'target_id', + 'ip_address', 'user_agent', 'http_method', 'http_status', 'event_time', + 'completed_time', 'duration_ms', 'post', 'object_data', 'details', + 'previous_hash', 'integrity_hash' + ); + $data = array(); + + foreach ($fields as $field) { + $data[$field] = $event[$field] ?? null; + } + + return $data; +} + function audit_external_log_format($data, $format = 'json') { if ($format === 'text') { $fields = array(); @@ -264,7 +342,7 @@ function audit_external_log_format($data, $format = 'json') { return implode(' ', $fields) . "\n"; } - foreach (array('post', 'object_data') as $name) { + foreach (array('post', 'object_data', 'details') as $name) { if (isset($data[$name]) && is_string($data[$name])) { $decoded = audit_json_decode($data[$name], $error); @@ -310,9 +388,35 @@ function audit_append_external_log($path, $message) { function audit_set_external_status($id, $status, $error = '') { db_execute_prepared('UPDATE audit_log - SET external_status = ?, external_error = ? + SET external_status = ?, + external_error = ?, + external_attempts = external_attempts + 1, + external_last_attempt = UTC_TIMESTAMP(6), + external_delivered_time = CASE WHEN ? = "delivered" THEN UTC_TIMESTAMP(6) ELSE external_delivered_time END WHERE id = ?', - array($status, $error, $id)); + array($status, $error, $status, $id)); +} + +function audit_deliver_external_event($id) { + if (read_config_option('audit_log_external') != 'on') { + return; + } + + $event = db_fetch_row_prepared('SELECT * FROM audit_log WHERE id = ?', array($id)); + if (!cacti_sizeof($event) || $event['request_status'] == 'started') { + return; + } + + $path = read_config_option('audit_log_external_path'); + if ($path == '' || !is_file($path) || is_link($path)) { + audit_set_external_status($id, 'failed', 'Destination is not a regular file or is a symbolic link.'); + return; + } + + $format = read_config_option('audit_log_external_format') === 'text' ? 'text' : 'json'; + $message = audit_external_log_format(audit_external_event_data($event), $format); + $delivery = audit_append_external_log($path, $message); + audit_set_external_status($id, $delivery['status'], $delivery['error']); } function audit_retry_external_logs() { @@ -330,24 +434,13 @@ function audit_retry_external_logs() { $events = db_fetch_assoc("SELECT * FROM audit_log - WHERE external_status = 'failed' + WHERE external_status IN ('pending', 'failed') + AND request_status <> 'started' ORDER BY id LIMIT 100"); foreach ($events as $event) { - $log_data = array( - 'page' => $event['page'], - 'user_id' => $event['user_id'], - 'action' => $event['action'], - 'request_status' => $event['request_status'], - 'ip_address' => $event['ip_address'], - 'user_agent' => $event['user_agent'], - 'event_time' => $event['event_time'], - 'post' => $event['post'], - 'object_data' => $event['object_data'] - ); - - $message = audit_external_log_format($log_data, $format); + $message = audit_external_log_format(audit_external_event_data($event), $format); $delivery = audit_append_external_log($path, $message); audit_set_external_status($event['id'], $delivery['status'], $delivery['error']); @@ -368,16 +461,89 @@ function audit_request_status($error = null, $status_code = 200) { return 'completed'; } -function audit_finalize_request($id) { +function audit_finalize_request($id, $started_at = null) { $status_code = http_response_code(); $status_code = is_int($status_code) ? $status_code : 200; $request_status = audit_request_status(error_get_last(), $status_code); + $outcome = $request_status == 'failed' ? 'failure' : 'unknown'; + $duration_ms = $started_at === null ? null : max(0, (int) round((microtime(true) - $started_at) * 1000)); + $completed_time = audit_utc_time(); db_execute_prepared("UPDATE audit_log - SET request_status = ? + SET request_status = ?, + operation_outcome = CASE WHEN operation_outcome = 'unknown' THEN ? ELSE operation_outcome END, + http_status = ?, + completed_time = ?, + duration_ms = ? WHERE id = ? AND request_status = 'started'", - array($request_status, $id)); + array($request_status, $outcome, $status_code, $completed_time, $duration_ms, $id)); + + $event = db_fetch_row_prepared('SELECT * FROM audit_log WHERE id = ?', array($id)); + if (cacti_sizeof($event)) { + db_execute_prepared('UPDATE audit_log SET integrity_hash = ? WHERE id = ?', + array(audit_event_integrity_hash($event), $id)); + } + + audit_deliver_external_event($id); +} + +function audit_record_event($event_type, $options = array()) { + if (read_config_option('audit_enabled') != 'on') { + return 0; + } + + $event_uuid = audit_uuid_v4(); + $correlation_id = $options['correlation_id'] ?? audit_request_correlation_id(); + $user_id = $options['user_id'] ?? ($_SESSION['sess_user_id'] ?? 0); + $page = $options['page'] ?? basename($_SERVER['SCRIPT_NAME'] ?? 'cli'); + $event_suffix = strrchr($event_type, '.'); + $action = $options['action'] ?? ($event_suffix === false ? $event_type : substr($event_suffix, 1)); + $event_time = $options['event_time'] ?? audit_utc_time(); + $details = audit_json_encode(audit_redact_sensitive_data($options['details'] ?? array())); + $external = read_config_option('audit_log_external') == 'on'; + $ip_address = $options['ip_address'] ?? (function_exists('get_client_addr') ? get_client_addr() : ''); + $user_agent = $options['user_agent'] ?? ($_SERVER['HTTP_USER_AGENT'] ?? ''); + + db_execute_prepared('INSERT INTO audit_log ( + page, user_id, action, request_status, ip_address, user_agent, event_time, + post, object_data, external_status, event_uuid, correlation_id, event_type, + event_category, severity, actor_type, target_type, target_id, + operation_outcome, outcome_reason, http_method, http_status, + completed_time, duration_ms, details + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)', + array( + $page, $user_id, $action, 'completed', $ip_address, $user_agent, $event_time, + '{}', '[]', $external ? 'pending' : 'disabled', $event_uuid, $correlation_id, + $event_type, $options['event_category'] ?? 'security', + $options['severity'] ?? 'info', $options['actor_type'] ?? ($user_id ? 'user' : 'system'), + $options['target_type'] ?? null, isset($options['target_id']) ? (string) $options['target_id'] : null, + $options['operation_outcome'] ?? 'success', $options['outcome_reason'] ?? null, + $options['http_method'] ?? ($_SERVER['REQUEST_METHOD'] ?? null), + $options['http_status'] ?? null, $options['completed_time'] ?? $event_time, + $options['duration_ms'] ?? 0, $details + )); + + $id = db_fetch_insert_id(); + $event = db_fetch_row_prepared('SELECT * FROM audit_log WHERE id = ?', array($id)); + if (cacti_sizeof($event)) { + db_execute_prepared('UPDATE audit_log SET integrity_hash = ? WHERE id = ?', + array(audit_event_integrity_hash($event), $id)); + } + audit_deliver_external_event($id); + + return $id; +} + +function audit_logout_pre_session_destroy() { + $reason = get_nfilter_request_var('action', 'user'); + $type = $reason == 'timeout' ? 'authentication.session.expired' : 'authentication.logout'; + + audit_record_event($type, array( + 'event_category' => 'authentication', + 'action' => $reason == 'timeout' ? 'timeout' : 'logout', + 'details' => array('reason' => $reason) + )); } @@ -386,6 +552,7 @@ function audit_config_insert() { global $action, $config; if (audit_log_valid_event()) { + $started_at = microtime(true); /* prepare post */ $post = filter_input_array(INPUT_POST, FILTER_UNSAFE_RAW); $post = is_array($post) ? $post : array(); @@ -412,10 +579,11 @@ function audit_config_insert() { $drop_action = false; } + $target_id = $post['id'] ?? null; $post = audit_json_encode($post); $page = basename($_SERVER['SCRIPT_NAME']); $user_id = (isset($_SESSION['sess_user_id']) ? $_SESSION['sess_user_id'] : 0); - $event_time = date('Y-m-d H:i:s'); + $event_time = audit_utc_time($started_at); /* Retrieve IP address */ $ip_address = get_client_addr(); @@ -459,8 +627,6 @@ function audit_config_insert() { $audit_log = read_config_option('audit_log_external_path'); $external_logging = read_config_option('audit_log_external') == 'on'; $external_status = $external_logging ? 'pending' : 'disabled'; - $external_format = read_config_option('audit_log_external_format'); - $external_format = $external_format === 'text' ? 'text' : 'json'; if (!defined('CACTI_PATH_BASE')) { $base = $config['base_path']; @@ -468,11 +634,25 @@ function audit_config_insert() { $base = CACTI_PATH_BASE; } - db_execute_prepared('INSERT INTO audit_log (page, user_id, action, request_status, ip_address, user_agent, event_time, post, object_data, external_status) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)', - array($page, $user_id, $action, 'started', $ip_address, $user_agent, $event_time, $post, $object_data, $external_status)); + $event_uuid = audit_uuid_v4(); + $correlation_id = audit_request_correlation_id(); + $event_type = audit_event_type_for_request($page, $action); + $category = in_array($page, array('user_admin.php', 'user_group_admin.php'), true) ? 'identity_access' : 'configuration'; + db_execute_prepared('INSERT INTO audit_log ( + page, user_id, action, request_status, ip_address, user_agent, event_time, + post, object_data, external_status, event_uuid, correlation_id, event_type, + event_category, severity, actor_type, target_type, target_id, + operation_outcome, http_method + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)', + array( + $page, $user_id, $action, 'started', $ip_address, $user_agent, $event_time, + $post, $object_data, $external_status, $event_uuid, $correlation_id, + $event_type, $category, 'info', $user_id ? 'user' : 'system', + preg_replace('/\.php$/', '', $page), $target_id, 'unknown', + $_SERVER['REQUEST_METHOD'] ?? null + )); $audit_id = db_fetch_insert_id(); - register_shutdown_function('audit_finalize_request', $audit_id); + register_shutdown_function('audit_finalize_request', $audit_id, $started_at); if ($external_logging && $audit_log == '') { set_config_option('audit_log_external_path', $base . '/log/audit.log'); @@ -492,39 +672,20 @@ function audit_config_insert() { } } - if ($external_logging && $audit_log != '' && is_file($audit_log) && !is_link($audit_log)) { - $log_data = array( - 'page' => $page, - 'user_id' => $user_id, - 'action' => $action, - 'request_status' => 'started', - 'ip_address' => $ip_address, - 'user_agent' => $user_agent, - 'event_time' => $event_time, - 'post' => $post, - 'object_data' => $object_data - ); - - $log_msg = audit_external_log_format($log_data, $external_format); - $delivery = audit_append_external_log($audit_log, $log_msg); - audit_set_external_status($audit_id, $delivery['status'], $delivery['error']); - - if ($delivery['status'] != 'delivered') { - cacti_log(sprintf('ERROR: Unable to append a complete record to Audit Log file \'%s\': %s', $audit_log, $delivery['error']), false, 'AUDIT'); - } - } elseif ($external_logging && $audit_log != '') { + if ($external_logging && $audit_log != '' && (!is_file($audit_log) || is_link($audit_log))) { $error = 'Destination is not a regular file or is a symbolic link.'; audit_set_external_status($audit_id, 'failed', $error); cacti_log(sprintf('ERROR: Audit Log file \'%s\' is not a regular file or is a symbolic link.', $audit_log), false, 'AUDIT'); } } elseif (isset($_SERVER['argv']) && cacti_sizeof($_SERVER['argv'])) { + $started_at = microtime(true); $arguments = audit_redact_cli_arguments($_SERVER['argv']); $page = basename($arguments[0]); $user_id = 0; $action = 'cli'; $ip_address = getHostByName(php_uname('n')); $user_agent = get_current_user(); - $event_time = date('Y-m-d H:i:s'); + $event_time = audit_utc_time($started_at); $post = implode(' ', $arguments); /* don't insert poller records */ @@ -534,9 +695,21 @@ function audit_config_insert() { strpos($arguments[0], 'script_server.php') === false && strpos($arguments[0], '_process.php') === false) { - db_execute_prepared('INSERT INTO audit_log (page, user_id, action, request_status, ip_address, user_agent, event_time, post, external_status) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)', - array($page, $user_id, $action, 'started', $ip_address, $user_agent, $event_time, $post, 'not_applicable')); + $external_status = read_config_option('audit_log_external') == 'on' ? 'pending' : 'disabled'; + db_execute_prepared('INSERT INTO audit_log ( + page, user_id, action, request_status, ip_address, user_agent, event_time, + post, object_data, external_status, event_uuid, correlation_id, event_type, + event_category, severity, actor_type, target_type, target_id, + operation_outcome + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)', + array( + $page, $user_id, $action, 'started', $ip_address, $user_agent, + $event_time, $post, '[]', $external_status, audit_uuid_v4(), + audit_request_correlation_id(), 'cacti.cli.executed', 'system', + 'info', 'system', 'cli_command', $page, 'unknown' + )); + $audit_id = db_fetch_insert_id(); + register_shutdown_function('audit_finalize_request', $audit_id, $started_at); } } } diff --git a/setup.php b/setup.php index 537a0f3..5ccb4e3 100644 --- a/setup.php +++ b/setup.php @@ -32,6 +32,7 @@ function plugin_audit_install() { api_plugin_register_hook('audit', 'draw_navigation_text', 'audit_draw_navigation_text', 'setup.php'); api_plugin_register_hook('audit', 'utilities_array', 'audit_utilities_array', 'setup.php'); api_plugin_register_hook('audit', 'is_console_page', 'audit_is_console_page', 'setup.php'); + api_plugin_register_hook('audit', 'logout_pre_session_destroy', 'audit_logout_pre_session_destroy', 'setup.php'); /* hook for table replication */ api_plugin_register_hook('audit', 'replicate_out', 'audit_replicate_out', 'setup.php'); @@ -101,6 +102,7 @@ function audit_check_upgrade() { ELSE request_status END"); db_execute("ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS external_status varchar(20) NOT NULL DEFAULT 'unknown' AFTER object_data"); db_execute('ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS external_error varchar(1024) DEFAULT NULL AFTER external_status'); + audit_upgrade_event_schema(); db_execute_prepared('UPDATE plugin_config SET version = ? @@ -118,6 +120,7 @@ function audit_check_upgrade() { /* hook for table replication */ api_plugin_register_hook('audit', 'replicate_out', 'audit_replicate_out', 'setup.php', '1'); api_plugin_register_hook('audit', 'is_console_page', 'audit_is_console_page', 'setup.php', 1); + api_plugin_register_hook('audit', 'logout_pre_session_destroy', 'audit_logout_pre_session_destroy', 'setup.php', 1); api_plugin_register_realm('audit', 'audit_manage.php', __('Manage Cacti Audit Log', 'audit'), 1); } } @@ -165,6 +168,7 @@ function audit_replicate_out($data) { ELSE request_status END", true, $rcnn_id); db_execute("ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS external_status varchar(20) NOT NULL DEFAULT 'unknown' AFTER object_data", true, $rcnn_id); db_execute('ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS external_error varchar(1024) DEFAULT NULL AFTER external_status', true, $rcnn_id); + audit_upgrade_event_schema($rcnn_id); } return $data; @@ -208,18 +212,91 @@ function audit_setup_table() { `object_data` longblob, `external_status` varchar(20) NOT NULL DEFAULT 'unknown', `external_error` varchar(1024) DEFAULT NULL, + `event_uuid` char(36) DEFAULT NULL, + `correlation_id` char(36) DEFAULT NULL, + `event_type` varchar(100) NOT NULL DEFAULT 'cacti.request', + `event_category` varchar(40) NOT NULL DEFAULT 'configuration', + `severity` varchar(12) NOT NULL DEFAULT 'info', + `actor_type` varchar(20) NOT NULL DEFAULT 'user', + `target_type` varchar(64) DEFAULT NULL, + `target_id` varchar(128) DEFAULT NULL, + `operation_outcome` varchar(20) NOT NULL DEFAULT 'unknown', + `outcome_reason` varchar(255) DEFAULT NULL, + `http_method` varchar(10) DEFAULT NULL, + `http_status` smallint unsigned DEFAULT NULL, + `completed_time` datetime(6) DEFAULT NULL, + `duration_ms` bigint unsigned DEFAULT NULL, + `details` longblob, + `previous_hash` char(64) DEFAULT NULL, + `integrity_hash` char(64) DEFAULT NULL, + `external_attempts` int unsigned NOT NULL DEFAULT 0, + `external_last_attempt` datetime(6) DEFAULT NULL, + `external_delivered_time` datetime(6) DEFAULT NULL, PRIMARY KEY (`id`), KEY `user_id` (`user_id`), KEY `page` (`page`), KEY `ip_address` (`ip_address`), KEY `event_time` (`event_time`), - KEY `action` (`action`)) + KEY `action` (`action`), + UNIQUE KEY `event_uuid` (`event_uuid`), + KEY `correlation_id` (`correlation_id`), + KEY `event_type` (`event_type`), + KEY `operation_outcome` (`operation_outcome`), + KEY `external_status` (`external_status`)) ENGINE=InnoDB COMMENT='Audit Log for all GUI activities'"); return true; } +function audit_upgrade_event_schema($rcnn_id = false) { + $remote = $rcnn_id !== false; + $args = $remote ? array(true, $rcnn_id) : array(); + $columns = array( + "event_uuid char(36) DEFAULT NULL", + "correlation_id char(36) DEFAULT NULL", + "event_type varchar(100) NOT NULL DEFAULT 'cacti.request'", + "event_category varchar(40) NOT NULL DEFAULT 'configuration'", + "severity varchar(12) NOT NULL DEFAULT 'info'", + "actor_type varchar(20) NOT NULL DEFAULT 'user'", + "target_type varchar(64) DEFAULT NULL", + "target_id varchar(128) DEFAULT NULL", + "operation_outcome varchar(20) NOT NULL DEFAULT 'unknown'", + "outcome_reason varchar(255) DEFAULT NULL", + "http_method varchar(10) DEFAULT NULL", + "http_status smallint unsigned DEFAULT NULL", + "completed_time datetime(6) DEFAULT NULL", + "duration_ms bigint unsigned DEFAULT NULL", + "details longblob", + "previous_hash char(64) DEFAULT NULL", + "integrity_hash char(64) DEFAULT NULL", + "external_attempts int unsigned NOT NULL DEFAULT 0", + "external_last_attempt datetime(6) DEFAULT NULL", + "external_delivered_time datetime(6) DEFAULT NULL" + ); + + foreach ($columns as $definition) { + call_user_func_array('db_execute', array_merge( + array('ALTER TABLE audit_log ADD COLUMN IF NOT EXISTS ' . $definition), + $args + )); + } + + $indexes = array( + 'event_uuid' => array('UNIQUE INDEX', array('event_uuid')), + 'correlation_id' => array('INDEX', array('correlation_id')), + 'event_type' => array('INDEX', array('event_type')), + 'operation_outcome' => array('INDEX', array('operation_outcome')), + 'external_status' => array('INDEX', array('external_status')) + ); + + foreach ($indexes as $name => $definition) { + if (!db_index_exists('audit_log', $name, false, $remote ? $rcnn_id : false)) { + db_add_index('audit_log', $definition[0], $name, $definition[1], true, $remote ? $rcnn_id : false); + } + } +} + function plugin_audit_version() { global $config; $info = parse_ini_file($config['base_path'] . '/plugins/audit/INFO', true); diff --git a/tests/controller_security_test.php b/tests/controller_security_test.php index 5d9e382..0f38d6d 100644 --- a/tests/controller_security_test.php +++ b/tests/controller_security_test.php @@ -27,7 +27,11 @@ 'ADD COLUMN IF NOT EXISTS external_status', 'ADD COLUMN IF NOT EXISTS external_error', 'SHOW CREATE TABLE $table', - 'audit_retry_external_logs()' + 'audit_retry_external_logs()', + 'logout_pre_session_destroy', + 'event_uuid char(36)', + 'operation_outcome', + 'external_attempts' ); foreach ($required_schema_fragments as $fragment) { diff --git a/tests/security_functions_test.php b/tests/security_functions_test.php index 7bc3dea..a87b1b3 100644 --- a/tests/security_functions_test.php +++ b/tests/security_functions_test.php @@ -91,6 +91,40 @@ function audit_test_assert_same($expected, $actual, $message) { 'Fatal errors must finalize as failed requests.' ); +$uuid = audit_uuid_v4(); +if (!preg_match('/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/', $uuid)) { + fwrite(STDERR, 'Event identifiers must be RFC 4122 version 4 UUIDs.' . PHP_EOL); + exit(1); +} + +audit_test_assert_same( + 'cacti.user_admin.save', + audit_event_type_for_request('user_admin.php', 'Save'), + 'Request event types must be normalized for downstream consumers.' +); +audit_test_assert_same( + 'cacti.host.submitted', + audit_event_type_for_request('host.php', 'none'), + 'Requests without a specific action must use the submitted event verb.' +); + +$hash_event = array( + 'event_uuid' => $uuid, + 'correlation_id' => audit_uuid_v4(), + 'event_type' => 'cacti.test.completed', + 'user_id' => 1, + 'action' => 'test', + 'event_time' => '2026-07-24 10:00:00', + 'operation_outcome' => 'success', + 'details' => '{}' +); +$first_hash = audit_event_integrity_hash($hash_event); +$hash_event['operation_outcome'] = 'failure'; +if ($first_hash === audit_event_integrity_hash($hash_event)) { + fwrite(STDERR, 'Integrity hashes must change when protected event fields change.' . PHP_EOL); + exit(1); +} + $external_record = array( 'event_time' => '2026-07-24 10:00:00', 'action' => "Update\nDevice", From d3fb6c5c2237975cae7ca302491e58cdee4b5d95 Mon Sep 17 00:00:00 2001 From: Sean Mancini Date: Fri, 24 Jul 2026 11:35:01 -0400 Subject: [PATCH 08/10] remove file --- review.md | 326 ------------------------------------------------------ 1 file changed, 326 deletions(-) delete mode 100644 review.md diff --git a/review.md b/review.md deleted file mode 100644 index a24331c..0000000 --- a/review.md +++ /dev/null @@ -1,326 +0,0 @@ -# Audit Plugin Code Review - -> Remediation update (branch `code_audit`): the implementation now addresses -> AUD-001 through AUD-010, adds focused security-helper coverage for AUD-011, -> and bumps the plugin schema/version to 1.3. The findings below describe the -> pre-remediation code that was reviewed and are retained as the audit record. -> Full browser/database integration coverage is still recommended before release. -> Generic requests are finalized as completed or failed, but operation-specific -> success cannot be proven without corresponding completion hooks in each Cacti -> page. Request collection uses recursive redaction and strict size/depth bounds; -> converting every supported page to a field allowlist remains a future, -> compatibility-sensitive change. -> Request lifecycle state is stored in `request_status` using `started`, -> `completed`, and `failed`; it is intentionally distinct from operation-level -> success. - -## Executive summary - -The plugin is small and readable, uses prepared statements for its primary insert and object lookups, and passes PHP syntax checks. However, it should not be released in its current form. The audit found two high-severity security defects, several confidentiality and audit-integrity weaknesses, broken remote-replication code, and no meaningful automated coverage for the sensitive paths. - -The highest priorities are: - -1. Escape every value rendered from `audit_log`. -2. Make purge a CSRF-protected POST operation with explicit authorization. -3. Replace the current credential redaction with recursive, allowlist-oriented collection, including CLI arguments. -4. Produce exports with a real CSV writer and neutralize spreadsheet formulas. -5. Define whether records represent attempts or successful changes and record that outcome accurately. - -## Scope and methodology - -Reviewed: - -- `.github/copilot-instructions.md` -- `setup.php` -- `audit.php` -- `audit_functions.php` -- `js/functions.js` -- `INFO`, `README.md`, `CHANGELOG.md` -- `.github/workflows/plugin-ci-workflow.yml` -- localization build assets and repository layout - -The review traced install/upgrade/uninstall, hook registration, GUI and CLI event capture, database writes and reads, record-detail rendering, filtering, export, purge, retention, external-file logging, and remote replication. Cacti core helpers in the adjacent Cacti checkout were consulted to verify authorization, CSRF, sorting, validation, and HTML helper behavior. - -Validation performed: - -```text -find . -name '*.php' -print0 | xargs -0 -n1 php -l -``` - -Result: every PHP file passed syntax validation. - -No plugin unit or functional test suite is present. The CI workflow installs the plugin, checks syntax, runs the poller, and asserts that at least one CLI audit row exists; it does not exercise the web UI or any negative/security cases. - -## Findings - -### AUD-001 — High — Stored XSS in audit detail and list views - -**Evidence** - -- `audit.php:54-60` directly concatenates CLI record fields into HTML. -- `audit.php:77-81` directly concatenates page, username, IP address, date, and action into HTML. -- `audit.php:95-104` directly renders request field names and values. -- `audit.php:121-123` embeds JSON record data inside `
` without HTML escaping.
-- `audit.php:432-448` passes database values such as `user_agent`, username, page, IP, and action to `form_selectable_cell()`. Cacti's `form_selectable_cell()` does not escape its content; `form_selectable_ecell()` is the escaping variant.
-- `audit_functions.php:131-158` stores attacker-influenced request data, and `audit_functions.php:167` stores the request's `User-Agent`.
-
-**Impact**
-
-An authenticated user who can cause an audited request can persist HTML or JavaScript in a request value or user-agent string. When an administrator with the audit realm views the list or hovers over the event, that content is inserted into the DOM. This can execute script in the administrator's Cacti session and may permit administrative account compromise.
-
-**Recommendation**
-
-- Escape all scalar values at the final HTML output boundary with `html_escape()`/`htmlspecialchars(..., ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8')`.
-- Recursively render arrays and objects as escaped text, never as markup.
-- Use `form_selectable_ecell()` for plain database values. Keep `form_selectable_cell()` only where the plugin itself constructs trusted markup, and escape the interpolated action inside that markup.
-- Return detail data as JSON and construct text nodes client-side, or keep the HTML endpoint but apply centralized contextual escaping.
-- Add regression tests containing `