Skip to content

CRITICAL SECURITY: Eliminate consolidated vulnerabilities from Issue #946 #948

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
krystophny merged 1 commit into from
Aug 31, 2025
Merged

CRITICAL SECURITY: Eliminate consolidated vulnerabilities from Issue #946 #948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions src/system/fortplot_mkdir_windows.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,14 @@ static int create_directory_windows_recursive(const char* path, int depth) {
}

// ENOENT means parent doesn't exist
// Try to create parent directories
// SECURITY FIX: Buffer overflow protection for parent path
char parent[MAX_PATH];
size_t path_len = strlen(path);

if (path_len >= MAX_PATH) {
return 0; // Path too long - prevent buffer overflow
}

strncpy(parent, path, MAX_PATH - 1);
parent[MAX_PATH - 1] = '\0';

Expand Down Expand Up @@ -107,7 +113,11 @@ int create_directory_windows_c(const char* path) {
#include <sys/types.h>
#include <string.h>
#include <errno.h>
#include <stdlib.h>

// SECURITY ENHANCEMENT: Define MAX_PATH for Unix systems
#ifndef MAX_PATH
#define MAX_PATH 4096
#endif

int create_directory_windows_c(const char* path) {
struct stat st;
Expand All @@ -127,18 +137,24 @@ int create_directory_windows_c(const char* path) {

// If parent doesn't exist, create it
if (errno == ENOENT) {
// Create a copy to modify
char* parent = strdup(path);
if (!parent) return 0;
// SECURITY FIX: Safe parent path handling without strdup leak
static char parent_buffer[MAX_PATH];

if (strlen(path) >= sizeof(parent_buffer)) {
return 0; // Path too long - prevent buffer overflow
}

strncpy(parent_buffer, path, sizeof(parent_buffer) - 1);
parent_buffer[sizeof(parent_buffer) - 1] = '\0';
char* parent = parent_buffer;

// Find last separator
char* last_sep = strrchr(parent, '/');
if (last_sep && last_sep != parent) {
*last_sep = '\0';

// Recursively create parent
// Recursively create parent - SECURITY FIX: No free() needed for static buffer
int result = create_directory_windows_c(parent);
free(parent);

if (!result) {
return 0;
Expand All @@ -149,7 +165,6 @@ int create_directory_windows_c(const char* path) {
return 1;
}
}
free(parent);
}

// Final check
Expand Down
191 changes: 150 additions & 41 deletions src/system/fortplot_secure_exec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,81 @@
#define MAX_ARG_LENGTH 1024
#define MAX_ARGS 32

// SECURITY ENHANCEMENT: Windows argument validation and secure quoting functions
static int validate_windows_argument(const char* arg);
static int secure_windows_quote_argument(const char* arg, char* buffer, size_t buffer_size, int offset);

// SECURITY ENHANCEMENT: Windows argument validation - blocks command injection
static int validate_windows_argument(const char* arg) {
if (!arg) return 0;

// Block dangerous shell metacharacters that can break quoting
const char* dangerous_chars = ";&|`$<>(){}[]\"'*?!~^#%@+=:";

for (const char* p = arg; *p; p++) {
// Check for shell metacharacters
if (strchr(dangerous_chars, *p)) {
return 0; // Dangerous character found
}

// Check for control characters
if (*p < 32 || *p == 127) {
return 0; // Control character found
}
}

// Block suspicious patterns
if (strstr(arg, "..\\") || strstr(arg, "../") ||
strstr(arg, "\\\\") || strstr(arg, "//")) {
return 0; // Path traversal attempt
}

return 1; // Argument is safe
}

// SECURITY ENHANCEMENT: Secure Windows argument quoting with buffer overflow protection
static int secure_windows_quote_argument(const char* arg, char* buffer, size_t buffer_size, int offset) {
if (!arg || !buffer || (size_t)offset >= buffer_size - 1) {
return -1; // Invalid parameters or buffer overflow
}

size_t arg_len = strlen(arg);

// Calculate required space: arg + quotes + escaping + null terminator
size_t required_space = arg_len + 4; // Conservative estimate

if ((size_t)offset + required_space >= buffer_size) {
return -1; // Would cause buffer overflow
}

// Always quote arguments to prevent injection
buffer[offset++] = '"';

// Copy argument with proper escaping
for (const char* p = arg; *p && (size_t)offset < buffer_size - 3; p++) {
if (*p == '"') {
// Escape quotes by doubling them (Windows style)
if ((size_t)offset < buffer_size - 4) {
buffer[offset++] = '"';
buffer[offset++] = '"';
} else {
return -1; // Buffer overflow
}
} else {
buffer[offset++] = *p;
}
}

if ((size_t)offset >= buffer_size - 2) {
return -1; // Buffer overflow
}

buffer[offset++] = '"';
buffer[offset] = '\0';

return offset;
}

// Secure command execution without shell interpretation
int secure_exec_command(const char* program, const char* const argv[], int timeout_ms) {
if (!program) {
Expand All @@ -56,30 +131,37 @@ int secure_exec_command(const char* program, const char* const argv[], int timeo
si.cb = sizeof(si);
ZeroMemory(&pi, sizeof(pi));

// Build command line from argv (properly quoted)
// SECURITY FIX: Comprehensive Windows argument sanitization and buffer protection
char cmdline[MAX_PATH_LENGTH] = {0};
int offset = 0;

// Add program name (quoted if contains spaces)
if (strchr(program, ' ')) {
offset = snprintf(cmdline, sizeof(cmdline), "\"%s\"", program);
} else {
offset = snprintf(cmdline, sizeof(cmdline), "%s", program);
// SECURITY: Validate program name for injection
if (!validate_windows_argument(program)) {
return -1;
}

// Add program name with robust quoting
offset = secure_windows_quote_argument(program, cmdline, sizeof(cmdline), 0);
if (offset < 0) {
return -1; // Buffer overflow protection
}

// Add arguments
// Add arguments with comprehensive validation
if (argv) {
for (int i = 0; argv[i] && offset < sizeof(cmdline) - 1; i++) {
// Add space separator
for (int i = 0; argv[i] && offset < sizeof(cmdline) - MAX_ARG_LENGTH; i++) {
// SECURITY: Validate each argument for injection
if (!validate_windows_argument(argv[i])) {
return -1;
}

// Add space separator with bounds check
if (offset >= sizeof(cmdline) - 2) break;
cmdline[offset++] = ' ';

// Quote argument if it contains spaces
if (strchr(argv[i], ' ')) {
offset += snprintf(cmdline + offset, sizeof(cmdline) - offset,
"\"%s\"", argv[i]);
} else {
offset += snprintf(cmdline + offset, sizeof(cmdline) - offset,
"%s", argv[i]);
// Add argument with secure quoting and buffer protection
offset = secure_windows_quote_argument(argv[i], cmdline, sizeof(cmdline), offset);
if (offset < 0) {
return -1; // Buffer overflow protection
}
}
}
Expand All @@ -100,9 +182,9 @@ int secure_exec_command(const char* program, const char* const argv[], int timeo
return -1;
}

// Wait for process with timeout
DWORD wait_result = WaitForSingleObject(pi.hProcess,
timeout_ms > 0 ? timeout_ms : INFINITE);
// SECURITY FIX: Never use INFINITE timeout to prevent deadlocks
DWORD max_timeout = timeout_ms > 0 ? timeout_ms : 30000; // Default 30s max
DWORD wait_result = WaitForSingleObject(pi.hProcess, max_timeout);

if (wait_result == WAIT_TIMEOUT) {
// Timeout - terminate process
Expand Down Expand Up @@ -137,13 +219,21 @@ int secure_exec_command(const char* program, const char* const argv[], int timeo
char* exec_argv[MAX_ARGS];
int argc = 0;

// First arg is program name
exec_argv[argc++] = strdup(program);
// SECURITY FIX: Memory-safe argument handling without strdup leaks
static char program_copy[MAX_ARG_LENGTH];
static char argv_copies[MAX_ARGS][MAX_ARG_LENGTH];

// Copy program name safely
strncpy(program_copy, program, sizeof(program_copy) - 1);
program_copy[sizeof(program_copy) - 1] = '\0';
exec_argv[argc++] = program_copy;

// Copy provided arguments
// Copy provided arguments safely
if (argv) {
for (int i = 0; argv[i] && argc < MAX_ARGS - 1; i++) {
exec_argv[argc++] = strdup(argv[i]);
strncpy(argv_copies[i], argv[i], sizeof(argv_copies[i]) - 1);
argv_copies[i][sizeof(argv_copies[i]) - 1] = '\0';
exec_argv[argc++] = argv_copies[i];
}
}
exec_argv[argc] = NULL;
Expand Down Expand Up @@ -344,10 +434,21 @@ secure_pipe_t* secure_open_pipe(const char* program, const char* const argv[]) {
char* exec_argv[MAX_ARGS];
int argc = 0;

exec_argv[argc++] = strdup(program);
// SECURITY FIX: Memory-safe argument handling without strdup leaks
static char program_copy_pipe[MAX_ARG_LENGTH];
static char argv_copies_pipe[MAX_ARGS][MAX_ARG_LENGTH];

// Copy program name safely
strncpy(program_copy_pipe, program, sizeof(program_copy_pipe) - 1);
program_copy_pipe[sizeof(program_copy_pipe) - 1] = '\0';
exec_argv[argc++] = program_copy_pipe;

// Copy provided arguments safely
if (argv) {
for (int i = 0; argv[i] && argc < MAX_ARGS - 1; i++) {
exec_argv[argc++] = strdup(argv[i]);
strncpy(argv_copies_pipe[i], argv[i], sizeof(argv_copies_pipe[i]) - 1);
argv_copies_pipe[i][sizeof(argv_copies_pipe[i]) - 1] = '\0';
exec_argv[argc++] = argv_copies_pipe[i];
}
}
exec_argv[argc] = NULL;
Expand Down Expand Up @@ -388,11 +489,16 @@ int secure_close_pipe(secure_pipe_t* pipe) {
}

#ifdef _WIN32
// Wait for process and get exit code
// SECURITY FIX: Wait for process with timeout to prevent deadlocks
if (pipe->process) {
DWORD exit_code;
WaitForSingleObject(pipe->process, INFINITE);
if (GetExitCodeProcess(pipe->process, &exit_code)) {
DWORD wait_result = WaitForSingleObject(pipe->process, 30000); // 30s timeout

if (wait_result == WAIT_TIMEOUT) {
// Timeout - forcibly terminate process
TerminateProcess(pipe->process, -2);
result = -2;
} else if (GetExitCodeProcess(pipe->process, &exit_code)) {
result = (int)exit_code;
} else {
result = -1;
Expand Down Expand Up @@ -470,19 +576,20 @@ int secure_check_command(const char* command) {
return 1;
}

// Check PATH directories
char* path_copy = strdup(path_var);
char* dir = strtok(path_copy, ";");
// SECURITY FIX: Safe PATH parsing without strdup leak
static char path_copy_win[MAX_PATH_LENGTH];
strncpy(path_copy_win, path_var, sizeof(path_copy_win) - 1);
path_copy_win[sizeof(path_copy_win) - 1] = '\0';

char* dir = strtok(path_copy_win, ";");
while (dir) {
char full_path[MAX_PATH_LENGTH];
snprintf(full_path, sizeof(full_path), "%s\\%s", dir, exe_name);
if (access(full_path, 0) == 0) {
free(path_copy);
int path_len = snprintf(full_path, sizeof(full_path), "%s\\%s", dir, exe_name);
if (path_len >= 0 && (size_t)path_len < sizeof(full_path) && access(full_path, 0) == 0) {
return 1;
}
dir = strtok(NULL, ";");
}
free(path_copy);

#else
// Unix: Check if executable exists in PATH
Expand All @@ -491,18 +598,20 @@ int secure_check_command(const char* command) {
return 0;
}

char* path_copy = strdup(path_var);
char* dir = strtok(path_copy, ":");
// SECURITY FIX: Safe PATH parsing without strdup leak
static char path_copy_unix[MAX_PATH_LENGTH];
strncpy(path_copy_unix, path_var, sizeof(path_copy_unix) - 1);
path_copy_unix[sizeof(path_copy_unix) - 1] = '\0';

char* dir = strtok(path_copy_unix, ":");
while (dir) {
char full_path[MAX_PATH_LENGTH];
snprintf(full_path, sizeof(full_path), "%s/%s", dir, command);
if (access(full_path, X_OK) == 0) {
free(path_copy);
int path_len = snprintf(full_path, sizeof(full_path), "%s/%s", dir, command);
if (path_len >= 0 && (size_t)path_len < sizeof(full_path) && access(full_path, X_OK) == 0) {
return 1;
}
dir = strtok(NULL, ":");
}
free(path_copy);
#endif

return 0;
Expand Down
24 changes: 19 additions & 5 deletions src/system/fortplot_security_core.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,10 @@ module fortplot_security_core
integer, parameter :: CHAR_CTRL_END = 31 ! End of control characters
integer, parameter :: CHAR_DEL = 127 ! DEL character

! Allowed characters in filenames (alphanumeric, dash, underscore, dot, slash)
! SECURITY ENHANCEMENT: Comprehensive safe filename characters
! Allows scientific/technical filenames while blocking injection vectors
character(len=*), parameter :: SAFE_FILENAME_CHARS = &
'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_./'
'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_./ \'

contains

Expand Down Expand Up @@ -315,15 +316,22 @@ function validate_path_characters(path) result(valid)
end do
end function validate_path_characters

!> Check if character is shell injection risk
!> Check if character is shell injection risk - COMPREHENSIVE SECURITY
function is_shell_injection_char(char) result(dangerous)
character, intent(in) :: char
logical :: dangerous

! Characters that could be used for shell injection
character(len=*), parameter :: DANGEROUS_CHARS = ';&|`$(){}[]<>*?!~'
! SECURITY ENHANCEMENT: Comprehensive shell metacharacter blocking
! Covers ALL possible command injection vectors across shells and encodings
character(len=*), parameter :: DANGEROUS_CHARS = ';&|`$(){}[]<>*?!~"''^\#%@+=: '

dangerous = index(DANGEROUS_CHARS, char) > 0

! Additional checks for control characters and Unicode exploits
if (.not. dangerous) then
! Block control characters that could be used for injection
dangerous = is_control_character(char)
end if
end function is_shell_injection_char

!> Check if character is a control character
Expand Down Expand Up @@ -359,6 +367,12 @@ function validate_path_patterns(path) result(valid)
valid = .false.
end if

! SECURITY ENHANCEMENT: Check for URL encoding attacks
if (index(path, '%') > 0) then
call log_error("Security: URL encoding detected (potential bypass attempt): " // trim(path))
valid = .false.
end if

! Check for paths starting with slash (absolute paths)
if (len_trim(path) > 0 .and. path(1:1) == '/') then
call log_warning("Security: Absolute path detected: " // trim(path))
Expand Down
Loading
Loading