Update MeshAgent from Fork#33
Open
ivan-flamingo wants to merge 36 commits into
Open
Conversation
* Removed trying to load Windows system DLLs from user folders Fixes Ylianst/MeshCentral#7606
Signed-off-by: Simon Smith <simonsmith5521@gmail.com>
Updated CodeQL GitHub Actions to use the latest versions.
Uncomment language selection for CodeQL analysis.
…dded monitor-border.js to the polyfills (#331) * Added option to limit the width of the desktop notification bar. * Added monitor-border to the embedded modules in the polyfills. Added auto-start too.
* Fix strcat_s implementation Old implementation didn't work, it would crash when it tried to dereference and set a null pointer (*x = i). This version works by setting the default size to destinationSize, so it will throw in the later check if no \0 is found. This function is otherwise unused in the code. * Added function to load certs from the system Windows doesn't respect the openssl standard, so that needed to be handled manuall. Since this project uses a statically linked openssl, the default locations may be inaccurate. If that directory doesn't exist, it tries to get it from the system openssl via a popen. If that also fails, it will not be able to load system certs. * Added system cert loading wherever an SSL context is established * Modified OnHttpsConnection callback so the binder can pass general data to the callback. This was necessary to get a handle on the agent during the connection callback. * Added check for validateWebCert config option This will cause the connection to fail if the value is set and the agent attempts to connect to a server with an invalid web certificate * Fixed some handling of rejectUnauthorized in duktape net handler Before it would override any user defined checks; now it will foil to connect if the system certs don't match, but will still run user checks if it passes those. * Added validatewebcert arg to meshcmd.js This will cause rejectUnauthorized to be true if you pass it in * Updated default recovery core to check for the "validateWebCert" option and use rejectUnauthorized if it is set.
… ARM systems (#224) * Force alignment of allocated memory. Without this change, I have noticed crashes on ARM based devices which have stricter alignment requirements than x86. Ideally, this code should be rewritten to pad the sizes of all "structs" and to consider all of the different architecture-specific alignment requirements. But in practice, I suspect that rounding up allocation sizes to a multiple of pointer sizes is likely good enough. This is a pragmatic one-line change that shouldn't break existing code and should prevent any of the crashes that I have observed. * Added a few more memory alignments when using both primary and extra memory in a back-to-back memory allocation.
Signed-off-by: Simon Smith <simonsmith5521@gmail.com>
System proxy detection executes the proxy-helper JavaScript path, which can create short-lived EventEmitter and ChildProcess object graphs during reconnect attempts. Without an explicit GC pass those objects can remain retained until a later heap cycle, causing repeated reconnects to drive up peak heap usage. Run a Duktape GC pass immediately after popping the proxy-helper result so the temporary proxy-detection objects are collected promptly.
…ut (#342) When MeshServer_ConnectEx_NetworkError fires (20s connection timeout), it frees j (==agent->controlChannelRequest) but does not set the pointer to NULL. ILibWebClient_CancelRequest, called immediately after, executes synchronously on the chain thread (ILibChain_RunOnMicrostackThread macro calls the handler inline when already on the chain thread). This triggers MeshServer_OnResponse(ReceiveStatus_Complete) which sees a non-NULL controlChannelRequest and calls ILibMemory_Free on the already-freed pointer, causing a double-free and heap corruption on every retry cycle. Fix: set agent->controlChannelRequest = NULL right after ILibMemory_Free(j) so MeshServer_OnResponse safely skips the cleanup block. Relates to: #110, #151, #281 Relates to: Ylianst/MeshCentral#7407 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…re (#344) On macOS Tahoe (26.x), the dashboard's Desktop tab shows a black frame on Apple-Silicon Macs because the user-context -kvm0 helper inherits the daemon's system-side audit session (asid != console user's gui session asid). Per-user XPC services like com.apple.replayd are registered in gui/<console-uid> and unreachable from a different audit session, so SLSHWCaptureDesktopProxying / ReplayKit returns empty frames after a 5-second timeout. This patch joins the console user's GUI audit session at TWO points: 1. Pre-exec, inside SpawnProcessEx4's fork-child while still root: does the actual auditon-based discovery and audit_session_join. On Tahoe, only root can call audit_session_port() for arbitrary asids or enumerate via auditon(A_GETPINFO_ADDR) across audit sessions, so discovery has to happen here. 2. Post-exec, in the helper's main() entry for argv[1] == "-kvm0": the kernel resets the asid on exec, so the pre-exec join doesn't persist on its own. Post-exec re-joins the same session; the pre-exec join leaves enough kernel state that auditon visibility for the user's gui session works from the dropped-privilege uid. Discovery picks the LOWEST visible asid belonging to the target uid (typically 100002, the user's primary gui session, vs higher asids for transient/per-app sessions where replayd doesn't register). A fallback brute-force range probe handles the case where Tahoe's cross-session auditon visibility filter blocks discovery entirely. Also includes a small zlib build fix in meshcore/zlib/zutil.h: skip the "fdopen() not available" macro on __APPLE__ because newer Xcode/ Clang treats the resulting macro expansion as a compile error against the system stdio.h prototype. macOS has fdopen, so the macro shouldn't fire on Apple platforms anyway. No behavior change on Linux/Windows. No new dependencies (audit APIs ship with the macOS SDK). Tested on macOS Tahoe 26.x, Apple Silicon, MeshCentral 1.x server. Before patch: 100% black canvas in dashboard Desktop tab. After patch: full desktop video streaming at native resolution. Co-authored-by: austin <me@austinrenfroe.com>
Signed-off-by: Simon Smith <simonsmith5521@gmail.com>
Signed-off-by: Simon Smith <simonsmith5521@gmail.com>
…workaround) (#349) * macOS Tahoe: post mouseMoved CGEvent instead of CGWarpMouseCursorPosition On Tahoe, CGWarpMouseCursorPosition fires hot-corner watchers when the warp destination sits in a corner zone — the OS treats the warp as a cursor 'presence' at the destination rather than silent teleportation. On every Desktop-tab connect from the MeshCentral browser UI, the initial mouse coordinate sent by the browser commonly lands in the top-right of the video pane, the warp puts the OS cursor at the top-right of the screen, and Tahoe fires Notification Center (surfacing Calendar + Weather widgets). Top-left maps to Mission Control / app-spread the same way. Posting a kCGEventMouseMoved CGEvent goes through the normal HID-event path. Hot-corner watchers fire on cursor *dwell*, not on synthetic movement events, so this stops triggering Notification Center / Mission Control on connect. Subsequent browser-side cursor movement generates normal CGEvents that move the OS cursor away from any corner the cursor briefly visits. This also makes mesh's mouse-move behavior more conformant with what macOS apps expect from input frameworks — CGWarpMouseCursorPosition is documented as 'does not generate mouse events,' which means apps that listen for mouseMoved (screensaver wake, sticky-corner activation, HID-tracking apps) miss out on cursor activity that's actually happening from the user's perspective. * macOS Tahoe: add -kvmagent mode + LaunchAgent plist (Phase 1) The user-context LaunchAgent runs natively in gui/<console-uid> with the user's audit session id (typically asid=100002), where com.apple.replayd registers. This eliminates the cross-session XPC bootstrap-lookup failure that breaks ScreenCaptureKit screen capture when the kvm helper is spawned from the system-side LaunchDaemon (asid=100001). Architecture matches Splashtop Streamer's: a separate per-user LaunchAgent with LimitLoadToSessionType = ['Aqua', 'LoginWindow'], loaded by launchd into the user's domain. The daemon connects to the agent over a Unix domain socket; same TLV protocol that flowed over the inherited stdio pipes in the -kvm0 / -kvm1 case. Phase 1 deliverables: - mac_kvm.c: convert KVM_Listener_Path from #define to a writable variable + setter, so the listener path can be parameterized for the per-user agent. Defaults preserve legacy -kvm1 behavior. - meshconsole/main.c: new -kvmagent mode. Optional argv[2] sets the socket path; default is /tmp/meshagent-kvm-<uid>.sock so a non- root user can bind it. - macos/com.meshagent.kvm.plist: LaunchAgent definition. RunAtLoad true, KeepAlive on non-clean-exit, ThrottleInterval 30s. Phase 2 (next): daemon's kvm_relay_setup() in mac_kvm.c connects to this socket instead of fork-exec'ing a -kvm0 helper. Phase 3: drop the audit_session_join workaround entirely once the daemon-side connect path is live and verified. * macOS Tahoe: daemon connects to user-LaunchAgent socket (Phase 2) Phase 2 of the launchagent-split refactor. Adds the daemon-side socket-connect path that pairs with the -kvmagent mode added in Phase 1. When MeshCentral initiates a Desktop session, kvm_relay_setup() now first tries to connect to /tmp/meshagent-kvm-<uid>.sock — the listener registered by the user-context LaunchAgent. If the agent is up, the daemon hands the KVM session off to it via the existing TLV protocol (now flowing over a Unix domain socket instead of inherited stdio pipes from a fork-exec'd helper). If the agent isn't reachable, falls through to the legacy fork-exec -kvm0 helper, preserving the existing behavior for fixtures that don't yet have the LaunchAgent installed. The agent's audit session is gui/<uid> natively (via launchd's LimitLoadToSessionType=Aqua loading), so com.apple.replayd is reachable from there and ScreenCaptureKit screen capture works without the audit_session_join workaround the daemon-spawn path needs. Cleanly supersedes PR #344's approach. Implementation: - New static state (g_kvmSocketPipe, g_kvmSocketFD, write handler + reserved pointer) tracking the socket-mode session. - kvm_relay_setup tries connect(); on success wraps the FD in ILibProcessPipe_Pipe_CreateFromExisting() so the existing pipe abstractions for write / pause / resume / free continue to work. - New kvm_relay_socket_ReadHandler with the ILibProcessPipe_Pipe_ReadHandler signature; reuses the same TLV parser as kvm_relay_StdOutHandler. - New kvm_relay_socket_BrokenHandler clears the static state on socket disconnect so subsequent kvm_relay_setup calls reconnect. - kvm_relay_feeddata branches: ILibProcessPipe_Pipe_Write to the socket pipe in socket mode; ILibProcessPipe_Process_WriteStdIn to the legacy child process in fork-exec mode. * macOS Tahoe: process key/mouse events locally when in socket mode Upstream's MNG_KVM_KEY and MNG_KVM_MOUSE cases in kvm_mainloopinput bail out when KVM_AGENT_FD != -1 — i.e., when the daemon is in the unused -kvm1 socket-mode (or now our -kvmagent mode). Presumably intended for an upstream design where the agent forwarded input messages elsewhere instead of dispatching them locally. That's wrong for the user-LaunchAgent architecture: the agent IS the process that should call KeyAction / MouseAction, since it's the only one in the right audit session and with the TCC permissions to post CGEvents into the user's session. The system-level daemon proxies bytes only. Symptom before fix: with the LaunchAgent up and the daemon connecting to it via socket, video streamed fine but every keystroke and mouse motion was silently dropped at the case-break. After fix: the agent handles input locally, video + keyboard + mouse all flow end-to-end through MeshCentral's Desktop tab. Verified empirically 2026-05-06. * macOS Tahoe: integrate -kvmagent into -fullinstall + harden socket-mode fallback Two follow-ups from the launchagent-split refactor's first working deployment: 1. agent-installer.js: -fullinstall now writes the LaunchAgent plist with parameters: ['-kvmagent'] (was ['-kvm1'] - never wired up on the daemon side) and sessionTypes: ['LoginWindow', 'Aqua'] (was ['LoginWindow'] only - meant the agent never loaded for logged-in user sessions). After installing the plist, immediately bootstraps it into gui/<console-uid> so the agent is available without requiring a logout/login cycle. 2. mac_kvm.c kvm_relay_setup hardening: only fall through to the legacy fork-exec helper if the LaunchAgent socket file is genuinely absent from disk. If the socket exists but connect() transiently fails (e.g. agent re-accepting between sessions), retry up to 5 times with exponential backoff (50ms..800ms, ~1.55s total). Returns NULL if all retries fail rather than firing the broken fork-exec path that produces black frames / dead input under Tahoe's audit-session isolation. Adds <sys/stat.h> for the stat() check. * macOS Tahoe: regenerate embedded modules with updated agent-installer.js Run code-utils expand/shrink to refresh the auto-generated block in microscript/ILibDuktape_Polyfills.c so the agent-installer module embedded in the binary reflects the LaunchAgent + -kvmagent changes made in modules/agent-installer.js earlier in this PR (commits b658692, 38d0120, 4a8a589, d77773f). Process used (per si458's note): ./meshagent_x86-64 -export \ --expandedPath=modules_expanded \ --filePath=microscript/ILibDuktape_Polyfills.c cp modules/agent-installer.js modules_expanded/agent-installer.js ./meshagent_x86-64 -exec \ 'require("code-utils").shrink({ expandedPath:"modules_expanded", filePath:"microscript/ILibDuktape_Polyfills.c", modulesPath:"modules"}); process.exit();' Diff is bounded to the BEGIN/END AUTO-GENERATED BODY markers. Module set is unchanged (1 net add: wget is now embedded as a real module rather than the prior lazy require() property; the corresponding process.arch lazy-getter remains in place outside the auto-gen region). --------- Co-authored-by: austin <me@austinrenfroe.com> Co-authored-by: Austin Renfroe <austin@earenfroe.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds macOS per-user KVM socket-mode and audit-session joining, Linux XRandR multi-monitor capture and protocol updates, extensive Windows KVM refactors, system CA loading into SSL contexts and HTTPS callback userdata wiring, plus related build, runtime, and UI adjustments. ChangesmacOS KVM Agent Socket Mode
Linux XRandR Multi-Monitor Support
Windows KVM Command Handling and Mainloop Refactor
System Certificate Loading and HTTPS Callback Enhancement
Infrastructure, Build, and Runtime Updates
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
meshcore/KVM/Windows/kvm.c (1)
1379-1387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing NULL check after spawning child process.
If
ILibProcessPipe_Manager_SpawnProcessEx3fails and returns NULL, the subsequent calls toILibProcessPipe_Process_GetPIDandILibProcessPipe_Process_AddHandlerswould operate on a NULL pointer.Suggested fix
gChildProcess = ILibProcessPipe_Manager_SpawnProcessEx3(pipeMgr, exePath, paused == 0 ? parms0 : parms1, gProcessSpawnType, (void *)(ULONG_PTR)gProcessTSID, 0); + if (gChildProcess == NULL) + { + KVMDEBUG("kvm_relay_restart() SpawnProcess failed", 0); + ILibMemory_Free(user); + return 0; + } gProcessSpawnType = (gProcessSpawnType == ILibProcessPipe_SpawnTypes_SPECIFIED_USER || gProcessSpawnType == ILibProcessPipe_SpawnTypes_USER) ? ILibProcessPipe_SpawnTypes_WINLOGON : (gProcessTSID < 0 ? ILibProcessPipe_SpawnTypes_USER : ILibProcessPipe_SpawnTypes_SPECIFIED_USER);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshcore/KVM/Windows/kvm.c` around lines 1379 - 1387, After calling ILibProcessPipe_Manager_SpawnProcessEx3 you must check whether gChildProcess is NULL before using it; if NULL, handle the error (log via process/logger, set appropriate state, and avoid calling ILibProcessPipe_Process_GetPID, ILibProcessPipe_Process_ResetMetadata, or ILibProcessPipe_Process_AddHandlers) and return/cleanup as needed; update the block around ILibProcessPipe_Manager_SpawnProcessEx3, gChildProcess, ILibProcessPipe_Process_GetPID, ILibProcessPipe_Process_ResetMetadata, and ILibProcessPipe_Process_AddHandlers to perform a NULL check and early error handling when spawn fails.
🧹 Nitpick comments (4)
com.meshagent.kvm.plist (1)
43-46: 💤 Low valueLog files in
/tmp/are world-readable.Debug logs written to
/tmp/meshagent-kvm.*.logare accessible to all users on the system. If these logs contain sensitive information (session data, user activity), consider using a more restrictive location or permissions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@com.meshagent.kvm.plist` around lines 43 - 46, StandardOutPath and StandardErrorPath currently point to /tmp which is world-readable; change the log destinations to a restrictive location (e.g. /var/log/meshagent/meshagent-kvm.stdout.log and /var/log/meshagent/meshagent-kvm.stderr.log) and ensure the installer or startup logic creates that directory and files owned by the service user with restrictive permissions (owner-only, e.g. 0600/0700). Update the plist keys StandardOutPath and StandardErrorPath accordingly and add deployment/setup steps to create /var/log/meshagent with correct ownership and permissions so logs are not world-readable.meshcore/KVM/MacOS/mac_kvm.c (1)
437-444: 💤 Low valueConsider checking write return value for error handling.
ILibProcessPipe_Pipe_Writereturns aILibTransport_DoneStatethat indicates success or error. Ignoring this could mask write failures. While the broken-pipe handler will eventually clear state, early error detection could improve reliability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshcore/KVM/MacOS/mac_kvm.c` around lines 437 - 444, The write to the socket-mode pipe currently ignores ILibProcessPipe_Pipe_Write's return (ILibTransport_DoneState); capture its return value and handle failures immediately: call ILibProcessPipe_Pipe_Write(g_kvmSocketPipe, buf, len, ILibTransport_MemoryOwnership_USER) into a variable of type ILibTransport_DoneState, check for a non-success state, and on error log the failure, close/cleanup the g_kvmSocketPipe (and set g_kvmSocketPipe = NULL) to avoid further writes, and return an error indicator (e.g., -1) instead of len so the caller knows the write failed. Ensure you reference g_kvmSocketPipe, ILibProcessPipe_Pipe_Write, and ILibTransport_DoneState when making the change.meshcore/KVM/Windows/kvm.c (1)
869-875: ⚡ Quick winPartial packet data may be discarded.
When
ptr < len, the remaining bytes inpchRequest2[ptr..len-1]are not preserved for the next read iteration. If a command packet spans two reads, the partial data from the first read is lost. The TODO comment acknowledges this.Suggested fix to preserve partial packets
- if (ptr == len) - { - len = 0; - ptr = 0; - } - // TODO: else move the reminder. + if (ptr == len) + { + len = 0; + ptr = 0; + } + else if (ptr > 0) + { + // Move remainder to the start of the buffer + memmove(pchRequest2, pchRequest2 + ptr, len - ptr); + len = len - ptr; + ptr = 0; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshcore/KVM/Windows/kvm.c` around lines 869 - 875, The code drops partial packet bytes when ptr < len; preserve the leftover bytes by memmoving pchRequest2[ptr..len-1] to the start before the next read. Replace the TODO with logic like: compute remaining = len - ptr; memmove(pchRequest2, pchRequest2 + ptr, remaining); set len = remaining and ptr = 0 (and handle the ptr == len case as before), ensuring you check buffer capacity before the next read to avoid overflow. Use the existing buffer variables (pchRequest2, ptr, len) and memmove to safely handle overlapping memory.meshcore/KVM/Linux/linux_kvm.c (1)
1418-1432: 💤 Low valueCursor edge clamping may produce incorrect visual positioning.
Lines 1425-1426 clamp
mx/mywhen the cursor image would extend beyond the capture region, but this shifts the cursor position rather than clipping the cursor image. A cursor near the right edge could appear "stuck" atSCREEN_WIDTH - wrather than partially off-screen.Additionally, if
w > SCREEN_WIDTHorh > SCREEN_HEIGHT(unusual but possible with custom cursors), the subtraction would underflow the unsigned short.Consider clipping the cursor image instead of shifting position, or add a bounds check to avoid the subtraction underflow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshcore/KVM/Linux/linux_kvm.c` around lines 1418 - 1432, The current clamping logic for mx/my mutates cursor position causing the cursor to snap inward and can underflow when w/h exceed SCREEN_WIDTH/HEIGHT; instead compute and clip the visible source rectangle and destination offsets without changing the cursor's intended position (use signed ints for local computations): keep mx/my as the intended destination coordinates (based on localRx/localRy/xhot/yhot), then calculate visible_w = min(w, SCREEN_WIDTH - mx) and visible_h = min(h, SCREEN_HEIGHT - my), compute src_x/src_y offsets (0 if fully on-screen, or positive if part of cursor is off-screen on the left/top), and pass those visible dimensions and source offsets to bitblt (function bitblt and variables mx,my,w,h,localRx,localRy,xhot,yhot are the relevant symbols); also guard against negative visible_w/visible_h by skipping drawing when the visible area is <= 0 to avoid unsigned underflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/codeql-analysis.yml:
- Line 16: The workflow currently references tag-based and deprecated actions
(uses: actions/checkout@v4, github/codeql-action/init@v2,
github/codeql-action/analyze@v2); update each of those uses: entries to pin to
full commit SHAs instead of tags and replace the deprecated CodeQL v2 references
with the maintained CodeQL action commit SHAs. Specifically, locate the uses:
lines for actions/checkout, github/codeql-action/init, and
github/codeql-action/analyze and change their version specifiers to full commit
SHAs (the exact commit SHAs for the current stable/main commits of those repos).
In `@meshcore/KVM/Linux/linux_kvm.c`:
- Around line 536-541: The code currently casts g_monitors[i].x/y to unsigned
short before calling htons which causes negative coordinates to wrap; change the
encoding to preserve signed 16-bit values by casting through int16_t and then to
uint16_t for network order (e.g., htons((uint16_t)(int16_t)g_monitors[i].x) and
similarly for y) and document that X/Y are now signed int16 on-the-wire (or
alternatively clamp/offset X/Y if you must keep unsigned); update the
corresponding receiver/decoder to interpret those fields as signed int16 (and
leave width/height as unsigned) so negative monitor positions are transmitted
correctly.
In `@meshcore/KVM/MacOS/mac_kvm.c`:
- Line 989: The call to ILibProcessPipe_Pipe_CreateFromExisting in mac_kvm.c
(assigning g_kvmSocketPipe) passes two arguments but on WIN32 that macro maps to
ILibProcessPipe_Pipe_CreateFromExistingWithExtraMemory and requires the extra
HandleType/extraMemory argument; fix by making the call conditional: when WIN32
is defined invoke the WithExtraMemory variant (or add the third HandleType
argument plus 0 as required), otherwise keep the current two-arg call—use the
ILibProcessPipe_Pipe_CreateFromExisting/ILibProcessPipe_Pipe_CreateFromExistingWithExtraMemory
symbols and g_kvmSocketPipe to locate the change.
In `@meshservice/ServiceMain.c`:
- Around line 144-146: Replace the unsafe LoadLibraryExW calls that pass 0 with
a System32-restricted load: when loading Gdiplus.dll and Shlwapi.dll (symbols
_gdip and _shm using LoadLibraryExW), call LoadLibraryExW with
LOAD_LIBRARY_SEARCH_SYSTEM32 (or build a full path via GetSystemDirectoryW() +
"\\Gdiplus.dll"/"\\Shlwapi.dll" and pass that path) so the DLLs are loaded only
from System32 consistent with the other hardened loads in this file.
In `@microscript/ILibDuktape_SimpleDataStore.c`:
- Around line 202-208: The scoped-key creation using sprintf_s into
ILibScratchPad2 can fail (returning -1) when cguid + "/" + key doesn't fit, and
that invalid keyLen is then passed to ILibSimpleDataStore_DeleteEx; modify the
block that builds the scoped key (references: cguid, key, ILibScratchPad2,
sprintf_s, keyLen, ILibSimpleDataStore_DeleteEx) to first compute or check
required size (e.g. size_t need = strlen(cguid) + 1 + strlen(key) + 1) and if
need > sizeof(ILibScratchPad2) either allocate a temporary buffer of size need
or bail with a safe error/return, then call snprintf/sprintf_s into the chosen
buffer and verify the function return is non-negative before assigning
key/keyLen and calling ILibSimpleDataStore_DeleteEx.
In `@microstack/ILibCrypto.c`:
- Around line 547-585: The util_load_system_certs function must detect and
propagate failures instead of always returning 0: on WIN32 check the return of
CertOpenSystemStoreW and fail if NULL, check X509_STORE_add_cert return and
abort on error; on non-WIN32 check the boolean returns of
SSL_CTX_set_default_verify_paths and SSL_CTX_load_verify_locations and return an
error if they fail; when using the openssl version -d branch validate that
tokenization actually finds a directory before calling
SSL_CTX_load_verify_locations, check stat/pclose results and propagate failures
(non-zero) back to the caller rather than ignoring them so callers can tell when
no trust store was loaded.
- Around line 37-39: Include <sys/stat.h> for non-Windows builds that use TLS so
struct stat is defined (e.g., change the include guard to something like: not
_WIN32 / or !defined(_MSC_VER) instead of only `#ifdef` _POSIX) so
util_load_system_certs can compile; in util_load_system_certs check the return
values of SSL_CTX_set_default_verify_paths and SSL_CTX_load_verify_locations and
propagate a non-zero error return when they fail instead of always returning 0;
and remove the popen("openssl version -d", "r") fallback (or replace it with a
call to the OpenSSL-provided default directory macro/function such as
OPENSSLDIR/OPENSSL_VERSION_DIR or equivalent API) to avoid shell/PATH reliance —
update handling around the symbols SSL_CTX_set_default_verify_paths,
SSL_CTX_load_verify_locations and the popen fallback so failures are detected
and reported correctly.
- Around line 555-580: The code currently uses popen("openssl version -d", "r")
to discover an OpenSSL cert dir (the block that reads into buf, tokenized,
builds dest and calls SSL_CTX_load_verify_locations), which is unsafe in
privileged/service contexts; replace the popen/tokenize/strcat_s/stat logic with
a safe approach: call SSL_CTX_set_default_verify_paths(ctx) first to use
OpenSSL’s built-in probe, then fall back to checking a whitelist of known
absolute directories (e.g., "/etc/ssl/certs", "/usr/local/ssl/certs",
"/etc/pki/tls/certs") by stat() and calling SSL_CTX_load_verify_locations(ctx,
NULL, "<dir>") if they exist; if you must invoke the openssl binary, use
execve() with an absolute path to a vetted binary and a sanitized environment
instead of popen(), and remove the popen/tokenized/strcat_s usage entirely.
In `@microstack/ILibParsers.c`:
- Around line 9410-9417: The loop in ILibString_Cat_s uses destinationSize - 1
which skips checking the final byte and underflows when destinationSize == 0,
causing x to remain destinationSize and triggering false ILIBCRITICALEXIT; fix
by iterating up to destinationSize (or using size_t i < destinationSize), handle
the destinationSize == 0 case up-front, clamp x to at most destinationSize, and
change the bounds check to correctly compare (x + sourceLen + 1) >
destinationSize before calling memcpy_s and writing the NUL; update references
in this function (variables destinationSize, x, i, memcpy_s, ILIBCRITICALEXIT)
accordingly.
In `@microstack/ILibParsers.h`:
- Line 483: ILibMemory_Init_Size currently adds the entire aligned offset
instead of just the padding delta, causing over-allocation and making
ILibMemory_Size_Validate out of sync; update ILibMemory_Init_Size to add only
the alignment padding (use ILibMemory_Init_AlignPad(primaryLen) or equivalent
aligned-padding expression when extraLen>0) and mirror that same align-pad
expression in ILibMemory_Size_Validate so the validation logic matches the
computed allocation, and ensure ILibMemory_SmartAllocateEx uses the corrected
macros (reference ILibMemory_Init_Size, ILibMemory_Size_Validate,
ILibMemory_Init_AlignPad, and ILibMemory_SmartAllocateEx).
In `@microstack/ILibProcessPipe.c`:
- Around line 94-100: The code in ILibProcessPipe_JoinUserAuditSession_OSX
misinterprets proc_listallpids' return value: proc_listallpids(NULL,0) and the
subsequent call return the number of PIDs (not bytes), so change the post-call
handling to treat 'got' as the number of PIDs written rather than bytes;
specifically update the logic around proc_listallpids, the 'pids' buffer
allocation and the 'count' variable so that 'count' is set directly from 'got'
(and not got / sizeof(pid_t)) and ensure calloc sizing still uses 'n *
sizeof(pid_t)' and free(pids) on error remains intact.
In `@modules/RecoveryCore.js`:
- Around line 155-156: The current code sets woptions.rejectUnauthorized only
when global._MSH().validateWebCert === "true", missing other supported truthy
values like "1"; update the logic around woptions (the variable set from
http.parseUri(xurl)) to derive a boolean from global._MSH().validateWebCert that
treats both "true" and "1" as enabled (e.g., check value === "true" || value ===
"1") and assign that boolean to woptions.rejectUnauthorized so RecoveryCore
tunnel requests honor the same validateWebCert settings as ValidateMeshServer().
In `@modules/win-wmi.js`:
- Around line 273-315: The SAFEARRAY is locked with
OleAut32.SafeArrayAccessData(safeArray, arrayData) but never unlocked; after
populating arrayValues you must call OleAut32.SafeArrayUnaccessData(safeArray)
to release the lock (use the same safeArray variable). Wrap the access/iteration
over arrayData in a try/finally (or ensure finally-like behavior) so
OleAut32.SafeArrayUnaccessData(safeArray) is always invoked even on errors/early
returns; update the block around SafeArrayAccessData/arrayData/safeArray (the
loop that builds arrayValues and the assignment to values[properties[i]]) to
guarantee UnaccessData is called.
---
Outside diff comments:
In `@meshcore/KVM/Windows/kvm.c`:
- Around line 1379-1387: After calling ILibProcessPipe_Manager_SpawnProcessEx3
you must check whether gChildProcess is NULL before using it; if NULL, handle
the error (log via process/logger, set appropriate state, and avoid calling
ILibProcessPipe_Process_GetPID, ILibProcessPipe_Process_ResetMetadata, or
ILibProcessPipe_Process_AddHandlers) and return/cleanup as needed; update the
block around ILibProcessPipe_Manager_SpawnProcessEx3, gChildProcess,
ILibProcessPipe_Process_GetPID, ILibProcessPipe_Process_ResetMetadata, and
ILibProcessPipe_Process_AddHandlers to perform a NULL check and early error
handling when spawn fails.
---
Nitpick comments:
In `@com.meshagent.kvm.plist`:
- Around line 43-46: StandardOutPath and StandardErrorPath currently point to
/tmp which is world-readable; change the log destinations to a restrictive
location (e.g. /var/log/meshagent/meshagent-kvm.stdout.log and
/var/log/meshagent/meshagent-kvm.stderr.log) and ensure the installer or startup
logic creates that directory and files owned by the service user with
restrictive permissions (owner-only, e.g. 0600/0700). Update the plist keys
StandardOutPath and StandardErrorPath accordingly and add deployment/setup steps
to create /var/log/meshagent with correct ownership and permissions so logs are
not world-readable.
In `@meshcore/KVM/Linux/linux_kvm.c`:
- Around line 1418-1432: The current clamping logic for mx/my mutates cursor
position causing the cursor to snap inward and can underflow when w/h exceed
SCREEN_WIDTH/HEIGHT; instead compute and clip the visible source rectangle and
destination offsets without changing the cursor's intended position (use signed
ints for local computations): keep mx/my as the intended destination coordinates
(based on localRx/localRy/xhot/yhot), then calculate visible_w = min(w,
SCREEN_WIDTH - mx) and visible_h = min(h, SCREEN_HEIGHT - my), compute
src_x/src_y offsets (0 if fully on-screen, or positive if part of cursor is
off-screen on the left/top), and pass those visible dimensions and source
offsets to bitblt (function bitblt and variables
mx,my,w,h,localRx,localRy,xhot,yhot are the relevant symbols); also guard
against negative visible_w/visible_h by skipping drawing when the visible area
is <= 0 to avoid unsigned underflow.
In `@meshcore/KVM/MacOS/mac_kvm.c`:
- Around line 437-444: The write to the socket-mode pipe currently ignores
ILibProcessPipe_Pipe_Write's return (ILibTransport_DoneState); capture its
return value and handle failures immediately: call
ILibProcessPipe_Pipe_Write(g_kvmSocketPipe, buf, len,
ILibTransport_MemoryOwnership_USER) into a variable of type
ILibTransport_DoneState, check for a non-success state, and on error log the
failure, close/cleanup the g_kvmSocketPipe (and set g_kvmSocketPipe = NULL) to
avoid further writes, and return an error indicator (e.g., -1) instead of len so
the caller knows the write failed. Ensure you reference g_kvmSocketPipe,
ILibProcessPipe_Pipe_Write, and ILibTransport_DoneState when making the change.
In `@meshcore/KVM/Windows/kvm.c`:
- Around line 869-875: The code drops partial packet bytes when ptr < len;
preserve the leftover bytes by memmoving pchRequest2[ptr..len-1] to the start
before the next read. Replace the TODO with logic like: compute remaining = len
- ptr; memmove(pchRequest2, pchRequest2 + ptr, remaining); set len = remaining
and ptr = 0 (and handle the ptr == len case as before), ensuring you check
buffer capacity before the next read to avoid overflow. Use the existing buffer
variables (pchRequest2, ptr, len) and memmove to safely handle overlapping
memory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 40531920-eb50-47eb-b9e7-e6b3618ebcfc
📒 Files selected for processing (35)
.github/workflows/codeql-analysis.ymlcom.meshagent.kvm.plistmeshconsole/MeshConsole-2022.vcxprojmeshconsole/main.cmeshcore/KVM/Linux/linux_kvm.cmeshcore/KVM/MacOS/mac_events.cmeshcore/KVM/MacOS/mac_kvm.cmeshcore/KVM/Windows/kvm.cmeshcore/KVM/Windows/kvm.hmeshcore/agentcore.cmeshcore/zlib/zutil.hmeshservice/MeshService-2022.vcxprojmeshservice/MeshService.rcmeshservice/ServiceMain.cmicroscript/ILibDuktape_Commit.hmicroscript/ILibDuktape_Polyfills.cmicroscript/ILibDuktape_SimpleDataStore.cmicroscript/ILibDuktape_net.cmicrostack/ILibCrypto.cmicrostack/ILibCrypto.hmicrostack/ILibParsers.cmicrostack/ILibParsers.hmicrostack/ILibProcessPipe.cmicrostack/ILibWebClient.cmicrostack/ILibWebClient.hmicrostack/ILibWebServer.cmicrostack/ILibWrapperWebRTC.cmodules/RecoveryCore.jsmodules/agent-installer.jsmodules/dbTool.jsmodules/meshcmd.jsmodules/monitor-border.jsmodules/notifybar-desktop.jsmodules/win-wmi.jsreadme.md
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v2 | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and display the workflow section around the referenced lines
FILE=".github/workflows/codeql-analysis.yml"
echo "== File exists? =="
test -f "$FILE" && echo "Found: $FILE" || (echo "Missing: $FILE" && exit 1)
echo
echo "== Lines 1-120 =="
nl -ba "$FILE" | sed -n '1,120p'Repository: flamingo-stack/meshagent
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/codeql-analysis.yml"
echo "== Relevant lines (1-120) with numbers =="
awk 'NR>=1 && NR<=120 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "== Lines mentioning github/codeql-action =="
rg -n "github/codeql-action/" "$FILE" || true
echo
echo "== Lines mentioning actions/checkout =="
rg -n "actions/checkout@" "$FILE" || true
echo
echo "== Full file (only if <= 200 lines) =="
LINES=$(wc -l < "$FILE" 2>/dev/null || echo 999999)
if [ "$LINES" -le 200 ]; then
awk '{printf "%d\t%s\n", NR, $0}' "$FILE"
else
echo "Skipping full file; file has $LINES lines."
fiRepository: flamingo-stack/meshagent
Length of output: 3846
🌐 Web query:
GitHub documentation github/codeql-action init analyze v2 deprecation v3 supported runners pin to SHA
💡 Result:
As of June 2, 2026, the GitHub CodeQL Action is currently in its v4 release cycle, which is the recommended version for all users [1]. Deprecation Status of Older Versions: - CodeQL Action v2: This version was officially retired and deprecated in early 2025 following the deprecation of GitHub Enterprise Server (GHES) 3.11 [2]. - CodeQL Action v3: This version is scheduled for official deprecation in December 2026, coinciding with the deprecation of GHES 3.19 [3][4]. Users are advised to migrate to v4 before this date to ensure continued access to new features and security updates [3][4]. Migration to v4: To upgrade your workflows, replace all references to v3 with v4 in your workflow files (e.g., init@v3 to init@v4, analyze@v3 to analyze@v4) [4]. Note that on older GHES versions, you may need to ensure your administrator has enabled GitHub Connect to download the necessary action versions [4]. Best Practices for Pinning: While using version tags like v4 is common for flexibility, it is not cryptographically immutable, as tags can be moved or deleted [5][6]. For maximum security, GitHub and security experts strongly recommend pinning GitHub Actions to a full-length commit SHA [5][7]. This prevents unauthorized code changes from being automatically pulled into your pipeline if an action repository is compromised [8][6]. Recommended Pinning Strategy: To maintain both security and maintainability, use the following pattern in your workflow files [6]: - uses: github/codeql-action/init@ # v4 By including the tag version in a comment, you maintain readability and compatibility with automated update tools like Dependabot, while the SHA ensures immutability [6]. You can audit your current workflow files for unpinned actions using grep or specialized security tooling [6].
Citations:
- 1: https://github.com/github/codeql-action
- 2: https://github.blog/changelog/2025-01-10-code-scanning-codeql-action-v2-is-now-deprecated/
- 3: [GitHub Actions] CodeQL Action v3 will be deprecated in December 2026 DSpace/dspace-angular#5153
- 4: https://github.blog/changelog/2025-10-28-upcoming-deprecation-of-codeql-action-v3/
- 5: https://docs.github.com/en/actions/reference/security/secure-use
- 6: https://dev.to/unbearablelabs/if-you-use-trivy-or-kics-in-ci-read-this-4d91
- 7: https://www.stepsecurity.io/blog/pinning-github-actions-for-enhanced-security-a-complete-guide
- 8: https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
Pin CodeQL GitHub Actions to commit SHAs (v2 is deprecated).
The workflow uses github/codeql-action/init@v2 (line 29) and github/codeql-action/analyze@v2 (line 52); CodeQL action v2 is deprecated/retired, so the scan may fail or stop receiving fixes. Also, actions/checkout@v4 (line 16) is tag-based and not pinned—pin these uses: entries to full commit SHAs for immutability.
Also applies to: 29-29, 52-52
🧰 Tools
🪛 zizmor (1.25.2)
[error] 16-16: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/codeql-analysis.yml at line 16, The workflow currently
references tag-based and deprecated actions (uses: actions/checkout@v4,
github/codeql-action/init@v2, github/codeql-action/analyze@v2); update each of
those uses: entries to pin to full commit SHAs instead of tags and replace the
deprecated CodeQL v2 references with the maintained CodeQL action commit SHAs.
Specifically, locate the uses: lines for actions/checkout,
github/codeql-action/init, and github/codeql-action/analyze and change their
version specifiers to full commit SHAs (the exact commit SHAs for the current
stable/main commits of those repos).
Comment on lines
+536
to
+541
| int base = 2 + (i * 5); // index into unsigned short array | ||
| ((unsigned short*)buffer)[base + 0] = (unsigned short)htons((unsigned short)g_monitors[i].id); | ||
| ((unsigned short*)buffer)[base + 1] = (unsigned short)htons((unsigned short)g_monitors[i].x); | ||
| ((unsigned short*)buffer)[base + 2] = (unsigned short)htons((unsigned short)g_monitors[i].y); | ||
| ((unsigned short*)buffer)[base + 3] = (unsigned short)htons((unsigned short)g_monitors[i].width); | ||
| ((unsigned short*)buffer)[base + 4] = (unsigned short)htons((unsigned short)g_monitors[i].height); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Windows KVM or protocol handles negative monitor coordinates
rg -n 'MNG_KVM_DISPLAY_INFO' --type c -C 5Repository: flamingo-stack/meshagent
Length of output: 3755
🏁 Script executed:
#!/bin/bash
# Inspect Windows callback that populates MNG_KVM_DISPLAY_INFO payload (X/Y/W/H handling)
rg -n 'DisplayInfoEnumProc_Info' meshcore/KVM/Windows/kvm.cRepository: flamingo-stack/meshagent
Length of output: 265
🏁 Script executed:
#!/bin/bash
# Show surrounding code where the callback writes monitor data into dwData
python3 - <<'PY'
import re,sys,subprocess,os,textwrap,json
path="meshcore/KVM/Windows/kvm.c"
# print a broad region around the rg hit line numbers by scanning file for token
with open(path,'r',encoding='utf-8',errors='ignore') as f:
lines=f.readlines()
# find all occurrences of DisplayInfoEnumProc_Info
idx=[i for i,l in enumerate(lines) if 'DisplayInfoEnumProc_Info' in l]
for i in idx:
start=max(0,i-40); end=min(len(lines), i+140)
print(f"\n--- {path}:{start+1}-{end} ---\n")
for j in range(start,end):
# keep line numbers
print(f"{j+1}:{lines[j].rstrip()}")
PYRepository: flamingo-stack/meshagent
Length of output: 14201
🏁 Script executed:
#!/bin/bash
rg -n 'MNG_KVM_DISPLAY_INFO' -S .Repository: flamingo-stack/meshagent
Length of output: 775
🏁 Script executed:
#!/bin/bash
# Find where MNG_KVM_DISPLAY_INFO is parsed/used (client/server-side decoding)
rg -n 'MNG_KVM_DISPLAY_INFO' .Repository: flamingo-stack/meshagent
Length of output: 775
🏁 Script executed:
#!/bin/bash
sed -n '1430,1515p' test/self-test.jsRepository: flamingo-stack/meshagent
Length of output: 3640
Fix negative monitor X/Y encoding to avoid wrap-around
meshcore/KVM/Linux/linux_kvm.c casts g_monitors[i].x/y to unsigned short before htons, which will wrap for negative coordinates. The Windows sender does the same for mi.rcMonitor.left/top, and the receiver decodes X/Y/W/H as UInt16BE, so negative values are never representable correctly. Document the “non-negative only” constraint, or change the protocol/encoding (e.g., signed int16 or an offset/clamping scheme).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@meshcore/KVM/Linux/linux_kvm.c` around lines 536 - 541, The code currently
casts g_monitors[i].x/y to unsigned short before calling htons which causes
negative coordinates to wrap; change the encoding to preserve signed 16-bit
values by casting through int16_t and then to uint16_t for network order (e.g.,
htons((uint16_t)(int16_t)g_monitors[i].x) and similarly for y) and document that
X/Y are now signed int16 on-the-wire (or alternatively clamp/offset X/Y if you
must keep unsigned); update the corresponding receiver/decoder to interpret
those fields as signed int16 (and leave width/height as unsigned) so negative
monitor positions are transmitted correctly.
| g_kvmSocketFD = fd; | ||
| g_kvmSocketWriteHandler = writeHandler; | ||
| g_kvmSocketReserved = reserved; | ||
| g_kvmSocketPipe = ILibProcessPipe_Pipe_CreateFromExisting((ILibProcessPipe_Manager)processPipeMgr, fd); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the macro definition for ILibProcessPipe_Pipe_CreateFromExisting
rg -n 'ILibProcessPipe_Pipe_CreateFromExisting' --type=c -A2Repository: flamingo-stack/meshagent
Length of output: 3286
Fix macro argument mismatch for ILibProcessPipe_Pipe_CreateFromExisting
microstack/ILibProcessPipe.hdefinesILibProcessPipe_Pipe_CreateFromExisting(PipeManager, ExistingPipe)with 2 args only whenWIN32is not defined; whenWIN32is defined it maps toILibProcessPipe_Pipe_CreateFromExistingWithExtraMemory(PipeManager, ExistingPipe, HandleType, 0)and therefore requires 3 args.meshcore/KVM/MacOS/mac_kvm.cpasses only 2 args at line 989, so adjust the call for theWIN32case (or ensureWIN32isn’t defined for this build).
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 989-989: failed to expand 'ILibProcessPipe_Pipe_CreateFromExisting', Wrong number of parameters for macro 'ILibProcessPipe_Pipe_CreateFromExisting'.
(syntaxError)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@meshcore/KVM/MacOS/mac_kvm.c` at line 989, The call to
ILibProcessPipe_Pipe_CreateFromExisting in mac_kvm.c (assigning g_kvmSocketPipe)
passes two arguments but on WIN32 that macro maps to
ILibProcessPipe_Pipe_CreateFromExistingWithExtraMemory and requires the extra
HandleType/extraMemory argument; fix by making the call conditional: when WIN32
is defined invoke the WithExtraMemory variant (or add the third HandleType
argument plus 0 as required), otherwise keep the current two-arg call—use the
ILibProcessPipe_Pipe_CreateFromExisting/ILibProcessPipe_Pipe_CreateFromExistingWithExtraMemory
symbols and g_kvmSocketPipe to locate the change.
Comment on lines
+144
to
+146
| _gdip = LoadLibraryExW(L"Gdiplus.dll", NULL, 0); | ||
| if (_gdip == NULL) { return; } | ||
| _shm = LoadLibraryExW(L"Shlwapi.dll", NULL, LOAD_LIBRARY_SEARCH_USER_DIRS); | ||
| if (_shm == NULL) { _gdip = LoadLibraryExW(L"Shlwapi.dll", NULL, 0); } | ||
| _shm = LoadLibraryExW(L"Shlwapi.dll", NULL, 0); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section around the reported lines
sed -n '120,170p' meshservice/ServiceMain.c
# Find any other DLL-loading hardening in this file or nearby
rg -n "LoadLibraryExW|LoadLibraryW|SetDefaultDllDirectories|AddDllDirectory|GetSystemDirectoryW|LOAD_LIBRARY_SEARCH_SYSTEM32|LOAD_LIBRARY_SEARCH_DEFAULT_DIRS|LOAD_LIBRARY_SEARCH_USER_DIRS" meshservice/ServiceMain.c meshservice -S || true
# If the snippet is part of a larger function, print the function header region too
# (best effort: show some lines above the first match)
start=$(rg -n "_gdip\s*=\s*LoadLibraryExW" -n meshservice/ServiceMain.c | cut -d: -f1 | head -n1 || true)
if [[ -n "${start:-}" ]]; then
low=$((start-40)); high=$((start+60))
sed -n "${low},${high}p" meshservice/ServiceMain.c
fiRepository: flamingo-stack/meshagent
Length of output: 9786
🌐 Web query:
LoadLibraryExW flags 0 DLL search order vs LOAD_LIBRARY_SEARCH_SYSTEM32 LOAD_LIBRARY_SEARCH_DEFAULT_DIRS hijack risk
💡 Result:
When calling LoadLibraryExW without specific LOAD_LIBRARY_SEARCH flags (often referred to as flags 0), Windows uses the default DLL search order [1][2][3]. This standard order includes highly vulnerable locations such as the application directory, the current working directory, and directories in the system PATH [4][5]. If an attacker places a malicious DLL with the same name as an expected library in one of these directories—particularly the application or current working directory—the system may load the malicious code instead of the intended DLL, a technique known as DLL search order hijacking [6][5][7][8]. In contrast, using LOAD_LIBRARY_SEARCH flags allows a developer to restrict the search to specific, trusted locations [1][2]. Specifically, LOAD_LIBRARY_SEARCH_DEFAULT_DIRS and the other LOAD_LIBRARY_SEARCH_* flags override the standard search order [1][2]. When these flags are used, the system only searches the directories explicitly permitted by the flags (e.g., system directories or user-defined paths added via AddDllDirectory) [1][2]. This effectively eliminates the risk associated with arbitrary directories like the current working directory [2][9]. Key differences and risk factors include: Standard Search Order (Flags 0): - Behavior: Searches multiple locations including the application directory, current working directory, and PATH environment variables in a predefined sequence [4][5]. - Hijack Risk: High. An attacker with write access to any of these directories can place a malicious DLL and trigger execution when a vulnerable application attempts to load it [6][5][7]. Secure Search Flags (e.g., LOAD_LIBRARY_SEARCH_SYSTEM32): - Behavior: Restricts the search to specifically authorized directories (such as System32 or paths added to the process search path) [1][2]. - Hijack Risk: Significantly reduced. By explicitly excluding vulnerable directories like the current working directory, this approach prevents the most common hijacking vectors [2][9]. To further mitigate risk, Microsoft recommends using fully qualified paths whenever the location of a DLL is known [4][7]. If using LoadLibraryExW, developers should consistently employ secure search flags to enforce a deterministic and hardened DLL loading process [10][9].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order
- 2: https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibraryexa
- 3: https://learn.microsoft.com/en-us/windows/win32/api/LibLoaderAPI/nf-libloaderapi-loadlibraryexa
- 4: https://www.microsoft.com/en-us/msrc/blog/2014/05/load-library-safely
- 5: https://dev.to/satyam_rastogi/muddywater-dll-side-loading-nine-country-espionage-campaign-analysis-3ld
- 6: https://redcanary.com/threat-detection-report/techniques/dll-search-order-hijacking/
- 7: https://unit42.paloaltonetworks.com/dll-hijacking-techniques/
- 8: https://www.okta.com/identity-101/dll-hijacking/
- 9: https://stillu.cc/threat-spotlight/2020/09/16/dll-hijacking/
- 10: https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-security
Restrict Gdiplus/Shlwapi DLL loads to System32
In meshservice/ServiceMain.c lines 144-146, the calls _gdip = LoadLibraryExW(L"Gdiplus.dll", NULL, 0); and _shm = LoadLibraryExW(L"Shlwapi.dll", NULL, 0); use flags 0 (default DLL search order), increasing DLL search-order hijack risk. Switch to LOAD_LIBRARY_SEARCH_SYSTEM32 (or an explicit System32-qualified path via GetSystemDirectoryW()), matching the other hardened LOAD_LIBRARY_SEARCH_SYSTEM32 loads already present later in this file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@meshservice/ServiceMain.c` around lines 144 - 146, Replace the unsafe
LoadLibraryExW calls that pass 0 with a System32-restricted load: when loading
Gdiplus.dll and Shlwapi.dll (symbols _gdip and _shm using LoadLibraryExW), call
LoadLibraryExW with LOAD_LIBRARY_SEARCH_SYSTEM32 (or build a full path via
GetSystemDirectoryW() + "\\Gdiplus.dll"/"\\Shlwapi.dll" and pass that path) so
the DLLs are loaded only from System32 consistent with the other hardened loads
in this file.
Comment on lines
+202
to
208
| if (cguid != NULL) | ||
| { | ||
| keyLen = sprintf_s(ILibScratchPad2, sizeof(ILibScratchPad2), "%s/%s", cguid, key); | ||
| key = ILibScratchPad2; | ||
| } | ||
|
|
||
| ILibSimpleDataStore_DeleteEx(ds, key, (int)keyLen); |
There was a problem hiding this comment.
Guard the scoped-key construction against sprintf_s() failure.
If cguid + "/" + key does not fit in ILibScratchPad2, sprintf_s() returns -1. Here that value is stored in keyLen and then passed to ILibSimpleDataStore_DeleteEx() as an int, so long keys can turn into an invalid delete length. Please check the required size first or reject/allocate oversized scoped keys instead of relying on sprintf_s() here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@microscript/ILibDuktape_SimpleDataStore.c` around lines 202 - 208, The
scoped-key creation using sprintf_s into ILibScratchPad2 can fail (returning -1)
when cguid + "/" + key doesn't fit, and that invalid keyLen is then passed to
ILibSimpleDataStore_DeleteEx; modify the block that builds the scoped key
(references: cguid, key, ILibScratchPad2, sprintf_s, keyLen,
ILibSimpleDataStore_DeleteEx) to first compute or check required size (e.g.
size_t need = strlen(cguid) + 1 + strlen(key) + 1) and if need >
sizeof(ILibScratchPad2) either allocate a temporary buffer of size need or bail
with a safe error/return, then call snprintf/sprintf_s into the chosen buffer
and verify the function return is non-negative before assigning key/keyLen and
calling ILibSimpleDataStore_DeleteEx.
Comment on lines
+9410
to
+9417
| size_t x = destinationSize; | ||
| for (i = 0; i < destinationSize - 1; ++i) | ||
| { | ||
| if (destination[i] == 0) { *x = i; break; } | ||
| if (destination[i] == 0) { x = i; break; } | ||
| } | ||
| if (x == NULL || ((*x + sourceLen + 1 )> destinationSize)) { ILIBCRITICALEXIT(254); } | ||
| memcpy_s(destination + *x, destinationSize - *x, source, sourceLen); | ||
| destination[*x + sourceLen] = 0; | ||
| if (((x + sourceLen + 1 )> destinationSize)) { ILIBCRITICALEXIT(254); } | ||
| memcpy_s(destination + x, destinationSize - x, source, sourceLen); | ||
| destination[x + sourceLen] = 0; |
There was a problem hiding this comment.
Fix boundary scan in ILibString_Cat_s to avoid false aborts and size-underflow iteration.
Line 9411 uses destinationSize - 1 as the loop bound, so a NUL at the last valid slot is never found; x stays destinationSize, and Line 9415 aborts even on safe empty appends. Also, destinationSize == 0 underflows the loop bound.
💡 Proposed fix
char* ILibString_Cat_s(char *destination, size_t destinationSize, char *source)
{
size_t sourceLen = strnlen_s(source, destinationSize);
size_t i;
- size_t x = destinationSize;
- for (i = 0; i < destinationSize - 1; ++i)
+ if (destinationSize == 0) { ILIBCRITICALEXIT(254); }
+ size_t x = destinationSize;
+ for (i = 0; i < destinationSize; ++i)
{
if (destination[i] == 0) { x = i; break; }
}
- if (((x + sourceLen + 1 )> destinationSize)) { ILIBCRITICALEXIT(254); }
+ if (x == destinationSize || sourceLen >= (destinationSize - x)) { ILIBCRITICALEXIT(254); }
memcpy_s(destination + x, destinationSize - x, source, sourceLen);
destination[x + sourceLen] = 0;
return(destination);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@microstack/ILibParsers.c` around lines 9410 - 9417, The loop in
ILibString_Cat_s uses destinationSize - 1 which skips checking the final byte
and underflows when destinationSize == 0, causing x to remain destinationSize
and triggering false ILIBCRITICALEXIT; fix by iterating up to destinationSize
(or using size_t i < destinationSize), handle the destinationSize == 0 case
up-front, clamp x to at most destinationSize, and change the bounds check to
correctly compare (x + sourceLen + 1) > destinationSize before calling memcpy_s
and writing the NUL; update references in this function (variables
destinationSize, x, i, memcpy_s, ILIBCRITICALEXIT) accordingly.
|
|
||
| #define ILibMemory_Size_Validate(primaryLen, extraLen) (((size_t)(primaryLen)<(UINT32_MAX - (size_t)(extraLen)))&&((size_t)(extraLen)<(UINT32_MAX-(size_t)(primaryLen)))&&((size_t)((primaryLen) + (extraLen))<(UINT32_MAX - sizeof(ILibMemory_Header)))&&((extraLen)==0 || ((size_t)((primaryLen)+(extraLen)+sizeof(ILibMemory_Header))<(UINT32_MAX-sizeof(ILibMemory_Header))))) | ||
| #define ILibMemory_Init_Size(primaryLen, extraLen) (primaryLen + extraLen + sizeof(ILibMemory_Header) + (extraLen>0?sizeof(ILibMemory_Header):0)) | ||
| #define ILibMemory_Init_Size(primaryLen, extraLen) (primaryLen + extraLen + sizeof(ILibMemory_Header) + (extraLen>0?sizeof(ILibMemory_Header) + (((primaryLen + sizeof(ILibMemory_Header)) + sizeof(void *) - 1) & ~(sizeof(void *) - 1)):0)) |
There was a problem hiding this comment.
Fix the aligned-size term and keep validation in sync.
Line 483 is adding the entire aligned offset, not just the padding delta, so extraLen > 0 now overstates the allocation by roughly primaryLen + sizeof(ILibMemory_Header). More importantly, ILibMemory_Size_Validate() on Line 482 still validates the old formula, so ILibMemory_SmartAllocateEx() can accept sizes that overflow the final allocation expression on constrained builds.
Suggested fix
+#define ILibMemory_Init_AlignPad(primaryLen) \
+ ((((primaryLen) + sizeof(ILibMemory_Header) + sizeof(void*) - 1) & ~(sizeof(void*) - 1)) - ((primaryLen) + sizeof(ILibMemory_Header)))
- `#define` ILibMemory_Init_Size(primaryLen, extraLen) (primaryLen + extraLen + sizeof(ILibMemory_Header) + (extraLen>0?sizeof(ILibMemory_Header) + (((primaryLen + sizeof(ILibMemory_Header)) + sizeof(void *) - 1) & ~(sizeof(void *) - 1)):0))
+ `#define` ILibMemory_Init_Size(primaryLen, extraLen) ((primaryLen) + (extraLen) + sizeof(ILibMemory_Header) + ((extraLen)>0 ? sizeof(ILibMemory_Header) + ILibMemory_Init_AlignPad(primaryLen) : 0))You should also mirror ILibMemory_Init_AlignPad(primaryLen) in ILibMemory_Size_Validate(...) so the guard and the computed size stay equivalent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@microstack/ILibParsers.h` at line 483, ILibMemory_Init_Size currently adds
the entire aligned offset instead of just the padding delta, causing
over-allocation and making ILibMemory_Size_Validate out of sync; update
ILibMemory_Init_Size to add only the alignment padding (use
ILibMemory_Init_AlignPad(primaryLen) or equivalent aligned-padding expression
when extraLen>0) and mirror that same align-pad expression in
ILibMemory_Size_Validate so the validation logic matches the computed
allocation, and ensure ILibMemory_SmartAllocateEx uses the corrected macros
(reference ILibMemory_Init_Size, ILibMemory_Size_Validate,
ILibMemory_Init_AlignPad, and ILibMemory_SmartAllocateEx).
Comment on lines
+94
to
+100
| int n = proc_listallpids(NULL, 0); | ||
| if (n <= 0) return -1; | ||
| pid_t *pids = (pid_t*)calloc(n, sizeof(pid_t)); | ||
| if (pids == NULL) return -1; | ||
| int got = proc_listallpids(pids, n * (int)sizeof(pid_t)); | ||
| if (got <= 0) { free(pids); return -1; } | ||
| int count = got / (int)sizeof(pid_t); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What does proc_listallpids return when buffer is NULL on macOS?
💡 Result:
When you call proc_listallpids with buffer set to NULL on macOS, it returns the total number of currently running processes [1][2]. This behavior is a common pattern used to determine the required size for a buffer before allocating memory for a subsequent call to retrieve the actual process IDs [3][2]. Specifically, developers often call the function with NULL as the buffer argument and 0 as the buffersize argument to obtain the process count [4][2]. Once the count is obtained, it can be used to allocate a buffer (usually of size count * sizeof(pid_t)) to store the process identifiers in a second call [3][2]. Note that while some similar functions in the libproc API return the required buffer size in bytes, proc_listallpids specifically returns the count of PIDs [1][3].
Citations:
- 1: https://github.com/apple-opensource/xnu/blob/4f43d4276fc6a87f2461a3ab18287e4a2e5a1cc0/libsyscall/wrappers/libproc/libproc.c
- 2: https://taomm.org/vol2/pdfs/CH%201%20Examining%20Processes.pdf
- 3: https://gist.github.com/de66b0ea2144e20ddd844c41c9d93eb9
- 4: https://stackoverflow.com/questions/76498083/how-to-interpret-results-of-calling-proc-listpids-function-on-macos
Fix proc_listallpids return-value handling in ILibProcessPipe_JoinUserAuditSession_OSX
On macOS, proc_listallpids(NULL, 0) returns a PID count (not a byte size), so calloc(n, sizeof(pid_t)) is consistent; the count computation is the problematic part. If the buffered call returns the number of PIDs written (as per the libproc contract), then count = got / sizeof(pid_t) is wrong and should just use got directly.
Proposed change
- int count = got / (int)sizeof(pid_t);
+ int count = got;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| int n = proc_listallpids(NULL, 0); | |
| if (n <= 0) return -1; | |
| pid_t *pids = (pid_t*)calloc(n, sizeof(pid_t)); | |
| if (pids == NULL) return -1; | |
| int got = proc_listallpids(pids, n * (int)sizeof(pid_t)); | |
| if (got <= 0) { free(pids); return -1; } | |
| int count = got / (int)sizeof(pid_t); | |
| int n = proc_listallpids(NULL, 0); | |
| if (n <= 0) return -1; | |
| pid_t *pids = (pid_t*)calloc(n, sizeof(pid_t)); | |
| if (pids == NULL) return -1; | |
| int got = proc_listallpids(pids, n * (int)sizeof(pid_t)); | |
| if (got <= 0) { free(pids); return -1; } | |
| int count = got; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@microstack/ILibProcessPipe.c` around lines 94 - 100, The code in
ILibProcessPipe_JoinUserAuditSession_OSX misinterprets proc_listallpids' return
value: proc_listallpids(NULL,0) and the subsequent call return the number of
PIDs (not bytes), so change the post-call handling to treat 'got' as the number
of PIDs written rather than bytes; specifically update the logic around
proc_listallpids, the 'pids' buffer allocation and the 'count' variable so that
'count' is set directly from 'got' (and not got / sizeof(pid_t)) and ensure
calloc sizing still uses 'n * sizeof(pid_t)' and free(pids) on error remains
intact.
Comment on lines
+273
to
+315
| if (isArray) | ||
| { | ||
| case 0x0000: // VT_EMPTY | ||
| case 0x0001: // VT_NULL | ||
| values[properties[i]] = null; | ||
| break; | ||
| case 0x0002: // VT_I2 | ||
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readInt16LE(); | ||
| break; | ||
| case 0x0003: // VT_I4 | ||
| case 0x0016: // VT_INT | ||
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readInt32LE(); | ||
| break; | ||
| case 0x000B: // VT_BOOL | ||
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readInt32LE() != 0; | ||
| break; | ||
| case 0x000E: // VT_DECIMAL | ||
| break; | ||
| case 0x0010: // VT_I1 | ||
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readInt8(); | ||
| break; | ||
| case 0x0011: // VT_UI1 | ||
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readUInt8(); | ||
| break; | ||
| case 0x0012: // VT_UI2 | ||
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readUInt16LE(); | ||
| break; | ||
| case 0x0013: // VT_UI4 | ||
| case 0x0017: // VT_UINT | ||
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readUInt32LE(); | ||
| break; | ||
| //case 0x0014: // VT_I8 | ||
| // break; | ||
| //case 0x0015: // VT_UI8 | ||
| // break; | ||
| case 0x0008: // VT_BSTR | ||
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).Deref().Wide2UTF8; | ||
| break; | ||
| default: | ||
| console.info1('VARTYPE: ' + tmp1.toBuffer().readUInt16LE()); | ||
| break; | ||
| // Handle array types (VT_ARRAY | base type) | ||
| var safeArray = tmp1.Deref(8, GM.PointerSize).Deref(); | ||
| var arrayLength = safeArray.Deref(GM.PointerSize == 8 ? 24 : 16, 4).toBuffer().readUInt32LE(); | ||
| var arrayData = GM.CreatePointer(); | ||
| OleAut32.SafeArrayAccessData(safeArray, arrayData); | ||
|
|
||
| var arrayValues = []; | ||
| for (var k = 0; k < arrayLength; ++k) | ||
| { | ||
| switch (baseType) | ||
| { | ||
| case 0x0002: // VT_I2 | ||
| arrayValues.push(arrayData.Deref().Deref(k * 2, 2).toBuffer().readInt16LE()); | ||
| break; | ||
| case 0x0003: // VT_I4 | ||
| case 0x0016: // VT_INT | ||
| arrayValues.push(arrayData.Deref().Deref(k * 4, 4).toBuffer().readInt32LE()); | ||
| break; | ||
| case 0x000B: // VT_BOOL | ||
| arrayValues.push(arrayData.Deref().Deref(k * 2, 2).toBuffer().readInt16LE() != 0); | ||
| break; | ||
| case 0x0010: // VT_I1 | ||
| arrayValues.push(arrayData.Deref().Deref(k, 1).toBuffer().readInt8()); | ||
| break; | ||
| case 0x0011: // VT_UI1 | ||
| arrayValues.push(arrayData.Deref().Deref(k, 1).toBuffer().readUInt8()); | ||
| break; | ||
| case 0x0012: // VT_UI2 | ||
| arrayValues.push(arrayData.Deref().Deref(k * 2, 2).toBuffer().readUInt16LE()); | ||
| break; | ||
| case 0x0013: // VT_UI4 | ||
| case 0x0017: // VT_UINT | ||
| arrayValues.push(arrayData.Deref().Deref(k * 4, 4).toBuffer().readUInt32LE()); | ||
| break; | ||
| case 0x0008: // VT_BSTR | ||
| arrayValues.push(arrayData.Deref().Deref(k * GM.PointerSize, GM.PointerSize).Deref().Wide2UTF8); | ||
| break; | ||
| } | ||
| } | ||
| values[properties[i]] = arrayValues; | ||
| } |
There was a problem hiding this comment.
Missing SafeArrayUnaccessData call causes resource leak.
Line 279 calls SafeArrayAccessData to lock and access the SAFEARRAY data, but there is no corresponding SafeArrayUnaccessData call after the array iteration completes (line 313). According to Microsoft documentation, every SafeArrayAccessData must be paired with SafeArrayUnaccessData to unlock the array data. Without this, the array remains locked, potentially leaking resources and preventing proper cleanup.
🔓 Proposed fix to unlock SAFEARRAY data
}
values[properties[i]] = arrayValues;
+ OleAut32.SafeArrayUnaccessData(safeArray);
}
else📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (isArray) | |
| { | |
| case 0x0000: // VT_EMPTY | |
| case 0x0001: // VT_NULL | |
| values[properties[i]] = null; | |
| break; | |
| case 0x0002: // VT_I2 | |
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readInt16LE(); | |
| break; | |
| case 0x0003: // VT_I4 | |
| case 0x0016: // VT_INT | |
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readInt32LE(); | |
| break; | |
| case 0x000B: // VT_BOOL | |
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readInt32LE() != 0; | |
| break; | |
| case 0x000E: // VT_DECIMAL | |
| break; | |
| case 0x0010: // VT_I1 | |
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readInt8(); | |
| break; | |
| case 0x0011: // VT_UI1 | |
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readUInt8(); | |
| break; | |
| case 0x0012: // VT_UI2 | |
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readUInt16LE(); | |
| break; | |
| case 0x0013: // VT_UI4 | |
| case 0x0017: // VT_UINT | |
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).toBuffer().readUInt32LE(); | |
| break; | |
| //case 0x0014: // VT_I8 | |
| // break; | |
| //case 0x0015: // VT_UI8 | |
| // break; | |
| case 0x0008: // VT_BSTR | |
| values[properties[i]] = tmp1.Deref(8, GM.PointerSize).Deref().Wide2UTF8; | |
| break; | |
| default: | |
| console.info1('VARTYPE: ' + tmp1.toBuffer().readUInt16LE()); | |
| break; | |
| // Handle array types (VT_ARRAY | base type) | |
| var safeArray = tmp1.Deref(8, GM.PointerSize).Deref(); | |
| var arrayLength = safeArray.Deref(GM.PointerSize == 8 ? 24 : 16, 4).toBuffer().readUInt32LE(); | |
| var arrayData = GM.CreatePointer(); | |
| OleAut32.SafeArrayAccessData(safeArray, arrayData); | |
| var arrayValues = []; | |
| for (var k = 0; k < arrayLength; ++k) | |
| { | |
| switch (baseType) | |
| { | |
| case 0x0002: // VT_I2 | |
| arrayValues.push(arrayData.Deref().Deref(k * 2, 2).toBuffer().readInt16LE()); | |
| break; | |
| case 0x0003: // VT_I4 | |
| case 0x0016: // VT_INT | |
| arrayValues.push(arrayData.Deref().Deref(k * 4, 4).toBuffer().readInt32LE()); | |
| break; | |
| case 0x000B: // VT_BOOL | |
| arrayValues.push(arrayData.Deref().Deref(k * 2, 2).toBuffer().readInt16LE() != 0); | |
| break; | |
| case 0x0010: // VT_I1 | |
| arrayValues.push(arrayData.Deref().Deref(k, 1).toBuffer().readInt8()); | |
| break; | |
| case 0x0011: // VT_UI1 | |
| arrayValues.push(arrayData.Deref().Deref(k, 1).toBuffer().readUInt8()); | |
| break; | |
| case 0x0012: // VT_UI2 | |
| arrayValues.push(arrayData.Deref().Deref(k * 2, 2).toBuffer().readUInt16LE()); | |
| break; | |
| case 0x0013: // VT_UI4 | |
| case 0x0017: // VT_UINT | |
| arrayValues.push(arrayData.Deref().Deref(k * 4, 4).toBuffer().readUInt32LE()); | |
| break; | |
| case 0x0008: // VT_BSTR | |
| arrayValues.push(arrayData.Deref().Deref(k * GM.PointerSize, GM.PointerSize).Deref().Wide2UTF8); | |
| break; | |
| } | |
| } | |
| values[properties[i]] = arrayValues; | |
| } | |
| if (isArray) | |
| { | |
| // Handle array types (VT_ARRAY | base type) | |
| var safeArray = tmp1.Deref(8, GM.PointerSize).Deref(); | |
| var arrayLength = safeArray.Deref(GM.PointerSize == 8 ? 24 : 16, 4).toBuffer().readUInt32LE(); | |
| var arrayData = GM.CreatePointer(); | |
| OleAut32.SafeArrayAccessData(safeArray, arrayData); | |
| var arrayValues = []; | |
| for (var k = 0; k < arrayLength; ++k) | |
| { | |
| switch (baseType) | |
| { | |
| case 0x0002: // VT_I2 | |
| arrayValues.push(arrayData.Deref().Deref(k * 2, 2).toBuffer().readInt16LE()); | |
| break; | |
| case 0x0003: // VT_I4 | |
| case 0x0016: // VT_INT | |
| arrayValues.push(arrayData.Deref().Deref(k * 4, 4).toBuffer().readInt32LE()); | |
| break; | |
| case 0x000B: // VT_BOOL | |
| arrayValues.push(arrayData.Deref().Deref(k * 2, 2).toBuffer().readInt16LE() != 0); | |
| break; | |
| case 0x0010: // VT_I1 | |
| arrayValues.push(arrayData.Deref().Deref(k, 1).toBuffer().readInt8()); | |
| break; | |
| case 0x0011: // VT_UI1 | |
| arrayValues.push(arrayData.Deref().Deref(k, 1).toBuffer().readUInt8()); | |
| break; | |
| case 0x0012: // VT_UI2 | |
| arrayValues.push(arrayData.Deref().Deref(k * 2, 2).toBuffer().readUInt16LE()); | |
| break; | |
| case 0x0013: // VT_UI4 | |
| case 0x0017: // VT_UINT | |
| arrayValues.push(arrayData.Deref().Deref(k * 4, 4).toBuffer().readUInt32LE()); | |
| break; | |
| case 0x0008: // VT_BSTR | |
| arrayValues.push(arrayData.Deref().Deref(k * GM.PointerSize, GM.PointerSize).Deref().Wide2UTF8); | |
| break; | |
| } | |
| } | |
| values[properties[i]] = arrayValues; | |
| OleAut32.SafeArrayUnaccessData(safeArray); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/win-wmi.js` around lines 273 - 315, The SAFEARRAY is locked with
OleAut32.SafeArrayAccessData(safeArray, arrayData) but never unlocked; after
populating arrayValues you must call OleAut32.SafeArrayUnaccessData(safeArray)
to release the lock (use the same safeArray variable). Wrap the access/iteration
over arrayData in a try/finally (or ensure finally-like behavior) so
OleAut32.SafeArrayUnaccessData(safeArray) is always invoked even on errors/early
returns; update the block around SafeArrayAccessData/arrayData/safeArray (the
loop that builds arrayValues and the assignment to values[properties[i]]) to
guarantee UnaccessData is called.
Signed-off-by: Simon Smith <simonsmith5521@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
meshcore/agentcore.c (1)
4363-4366:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
validateWebCertcan silently stay disabled because this parser only accepts NUL-terminated lowercase values.Lines 4363-4366 only treat
"1\0"or"true\0"as enabled. But command-line settings are cached without the trailing NUL on Line 4880, so--validateWebCert=1/--validateWebCert=truefalls through toreturn 1and bypasses TLS preverification.TRUE/Truealso miss for the same reason.Suggested fix
int ValidateMeshServer(ILibWebClient_RequestToken sender, int preverify_ok, STACK_OF(X509) *certs, struct sockaddr_in6 *address, void *user) { MeshAgentHostContainer *agent = (MeshAgentHostContainer*)user; int len = ILibSimpleDataStore_Get(agent->masterDb, "validateWebCert", ILibScratchPad, sizeof(ILibScratchPad)); - // Values here are 0 terminated, but the 0 is counted in size, so add one to the length check. - if ((len == 2 && strncmp("1", ILibScratchPad, 1) == 0) || - (len == 5 && strncmp("true", ILibScratchPad, 4) == 0)) { + if (((len == 1 || len == 2) && ILibScratchPad[0] == '1') || + ((len == 4 || len == 5) && strncasecmp(ILibScratchPad, "true", 4) == 0)) { return preverify_ok; } // Server validation is always true here. We will do a second round within the websocket to see if the server is really valid or not. return 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@meshcore/agentcore.c` around lines 4363 - 4366, The code that reads the "validateWebCert" entry uses ILibSimpleDataStore_Get and then only accepts exact NUL‑terminated lowercase values ("1\0" or "true\0"), causing command-line cached values without trailing NUL or different case to be ignored; fix validateWebCert parsing by using the returned length from ILibSimpleDataStore_Get (len) rather than assuming it includes the terminator and perform case-insensitive comparisons against expected tokens ("1", "true", optionally "yes") using strncasecmp (or equivalent) on ILibScratchPad for the token lengths, and allow len >= token_length (or trim whitespace) so that entries cached without a trailing NUL or with mixed case are recognized; update the branch that currently checks len and strncmp so it uses ILibSimpleDataStore_Get, ILibScratchPad, and strncasecmp to correctly enable validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@meshcore/agentcore.c`:
- Around line 4363-4366: The code that reads the "validateWebCert" entry uses
ILibSimpleDataStore_Get and then only accepts exact NUL‑terminated lowercase
values ("1\0" or "true\0"), causing command-line cached values without trailing
NUL or different case to be ignored; fix validateWebCert parsing by using the
returned length from ILibSimpleDataStore_Get (len) rather than assuming it
includes the terminator and perform case-insensitive comparisons against
expected tokens ("1", "true", optionally "yes") using strncasecmp (or
equivalent) on ILibScratchPad for the token lengths, and allow len >=
token_length (or trim whitespace) so that entries cached without a trailing NUL
or with mixed case are recognized; update the branch that currently checks len
and strncmp so it uses ILibSimpleDataStore_Get, ILibScratchPad, and strncasecmp
to correctly enable validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 36c59292-542e-4563-af8e-ee1f5d7220dd
📒 Files selected for processing (3)
meshcore/KVM/MacOS/mac_kvm.cmeshcore/agentcore.cmicroscript/ILibDuktape_Commit.h
✅ Files skipped from review due to trivial changes (1)
- microscript/ILibDuktape_Commit.h
🚧 Files skipped from review as they are similar to previous changes (1)
- meshcore/KVM/MacOS/mac_kvm.c
* Corrected kvm_server_SetResolution definition and call. Fixes UAC delay (issue #347) * Fix for possible spaces in UninstallString
Signed-off-by: Simon Smith <simonsmith5521@gmail.com>
Signed-off-by: Simon Smith <simonsmith5521@gmail.com>
Signed-off-by: si458 <simonsmith5521@gmail.com>
Removed unnecessary architecture addition step and updated the ARM runner.
Signed-off-by: Simon Smith <simonsmith5521@gmail.com>
Signed-off-by: Simon Smith <simonsmith5521@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements