From e44059865b87639a9459126b633745565260a015 Mon Sep 17 00:00:00 2001 From: Jacquin Antoine Date: Thu, 26 Feb 2026 23:37:30 +0100 Subject: [PATCH] Security: fix critical vulnerabilities and harden module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security fixes: #1 Buffer overflow: Validate socket path length against sun_path limit - Add MAX_SOCKET_PATH_LEN constant - Reject paths >= 108 bytes before snprintf #2,#3 NULL pointer dereference: Add NULL checks - r->connection->local_ip: use conditional append - r->protocol: fallback to "UNKNOWN" if NULL #4 Sensitive headers blacklist: Prevent credential leakage - Add DEFAULT_SENSITIVE_HEADERS[] blacklist - Block: Authorization, Cookie, Set-Cookie, X-Api-Key, etc. - Log skipped headers at DEBUG level only #5 Memory exhaustion DoS: Add MAX_JSON_SIZE limit (64KB) - Check buffer size before adding headers - Truncate header list if limit reached #6 Socket permissions: Change 0o666 → 0o660 - Owner and group only (not world-writable) - Apache user must be in socket's group #7 Race condition: Add mutex for FD access in worker/event MPMs - apr_thread_mutex_t protects socket_fd - FD_MUTEX_LOCK/UNLOCK macros - Created in reqin_log_create_server_conf() #8 Timestamp overflow: Document 2262 limitation - Add comment explaining apr_time_t limits - Safe until ~2262 (uint64 nanoseconds) #9 Error logging verbosity: Reduce information disclosure - APLOG_ERR: Generic messages only - APLOG_DEBUG: Detailed error information #10 Socket path security: Move from /tmp to /var/run - Update socket_consumer.py, test scripts - Use environment variable MOD_REQIN_LOG_SOCKET - More secure default location Files modified: - src/mod_reqin_log.c: All security fixes - scripts/socket_consumer.py: Permissions, path - scripts/run_integration_tests.sh: Path security - scripts/test_unix_socket.sh: Path security - tests/integration/test_integration.py: Path security Co-authored-by: Qwen-Coder --- scripts/run_integration_tests.sh | 5 +- scripts/socket_consumer.py | 8 +- scripts/test_unix_socket.sh | 5 +- src/mod_reqin_log.c | 155 +++++++++++++++++++++++--- tests/integration/test_integration.py | 3 +- 5 files changed, 151 insertions(+), 25 deletions(-) diff --git a/scripts/run_integration_tests.sh b/scripts/run_integration_tests.sh index d88fce7..f954ba4 100755 --- a/scripts/run_integration_tests.sh +++ b/scripts/run_integration_tests.sh @@ -11,8 +11,9 @@ set -e SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -SOCKET_PATH="${SOCKET_PATH:-/tmp/mod_reqin_log.sock}" -LOG_FILE="/tmp/mod_reqin_log_test.log" +# Use /var/run for production (more secure than /tmp) +SOCKET_PATH="${SOCKET_PATH:-/var/run/mod_reqin_log.sock}" +LOG_FILE="${LOG_FILE:-/var/log/mod_reqin_log_test.log}" APACHE_URL="${APACHE_URL:-http://localhost:8080}" # Colors for output diff --git a/scripts/socket_consumer.py b/scripts/socket_consumer.py index c9286ed..959d810 100755 --- a/scripts/socket_consumer.py +++ b/scripts/socket_consumer.py @@ -22,7 +22,8 @@ import argparse from datetime import datetime # Default socket path -DEFAULT_SOCKET_PATH = "/tmp/mod_reqin_log.sock" +# Use /var/run for production (more secure than /tmp) +DEFAULT_SOCKET_PATH = os.environ.get("MOD_REQIN_LOG_SOCKET", "/var/run/mod_reqin_log.sock") # Global flag for graceful shutdown shutdown_requested = False @@ -76,8 +77,9 @@ def create_socket(socket_path): server.bind(socket_path) server.listen(5) - # Set permissions (allow Apache to connect) - os.chmod(socket_path, 0o666) + # Set permissions (owner and group only, not world-writable) + # Apache user must be in the socket's group to connect + os.chmod(socket_path, 0o660) return server diff --git a/scripts/test_unix_socket.sh b/scripts/test_unix_socket.sh index 1cbce61..5073f42 100755 --- a/scripts/test_unix_socket.sh +++ b/scripts/test_unix_socket.sh @@ -10,8 +10,9 @@ set -e -SOCKET_PATH="/tmp/mod_reqin_log_test.sock" -LOG_OUTPUT="/tmp/mod_reqin_log_output.jsonl" +# Use /var/run for production (more secure than /tmp) +SOCKET_PATH="${SOCKET_PATH:-/var/run/mod_reqin_log_test.sock}" +LOG_OUTPUT="${LOG_OUTPUT:-/var/log/mod_reqin_log_output.jsonl}" APACHE_PORT="${APACHE_PORT:-8080}" TIMEOUT=30 diff --git a/src/mod_reqin_log.c b/src/mod_reqin_log.c index 70b10a9..872abd0 100644 --- a/src/mod_reqin_log.c +++ b/src/mod_reqin_log.c @@ -27,6 +27,24 @@ #define MOD_REQIN_LOG_NAME "mod_reqin_log" +/* Maximum Unix socket path length (sun_path is typically 108 bytes) */ +#define MAX_SOCKET_PATH_LEN (sizeof(((struct sockaddr_un *)0)->sun_path) - 1) + +/* Default sensitive headers blacklist - prevents accidental logging of credentials */ +static const char *const DEFAULT_SENSITIVE_HEADERS[] = { + "Authorization", + "Cookie", + "Set-Cookie", + "X-Api-Key", + "X-Auth-Token", + "Proxy-Authorization", + "WWW-Authenticate", + NULL +}; + +/* Maximum JSON log line size (64KB) - prevents memory exhaustion DoS */ +#define MAX_JSON_SIZE (64 * 1024) + /* Default configuration values */ #define DEFAULT_MAX_HEADERS 10 #define DEFAULT_MAX_HEADER_VALUE_LEN 256 @@ -52,9 +70,11 @@ typedef struct { apr_pool_t *pool; } dynbuf_t; -/* Per-child process state - stored in server config */ +/* Per-child process state - stored in server config + * Includes mutex for thread safety in worker/event MPMs */ typedef struct { int socket_fd; + apr_thread_mutex_t *fd_mutex; /* Protects socket_fd from concurrent access */ apr_time_t last_connect_attempt; apr_time_t last_error_report; int connect_failed; @@ -130,19 +150,26 @@ static void *reqin_log_create_server_conf(apr_pool_t *pool, server_rec *s) { (void)s; reqin_log_server_conf_t *srv_conf = apr_pcalloc(pool, sizeof(reqin_log_server_conf_t)); - + srv_conf->config = apr_pcalloc(pool, sizeof(reqin_log_config_t)); srv_conf->config->headers = apr_array_make(pool, 5, sizeof(const char *)); srv_conf->config->max_headers = DEFAULT_MAX_HEADERS; srv_conf->config->max_header_value_len = DEFAULT_MAX_HEADER_VALUE_LEN; srv_conf->config->reconnect_interval = DEFAULT_RECONNECT_INTERVAL; srv_conf->config->error_report_interval = DEFAULT_ERROR_REPORT_INTERVAL; - + srv_conf->child_state.socket_fd = -1; + srv_conf->child_state.fd_mutex = NULL; srv_conf->child_state.last_connect_attempt = 0; srv_conf->child_state.last_error_report = 0; srv_conf->child_state.connect_failed = 0; - + + /* Create mutex for thread-safe socket FD access in worker/event MPMs */ + if (apr_thread_mutex_create(&srv_conf->child_state.fd_mutex, + APR_THREAD_MUTEX_DEFAULT, pool) != APR_SUCCESS) { + srv_conf->child_state.fd_mutex = NULL; + } + return srv_conf; } @@ -331,6 +358,37 @@ static const char *cmd_set_error_report_interval(cmd_parms *cmd, void *dummy, co /* ============== Socket Functions ============== */ +/** + * Check if a header name is in the sensitive headers blacklist. + * Returns 1 if header should be excluded, 0 otherwise. + */ +static int is_sensitive_header(const char *name) +{ + if (name == NULL) { + return 0; + } + + for (int i = 0; DEFAULT_SENSITIVE_HEADERS[i] != NULL; i++) { + if (strcasecmp(name, DEFAULT_SENSITIVE_HEADERS[i]) == 0) { + return 1; + } + } + return 0; +} + +/* Mutex helper macros for thread-safe socket FD access */ +#define FD_MUTEX_LOCK(state) do { \ + if ((state)->fd_mutex) { \ + apr_thread_mutex_lock((state)->fd_mutex); \ + } \ +} while(0) + +#define FD_MUTEX_UNLOCK(state) do { \ + if ((state)->fd_mutex) { \ + apr_thread_mutex_unlock((state)->fd_mutex); \ + } \ +} while(0) + static int try_connect(reqin_log_config_t *cfg, reqin_log_child_state_t *state, server_rec *s) { apr_time_t now = apr_time_now(); @@ -343,11 +401,25 @@ static int try_connect(reqin_log_config_t *cfg, reqin_log_child_state_t *state, state->last_connect_attempt = now; + /* Validate socket path length before use */ + if (strlen(cfg->socket_path) >= MAX_SOCKET_PATH_LEN) { + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + MOD_REQIN_LOG_NAME ": Configuration error - socket path too long"); + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, + MOD_REQIN_LOG_NAME ": Path length %zu exceeds maximum %zu", + strlen(cfg->socket_path), MAX_SOCKET_PATH_LEN); + return -1; + } + + FD_MUTEX_LOCK(state); if (state->socket_fd < 0) { state->socket_fd = socket(AF_UNIX, SOCK_STREAM, 0); if (state->socket_fd < 0) { - ap_log_error(APLOG_MARK, APLOG_ERR, errno, s, + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, MOD_REQIN_LOG_NAME ": Failed to create socket"); + ap_log_error(APLOG_MARK, APLOG_DEBUG, errno, s, + MOD_REQIN_LOG_NAME ": Socket creation error detail"); + FD_MUTEX_UNLOCK(state); return -1; } @@ -367,10 +439,13 @@ static int try_connect(reqin_log_config_t *cfg, reqin_log_child_state_t *state, close(state->socket_fd); state->socket_fd = -1; state->connect_failed = 1; + FD_MUTEX_UNLOCK(state); if ((now - state->last_error_report) >= apr_time_from_sec(cfg->error_report_interval)) { - ap_log_error(APLOG_MARK, APLOG_ERR, err, s, - MOD_REQIN_LOG_NAME ": Unix socket connect failed: %s", cfg->socket_path); + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + MOD_REQIN_LOG_NAME ": Unix socket connection failed"); + ap_log_error(APLOG_MARK, APLOG_DEBUG, err, s, + MOD_REQIN_LOG_NAME ": Connect failed at %s: %s", cfg->socket_path, strerror(err)); state->last_error_report = now; } return -1; @@ -378,6 +453,7 @@ static int try_connect(reqin_log_config_t *cfg, reqin_log_child_state_t *state, } state->connect_failed = 0; + FD_MUTEX_UNLOCK(state); return 0; } @@ -389,40 +465,53 @@ static int ensure_connected(reqin_log_config_t *cfg, reqin_log_child_state_t *st return try_connect(cfg, state, s); } -static int write_to_socket(const char *data, apr_size_t len, server_rec *s, +static int write_to_socket(const char *data, apr_size_t len, server_rec *s, reqin_log_config_t *cfg, reqin_log_child_state_t *state) { - if (state->socket_fd < 0) { + int fd; + int result = -1; + + FD_MUTEX_LOCK(state); + fd = state->socket_fd; + if (fd < 0) { + FD_MUTEX_UNLOCK(state); return -1; } apr_size_t total_written = 0; while (total_written < len) { - ssize_t n = write(state->socket_fd, data + total_written, len - total_written); + ssize_t n = write(fd, data + total_written, len - total_written); if (n < 0) { int err = errno; if (err == EAGAIN || err == EWOULDBLOCK) { + FD_MUTEX_UNLOCK(state); return -1; } if (err == EPIPE || err == ECONNRESET) { - close(state->socket_fd); + close(fd); state->socket_fd = -1; state->connect_failed = 1; + FD_MUTEX_UNLOCK(state); apr_time_t now = apr_time_now(); if ((now - state->last_error_report) >= apr_time_from_sec(cfg->error_report_interval)) { - ap_log_error(APLOG_MARK, APLOG_ERR, err, s, - MOD_REQIN_LOG_NAME ": Unix socket write failed: %s", strerror(err)); + ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, + MOD_REQIN_LOG_NAME ": Unix socket write failed, connection lost"); + ap_log_error(APLOG_MARK, APLOG_DEBUG, err, s, + MOD_REQIN_LOG_NAME ": Write error detail: %s", strerror(err)); state->last_error_report = now; } return -1; } + FD_MUTEX_UNLOCK(state); return -1; } total_written += n; } - return 0; + result = 0; + FD_MUTEX_UNLOCK(state); + return result; } /* ============== Request Logging Functions ============== */ @@ -461,7 +550,13 @@ static void log_request(request_rec *r, reqin_log_config_t *cfg, reqin_log_child format_iso8601(&buf, r->request_time); dynbuf_append(&buf, "\",", 2); - /* timestamp */ + /* timestamp (nanoseconds since epoch) + * Note: apr_time_now() returns microseconds. Multiplying by 1000 gives nanoseconds. + * LIMITATION: apr_time_t is 64-bit microseconds. Max value ~9.22e18 microseconds. + * In nanoseconds, this overflows around year 2262. However, practical overflow of + * the JSON integer field (uint64) occurs at ~1.84e19 nanoseconds = year 2262. + * Current implementation is safe until ~2262. + */ apr_time_t now = apr_time_now(); apr_uint64_t ns = (apr_uint64_t)now * 1000; char ts_buf[32]; @@ -486,7 +581,9 @@ static void log_request(request_rec *r, reqin_log_config_t *cfg, reqin_log_child /* dst_ip */ dynbuf_append(&buf, "\"dst_ip\":\"", 10); - dynbuf_append(&buf, r->connection->local_ip, -1); + if (r->connection->local_ip) { + dynbuf_append(&buf, r->connection->local_ip, -1); + } dynbuf_append(&buf, "\",", 2); /* dst_port */ @@ -516,9 +613,16 @@ static void log_request(request_rec *r, reqin_log_config_t *cfg, reqin_log_child /* http_version */ dynbuf_append(&buf, "\"http_version\":\"", 16); - dynbuf_append(&buf, r->protocol, -1); + dynbuf_append(&buf, r->protocol ? r->protocol : "UNKNOWN", -1); dynbuf_append(&buf, "\"", 1); + /* Check buffer size before adding headers to prevent memory exhaustion */ + if (buf.len >= MAX_JSON_SIZE) { + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, + MOD_REQIN_LOG_NAME ": JSON buffer size limit reached before headers"); + return; + } + /* headers - flat structure at same level as other fields */ if (cfg->headers && cfg->headers->nelts > 0) { int header_count = 0; @@ -527,9 +631,26 @@ static void log_request(request_rec *r, reqin_log_config_t *cfg, reqin_log_child for (int i = 0; i < cfg->headers->nelts && header_count < max_to_log; i++) { const char *header_name = header_names[i]; + + /* Skip sensitive headers to prevent credential leakage */ + if (is_sensitive_header(header_name)) { + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, + MOD_REQIN_LOG_NAME ": Skipping sensitive header: %s", header_name); + continue; + } + const char *header_value = get_header(r, header_name); if (header_value != NULL) { + /* Check if adding this header would exceed size limit */ + apr_size_t header_contrib = 9 + strlen(header_name) + 3 + + strlen(header_value) + 1; + if (buf.len + header_contrib >= MAX_JSON_SIZE) { + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, + MOD_REQIN_LOG_NAME ": JSON size limit reached, truncating headers"); + break; + } + dynbuf_append(&buf, ",\"header_", 9); append_json_string(&buf, header_name); dynbuf_append(&buf, "\":\"", 3); diff --git a/tests/integration/test_integration.py b/tests/integration/test_integration.py index a19e089..d1c2554 100644 --- a/tests/integration/test_integration.py +++ b/tests/integration/test_integration.py @@ -23,7 +23,8 @@ from datetime import datetime from http.server import HTTPServer, BaseHTTPRequestHandler # Default paths -DEFAULT_SOCKET_PATH = "/tmp/mod_reqin_log_test.sock" +# Use /var/run for production (more secure than /tmp) +DEFAULT_SOCKET_PATH = os.environ.get("MOD_REQIN_LOG_SOCKET", "/var/run/mod_reqin_log_test.sock") DEFAULT_APACHE_URL = "http://localhost:8080" # Test results