Repository navigation
fix(adhoc-sweep-fixes): CU-86akq5g67 24 review findings across 21 files #173
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
66afc3e
2f21ec7
194ff0e
f7c8ad3
d7e63c7
99ac3ae
4612464
2619a83
63de17c
d18b924
da1421f
d99da47
914e5c8
980f886
8144cc4
a947564
29b65f1
16c034a
19493c0
cb447bb
c47e055
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -287,7 +287,7 @@ void KeyAction(unsigned char vk, int up) | |
| } | ||
| } | ||
|
|
||
| if (i == g_keymapLen) { return; } | ||
| if (i == g_keymapLen) { if (source != NULL) CFRelease(source); return; } | ||
| if (vk == VK_CAPITAL && up) { g_capsLock = g_capsLock ? 0 : 1; } | ||
|
|
||
| CGEventRef key = CGEventCreateKeyboardEvent(source, keycode, !up); | ||
|
Comment on lines
287
to
293
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΅ KeyAction() returns early without releasing CGEventSourceRef 'source' when vk is not found in keymap In KeyAction(), changed π€ Prompt for AI agentsfix confidence: π’ 95 high β react π/π to teach the reviewer |
||
|
|
@@ -308,3 +308,4 @@ void KeyActionUnicode(uint16_t unicode, int up) | |
| CFRelease(key); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,10 +91,23 @@ int codesign_matches(SecCodeRef code1, SecCodeRef code2) { | |
|
|
||
| /** | ||
| * Verify peer process connected to socket is legitimate meshagent | ||
| * | ||
| * NOTE: This verification is PID-based (SecCodeCreateWithPID). PIDs are | ||
| * recycled by the OS, so there is a narrow TOCTOU window between the | ||
| * connection being accepted and this lookup during which the original | ||
| * process could exit and the PID be reassigned to a different process. | ||
| * To reduce this window, we re-read the peer PID immediately before | ||
| * creating the SecCode reference and re-validate it immediately after, | ||
| * rejecting the connection if the PID changed underneath us. This does | ||
| * not fully eliminate the race (only an audit-token based check can do | ||
| * that - see verify_peer_codesign_audit() below), but it narrows the | ||
| * window significantly for this actively-used code path. | ||
| */ | ||
| int verify_peer_codesign(int socket_fd) { | ||
| pid_t peer_pid = 0; | ||
| pid_t peer_pid_recheck = 0; | ||
| socklen_t len = sizeof(peer_pid); | ||
| socklen_t len_recheck = sizeof(peer_pid_recheck); | ||
| OSStatus status; | ||
| SecCodeRef self_code = NULL; | ||
| SecCodeRef peer_code = NULL; | ||
|
Comment on lines
91
to
113
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΅ PID-based peer verification is vulnerable to PID reuse race (acknowledged but shipped as the active path) In verify_peer_codesign() (meshcore/KVM/MacOS/mac_kvm_auth.c), added a second getsockopt(LOCAL_PEERPID) re-check performed after SecCodeCreateWithPID/SecCodeCheckValidity succeed; if the peer PID reported by the socket has changed between the initial read and this point, the function now rejects the connection instead of proceeding to compare signatures. This narrows (but does not eliminate) the TOCTOU window described in the finding, since SecCodeCreateWithPID/SecCodeCheckValidity still operate on a PID snapshot rather than an atomic audit-token-bound handle. A complete fix would require switching to audit-token-based verification (SecCodeCreateWithAuditToken via the kernel-supplied audit token, e.g. through LOCAL_PEERTOKEN/xucred extensions or SO_PEERSEC equivalents available on recent macOS), which the file's own disabled verify_peer_codesign_audit() stub gestures at but does not implement; wiring that up is a larger platform-API change outside the scope of this minimal fix and is not verified to compile/work without access to the actual macOS SDK audit-token APIs in this environment. π€ Prompt for AI agentsfix confidence: π΄ 35 low β review closely β react π/π to teach the reviewer |
||
|
|
@@ -144,6 +157,21 @@ int verify_peer_codesign(int socket_fd) { | |
| goto cleanup; | ||
| } | ||
|
|
||
| // Re-check that the socket peer PID has not changed since we started | ||
| // validating. If it has, the original process has exited and the PID | ||
| // may have been reassigned to a different (attacker-controlled) | ||
| // process - reject the connection to narrow the PID-reuse TOCTOU window. | ||
| if (getsockopt(socket_fd, SOL_LOCAL, LOCAL_PEERPID, &peer_pid_recheck, &len_recheck) < 0) { | ||
| ILIBLOGMESSAGEX("MSG_KVM_AUTH_VERIFY_PEER_FAIL: getsockopt(LOCAL_PEERPID) recheck failed: %s", strerror(errno)); | ||
| fprintf(stderr, "KVM Auth: Failed to recheck peer PID: %s\n", strerror(errno)); | ||
| goto cleanup; | ||
| } | ||
| if (peer_pid_recheck != peer_pid) { | ||
| ILIBLOGMESSAGEX("MSG_KVM_AUTH_VERIFY_PEER_FAIL: Peer PID changed during verification (was %d, now %d) - possible PID reuse", peer_pid, peer_pid_recheck); | ||
| fprintf(stderr, "KVM Auth: Peer PID changed during verification (possible PID reuse), rejecting\n"); | ||
| goto cleanup; | ||
| } | ||
|
|
||
| ILIBLOGMESSAGEX("MSG_KVM_AUTH_VERIFY_PEER: Peer code is valid, comparing signatures..."); | ||
|
|
||
| // Compare code signatures - must be same binary | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,39 @@ static FILE* g_log_file = NULL; | |
| static char g_log_directory[LOG_PATH_SIZE] = {0}; | ||
| static long g_current_log_size = 0; | ||
|
|
||
| /* Thread-safe mutex for logging */ | ||
| #ifdef WIN32 | ||
| static CRITICAL_SECTION g_log_mutex; | ||
| static int g_log_mutex_initialized = 0; | ||
| #else | ||
| static pthread_mutex_t g_log_mutex = PTHREAD_MUTEX_INITIALIZER; | ||
| #endif | ||
|
|
||
| static inline void init_log_mutex(void) { | ||
| #ifdef WIN32 | ||
| if (!g_log_mutex_initialized) { | ||
| InitializeCriticalSection(&g_log_mutex); | ||
| g_log_mutex_initialized = 1; | ||
| } | ||
| #endif | ||
| } | ||
|
|
||
| static inline void lock_log(void) { | ||
| #ifdef WIN32 | ||
| EnterCriticalSection(&g_log_mutex); | ||
| #else | ||
| pthread_mutex_lock(&g_log_mutex); | ||
| #endif | ||
| } | ||
|
|
||
| static inline void unlock_log(void) { | ||
| #ifdef WIN32 | ||
| LeaveCriticalSection(&g_log_mutex); | ||
| #else | ||
| pthread_mutex_unlock(&g_log_mutex); | ||
| #endif | ||
| } | ||
|
|
||
| #ifdef WIN32 | ||
| static int g_original_stdout_fd = -1; | ||
| static HANDLE g_tee_thread = INVALID_HANDLE_VALUE; | ||
|
|
@@ -83,10 +116,13 @@ static DWORD WINAPI tee_thread_func(LPVOID lpParam) { | |
| } | ||
|
|
||
| /* Write to log file */ | ||
| init_log_mutex(); | ||
| lock_log(); | ||
| if (g_log_file != NULL) { | ||
| fwrite(buffer, 1, bytes_read, g_log_file); | ||
| fflush(g_log_file); | ||
| } | ||
| unlock_log(); | ||
| } | ||
|
|
||
| return 0; | ||
|
|
@@ -118,52 +154,22 @@ static void* tee_thread_func(void* arg) { | |
| } | ||
|
|
||
| /* Write to log file */ | ||
| init_log_mutex(); | ||
| lock_log(); | ||
| if (g_log_file != NULL) { | ||
| fwrite(buffer, 1, bytes_read, g_log_file); | ||
| fflush(g_log_file); | ||
| } | ||
| unlock_log(); | ||
| } | ||
|
|
||
| return NULL; | ||
| } | ||
| #endif | ||
|
|
||
| /* Thread-safe mutex for logging */ | ||
| #ifdef WIN32 | ||
| static CRITICAL_SECTION g_log_mutex; | ||
| static int g_log_mutex_initialized = 0; | ||
| #else | ||
| static pthread_mutex_t g_log_mutex = PTHREAD_MUTEX_INITIALIZER; | ||
| #endif | ||
|
|
||
| static volatile int g_disk_error_count = 0; | ||
| static volatile int g_disk_available = 1; | ||
|
|
||
| static inline void init_log_mutex(void) { | ||
| #ifdef WIN32 | ||
| if (!g_log_mutex_initialized) { | ||
| InitializeCriticalSection(&g_log_mutex); | ||
| g_log_mutex_initialized = 1; | ||
| } | ||
| #endif | ||
| } | ||
|
|
||
| static inline void lock_log(void) { | ||
| #ifdef WIN32 | ||
| EnterCriticalSection(&g_log_mutex); | ||
| #else | ||
| pthread_mutex_lock(&g_log_mutex); | ||
| #endif | ||
| } | ||
|
|
||
| static inline void unlock_log(void) { | ||
| #ifdef WIN32 | ||
| LeaveCriticalSection(&g_log_mutex); | ||
| #else | ||
| pthread_mutex_unlock(&g_log_mutex); | ||
| #endif | ||
| } | ||
|
|
||
| static inline long get_file_size(const char* filepath) { | ||
| struct stat st; | ||
| if (stat(filepath, &st) == 0) { | ||
|
|
@@ -222,6 +228,7 @@ static inline int compress_file_to_gzip(const char* source_path, const char* des | |
| #endif | ||
| } | ||
|
|
||
| /* Caller must hold g_log_mutex (via lock_log()) before calling this. */ | ||
| static inline int rotate_log_file(void) { | ||
| char log_path[LOG_PATH_SIZE]; | ||
| char archive_path[LOG_PATH_SIZE]; | ||
|
Comment on lines
228
to
234
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π g_log_file and rotation state are accessed from the tee thread and the main thread without the log mutex being used anywhere Fixed the TOCTOU/use-after-close race on g_log_file by making all accesses to it (and to rotation state g_current_log_size) go through the existing lock_log()/unlock_log() mutex. Specifically: (a) moved the mutex declaration and init_log_mutex/lock_log/unlock_log definitions above tee_thread_func (both WIN32 and POSIX variants) so they can be used there; (b) wrapped the fwrite/fflush on g_log_file inside both tee_thread_func implementations with lock_log()/unlock_log(); (c) wrapped every fopen/fclose/fprintf/fflush touching g_log_file in enable_file_logging() (initial open, and all the error-path fcloses on both WIN32 and POSIX branches) with lock_log()/unlock_log(); (d) wrapped the final fclose/fprintf block and state reset in disable_file_logging() with lock_log()/unlock_log(); rotate_log_file() and check_and_rotate() are documented as requiring the caller to hold the lock, which openframe_printf already does since it calls check_and_rotate() while holding lock_log(). This serializes all readers/writers of g_log_file across the tee thread, rotation, enable, and disable paths, removing the race described in the finding. Risk: the tee thread now blocks on lock_log() for every chunk read from the pipe (previously finding noted it never acquired it at all), which adds minor lock contention under heavy log volume, but is necessary for correctness; a complete fix would also want a condition to avoid the thread blocking on a potentially long rotation operation, but that is a performance nuance rather than a correctness gap. π€ Prompt for AI agentsfix confidence: π‘ 75 medium β react π/π to teach the reviewer |
||
|
|
@@ -284,6 +291,7 @@ static inline int rotate_log_file(void) { | |
| return 1; | ||
| } | ||
|
|
||
| /* Caller must hold g_log_mutex (via lock_log()) before calling this. */ | ||
| static inline void check_and_rotate(int bytes_written) { | ||
| g_current_log_size += bytes_written; | ||
|
|
||
|
|
@@ -305,6 +313,8 @@ static inline int enable_file_logging(const char* log_directory, const char* log | |
|
|
||
| (void)log_prefix; | ||
|
|
||
| init_log_mutex(); | ||
|
|
||
| if (g_log_file != NULL) { | ||
| fprintf(stderr, "WARNING: File logging is already enabled\n"); | ||
| return 1; | ||
|
|
@@ -325,8 +335,10 @@ static inline int enable_file_logging(const char* log_directory, const char* log | |
| snprintf(logfile_path, sizeof(logfile_path), "%s", LOG_FILENAME); | ||
| } | ||
|
|
||
| lock_log(); | ||
| g_log_file = fopen(logfile_path, "a"); | ||
| if (g_log_file == NULL) { | ||
| unlock_log(); | ||
| fprintf(stderr, "WARNING: Failed to open log file: %s\n", logfile_path); | ||
| return 0; | ||
| } | ||
|
|
@@ -350,77 +362,98 @@ static inline int enable_file_logging(const char* log_directory, const char* log | |
| fprintf(g_log_file, "Max size before rotation: %d MB\n", LOG_MAX_SIZE / (1024 * 1024)); | ||
| fprintf(g_log_file, "========================================\n\n"); | ||
| fflush(g_log_file); | ||
| unlock_log(); | ||
|
|
||
| #ifdef WIN32 | ||
| /* Create pipe for tee functionality */ | ||
| if (_pipe(g_pipe_fds, LOG_BUFFER_SIZE, _O_BINARY) != 0) { | ||
| fprintf(stderr, "WARNING: Failed to create pipe for logging\n"); | ||
| lock_log(); | ||
| fclose(g_log_file); | ||
| g_log_file = NULL; | ||
| unlock_log(); | ||
| return 0; | ||
| } | ||
|
|
||
| /* Save original stdout and check if we're running as service */ | ||
| g_original_stdout_fd = _dup(1); /* 1 is stdout */ | ||
| if (g_original_stdout_fd < 0) { | ||
| lock_log(); | ||
| fprintf(g_log_file, "WARNING: Failed to duplicate stdout - running as service\n"); | ||
| fflush(g_log_file); | ||
| unlock_log(); | ||
| g_daemon_mode = 1; | ||
| } else { | ||
| /* Check if stdout is redirected (common in service mode) */ | ||
| if (!_isatty(g_original_stdout_fd)) { | ||
| /* stdout is not a console - likely service mode */ | ||
| g_daemon_mode = 1; | ||
| lock_log(); | ||
| fprintf(g_log_file, "INFO: Detected service mode - console output disabled\n"); | ||
| fflush(g_log_file); | ||
| unlock_log(); | ||
| } else { | ||
| lock_log(); | ||
| fprintf(g_log_file, "INFO: Console mode detected - output will go to both console and file\n"); | ||
| fflush(g_log_file); | ||
| unlock_log(); | ||
| } | ||
| } | ||
|
|
||
| /* Start tee thread */ | ||
| g_logging_active = 1; | ||
| g_tee_thread = CreateThread(NULL, 0, tee_thread_func, NULL, 0, NULL); | ||
| if (g_tee_thread == NULL) { | ||
| lock_log(); | ||
| fprintf(g_log_file, "WARNING: Failed to create logging thread\n"); | ||
| fflush(g_log_file); | ||
| unlock_log(); | ||
| if (g_original_stdout_fd >= 0) _close(g_original_stdout_fd); | ||
| _close(g_pipe_fds[0]); | ||
| _close(g_pipe_fds[1]); | ||
| lock_log(); | ||
| fclose(g_log_file); | ||
| g_log_file = NULL; | ||
| unlock_log(); | ||
| g_logging_active = 0; | ||
| return 0; | ||
| } | ||
|
|
||
| /* Redirect stdout to pipe */ | ||
| if (_dup2(g_pipe_fds[1], 1) < 0) { /* 1 is stdout */ | ||
| lock_log(); | ||
| fprintf(g_log_file, "WARNING: Failed to redirect stdout\n"); | ||
| fflush(g_log_file); | ||
| unlock_log(); | ||
| g_logging_active = 0; | ||
| _close(g_pipe_fds[1]); /* Close write end to signal thread to exit */ | ||
| WaitForSingleObject(g_tee_thread, 1000); | ||
| CloseHandle(g_tee_thread); | ||
| if (g_original_stdout_fd >= 0) _close(g_original_stdout_fd); | ||
| _close(g_pipe_fds[0]); | ||
| lock_log(); | ||
| fclose(g_log_file); | ||
| g_log_file = NULL; | ||
| unlock_log(); | ||
| return 0; | ||
| } | ||
|
|
||
| if (_dup2(g_pipe_fds[1], 2) < 0) { /* 2 is stderr */ | ||
| lock_log(); | ||
| fprintf(g_log_file, "WARNING: Failed to redirect stderr\n"); | ||
| fflush(g_log_file); | ||
| unlock_log(); | ||
| if (g_original_stdout_fd >= 0) _dup2(g_original_stdout_fd, 1); | ||
| g_logging_active = 0; | ||
| _close(g_pipe_fds[1]); /* Close write end to signal thread to exit */ | ||
| WaitForSingleObject(g_tee_thread, 1000); | ||
| CloseHandle(g_tee_thread); | ||
| if (g_original_stdout_fd >= 0) _close(g_original_stdout_fd); | ||
| _close(g_pipe_fds[0]); | ||
| lock_log(); | ||
| fclose(g_log_file); | ||
| g_log_file = NULL; | ||
| unlock_log(); | ||
| return 0; | ||
| } | ||
|
|
||
|
|
@@ -435,16 +468,20 @@ static inline int enable_file_logging(const char* log_directory, const char* log | |
| /* Create pipe for tee functionality */ | ||
| if (pipe(g_pipe_fds) != 0) { | ||
| fprintf(stderr, "WARNING: Failed to create pipe for logging\n"); | ||
| lock_log(); | ||
| fclose(g_log_file); | ||
| g_log_file = NULL; | ||
| unlock_log(); | ||
| return 0; | ||
| } | ||
|
|
||
| /* Save original stdout and check if we're running as daemon */ | ||
| g_original_stdout_fd = dup(STDOUT_FILENO); | ||
| if (g_original_stdout_fd < 0) { | ||
| lock_log(); | ||
| fprintf(g_log_file, "WARNING: Failed to duplicate stdout - running in daemon mode\n"); | ||
| fflush(g_log_file); | ||
| unlock_log(); | ||
| g_daemon_mode = 1; | ||
| } else { | ||
| /* Check if stdout is redirected (common in daemon mode) */ | ||
|
|
@@ -453,25 +490,33 @@ static inline int enable_file_logging(const char* log_directory, const char* log | |
| if (!isatty(g_original_stdout_fd)) { | ||
| /* stdout is not a terminal - likely daemon mode */ | ||
| g_daemon_mode = 1; | ||
| lock_log(); | ||
| fprintf(g_log_file, "INFO: Detected daemon mode - console output disabled\n"); | ||
| fflush(g_log_file); | ||
| unlock_log(); | ||
| } else { | ||
| lock_log(); | ||
| fprintf(g_log_file, "INFO: Console mode detected - output will go to both console and file\n"); | ||
| fflush(g_log_file); | ||
| unlock_log(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /* Start tee thread */ | ||
| g_logging_active = 1; | ||
| if (pthread_create(&g_tee_thread, NULL, tee_thread_func, NULL) != 0) { | ||
| lock_log(); | ||
| fprintf(g_log_file, "WARNING: Failed to create logging thread\n"); | ||
| fflush(g_log_file); | ||
| unlock_log(); | ||
| if (g_original_stdout_fd >= 0) close(g_original_stdout_fd); | ||
| close(g_pipe_fds[0]); | ||
| close(g_pipe_fds[1]); | ||
| lock_log(); | ||
| fclose(g_log_file); | ||
| g_log_file = NULL; | ||
| unlock_log(); | ||
| g_logging_active = 0; | ||
| return 0; | ||
| } | ||
|
|
@@ -682,6 +727,7 @@ static inline void disable_file_logging(void) | |
| #endif | ||
|
|
||
| /* Close log file */ | ||
| lock_log(); | ||
| if (g_log_file != NULL) { | ||
| fprintf(g_log_file, "\n========================================\n"); | ||
| fprintf(g_log_file, "MeshAgent Log Ended\n"); | ||
|
|
@@ -696,6 +742,7 @@ static inline void disable_file_logging(void) | |
| g_disk_error_count = 0; | ||
| g_current_log_size = 0; | ||
| g_log_directory[0] = '\0'; | ||
| unlock_log(); | ||
|
|
||
| #ifdef WIN32 | ||
| if (g_log_mutex_initialized) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
𦩠π΅ calc_opt_compr_send leaks previous tilebuffer contents and returns 0/buffer=NULL without signalling malloc failure to caller consistently
In calc_opt_compr_send (meshcore/KVM/Linux/linux_tile.c), reordered the tilebuffer resize logic: malloc is now attempted first using the new size directly, and
tilebuffersizeis only updated to the new size after the malloc succeeds. On malloc failure,tilebuffersizeis explicitly reset to 0 before returning 0, so the stale/NULLtilebufferwith a mismatchedtilebuffersizecan no longer occur, and the subsequent call will correctly detect the size mismatch and retry allocation rather than calling get_tile_buffer with a NULL buffer.π€ Prompt for AI agents
fix confidence: π’ 92 high β react π/π to teach the reviewer