Claude/zabbix plugins security review sltq4b - #1
Merged
Conversation
…rk-connections plugins windows_needs_reboot_check: - Registry paths and the SCCM WMI namespace were raw-string literals with doubled backslashes (\\). Go raw strings do not process escapes, so every path contained empty components (SOFTWARE\\Microsoft\\...), making all three registry reboot checks always return "not found" and the WMI ConnectServer fail. Corrected to single backslashes so detection actually works. - Export returned "0" (no reboot needed) when the authoritative registry checks errored, hiding broken hosts forever. Propagate the error so the item goes unsupported and a nodata trigger can catch it. windows_network_connections: - ipFromDWORD used BigEndian on a value already read little-endian from the Windows MIB struct, reversing every IPv4 octet (127.0.0.1 -> 1.0.0.127). Switched to LittleEndian and replaced exact "127.0.0.1" loopback string checks with net.IP.IsLoopback() so the whole 127/8 range is skipped. - Template had two 'Templates' template_groups with different UUIDs, which breaks import; removed the duplicate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016bQStEBwMjdiDpGMKcaNoL
…acing linux_needs_reboot_check: - Every external command (needs-restarting, zypper, rpm, uname, sort) ran with no timeout, so a hung package-manager lock blocked Export forever and leaked a child per poll. Run them all under a 15s context timeout. - The in-process kernel-version fallback used a lexical sort.Strings, which mis-orders versions (5.14.0-9 > 5.14.0-10) and could silently disable the kernel-mismatch reboot signal; use the existing version-aware comparator. linux_network_connections: - Export masked a real collection failure (e.g. /proc/net/tcp unreadable) as a healthy empty JSON payload, defeating the template's nodata/parse triggers. Propagate the error so the item goes unsupported; a genuinely empty host still returns valid JSON with empty arrays. linux_btg_account_check (break-glass account monitor, runs as root): - bufio.ReadBytes buffered a whole line before the size guard, so one giant newline-less log line could OOM the agent. Replaced with a bounded reader that caps in-memory buffering at 1 MiB and drains the rest, plus a 64 MiB per-poll scan budget that checkpoints and resumes. - The log path comes from an item-key parameter; refuse non-regular files so a malicious server cannot aim the root agent at /dev/zero, a FIFO, or a socket. - A checkpoint-save hiccup discarded real detections as a generic ERROR; report the detections (the plugin's whole purpose) and only fall back to ERROR when there is nothing to report. - checkCheckpointDir cached transient failures permanently (stuck in ERROR until restart); only cache success and re-check otherwise. - Checkpoint save used a shared <fn>.tmp name; use a unique temp file so concurrent saves cannot clobber each other before the atomic rename. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016bQStEBwMjdiDpGMKcaNoL
linux_service_listening_port: - ss ran with no timeout and was resolved via PATH; a wedged ss blocked Export forever and leaked a child per poll. Run it under a 10s context timeout and resolve it from standard system locations before falling back to PATH. - Collection failure (ss missing/failed/timed out) was masked as an empty '[]' that reads as 'no listening ports'; propagate the error so the item goes unsupported. Hoisted the users() regexp to package scope (compiled once). - Standalone test mode now surfaces errors instead of printing <nil>. linux_ntp_sync: - QueryOptions.Timeout bounds only the UDP exchange, not DNS, so a hung resolver could stretch Export past the item timeout. Resolve the server once up front with a bounded-context lookup (literal IPs skip DNS) and query the result. - Kiss-of-Death responses were retried, which can get the agent's IP rate-limited or blocked (RFC 5905). Detect r.IsKissOfDeath() and fail immediately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016bQStEBwMjdiDpGMKcaNoL
.github/workflows/build-plugins.yml: - Least-privilege permissions: default the workflow to contents:read and grant contents:write only to the release job (previously the whole workflow, all jobs, had write). - Scope the Authenticode signing secrets (WINDOWS_CODE_SIGN_PFX_*) to the signing step's env only, not the job-wide env, so go build / module downloads / govulncheck install never see the code-signing key or password. - Add a concurrency group keyed on the ref so overlapping pushes to main cannot race the make_latest release; superseded PR runs are cancelled, main runs run to completion. - Validate pull requests (previously code was only ever built after landing on main): run on pull_request, add a 'go vet' step, and gate the release job to non-PR events so PRs never publish. - Pin govulncheck to @v1.5.0 instead of @latest for reproducible runs. - Install ClamAV and download its signature DB once in a dedicated scan job instead of in all 14 matrix build jobs; release now needs [build, scan]. - Add timeout-minutes to every job (was the 6h default) and include the workflow file and Windows/versioninfo.base.json in the paths filters. Repo hygiene: - Add an MIT LICENSE file matching the MIT declaration already stamped into the Windows binaries' LegalCopyright (previously no license text existed). - renovate.json: add postUpdateOptions gomodTidy so dependency PRs keep go.sum tidy across the multi-module repo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016bQStEBwMjdiDpGMKcaNoL
windows_failover_cluster: - collectLive used cmd.CombinedOutput(), merging PowerShell stderr into the JSON on stdout; a single warning/verbose line made json.Unmarshal fail and dropped an otherwise-successful collection. Capture stdout and stderr separately and set cmd.WaitDelay so a child inheriting the pipe cannot block past the deadline. windows_service_listening_port: - verQueryString read a NUL-terminated UTF-16 string ignoring the returned valueLen. A version resource (from the target process image, possibly malformed/attacker-influenced) without a NUL could cause an out-of-bounds read past the buffer. Bound the read to valueLen via unsafe.Slice + UTF16ToString. - GetExtendedTcpTable had no retry on ERROR_INSUFFICIENT_BUFFER, so on a busy host the table growing between the size probe and the fetch produced intermittent hard failures. Added a shared retry helper (used for both IPv4 and IPv6). - Export masked collection failures as an empty '[]'; propagate the error. CI: scope the new 'go vet' step to -unsafeptr=false. The Win32/syscall interop legitimately converts OS-returned uintptr values to unsafe.Pointer (memory owned by the OS, not the Go GC), which the unsafeptr analyzer false-positives on; all other vet analyzers stay enabled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016bQStEBwMjdiDpGMKcaNoL
windows_application_inventory: - marshalPowerShellJSON emitted 4-space-indented JSON and then rewrote every newline to CRLF, inflating the item value ~3x. On hosts with many installed applications this risked exceeding Zabbix's TEXT value limit and truncating to invalid JSON. Emit compact JSON (consumers ignore whitespace). windows_ntp_sync: - Mirror the linux_ntp_sync fix: QueryOptions.Timeout does not bound DNS, so a hung resolver could push Export past the item timeout. Resolve the server once up front with a bounded-context lookup (literal IPs skip DNS), and stop retrying Kiss-of-Death responses (RFC 5905) to avoid the agent's IP being rate-limited or blocked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016bQStEBwMjdiDpGMKcaNoL
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
2 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.
No description provided.