Skip to content

Fix validated Codex Security findings - #283

Open
ricfog wants to merge 2 commits into
mainfrom
fix/codex-security-findings
Open

Fix validated Codex Security findings#283
ricfog wants to merge 2 commits into
mainfrom
fix/codex-security-findings

Conversation

@ricfog

@ricfog ricfog commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • prevent Azure Foundry AAD credentials from being persisted in scenario-tester transcript events by reusing the sanitized LLM call trace
  • harden HTTP endpoint targets against redirects and DNS rebinding by validating the exact connection-time addresses, canonicalizing mapped/NAT64/IPv4-compatible endpoints before policy checks, rejecting non-public targets, and cleaning up partial client initialization
  • preserve content-filter failures as scoped per-test-case errors instead of crashing the inference coordinator

Security validation

A complete Codex Security run at xhigh effort covered all 98 scoped files. The reported high-severity credential leak and two medium-severity findings were reproduced safely and confirmed before these fixes were implemented. Review follow-up also closed IPv4-in-IPv6 SSRF bypasses for the RFC 6052 NAT64 well-known prefix and deprecated IPv4-compatible addresses.

Tests

  • python -m pytest -q: 1193 passed, 23 skipped, 474 subtests passed
  • Python 3.11, 3.12, and 3.13 compatibility verified for the IPv4-in-IPv6 canonicalization
  • npm run check in viewer: 0 errors (6 existing warnings)

@MohammadHaroonAbuomar MohammadHaroonAbuomar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: request-changes. One blocking SSRF gap in the new IP canonicalization (inline); the rest is correct.

Implemented correctly and covered by tests:

  • Credential leak: the tester transcript event reuses the sanitized build_llm_call_trace output; sanitize_payload redacts api_key, and test_transcript_redacts_tester_request_credentials asserts the token is absent from the serialized transcript.
  • Content-filter handling: LLMContentFilterError is caught as a scoped per-case error and counted in errored_count instead of aborting the coordinator. The seed_row.get("seed_id") it replaces was a latent NameError — the sibling handlers already use test_case_row.
  • Redirect / DNS-rebinding hardening: allow_redirects=False with explicit 3xx rejection, connection-time revalidation via _ValidatingResolver, and connector/resolver cleanup on partial init, each with a regression test.

CI is green (CodeQL, Tier 1 unit tests, install matrix). Local tests/test_security.py: 58 passed.

Blocking finding: the connection-IP validator canonicalizes embedded IPv4 only for IPv4-mapped addresses and treats other IPv4-in-IPv6 embeddings as global unicast. NAT64 (64:ff9b::/96) and IPv4-compatible (::/96) forms pass validation while embedding private / loopback / metadata IPv4, giving a working SSRF bypass on a NAT64 network. Details and fix inline in assert_ai/core/security.py.

Comment thread assert_ai/core/security.py Outdated

@MohammadHaroonAbuomar MohammadHaroonAbuomar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. The IPv4-in-IPv6 SSRF gap from the previous review is closed and verified against ab4ee6b.

_canonicalize_endpoint_ip now resolves the embedded IPv4 for IPv4-mapped (::ffff:), NAT64 well-known prefix (64:ff9b::/96), and IPv4-compatible (::/96, excluding :: and ::1) addresses before the range and is_global checks. Re-probed on this head:

  • 64:ff9b::7f00:1, 64:ff9b::a9fe:a9fe (metadata), 64:ff9b::a83f:8110 (Azure wireserver), and ::127.0.0.1 are all rejected.
  • NAT64/mapped/compatible forms that embed a public IPv4 (64:ff9b::5db8:d822, ::ffff:93.184.216.34) are still allowed, so the canonicalization does not over-block.
  • :: and ::1 remain IPv6 and are caught by the blocked ranges / is_global check.

Rejection and allow tests were added for each case; tests/test_security.py passes (65). CI is green (15/15). The credential-leak, content-filter, redirect, and DNS-rebinding fixes from the first pass are unchanged. No outstanding items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants