security: critical race, spine.conf policy, signal handler, fd leaks#538
security: critical race, spine.conf policy, signal handler, fd leaks#538somethingwithproof wants to merge 195 commits intoCacti:developfrom
Conversation
- Add CMakeLists.txt mirroring configure.ac feature checks - Add config/config.h.cmake.in template for cmake builds - Add build-cmake-linux CI job (gcc/clang matrix) - Add build-windows CI job (MSYS2/MinGW-w64, continue-on-error) - Windows crash dump collection via WER LocalDumps - Gate -Wall by compiler ID (GNU/Clang vs MSVC) - Use CMAKE_DL_LIBS instead of hardcoded -ldl - Parse net-snmp-config --libs into proper NETSNMP_LIBRARIES - Add SNMP_LOCALNAME compile check for feature parity - No C source files modified Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
spine.h depends on types from common.h (MYSQL, pid_t, size_t, pthread types, RESULTS_BUFFER from config.h). Add a compile-time guard that produces a clear error if spine.h is included without common.h, rather than cascading type errors. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
strncasecmp() returns 0 on match, but the comparisons used the raw return value as truthy, inverting the logic. TCP matched non-TCP strings and vice versa. Also fix: comparisons against 'hostname' instead of 'token' (lines 1020, 1032), and strncasecmp length 3 for 4-char strings "TCP6"/"UDP6" (should be 4). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
getarg(opt, &argv) advances the argv pointer on each call. Three successive calls in the --mode handler consumed three argv entries instead of one, corrupting subsequent argument parsing when using the space-separated form (--mode online). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
… main() entry Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…t vars Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…ncation Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
platform_sandbox_linux.c uses uint64_t in add_path_rule() and the apply_landlock() ro_roots loop but the HAVE_LANDLOCK block only included linux/landlock.h and sys/syscall.h. On gcc without a transitively-included stdint.h the type is unknown, which cascades into a parse failure on the add_path_rule signature and then an implicit-declaration error at the first call site (lines 110, 221, 247 in the CI error report). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
glibc's <features.h> freezes the feature-test bitmap on first inclusion. platform.h pulls <time.h> and platform_process.h pulls <sys/types.h>, both of which reach <features.h> before CMake's -D_GNU_SOURCE=1 flag can take effect in the preprocessor pipeline on clang/Linux. Without the macro visible at that point, pthread_setname_np and pipe2 remain undeclared, producing -Werror=implicit-function-declaration failures. Add a `#if defined(__linux__) && !defined(_GNU_SOURCE)` guard at the top of each affected TU so the macro is always set before any system header is included, regardless of include order or compiler. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Replace the bash case pattern *[^A-Za-z0-9._+-\ \t]*) with a tr-based check. The old pattern treated +-\ as a character range (0x2B-0x5C), which excluded space and tab and caused all multi-package strings like 'cmake make pkg-config gcc' to be rejected. The new approach pipes the input through LC_ALL=C tr -d with the explicit allowed set (alnum, . _ + - space tab); any bytes surviving the deletion are disallowed characters, reported verbatim in the error message. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…rns -1 nft_pchild returns -1 on lookup failure. kill(-1, SIGKILL) signals every process owned by the spine uid; kill(0, ...) hits the whole process group. Accept only pids > 1. Type the local to spine_pid_t so Windows stays aligned with the POSIX pid_t width. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
SECURITY.md promises spine refuses to start when spine.conf has any group or world bit set or is owned by someone other than root or the startup euid. The old soft-warn path leaked credentials on a misconfigured deploy. Open the file with O_NOFOLLOW so a planted symlink at /etc/spine.conf cannot redirect credential loading, add O_CLOEXEC so the fd does not leak into poll-script children, and treat ELOOP, mode drift, and owner mismatch as fatal. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The previous handler called stdio, time(3), localtime(3), strftime(3), and exit(3). None are async-signal-safe, so a SIGSEGV mid-allocation could deadlock inside libc. Pre-format per-signal messages at startup, emit with write(2), and use _exit(128 + signo) on SIGSEGV/SIGBUS/SIGFPE/ SIGABRT to skip atexit handlers. Drop SIGPIPE from the fatal set and ignore it process-wide so a poll script closing stdout early no longer kills spine. Remove the redundant legacy signal() loop. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…e into children Every nft_popen'd poll script used to inherit the full descriptor table, including privileged ICMP sockets, the authenticated MySQL connection, the audit netlink fd, and the log file. A poll script that shells out could reuse any of them. Set cloexec at creation time: SOCK_CLOEXEC atomically on Linux/BSD with fcntl fallback for macOS, fcntl after each raw ICMP socket(), the MySQL net.fd after connect, the audit fd after audit_open(), and O_CLOEXEC on the log open(). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Two bugs: strncpy(name->hostname, hostname, sizeof(name->hostname)) leaves the buffer unterminated when the source equals or exceeds the bound, and strncpy(...) followed by name->hostname[strlen(token)] = 0 writes one-past-the-buffer for tokens >= sizeof. Funnel both branches through strncopy(), which caps at the bound and always NUL-terminates. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
SNMPv3 engine IDs are opaque octet strings (RFC 3411). strlen() on the engine-ID buffer truncates at the first 0x00, which any RFC-compliant engine ID is free to contain. Decode the Cacti hex-string form at DB load time, carry bytes + length in target_t and host_t, and pass both to snmp_host_init(). The hex path stays as a transitional fallback. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR addresses multiple security-review findings (process-kill guard, config hardening, async-signal-safe fatal handling, FD_CLOEXEC, ping hostname bounds, and SNMPv3 engine ID handling) and adds/updates a significant amount of supporting infrastructure (new platform abstraction layer, systemd integration, audit hooks, packaging/CI/test harness changes).
Changes:
- Hardens security-sensitive runtime behavior (signal handling, descriptor inheritance, SNMPv3 engine ID handling).
- Introduces new operational/security integration surfaces (systemd sd_notify helpers, audit logging + rate limiting, sandbox stubs, USDT probes).
- Expands/updates CI, packaging, and test scaffolding (new workflows, distro matrix scripts, CMake-centric packaging/build).
Reviewed changes
Copilot reviewed 140 out of 179 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_cloexec.c | Adds unit tests asserting sockets/log fd carry FD_CLOEXEC. |
| tests/unit/test_circuit_breaker.c | Adds unit tests for new circuit breaker state machine and concurrency smoke coverage. |
| tests/unit/test_check_mode.c | Adds unit coverage for spine_health_check() failure JSON envelope. |
| tests/unit/test_build_fixes.c | Adjusts test to avoid null deref in assertions; removes legacy field. |
| tests/unit/test_arc4random.c | Adds BSD-only arc4random smoke test. |
| tests/unit/json_escape_tu.c | Adds test-only TU copy of JSON escaping helper. |
| tests/unit/build_child_env_tu.c | Adds test-only TU copy of child env scrubber helper. |
| tests/unit/Makefile | Reworks unit test makefile into lightweight platform smoke test harness. |
| tests/snmpv3/snmpd/snmpv3.conf | Updates SNMPv3 auth protocol in test fixture to SHA. |
| tests/snmpv3/scripts/run_test.sh | Refactors shell helper functions and error handling blocks. |
| tests/snmpv3/docker-compose.yml | Updates healthcheck SNMP auth protocol to SHA. |
| tests/snmpv3/db/init.sql | Updates seeded Cacti DB fixture auth protocol to SHA. |
| tests/integration/test_output_regex.sh | Improves integration script robustness; uses --no-deps. |
| tests/integration/test_ipv6_transport.sh | Adds new IPv6 integration test script. |
| tests/integration/test_db_column_detect.sh | Improves integration script robustness; uses --no-deps. |
| tests/integration/smoke_test.sh | Updates integration harness; adjusts compose runs and error handling. |
| src/util.h | Adds startup EUID capture API; adds CLI helper declarations; removes add_slashes decl. |
| src/systemd_notify.h | Adds systemd sd_notify integration header API. |
| src/systemd_notify.c | Adds systemd sd_notify integration implementation (guarded by HAVE_LIBSYSTEMD). |
| src/sql.h | Fixes macro trailing continuation (brace/backslash). |
| src/spine_probes.h | Adds USDT probe macros with portable no-op fallbacks. |
| src/spine_audit.h | Adds audit logging API + test seams (clock/reset). |
| src/spine_audit.c | Adds audit event emission with token-bucket rate limiting and CLOEXEC hardening. |
| src/spine.h | Adds include-order enforcement; adds log-format/config fields; expands SNMP protocol buffer sizes; adds binary engine-id fields; uses spine_pid_t. |
| src/snmp_engine_id.c | Adds standalone hex engine-id decoder TU for unit testing. |
| src/snmp.h | Extends snmp_host_init signature to accept binary engine ID + length; declares decoder. |
| src/snmp.c | Uses platform setenv; supports binary engine ID; adds USDT probe site; tweaks credential zeroing behavior. |
| src/platform/platform_win.c | Adds Windows platform backend helpers (sleep, env, thread naming, job object). |
| src/platform/platform_socket_win.c | Adds Windows socket wrapper implementation. |
| src/platform/platform_socket_posix.c | Adds POSIX socket wrapper with atomic CLOEXEC where supported. |
| src/platform/platform_socket.h | Adds platform socket abstraction header. |
| src/platform/platform_sandbox_win.c | Adds Windows sandbox stub TU. |
| src/platform/platform_sandbox_posix.c | Adds generic POSIX sandbox stub TU. |
| src/platform/platform_sandbox_openbsd.c | Adds OpenBSD pledge/unveil implementation. |
| src/platform/platform_sandbox_freebsd.c | Adds FreeBSD sandbox stub with Capsicum rationale. |
| src/platform/platform_sandbox.h | Adds sandbox abstraction header. |
| src/platform/platform_process_posix.c | Adds POSIX process helpers (pipe2 CLOEXEC, spawn retry, wait/terminate). |
| src/platform/platform_process.h | Adds process abstraction header (spine_pid_t, spawn/wait/terminate APIs). |
| src/platform/platform_posix.c | Adds POSIX platform backend helpers (sleep via nanosleep, thread naming, isatty). |
| src/platform/platform_icmp_posix.c | Adds POSIX ICMP façade forwarding into ping.c numeric helpers. |
| src/platform/platform_icmp.h | Adds platform ICMP abstraction header. |
| src/platform/platform_fd_win.c | Adds Windows fd read/write/wait wrappers. |
| src/platform/platform_fd_posix.c | Adds POSIX fd read/write/wait wrappers. |
| src/platform/platform_fd.h | Adds fd abstraction header. |
| src/platform/platform_error_win.c | Adds Windows error-string helper. |
| src/platform/platform_error_posix.c | Adds POSIX error-string helper with glibc/non-glibc strerror_r handling. |
| src/platform/platform_error.h | Adds platform error abstraction header. |
| src/platform/platform_common.c | Adds reference-counted platform init/cleanup shim. |
| src/platform/platform.h | Adds platform abstraction header (init, env, sleep, tty, thread naming, Win job hooks). |
| src/ping_wire.h | Centralizes ICMP wire payload struct + validation API/constant. |
| src/ping_validate.c | Adds standalone ICMP payload validation TU. |
| src/ping_ipv6_scope.c | Adds standalone IPv6 scope_id resolver TU. |
| src/ping.h | Removes large Cygwin ICMP struct block; adds ping_init decl. |
| src/nft_popen.h | Documents and exports spine_build_child_env API. |
| src/error.h | Adds spine_signal_handler_init decl. |
| src/error.c | Replaces prior signal handling with async-signal-safe handler + SIGPIPE ignore. |
| src/common.h | Removes Cygwin-specific hacks; includes platform/platform.h. |
| src/circuit_breaker.h | Adds circuit breaker public API header. |
| src/circuit_breaker.c | Adds per-host circuit breaker implementation with audit emission. |
| scripts/verify.sh | Switches scan-build path to CMake/Ninja build output. |
| scripts/test-workflows.sh | Adds local GitHub Actions workflow testing helper script (act). |
| scripts/test-vagrant.sh | Adds Vagrant-based BSD/niche OS test runner helper script. |
| scripts/test-distros.sh | Adds Docker-based distro build+smoke test matrix script. |
| packaging/rpm/spine.spec | Migrates RPM spec build to CMake + Ninja. |
| packaging/rpm/README | Notes RPM spec uses CMake + Ninja. |
| packaging/debian/rules | Migrates Debian rules to CMake + Ninja buildsystem. |
| packaging/debian/control | Replaces autotools deps with cmake/ninja-build. |
| packaging/debian/README | Notes Debian packaging uses CMake + Ninja. |
| packaging/README.md | Updates packaging docs to reference CMake-based builds. |
| package | Updates release packaging script to validate via CMake config; removes bootstrap dependency. |
| flake.nix | Adds Nix flake build + devshell for CMake build path. |
| etc/systemd/spine.timer | Adds systemd timer unit for periodic runs. |
| etc/systemd/spine.service | Adds hardened systemd service unit using Type=notify and sandboxing knobs. |
| etc/spine.conf.dist | Documents Script_Policy option in distributed config. |
| etc/selinux/spine.te | Adds SELinux skeleton module for spine domain/types. |
| etc/selinux/spine.if | Adds SELinux interfaces for domain transition/log/pid access. |
| etc/selinux/spine.fc | Adds SELinux file contexts for spine paths. |
| etc/apparmor.d/usr.local.spine.bin.spine | Adds AppArmor profile for spine. |
| error.c | Removes legacy error.c implementation (file deleted). |
| docs/security-selinux.md | Adds SELinux enablement documentation. |
| docs/security-hardening.md | Adds runtime hardening documentation and guidance. |
| docs/security-alerts.md | Adds audit trail of dismissed security alerts. |
| docs/platform-idioms.md | Adds platform implementation idioms doc. |
| docs/debugging.md | Adds debugging guide, including systemd/gdb and USDT tracing notes. |
| debug | Adjusts debug helper script formatting. |
| config/config.h.cmake.in | Adds CMake-generated config.h template. |
| bootstrap | Removes legacy autotools bootstrap script (file deleted). |
| Vagrantfile | Adds Vagrant boxes + provisioning for BSD/Alpine testing. |
| VERSION | Adds VERSION file (1.3.0). |
| SECURITY.md | Adds repo security policy and deployment guidance. |
| Makefile.am | Removes autotools build file (file deleted). |
| Dockerfile.dev | Migrates dev Docker build to CMake/Ninja and sanitizer flags. |
| Dockerfile | Migrates runtime Docker build to CMake/Ninja; adds non-root user and config install. |
| CONTRIBUTING.md | Adds contributing guide including local CI testing and distro matrix instructions. |
| CMakePresets.json | Adds CI-oriented CMake presets. |
| CHANGELOG | Adds notes/issues/features entries related to platform/CI/security work. |
| .github/workflows/weekly.yml | Adds scheduled weekly deep checks workflow. |
| .github/workflows/security-posture.yml | Adds security posture workflow (TruffleHog, Semgrep, Scorecard, policy gate). |
| .github/workflows/release.yml | Adds release workflow with SBOM generation and cosign signing. |
| .github/workflows/release-verification.yml | Adds release verification (hardening checks, staging install test, attestation). |
| .github/workflows/oci.yml | Adds multi-arch OCI publish workflow with cosign signing. |
| .github/workflows/fuzzing.yml | Adds sanitizer-based CLI fuzz smoke workflow. |
| .github/workflows/coverage.yml | Adds GCC coverage workflow with lcov gate. |
| .github/workflows/codeql.yml | Adds CodeQL workflow for C/C++. |
| .github/scripts/cppcheck_to_sarif.py | Adds cppcheck output to SARIF converter. |
| .github/scripts/clang_tidy_to_sarif.py | Adds clang-tidy output to SARIF converter. |
| .github/scripts/check-workflow-policy.py | Adds workflow hygiene policy checker. |
| .github/scripts/check-unsafe-api-additions.sh | Adds guardrail disallowing newly-added unsafe C APIs in diffs. |
| .github/scripts/check-leak-trend.py | Adds nightly leak trend parser/gate for ASan/Valgrind logs. |
| .github/perf-baseline.json | Adds performance baseline configuration. |
| .github/nightly-leak-baseline.json | Adds nightly leak baseline thresholds. |
| .github/instructions/instructions.md | Updates contributor instructions with API and CI guardrails. |
| .github/cppcheck-baseline.txt | Adds placeholder cppcheck baseline file. |
| .github/actions/install-apt-deps/action.yml | Adds composite action for safe apt dependency installs. |
| .dockerignore | Expands docker ignore patterns (excludes CI/test/build artifacts). |
| .devcontainer/devcontainer.json | Adds devcontainer for consistent Ubuntu 24.04 C++ tooling. |
| .clang-tidy | Adds clang-tidy configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * -1 means "not yet attempted"; -2 means "attempted and failed, stop | ||
| * retrying" so a non-audit-enabled kernel doesn't burn syscalls forever. */ | ||
| #ifdef HAVE_LIBAUDIT | ||
| static int g_audit_fd = -1; |
There was a problem hiding this comment.
spine_audit_event()/rate_allow() mutate shared globals (g_buckets and g_audit_fd) without synchronization. In a multi-threaded poller this is a data race: counters can corrupt, rate limiting becomes unreliable, and g_audit_fd initialization can race (double audit_open(), fd leak, and inconsistent -2 state). Fix by adding a mutex (or atomics) that protects both the token-bucket state and the lazy audit_open/FD_CLOEXEC setup, or by using pthread_once for fd init plus per-bucket atomics for counters.
| static struct rate_bucket g_buckets[AUDIT_BUCKET__COUNT]; | ||
|
|
||
| static time_t default_clock(void) { return time(NULL); } | ||
| static spine_audit_clock_fn g_clock = default_clock; |
There was a problem hiding this comment.
spine_audit_event()/rate_allow() mutate shared globals (g_buckets and g_audit_fd) without synchronization. In a multi-threaded poller this is a data race: counters can corrupt, rate limiting becomes unreliable, and g_audit_fd initialization can race (double audit_open(), fd leak, and inconsistent -2 state). Fix by adding a mutex (or atomics) that protects both the token-bucket state and the lazy audit_open/FD_CLOEXEC setup, or by using pthread_once for fd init plus per-bucket atomics for counters.
| static int rate_allow(enum audit_bucket b, const char *op) { | ||
| time_t now = g_clock(); | ||
| struct rate_bucket *rb = &g_buckets[b]; | ||
|
|
||
| if (rb->window_start == 0 || now - rb->window_start >= SPINE_AUDIT_RATE_WINDOW_SECS) { | ||
| rb->window_start = now; | ||
| rb->count = 0; | ||
| rb->notified = 0; | ||
| } | ||
|
|
||
| if (rb->count < SPINE_AUDIT_RATE_MAX) { | ||
| rb->count++; | ||
| return 1; | ||
| } | ||
|
|
||
| if (!rb->notified) { | ||
| fprintf(stderr, "NOTE: audit rate limit reached for op=%s\n", | ||
| op ? op : "(null)"); | ||
| rb->notified = 1; | ||
| } | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
spine_audit_event()/rate_allow() mutate shared globals (g_buckets and g_audit_fd) without synchronization. In a multi-threaded poller this is a data race: counters can corrupt, rate limiting becomes unreliable, and g_audit_fd initialization can race (double audit_open(), fd leak, and inconsistent -2 state). Fix by adding a mutex (or atomics) that protects both the token-bucket state and the lazy audit_open/FD_CLOEXEC setup, or by using pthread_once for fd init plus per-bucket atomics for counters.
| if (g_audit_fd == -1) { | ||
| g_audit_fd = audit_open(); | ||
| if (g_audit_fd < 0) { | ||
| g_audit_fd = -2; | ||
| return; | ||
| } |
There was a problem hiding this comment.
spine_audit_event()/rate_allow() mutate shared globals (g_buckets and g_audit_fd) without synchronization. In a multi-threaded poller this is a data race: counters can corrupt, rate limiting becomes unreliable, and g_audit_fd initialization can race (double audit_open(), fd leak, and inconsistent -2 state). Fix by adding a mutex (or atomics) that protects both the token-bucket state and the lazy audit_open/FD_CLOEXEC setup, or by using pthread_once for fd init plus per-bucket atomics for counters.
| static void spine_signal_handler(int spine_signal) { | ||
| const char *msg; | ||
| size_t len; |
There was a problem hiding this comment.
The new fatal handler only terminates the process for SIGSEGV/SIGBUS/SIGFPE/SIGABRT. For signals that are listed as fatal (SIGINT/SIGQUIT/SIGSYS), the handler writes a 'FATAL' message but then returns, allowing the process to continue in an interrupted state. If the intended behavior is 'die on fatal signals' (as described in the PR text), this should terminate for all signals handled here (e.g., _exit(128 + signo)), or it should set a volatile sig_atomic_t shutdown flag that the main loop observes to exit cleanly.
| #include "common.h" | ||
| #include "spine.h" | ||
| #include "circuit_breaker.h" |
There was a problem hiding this comment.
The PR description states the scope is six targeted security fixes (C1, H1, H2, H3, H8, H9), but the diff introduces substantial additional features/integration surfaces (circuit breaker implementation, systemd sd_notify, audit logging, sandbox framework, CI/workflow additions, packaging migration, etc.). This mismatch makes it difficult to review and risk-assess. Consider splitting non-security-review changes into separate PRs, or updating the PR description and risk assessment to explicitly include these new components.
| #define SPINE_AUDIT_RATE_MAX 10 | ||
| #define SPINE_AUDIT_RATE_WINDOW_SECS 60 |
There was a problem hiding this comment.
This introduces non-trivial behavior (token-bucket rate limiting, op classification, test seams for clock/reset, and lazy audit fd initialization). There are no corresponding unit tests shown here that validate: (1) per-bucket limiting behavior across windows, (2) the 'single NOTE' notification behavior, and (3) the fd initialization path sets FD_CLOEXEC only once. Adding a focused unit test would help prevent regressions in both security posture and operational logging.
| static time_t default_clock(void) { return time(NULL); } | ||
| static spine_audit_clock_fn g_clock = default_clock; | ||
|
|
||
| void spine_audit_set_clock(spine_audit_clock_fn fn) { |
There was a problem hiding this comment.
This introduces non-trivial behavior (token-bucket rate limiting, op classification, test seams for clock/reset, and lazy audit fd initialization). There are no corresponding unit tests shown here that validate: (1) per-bucket limiting behavior across windows, (2) the 'single NOTE' notification behavior, and (3) the fd initialization path sets FD_CLOEXEC only once. Adding a focused unit test would help prevent regressions in both security posture and operational logging.
| g_clock = fn ? fn : default_clock; | ||
| } | ||
|
|
||
| void spine_audit_reset_for_test(void) { |
There was a problem hiding this comment.
This introduces non-trivial behavior (token-bucket rate limiting, op classification, test seams for clock/reset, and lazy audit fd initialization). There are no corresponding unit tests shown here that validate: (1) per-bucket limiting behavior across windows, (2) the 'single NOTE' notification behavior, and (3) the fd initialization path sets FD_CLOEXEC only once. Adding a focused unit test would help prevent regressions in both security posture and operational logging.
| return 0; | ||
| } | ||
|
|
||
| void spine_audit_event(const char *op, const char *detail, int result) { |
There was a problem hiding this comment.
This introduces non-trivial behavior (token-bucket rate limiting, op classification, test seams for clock/reset, and lazy audit fd initialization). There are no corresponding unit tests shown here that validate: (1) per-bucket limiting behavior across windows, (2) the 'single NOTE' notification behavior, and (3) the fd initialization path sets FD_CLOEXEC only once. Adding a focused unit test would help prevent regressions in both security posture and operational logging.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| chars = fgets(buff, BUFSIZE, fp); | ||
|
|
||
| if (chars != NULL && !feof(fp) && *buff != '#' && *buff != ' ' && *buff != '\n') { | ||
| sscanf(buff, "%15s %255s", p1, p2); |
| chars = fgets(buff, BUFSIZE, fp); | ||
|
|
||
| if (chars != NULL && !feof(fp) && *buff != '#' && *buff != ' ' && *buff != '\n') { | ||
| sscanf(buff, "%15s %255s", p1, p2); |
| char *exec_poll(host_t *current_host, char *command, int id, const char *type) { | ||
| int cmd_fd; | ||
| int pid; | ||
| spine_pid_t pid; |
H1 (SECURITY.md) refuses startup when spine.conf has any group or world bit set or is owned by someone other than root or the startup euid. The integration fixtures shipped the file at the COPY/checkout default of 0644, which H1 correctly rejects. Dockerfile: add RUN chmod 0600 after the COPY so the baked conf (owned by root, mode 0600) satisfies both H1 checks. This fixes the Docker Integration Tests, Poll Timing Benchmark, and build-cmake-linux- sanitizers jobs that use the production image without a bind-mount override. docker-compose.yml: the bind-mount of tests/snmpv3/spine/spine.conf arrives at 0644 from the git checkout and cannot be chmod'd through a :ro mount. Switch to a staging mount at spine.conf.src, set user: "0:0" so the process euid is root, and add a wrapper entrypoint that installs the staged conf to /tmp/spine.conf at 0600 before exec-ing spine. ci.yml / distro-matrix.yml: add chmod 0600 tests/snmpv3/spine/spine.conf before the cmake build in both FreeBSD jobs so ctest passes on FreeBSD 14 (Tier 1). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
Superseded by consolidated PR #535 per maintainer's precedent in #523 (which explicitly cites merge-order conflict risk across util.c / spine.c / platform / CI files). All commits (C1 kill guard, H1 config perms, H2 signal handler, H3 FD_CLOEXEC, H8 strncopy, H9 engine ID binary length) are now on feat/distro-test-matrix via fast-forward. No content lost. |
Six fixes from the recent security review (see internal report, 2026-04-17). Each is one commit with a matching unit test.
Scope
poller.c): guardkill(pid, SIGKILL)againstnft_pchildreturning -1. Without the guard,kill(-1, SIGKILL)wipes every process owned by the spine uid when the pid lookup fails (reachable from the select-timeout branch racing cleanup).util.c): tighten spine.conf validation to matchSECURITY.md. Refuse start on world-readable, group-readable, or owner-mismatch. Open config withO_NOFOLLOW|O_CLOEXEC. Previous behavior warned and continued.error.c): rewrite fatal signal handler to async-signal-safe primitives. Pre-formatted messages +write(STDERR_FILENO, ...)+_exit(128+signo). Drops SIGPIPE from the fatal list and ignores it process-wide.FD_CLOEXECso poll-script children do not inherit the raw ICMP socket, MySQL session, audit netlink, or log fd.ping.c): fix 1-byte OOB write inget_namebyhostwhen the hostname token equalssizeof(name->hostname). Uses existingstrncopyhelper.snmp.c): carry SNMPv3 contextEngineID as binary with explicit length. Previousstrlen(engine_id)path truncated at the first0x00, weakening USM key localization.Verification
cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug && cmake --build build && ctest --test-dir build --output-on-failure— 24/24 pass. New tests:test_poller_pid_guard,test_config_perms,test_signal_handler,test_cloexec,test_get_namebyhost,test_snmp_engine_id.Risk
chmod 0600.