Audit: triage report + 3 CRITICAL fixes (read-only bypass, credential leak, Junos false-blocked) - #22
Open
Krishcalin wants to merge 4 commits into
Open
Krishcalin wants to merge 4 commits into
Krishcalin wants to merge 4 commits into
Conversation
Full findings from the repo-wide adversarial audit: 48 findings (3 critical, 23 high, 18 medium, 4 low), each confirmed by >=2 of 3 independent adversarial verifiers. Includes the executive summary, cross-cutting themes, prioritised fixes, and the complete per-finding appendix (location, category, failure scenario, evidence) so each can be triaged and assigned. No fixes yet — those follow as their own commits. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The PAN-OS XML API serves reads and writes on one path (/api/), distinguished by the type/action/cmd parameters, which travel in a POST body or a GET query string interchangeably. The POST rule carried the panos_read_only predicate; the GET rule carried none, so `if rule.body_predicate and ...` at readonly.py skipped the check and a config write refused as POST (type=config&action=set, type=commit, op <request>...) was PERMITTED as the identical GET — a write-to-device vector past the read-only guard (SRS §8). The 2026-09-29 change taught check_request to read GET query parameters, but it was inert here because no GET rule had a predicate to evaluate them against. This attaches panos_read_only to the GET /api/ rule, so GET is judged exactly as POST: config show/get, op <show>, keygen and read exports are permitted; set/edit/delete/rename/ move, commit and op <request> are refused. Adds TestPanOsXmlApiOverGet — the GET-side parity the suite lacked (every prior PAN-OS write-refusal test used POST). 338 readonly tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PAN-OS keygen-by-password builds `/api/?type=keygen&user=...&password=<cleartext>`, and DeviceSession.request recorded the command as `METHOD path` verbatim — so the firewall administrator's cleartext password (and, elsewhere, the issued API key in `key=`) was written into the tamper-evident audit log's command_text and served by GET /api/v1/audit-log to any AUDIT_READ holder. Mask credential-bearing query-string values (password/passwd/pwd/secret/api_key/ apikey/key/token) in the request path before it is recorded or logged — at the record, the record_violation, and the readonly.violation log sites. The real secret still goes on the wire to the device (keygen needs it); only what is persisted/served is masked. The preferred shape remains a pre-issued API key, where NetSecOps never holds the password at all (auth_exchange returns None then). Adds TestHttpRecordingRedactsCredentials: keygen password and api key are redacted in the recorded command while still sent to the device, and a refused write over GET does not log its query secret either. 22 device-session tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…it CRITICAL) _policies only accepted `from-zone ... to-zone ... policy ...` statements and dropped everything else, so `global policy ... then permit` and `default-policy permit-all` were never turned into SecurityRule objects. The connectivity/segmentation analysis keys off security_rules, so an SRX whose traffic is permitted by a global or default policy resolved to an empty rulebase, hit the implicit deny, and reported a false `blocked` — the inverse false-green (invariant 2/3). - Global policies parse like zone-pair policies but with empty src/dst zone sets, which the matcher (firewall/analysis.py:559) reads as "any zone" — the correct semantics. - default-policy permit-all/deny-all becomes a terminal any->any rule with the device's default action. - Rules are emitted in Junos evaluation order: zone-specific, then global, then default, which is what a first-match connectivity walk depends on. The shared match/then handling is factored into _apply_policy_tail. Adds regression tests for global parsing, permit-all/deny-all defaults, and the zone->global->default ordering (the audit noted none existed). 51 Junos parser tests pass; the full junos/juniper suite (65) is green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Sep 30, 2026
Merged
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
1 participant
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.
Adversarial audit — triage report + the 3 CRITICAL fixes
Repo-wide adversarial audit (18 finder dimensions, 3-lens adversarial verification per finding — kept only if ≥2 of 3 confirmed). 48 findings: 3 critical, 23 high, 18 medium, 4 low. Full report:
docs/audit/2026-09-30-adversarial-audit.md.This PR lands the report plus fixes for all three criticals. The 23 highs (dominated by the false-green-under-incomplete-input cluster) follow in a separate batch.
CRIT#1 — PAN-OS read-only bypass over GET (
3a9b06a)The PAN-OS XML API serves reads and writes on one path (
/api/), distinguished bytype/action/cmd, which travel in a POST body or a GET query string interchangeably. The POST rule carried thepanos_read_onlypredicate; the GET rule carried none, sotype=config&action=set,type=commit,op <request>were permitted over GET while refused over POST — a write-to-device vector past the read-only guard (SRS §8). The 2026-09-29 change taught the guard to read GET query params but was inert because no GET rule had a predicate to check. Fix: attachpanos_read_onlyto the GET/api/rule. +10 GET-parity tests (the suite was POST-only).CRIT#2 — Device credential leaked into the audit log (
3f0c866)PAN-OS keygen-by-password builds
/api/?type=keygen&user=…&password=<cleartext>, andDeviceSession.requestrecorded the command asMETHOD pathverbatim — persisting the cleartext password (and, elsewhere, the API key inkey=) into the tamper-evident audit log'scommand_text, served byGET /api/v1/audit-log. Fix: redact credential-bearing query params before the path is ever recorded or logged (record, record_violation, and the violation log). The real secret still goes on the wire to the device. +3 redaction tests.CRIT#3 — Junos global/default policies dropped → false "blocked" (
3935687)_policiesonly acceptedfrom-zone … policy …statements, silently droppingglobal policy …anddefault-policy permit-all. The connectivity/segmentation analysis keys offsecurity_rules, so an SRX whose traffic is permitted by a global/default policy resolved to an empty rulebase, hit the implicit deny, and reported a falseblocked(inverse false-green). Fix: parse global policies (empty zone sets = "any zone" to the matcher) and default-policy (terminal any→any rule), emitted in Junos evaluation order (zone → global → default). +4 regression tests.Verification
563 tests green across the touched areas (readonly, device-session, http-transport, profiles, junos parser), no regressions. Each fix added the regression test the audit noted was missing.
🤖 Generated with Claude Code