Security: fix critical vulnerabilities and harden module

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 <qwen-coder@alibabacloud.com>

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}"
+# Use /var/run for production (more secure than /tmp)
-LOG_FILE="/tmp/mod_reqin_log_test.log"
+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

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)
+    # Set permissions (owner and group only, not world-writable)
-    os.chmod(socket_path, 0o666)
+    # Apache user must be in the socket's group to connect
+    os.chmod(socket_path, 0o660)
     
     return server
 

scripts/test_unix_socket.sh

@@ -10,8 +10,9 @@
 
 set -e
 
-SOCKET_PATH="/tmp/mod_reqin_log_test.sock"
+# Use /var/run for production (more secure than /tmp)
-LOG_OUTPUT="/tmp/mod_reqin_log_output.jsonl"
+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
 

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;
@@ -139,10 +159,17 @@ static void *reqin_log_create_server_conf(apr_pool_t *pool, server_rec *s)
     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,
+                ap_log_error(APLOG_MARK, APLOG_ERR, 0, s,
-                             MOD_REQIN_LOG_NAME ": Unix socket connect failed: %s", cfg->socket_path);
+                             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;
 }
 
@@ -392,37 +468,50 @@ static int ensure_connected(reqin_log_config_t *cfg, reqin_log_child_state_t *st
 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,
+                    ap_log_error(APLOG_MARK, APLOG_ERR, 0, s,
-                                 MOD_REQIN_LOG_NAME ": Unix socket write failed: %s", strerror(err));
+                                 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);

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