fix: restore safe SNMPv3 session construction - #620
Open
somethingwithproof wants to merge 26 commits into
Open
Conversation
Cacti stores the literal [None] when no SNMPv3 protocol is selected, and an empty string for an absent passphrase. Since Cacti#373 spine treated both as errors, so two of the three security levels stopped working while cmd.php kept accepting them. usm_lookup_auth_type([None]) returns -1, and the lookup returned 0 from the session setup on any non-positive result, so a noAuthNoPriv device never opened a session. Separately the authentication key was derived only on the privacy path, so an authNoPriv session authenticated with securityAuthKeyLen still 0 and the agent answered with a USM authentication failure, which points the operator at their credentials rather than at spine. Decide the level first, from the four values Cacti supplies, and only require a recognised protocol when authentication is actually in use. Derive the key whenever the level authenticates rather than only when it also encrypts. spine_snmpv3_security_level() is pure and covered by five cases in test_linked: the [None] sentinel, each level, a protocol without its passphrase, and privacy selected without authentication, which SNMPv3 does not offer and which must not raise the level. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
SNMP_DEFAULT_PRIV_PROTO names usmDESPrivProtocol unless the header defines NETSNMP_DISABLE_DES. Fedora shipped a net-snmp where the header took the DES branch and the library exported no such symbol, so spine failed to link rather than degrading. Checking the macro cannot catch that, because that packaging did not set it. Refs Cacti#575. configure now link-probes the symbol. Where it is absent and the header still names it, the session falls back to AES, which every net-snmp that omits DES provides. Current Fedora no longer has the mismatch, so this cannot be reproduced end to end here. What is verified: the probe answers yes on Ubuntu 24.04 and no on Fedora 44, both build and link, behaviour on each is unchanged because each is internally consistent, and the fallback branch compiled on its own references usmAESPrivProtocol and nothing else. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The security level was decided first and the protocol only validated on the authenticated path, so a device with an unrecognised protocol and no passphrase never reached the check and opened a noAuthNoPriv session. develop validates unconditionally before choosing a level; this restores that while keeping the level logic that made authNoPriv usable again. Only the no-passphrase case differed. A device with a passphrase already took the validated branch, so no authenticated session was ever downgraded. Also guards poll_host_build_queries against NULL. regex_col and limits are interpolated with %s, where a NULL is undefined rather than an empty column list, and every other extracted function already checked. Both raised by the pre-push review. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…levels USM has no privacy without authentication. A device configured with a privacy protocol and passphrase but no auth passphrase was being opened as noAuthNoPriv, which leaves the operator believing traffic is encrypted when it is not. It is refused now, naming the cause. The old code also refused it, but by failing key derivation with a message about passphrase length that named neither the cause nor the fix. snmp_sess_open() builds a session without contacting anything, so the level spine settles on is assertable directly. Six cases in a new binary, checked against both earlier states rather than only the current one: develop fails test_no_credentials_is_noauthnopriv, which is Cacti#582 ea71557, my first fails test_unknown_auth_protocol_is_refused_even_without fix for it _a_password, which is the validation bypass I introduced Correcting something I wrote in a778b5b: that message said validating whenever a protocol is set restores develop's behaviour. It does not. develop refuses any device whose protocol does not resolve, including the legitimate [None], which is the bug Cacti#582 is about. Distinguishing unset from invalid is new, not restored. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
… tests The authNoPriv path added for Cacti#582 strdup'd the auth passphrase and freed it only on the error leg, so every successful authNoPriv session leaked a cleartext copy. The privacy path already released its own at the equivalent point. Found by running the full build matrix rather than the default build: LeakSanitizer reported it through test_auth_without_privacy_is_authnopriv, a test written an hour earlier for something else. test_ping_icmp skips where no raw ICMP socket is available, and a GitHub runner is not root, so make check was reporting a pass without executing any of it. Confirmed here: as root all three run, as an unprivileged user all three skip. ping_icmp() is a SUID-root path whose exits were just restructured, so CI runs it under sudo as its own step. Also covers hex2dec()'s separators directly. The ':' case cannot be reached through poller_store_result(), because is_multipart_output() claims colon forms first, so nothing exercised it. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
zero_sensitive was declared, never assigned, and guarded every scrub in the file, so none of them ran. Drop the flag and wipe snmp_host_init's own copies unconditionally as they are released. The caller's snmp_password and snmp_priv_passphrase are deliberately left alone: poller.c compares them against last_snmp_password to decide whether the next item can keep the open session, and blanking them would re-derive the USM keys for every remaining item on the device. The wipe writes through a volatile pointer because a plain memset() before free() is a dead store that gcc -O2 removes, leaving the passphrase in the heap. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Carry the SNMP-specific review fixes from the Cacti#597 integration branch into the focused runtime-safety series. Guard null defaults and only compile the SHA-1 fallback when configure proves the linked net-snmp exposes it. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Map the BSD ICMP_UNREACH name to the Linux ICMP_DEST_UNREACH spelling used by the linked-object regression test so the focused branch remains portable. 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>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Member
Author
|
@copilot review |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It modifies security-sensitive SNMPv3 session construction and failure policy in C, so a final human review is warranted even though tests are added.
Pull request overview
Restores correct and safe SNMPv3 session construction in snmp_host_init() (especially noAuthNoPriv/authNoPriv), aligning Spine’s effective security-level behavior with Cacti while failing closed on ambiguous/unsafe configurations and unknown algorithms. It also adds deterministic unit coverage for SNMPv3 session-building decisions.
Changes:
- Add SNMPv3 helpers for protocol/passphrase presence and effective security-level computation; update
snmp_host_init()to validate combinations, derive auth keys on the authNoPriv path, and wipe local passphrase copies. - Add a deterministic SNMPv3 session-construction unit test that intercepts
snmp_sess_open()to assert the assembled session fields without network I/O. - Extend unit-test portability with an ICMP constant compatibility define and wire the new unit test into
make check.
File summaries
| File | Description |
|---|---|
| tests/unit/test_snmpv3_session.c | New deterministic unit tests covering SNMPv3 effective-level selection, refusal paths, algorithm OID installation, and key derivation behavior. |
| tests/unit/test_linked.c | Adds an ICMP constant compatibility mapping for unit-test portability. |
| spine.h | Exposes new SNMPv3 helper APIs for tests/consumers. |
| snmp.c | Reworks SNMPv3 session construction: effective-level computation, validation/refusal rules, auth key derivation for authNoPriv, protocol validation, and passphrase wiping. |
| Makefile.am | Adds test_snmpv3_session to check_PROGRAMS / TESTS and defines its sources/linking. |
| CHANGELOG | Documents the SNMPv3 restoration and safety/validation behavior. |
Review details
Suppressed comments (1)
snmp.c:372
- This warning implies polling will proceed, but later validation/refusal (e.g., invalid auth protocol or invalid privacy protocol on the authPriv path) can still abort session construction. Rewording the message to describe the effective level avoids promising a successful poll.
SPINE_LOG(("SNMP: Device[%i] WARNING incomplete privacy settings; polling without encryption to match Cacti's effective security level.", host_id));
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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>
Member
Author
|
@copilot review |
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
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.
Extracted from #597 into a focused SNMPv3 correctness and safety change.
This restores noAuthNoPriv and authNoPriv session construction, derives authentication keys on the authNoPriv path, validates configured algorithms when they are used, wipes temporary passphrase copies, and makes the effective security-level decision agree with Cacti. It also prevents a failed mid-loop session rebuild from passing NULL into Net-SNMP multi-get.
Compatibility and failure policy:
Verification:
Closes #582
Refs #597