Skip to content

feat(threat-intel): recognize the CISA Known Exploited Vulnerabilities (KEV) catalog - #133

Merged
seonghobae merged 19 commits into
mainfrom
feat/cisa-kev-catalog-ingest
Aug 31, 2026
Merged

feat(threat-intel): recognize the CISA Known Exploited Vulnerabilities (KEV) catalog#133
seonghobae merged 19 commits into
mainfrom
feat/cisa-kev-catalog-ingest

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Why

"KEV" (Known Exploited Vulnerabilities — CISA's authoritative federal catalog of confirmed, actively-exploited CVEs, https://www.cisa.gov/known-exploited-vulnerabilities-catalog) was the actual ask behind this PR's original working title; wardnet's Threat Intelligence surface (AGENTS.md/CLAUDE.md: "integrate proven security engines instead of inventing detections") already adapts STIX/MISP/TAXII/OpenCTI but had no KEV adapter. CISA KEV tracking is also a concrete, checkable line item in enterprise buyer due-diligence and federal-agency vendor questionnaires (BOD 26-04), so this closes a real gap in docs/security/compliance-mapping.md's Threat Intelligence row.

What

  • src/kev_import.rs (new): pure parser mirroring misp_import.rs/opencti_import.rs/stix_import.rs exactly — parse_kev_document(body, source, ttl_seconds) -> Result<KevImportMaterial, String>. Accepts the real CISA catalog shape ({"vulnerabilities": [...]}, schema verified live against the current feed) or a bare array. Maps each entry to a ThreatIndicator { indicator_type: "cve", value: <CVE ID, canonicalized upper-case>, ... }. Severity: Critical when CISA's knownRansomwareCampaignUse == "Known", High otherwise (catalog inclusion alone already means CISA-confirmed active exploitation). No IP/domain/URL/hash observables exist in KEV data, so dnsbl stays empty (kept only for ThreatFeedImport shape parity).
  • src/lib.rs: POST /api/threat-intel/cisa-kev (admin-token gated). Unlike the MISP/STIX/OpenCTI "operator pastes a document" pattern, this follows the phishing-database fetch-by-URL pattern — KEV has one canonical, stable, public URL, so the gateway fetches it directly (defaults to the real CISA URL; SSRF-safe via the existing fetch_text_feed/validate_http_url machinery, restricted to www.cisa.gov unless allow_non_default_hosts is set). validate_http_url gained a third allowed_hosts: &[&str] parameter (was hardcoded to the phishing-database allowlist) so KEV gets its own host allowlist; all other call sites (TAXII poll, phishing-database, fetch_text_feed's internal check) updated accordingly. Reuses apply_threat_feed_import untouched, so upserts, audit logging, and feed-freshness tracking (KPIs, /api/commercial/readiness, support bundle, admin console) all pick it up for free, exactly like the other four adapters.
  • crates/waf-ids-core: new buyer_evidence_endpoints() entry for /api/threat-intel/cisa-kev. Also: score_request now excludes indicator_type == "cve" from content-substring matching (see Devin Review fix below).
  • docs/architecture.md, docs/security/compliance-mapping.md: documented alongside the other threat-intel adapters, plus a "Further reading" research-grounding section (see below).
  • Admin console gained a matching "CISA KEV catalog" card describing the endpoint.

Fixes from Devin Review

  • 🔴 Real bug, fixed (1414f89): every KEV CVE indicator was entering score_request's generic substring matcher at High/Critical severity — a legitimate request merely referencing a cataloged CVE ID (e.g. a vulnerability-management dashboard proxied through the gateway) could trip a default block-mode route. Fixed by excluding indicator_type == "cve" from content matching (mirrors the existing ip-type special case); the indicator stays fully visible via /api/threats, freshness, and buyer evidence. Added a core-crate unit test and an HTTP-level integration test (block-mode route + a request literally containing the CVE ID, asserting it's not blocked) — verified both fail without the fix and pass with it.
  • Research grounding added (449bc73): AGENTS.md's org rule ("substantive feature/process PRs should find the relevant academic papers... commit PDFs... or cite+link+summarize") was correctly flagged as unaddressed. Added a "Further reading" section to docs/architecture.md citing CISA's BOD 22-01, Jacobs et al. (2021)'s EPSS paper, and Shimizu & Hashimoto (2025, arXiv:2506.01220, CC BY 4.0, PDF committed under docs/papers/).
  • Raised, not fixed — reconciliation on refresh: Devin correctly noted a withdrawn CVE omitted from a later catalog refresh is never removed (apply_threat_feed_import is upsert-only). This is shared behavior across all five existing feed adapters (STIX/MISP/TAXII/OpenCTI/phishing-database), not something this PR introduces — proposed a follow-up direction in the PR thread rather than folding a cross-adapter reconciliation feature into this PR.
  • Ported unrelated CI fix (477b638): this branch forked from main before PR fix(deploy): disable service account token automount (KSV-0036) #132's SIGTERM-handler race fix merged, so tests/binary.rs's graceful-shutdown test was flaky here too. Ported the identical fix so it doesn't block this PR's CI; it will no-op once main carries it.

Deliberately out of scope

  • No cross-referencing into other detection sources. Confirmed via full read of suricata_eve.rs, coraza_audit.rs, builtin_signatures(): no CVE/CWE identifier exists anywhere else in the codebase, so there's no existing hook to enrich — inventing one would mean hand-rolling new detection logic, which is exactly what this project's architecture avoids.
  • No cargo-fuzz target. None of the five existing JSON-import adapters have one — CLAUDE.md's fuzzed-surface list is a closed set that doesn't cover this adapter family. Matched the actual sibling convention: a parse_never_panics_on_arbitrary_text unit test.

Verification

🤖 Generated with Claude Code

Summary by CodeRabbit

  • 새 기능

    • CISA KEV 카탈로그를 가져와 알려진 악용 취약점을 위협 인텔리전스에 등록합니다.
    • 랜섬웨어와 연관된 취약점은 더 높은 심각도로 분류됩니다.
    • 재수집 시 해당 피드에서 제거된 취약점도 정리됩니다.
    • 카탈로그는 공식 CISA 경로를 통해 안전하게 가져오며, 잘못된 항목은 자동으로 건너뜁니다.
  • 버그 수정

    • CVE 식별자가 요청 본문 검색 점수에 반영되지 않도록 개선했습니다.
    • 종료 신호 처리를 더 안정적으로 개선했습니다.
  • 문서

    • CISA KEV 연동, 증거 우선순위 및 컴플라이언스 매핑 내용을 업데이트했습니다.

Adds a new threat-intel adapter alongside the existing STIX/MISP/TAXII/
OpenCTI family: POST /api/threat-intel/cisa-kev fetches the CISA Known
Exploited Vulnerabilities catalog (a single well-known public JSON feed,
https://www.cisa.gov/known-exploited-vulnerabilities-catalog) and upserts
one `cve`-typed ThreatIndicator per entry, keyed by CVE ID, severity
escalated to Critical when CISA has tied the CVE to a known ransomware
campaign (High otherwise — mere inclusion in the catalog already signals
CISA-confirmed active exploitation).

- src/kev_import.rs: pure parser (`parse_kev_document`), mirroring the
  misp_import/opencti_import/stix_import module shape exactly. Accepts
  the real catalog shape (`{"vulnerabilities": [...]}`) or a bare array.
  KEV entries carry no IP/domain/URL/hash observable, so `dnsbl` stays
  empty (kept only for ThreatFeedImport parity).
- src/lib.rs: `import_kev_feed` handler follows the phishing-database
  fetch-by-URL pattern (not the MISP/STIX/OpenCTI paste-a-document
  pattern) since KEV has one canonical, stable, publicly known URL that
  every consumer wants pulled automatically rather than hand-relayed.
  Reuses the existing `fetch_text_feed`/`apply_threat_feed_import`
  plumbing untouched. `validate_http_url` gained a third `allowed_hosts`
  parameter (was hardcoded to the phishing-database allowlist) so KEV
  gets its own `www.cisa.gov`-only default, matching the SSRF-safety
  posture phishing-database already has (opt-out via
  `allow_non_default_hosts`); all other call sites updated.
- crates/waf-ids-core: new buyer-evidence-manifest endpoint entry.
- docs/architecture.md, docs/security/compliance-mapping.md: documented
  alongside the other threat-intel adapters.

Deliberately not adding a cargo-fuzz target: none of the five existing
JSON-import adapters (misp_import, opencti_import, stix_import,
suricata_eve, coraza_audit) have one today — CLAUDE.md's fuzzed-surface
list (request scorer, state deserializer, admin-token parser, DNSBL zone
export) doesn't cover this adapter family. Matched the existing
convention instead: a `parse_never_panics_on_arbitrary_text` unit test,
same as its siblings.

Verified: cargo fmt --check, cargo test --locked --workspace (all pass
except the pre-existing root-sandbox-only `load_surfaces_state_rewrite_failures`
flake tracked by PR #93, unrelated to this change), cargo clippy
--locked --workspace --all-targets -- -D warnings.
@seonghobae
seonghobae marked this pull request as ready for review August 30, 2026 11:44
@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 194a63d5-e11c-429e-8abf-82fa7fac843d

📥 Commits

Reviewing files that changed from the base of the PR and between b5115dc and 3c6eed6.

📒 Files selected for processing (4)
  • crates/waf-ids-core/src/lib.rs
  • src/credentials.rs
  • src/kev_import.rs
  • src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • crates/waf-ids-core/src/lib.rs
  • src/kev_import.rs
  • src/credentials.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

CISA KEV 카탈로그를 CVE 위협 지표로 가져오는 기능을 추가했습니다. 피드별 지표 소유권을 추적합니다. CVE 지표는 콘텐츠 점수에 사용하지 않습니다. KEV URL은 내장 CISA URL을 사용합니다. 시작 전 종료 신호 등록과 관련 문서 및 테스트를 갱신했습니다.

Changes

CISA KEV 통합

Layer / File(s) Summary
KEV 문서 파서
src/kev_import.rs
KEV 객체 또는 배열 JSON을 파싱합니다. 유효한 cveIDcve 위협 지표로 변환합니다. 랜섬웨어 연관 항목은 Critical, 그 외 항목은 High로 분류합니다. 유효 항목이 과반수 이하이면 문서를 거부합니다.
KEV URL 런타임 구성
src/credentials.rs, src/lib.rs, CLAUDE.md
KEV 카탈로그는 내장 CISA URL을 사용합니다. URL은 허용된 서버 측 재정의만 지원합니다. KEV_CATALOG_URL 환경 변수는 시작 구성에 영향을 주지 않습니다.
KEV 엔드포인트와 피드 소유권
src/lib.rs, crates/waf-ids-core/src/lib.rs
관리자 인증이 필요한 POST /api/threat-intel/cisa-kev를 연결합니다. 허용된 URL에서 카탈로그를 가져오고, CVE 지표와 피드 소유권을 갱신합니다. 다른 피드가 소유하거나 운영자가 별도로 등록한 지표는 보존합니다.
CVE 점수와 증거 문서
crates/waf-ids-core/src/lib.rs, docs/architecture.md, docs/security/compliance-mapping.md
cve 지표가 요청 경로와 본문을 매칭하지 않도록 변경합니다. 구매자 증거 목록과 CISA KEV 관련 문서를 갱신합니다.

종료 신호 등록

Layer / File(s) Summary
시작 전 종료 핸들러 등록
src/main.rs
run_from_env 호출 전에 Unix SIGTERM 또는 Windows Ctrl-C 핸들러를 등록합니다. Windows 대상 조건을 windows로 제한합니다.

Estimated code review effort: 4 (Complex) | ~60 minutes

Merge Risk: 🟡 Moderate · up to 3c6ee

The PR adds an authenticated server-side CISA KEV refresh that updates shared threat state and removes indicators no longer present. Concurrent refreshes can apply an older catalog snapshot after a newer one, and legacy records lack ownership metadata needed for reconciliation, so these lifecycle risks should be fixed or explicitly accepted before merging.

Sequence Diagram(s)

sequenceDiagram
  participant 관리자
  participant import_kev_feed
  participant CISA
  participant ThreatStore
  관리자->>import_kev_feed: POST /api/threat-intel/cisa-kev
  import_kev_feed->>CISA: 허용된 URL로 KEV 카탈로그 요청
  CISA-->>import_kev_feed: KEV JSON 응답
  import_kev_feed->>ThreatStore: CVE 지표와 피드 소유권 갱신
  ThreatStore-->>관리자: KevImportResult 응답
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.72% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 69 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed PR 제목은 CISA Known Exploited Vulnerabilities (KEV) 카탈로그 인제스트 기능 추가라는 주요 변경 사항을 정확하고 구체적으로 요약합니다.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/cisa-kev-catalog-ingest

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 6 potential issues.

Devin Review

Comment thread src/kev_import.rs
Comment thread src/lib.rs
Comment thread src/kev_import.rs
Comment thread src/kev_import.rs
Comment thread src/lib.rs
Comment thread src/lib.rs Outdated
claude added 4 commits August 30, 2026 11:52
Devin Review flagged (PR #133): every KEV-imported CVE indicator was
entering score_request's generic substring matcher at High/Critical
severity. A legitimate request that happens to reference a cataloged
CVE literally (e.g. a vulnerability-management dashboard's own traffic
hitting `/api/cve/CVE-2021-44228` through the gateway) would score high
enough to trip a default block-mode route -- a real false-positive/
denial-of-service risk, not a hypothetical one.

A CVE identifier is vulnerability-catalog metadata, not a request-
content attack observable (unlike domain/url/hash/ip indicators from
the other threat-intel adapters, which genuinely can appear in
malicious traffic). Fix: score_request now short-circuits indicator_type
"cve" to never content-match, mirroring the existing ip-type special
case. The indicator stays fully visible via /api/threats, feed
freshness, KPIs, and buyer evidence -- only request-content scoring is
excluded.

Added a core-crate unit test (score_request_never_content_matches_cve_indicators)
and an HTTP-level regression test (kev_cve_indicators_never_block_legitimate_requests:
imports a KEV catalog, creates a default block-mode route, and asserts
a request literally containing the cataloged CVE ID is NOT blocked).
Verified both fail without the fix and pass with it.
This branch forked from main before #132's shutdown-handler fix
merged, so its copy of src/main.rs still had the pre-existing race
(readiness announced before the signal handler was registered),
making tests/binary.rs::binary_serves_then_shuts_down_on_sigterm flaky
here too -- unrelated to this PR's KEV work but blocking its CI green.
Ported the identical fix rather than waiting on #132 to merge first;
it will no-op once main carries it.
Devin Review flagged (PR #133) that this substantive feature PR was
missing the research grounding AGENTS.md's org rule requires
(commit paper PDFs + full citations, or cite+link+summarize when
redistribution isn't permitted).

Added a "Further reading" section to docs/architecture.md (matching
docs/fuzzing.md's existing citation convention) citing:
- CISA's BOD 22-01, the directive establishing KEV's confirmed-active-
  exploitation inclusion criterion -- why catalog membership alone
  already implies at least High severity in kev_import.rs.
- Jacobs et al. (2021), the EPSS paper -- the data-driven basis for
  treating exploitation evidence as a stronger prioritization signal
  than static CVSS severity.
- Shimizu & Hashimoto (2025, arXiv:2506.01220, CC BY 4.0) -- empirical
  evidence that KEV-first triage cuts urgent-remediation workload
  ~95% but still misses exploited CVEs EPSS catches, supporting KEV as
  one adapter among the existing STIX/MISP/TAXII/OpenCTI family rather
  than a standalone replacement.

Committed the arXiv PDF (CC BY 4.0, redistribution explicitly
permitted) into docs/papers/, matching the one existing precedent
(the fuzzing survey PDF cited from docs/fuzzing.md). The EPSS paper is
ACM/SSRN-hosted without a redistributable PDF, so it is cited+linked
per AGENTS.md's explicit fallback instead of attached.
… default

CodeQL flagged (PR #133, critical severity, rust/request-forgery at
src/lib.rs:2646 on the pre-fix commit): "The URL of this request
depends on a user-provided value" -- import_kev_feed fetched
request.kev_url directly, so a JSON body field flowed into the
outbound HTTP client call. validate_kev_import_request already
allowlists the host (www.cisa.gov) unless allow_non_default_hosts is
set, but that's a value-equality check a couple of calls away from the
fetch, not a pattern static analysis reliably recognizes as a
sanitizer for the same tainted string reused later.

Removed the taint at the source instead of arguing with the analyzer:
on the default (non-override) path, the fetch now uses the hardcoded
KEV_DEFAULT_URL constant, never request.kev_url -- there is no
request-controlled string reaching the HTTP client unless the operator
explicitly sets allow_non_default_hosts: true (the same opt-in gate
that already exists for the host allowlist). All existing tests point
at a local mock server via that same flag, so they're unaffected and
still pass.

Note: the identical fetch-an-operator-supplied-URL-after-an-allowlist-
check pattern also exists in the pre-existing phishing-database and
TAXII-poll endpoints (unmodified by this PR) -- out of scope here, but
worth the same treatment in a follow-up if CodeQL flags them too.
devin-ai-integration[bot]

This comment was marked as resolved.

claude added 2 commits August 30, 2026 12:02
Devin Review flagged (PR #133): with allow_non_default_hosts false,
validate_kev_import_request still checked kev_url against the CISA
host allowlist even though import_kev_feed (65d60a9) now always
fetches the hardcoded default in that mode -- so a validated
same-host custom path was silently discarded, and the validation
itself was misleading (accept-then-ignore).

Chose Devin's second suggested option over restoring same-host
customization: honoring a validated-but-still-request-controlled URL
on the default path is exactly the pattern that triggered the
original CodeQL SSRF alert, so reintroducing it isn't a real fix.
Instead, kev_url is now only validated when allow_non_default_hosts
is true (the one mode where it's actually fetched) -- and once
opted in, any well-formed http(s) URL is accepted with no host
restriction, matching how that same flag already works for
phishing-database and TAXII. KEV_ALLOWED_HOSTS is removed as dead
code; there is no longer a partial-trust "same host, no opt-in"
tier to enforce.

Replaced the now-invalid kev_feed_import_rejects_disallowed_host_by_default
HTTP test (there is no more "disallowed host" rejection under the new
contract) with a validate_kev_import_request unit test covering both
modes directly.
CodeQL's critical SSRF alert (rust/request-forgery) was still firing
on the previous fix (65d60a9): gating the tainted request.kev_url
behind an allow_non_default_hosts runtime flag doesn't register as a
sanitizer to CodeQL's dataflow analysis -- the string still
originates from the HTTP request body and still reaches
fetch_text_feed on that code path, so the alert (rightly, from a
pure taint-tracking standpoint) persisted.

Rather than keep trying to convince the analyzer a runtime guard is
safe, removed the taint source outright: kev_url and
allow_non_default_hosts are gone from KevImportRequest entirely.
import_kev_feed always fetches AppState::kev_catalog_url, which is
deployment-time config only -- set via the new KEV_CATALOG_URL env
var (validated at startup, alongside run_from_env's other env
parsing) or AppState::with_kev_catalog_url() in tests -- never
sourced from a client request. There is now no code path in this
handler where request-supplied data reaches an outbound fetch at
all.

This is a stronger fix than the previous one on the merits too, not
just for the analyzer: KEV genuinely has one canonical, stable,
government-published URL, so per-request override was never load-
bearing functionality, just a testing convenience that's now served
by the AppState builder instead.

Updated the three affected tests to point AppState::kev_catalog_url
at a local mock server instead of the request body, and replaced the
now-obsolete kev_url validation test with a plain feed-metadata
validation test. CLAUDE.md's Runtime Configuration section documents
the new env var.

Copy link
Copy Markdown
Contributor Author

Update on the CodeQL critical SSRF alert (rust/request-forgery, src/lib.rs): my previous fix (65d60a9 — gate the request-supplied kev_url behind an allow_non_default_hosts runtime flag) did not clear it. Checked the actual annotation after that push and it was still firing at the same shape, because a boolean-gated branch isn't a sanitizer CodeQL's dataflow analysis recognizes — the tainted string still originates from the HTTP request body and still reaches the outbound fetch on that code path.

Pushed a stronger fix instead (1cf1010): removed kev_url/allow_non_default_hosts from the request contract entirely. import_kev_feed now always fetches AppState::kev_catalog_url, which is deployment-time config only — set via a new KEV_CATALOG_URL env var (validated at startup) or AppState::with_kev_catalog_url() in tests, never from a client request. There is no longer any code path where request-supplied data reaches an outbound fetch in this handler at all, which is a stronger property than "validated before use" on the merits, not just something aimed at satisfying the analyzer.

cargo fmt/test --workspace/clippy -D warnings all clean on this commit. Watching for the fresh CodeQL run on 1cf1010 to confirm the alert actually clears this time before considering this resolved.


Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

fetch_text_feed's url parameter is fed by phishing-database's
request-supplied domain_url/ip_url (a pre-existing, admin-gated
"fetch from an operator-chosen URL" design on main). Routing the KEV
catalog pull through that same shared function -- confirmed via diff
against origin/main that fetch_text_feed/validate_http_url were
otherwise byte-identical -- was enough for CodeQL's rust/request-forgery
query to keep flagging the shared sink as newly touched by this PR,
even after KEV_CATALOG_URL was made config-only.

Give KEV its own fetch_kev_catalog function (same pattern already used
by fetch_taxii_objects) so the config-only path never shares a
function with the request-URL adapters, and revert validate_http_url
to its original two-argument form now that KEV no longer needs a
custom allowed-hosts list.

Also add KEV_CATALOG_URL to clear_run_env: Devin Review flagged that
an inherited value would leak across run_from_env tests, since the
list omitted it while validating every other run_from_env env var.

Copy link
Copy Markdown
Contributor Author

Root cause found for the CodeQL rust/request-forgery critical that survived the last two fixes (commit 1cf1010).

The flagged sink was feed_http.get(url) inside fetch_text_feed. I diffed that function (and validate_http_url) against origin/main and they were byte-identical except for the one no-op parameter I'd added — the actual request-tainted flow into that sink is PhishingDatabaseImportRequest.domain_url/ip_url (an existing, pre-#133, admin-gated "fetch from an operator-chosen URL" feature on main). My PR never introduced that taint; it just added a new call into the same shared function, and CodeQL's diff-scoped view attributed the (pre-existing) sink to this PR because I'd touched an adjacent line inside it.

Fix pushed in 8915b16: KEV now fetches through its own fetch_kev_catalog, structurally decoupled from fetch_text_feed — mirroring the separation fetch_taxii_objects already uses for the same reason. validate_http_url is reverted to its original two-argument form (byte-identical to main again), and fetch_text_feed is untouched. KEV's fetch path now shares no function with the request-URL adapters, so it can't be conflated with their taint by static analysis or otherwise.

Also fixed the "KEV environment escapes test cleanup" finding: KEV_CATALOG_URL is now included in clear_run_env.

Not actioned: the "environment controls privileged catalog fetches" 🟨 finding (no host allowlist on KEV_CATALOG_URL). This mirrors the existing, accepted design of TAXII's operator-supplied objects_url (also allow_non_default_hosts=true, admin-token-gated, deployment/operator-trusted input, not attacker input) — loopback is intentionally allowed since the test suite itself relies on it. Flagging as a considered, not a missed, finding.

cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace (109 lib tests + integration suites) are all green locally on this commit. Waiting for CodeQL's definitive result on 8915b16 before declaring this resolved — will confirm once the completed conclusion lands.


Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

github-advanced-security[bot]

This comment was marked as resolved.

Devin Review flagged that KEV_CATALOG_URL had no host allowlist, so an
unintended inherited or misconfigured value could redirect this
privileged catalog fetch -- a real SSRF sub-pattern (env-var-sourced
URLs are externally influenceable via container/orchestrator
inheritance, not just via request bodies), which lines up with why
CodeQL kept flagging feed_http.get(url) here even after the previous
commit fully decoupled the fetch from any request-derived data.

Add validate_kev_catalog_url, enforced both at startup (fail fast on a
bad env var) and again immediately before the fetch: the host must be
www.cisa.gov or loopback (loopback only so tests can point it at a
local mock server), with no override -- unlike the operator-URL
adapters, KEV has no legitimate reason to fetch from anywhere else.

Copy link
Copy Markdown
Contributor Author

Commit 8915b16 (fully decoupling KEV's fetch from fetch_text_feed) still hit the same critical CodeQL rust/request-forgery alert — now on the new fetch_kev_catalog function's own .get(url), with url sourced only from AppState::kev_catalog_url. I verified there is no request-handler write path into that field at all (grepped every reference), which ruled out request-tainted flow as the cause.

Re-reading Devin's earlier 🟨 finding on this same PR ("KEV_CATALOG_URL bypasses the required registry and accepts any HTTPS host or loopback service") in that light changed my assessment: an env-var-sourced URL used in a privileged fetch is a legitimate SSRF sub-pattern independent of request tainting — it's externally influenceable through container/orchestrator env inheritance or misconfiguration, not just through request bodies. I'd initially stood down on that finding as "matches TAXII's existing operator-URL design," but TAXII's URL is a per-request, admin-authenticated choice; KEV's is ambient environment state with no such gate, so the two aren't actually equivalent.

Pushed 3a47ce8: KEV_CATALOG_URL is now restricted to www.cisa.gov (or loopback, so tests can still point it at a local mock server), enforced both at startup and immediately before each fetch, with no override — KEV has no legitimate reason to fetch from anywhere else, unlike the operator-chosen-URL adapters. This should also satisfy CodeQL, since it's the canonical explicit-host-equality-guard-before-the-sink shape rather than a boolean-gated bypass.

cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace (109 lib tests + integration suites, existing KEV redirect/host tests included) all green locally on this commit. Watching for CodeQL's result on 3a47ce8.


Generated by Claude Code

devin-ai-integration[bot]

This comment was marked as resolved.

…allowlist

Devin Review caught that AppState::with_kev_catalog_url's doc comment
still promised support for pointing KEV_CATALOG_URL at an internal
mirror, which the host-allowlist fix in 3a47ce8 now rejects at startup
-- a real contract mismatch, not just stale wording. There was never
an actual mirror requirement (that phrase was leftover illustrative
language from an earlier draft); update the builder's doc comment and
CLAUDE.md's Runtime Configuration entry to describe the real contract
(CISA's own host, or loopback for local test mocking) rather than
inventing a mirror-allowlist feature nobody asked for.
coderabbitai[bot]

This comment was marked as resolved.

CodeRabbit review, both real:

- kev_import.rs: any non-empty cveID (e.g. "not-a-cve") was accepted
  and stored as a cve threat indicator. Add is_valid_cve_id, checking
  CVE.org's CVE-<4-digit year>-<4+ digit sequence> syntax
  case-insensitively; entries that fail it are now Skipped like a
  missing cveID already was. Panic-safe on arbitrary catalog text via
  str::get instead of direct slicing (added a multi-byte-boundary case
  to parse_never_panics_on_arbitrary_text to cover it).

- main.rs: install_shutdown_signal's second definition was gated on
  cfg(not(unix)), but its body calls tokio::signal::windows::ctrl_c,
  which only exists on Windows -- any other non-Unix target would fail
  to compile there. Scope the cfg to windows specifically.

Copy link
Copy Markdown
Contributor Author

Addressing CodeRabbit's 4 findings on commit 3a47ce8 (pushed as 9494144, plus 0760e7b which already covered one of them):

  1. kev_import.rs:90-95 — validate cveID format (🟡 real bug, fixed): any non-empty string was accepted and stored as a cve indicator (e.g. "not-a-cve" would pass through). Added is_valid_cve_id, checking CVE.org's CVE-<4-digit year>-<4+ digit sequence> syntax case-insensitively; entries that fail it are now Skipped, same as a missing cveID. Added a unit test for the syntax check plus a document-level test, and covered the panic-safety edge case (a multi-byte char straddling the 4-byte prefix boundary) since the parser has a "never panics on arbitrary text" invariant to uphold.

  2. lib.rs:148-150with_kev_catalog_url doc vs. allowlist (🟡, already fixed in 0760e7b): this was the same "internal mirror" doc mismatch Devin caught earlier; already corrected before this CodeRabbit pass ran.

  3. main.rs:27-31 — scope the Windows cfg precisely (🟡 real bug, fixed): install_shutdown_signal's second definition was gated on cfg(not(unix)) but its body calls tokio::signal::windows::ctrl_c, which only exists on Windows — any other non-Unix target would fail to compile there. Scoped to cfg(windows) specifically.

  4. lib.rs:3257 — read KEV_CATALOG_URL from a KV/credential registry, not env (🟠 major, raised not fixed): this cites a real org rule (AGENTS.md: "do not read config/secrets from raw environment variables... read them from a KV / credential registry"). But AGENTS.md itself documents this as a known, already-accepted, whole-codebase deviation: only ADMIN_TOKEN/ADMIN_TOKENS (actual secrets) go through CredentialRegistry today; every other non-secret operational config value (BIND_ADDR, WAF_IDS_STATE_PATH, DNSBL_ORIGIN, EVENT_LIMIT, RATE_LIMIT, RATE_LIMIT_WINDOW, MAX_BODY_BYTES) reads std::env::var directly too, with the doc's own note: "migrate those behind the same registry/KV when a durable credential store is adopted." KEV_CATALOG_URL follows the exact same established pattern as those — not a regression this PR introduces, and migrating the whole config surface to a KV/registry is a large, pre-existing, tracked, cross-cutting item well outside this PR's scope (matches CodeRabbit's own "🏗️ Heavy lift" tag on the finding). Leaving as-is, consistent with every sibling env var.

cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace (111 lib tests + integration suites) all green locally on 9494144.

Also: opencode-review shows failure on recent commits — confirmed via its job log this is the fail-closed design ("No APPROVED or CHANGES_REQUESTED from opencode-agent on the current head") rather than a code problem; it should resolve once the bot posts a verdict on the latest head now that pushes have settled. Still watching for CodeQL's definitive result.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Status update on the CodeQL rust/request-forgery critical: it's still flagged on commit 3a47ce8, same rule, same sink (src/lib.rs:2219, .get(url) inside fetch_kev_catalog) — even with the host-allowlist check from that exact commit sitting immediately above it (validate_kev_catalog_url(url)? on the line right before).

Summarizing the full trail across four independently-designed fixes, each verified at the code level before pushing:

  1. 1cf1010 — removed kev_url from the request body entirely (KevImportRequest no longer accepts a URL at all).
  2. 8915b16 — gave KEV its own fetch_kev_catalog, structurally decoupled from fetch_text_feed (the function phishing-database's actually-request-tainted URLs flow through); verified fetch_text_feed/validate_http_url were byte-identical to main afterward.
  3. 3a47ce8 — added validate_kev_catalog_url, an explicit host-equality allowlist (www.cisa.gov or loopback) called immediately before the sink — the canonical "guard right before the sink" shape most taint-tracking SSRF queries are built to recognize as a sanitizer.

Each of these was verified to actually close the specific gap it targeted (confirmed via direct source-level diffing and, for #2, byte-for-byte comparison against main). None moved this alert. Every attempt lands on the identical rule at the identical logical sink, sourced only from AppState::kev_catalog_url — which I've also re-confirmed has no write path from any request handler (grepped every reference; it's set only by run_from_env reading KEV_CATALOG_URL, or by a #[cfg(test)]-only builder method).

At this point I think it's more likely than not that this is a modeling gap in CodeQL's Rust/Axum support — specifically, that State<AppState> (server-owned shared state) isn't being distinguished from per-request extractors like Json<T>/Query<T>, so any value read off AppState inside a handler-reachable function gets treated as "user-provided" regardless of actual provenance or in-code validation. I can't confirm this directly: this session doesn't have Security-tab access to inspect the alert's actual flow-path trace or to dismiss it, only the check-run's summary/annotation text.

I don't want to keep reshaping code against an alert that hasn't moved through three structurally different, verified-effective attempts — that's very likely chasing a tool limitation rather than fixing anything real at this point. Flagging for a human with Security-tab access: could someone open the alert (/security/code-scanning, PR #133) and check whether the flow-path trace actually shows a request-derived source, or whether it terminates at AppState/env config? If it's the latter, this looks like a legitimate case for dismissing the alert as a false positive with that reasoning attached.

Everything else on this PR is green locally (fmt/clippy/test, 111 tests) and I'll keep watching for review/CI activity, but I'm pausing further SSRF-alert-specific code changes absent new diagnostic information.


Generated by Claude Code

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 3 new potential issues.

Devin Review

Comment thread src/lib.rs Outdated
Comment thread crates/waf-ids-core/src/lib.rs
Comment thread src/lib.rs Outdated
Devin Review flagged a real bug in the feed-ownership reconciliation
logic introduced by an external merge (192ffba, authored outside this
session): apply_threat_feed_import removed every key the refreshing
feed had previously owned from the global threats collection
unconditionally, without checking whether another feed's ownership
record still claimed the same indicator_type+value+source key. Two
feeds sharing a key (e.g. the same CVE imported under a shared
`source`) meant refreshing either one made the indicator vanish from
enforcement until the other feed's next refresh happened to re-add it.

Track "still owned by some other feed" via the current (post-replace)
ownership table and only reap a dropped key when no feed claims it any
more. Also switches previous_keys from a Vec to a HashSet, which
incidentally fixes a second (lower-severity) finding on the same
commit: the per-threat previous_keys.contains() scan was O(n) per
retained item, made O(1) here.

Added feed_refresh_preserves_indicators_still_owned_by_another_feed,
which fails against the pre-fix logic (the shared indicator disappears
after the first feed's refresh) and passes with it -- verified by
reasoning through both code paths.

Not actioned (informational/acceptable trade-offs, not bugs):
- "Upgraded feeds retain withdrawn indicators": state persisted before
  this feature existed has no ownership history, so the first refresh
  per feed after upgrade can't retroactively reconcile pre-existing
  drift. Self-heals from each feed's first post-upgrade refresh
  onward; there's no safe way to backfill ownership for data imported
  before it was tracked.
- "DNSBL refresh remains append-only": reconciliation covers threats
  only, matching the scope of the original finding this responds to
  (KEV catalogs carry no DNSBL entries). Extending it to DNSBL entries
  for the other four adapters is a follow-up, not a regression here.
coderabbitai[bot]

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

CodeRabbit's latest walkthrough flags "Merge Risk: 🟠 High" based on commit 192ff (before my latest push) — two points, both already addressed or pre-existing:

  1. "Feed identity collisions can temporarily remove indicators supplied by another source" — this is exactly the bug Devin also caught on that same commit, already fixed and pushed in 64dcbfc (see my reply above). Stale relative to the current head.
  2. "Deployments without administrator credentials may allow unauthenticated callers to trigger persistent changes" — verified this is real but pre-existing, unrelated to this PR: admin_authorized's fallback (src/lib.rs:2586-2588) is None means auth is disabled by explicit design when neither ADMIN_TOKEN nor ADMIN_TOKENS is configured — and that's true of every management endpoint in the codebase (routes, DNSBL, threat feeds, license, etc.), not something specific to the new KEV endpoint. It's the documented security model (docs/architecture.md line 61: "Remote management requires ADMIN_TOKEN plus external TLS and identity controls"; default bind is localhost). KEV's endpoint uses the exact same admin_authorized gate as its four sibling adapters — changing that gate's default behavior would be a large, cross-cutting change well outside this PR's scope, not a KEV-specific gap.

Current head (64dcbfc) is green on everything except the still-open CodeQL question (awaiting Security-tab confirmation) and the known strix/opencode-review waits.


Generated by Claude Code

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 1 new potential issue.

Devin Review

Comment thread src/lib.rs

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 4 new potential issues.

Devin Review

Comment thread src/credentials.rs Outdated
Comment thread src/lib.rs
Comment thread crates/waf-ids-core/src/lib.rs
Comment thread src/kev_import.rs
…to admin creds

Devin Review, both real:

- kev_import.rs: apply_threat_feed_import now reconciles (a refresh
  treats keys missing from the new snapshot as withdrawn and removes
  them). kev_material_from_value's only acceptance bar was "at least
  one usable cveID," so a catalog that's mostly unparsable -- a fetch
  truncated mid-transfer, a CISA response format regression -- would
  have been accepted and, via reconciliation, read as a mass
  withdrawal of still-exploited CVEs instead of the bad fetch it
  actually was. Require a real majority of entries to have parsed
  (skipped_entries <= threats.len()) before trusting a snapshot as
  authoritative. Adjusted skips_entries_with_malformed_cve_id's fixture
  to stay under the new bar and added
  rejects_catalog_where_most_entries_are_unparsable.

- credentials.rs: bootstrap_secrets set the registry-wide
  CredentialSource to File whenever ANY key -- including a file that
  supplies only kev_catalog_url -- came from the credentials file, even
  when ADMIN_TOKEN/ADMIN_TOKENS actually came from env. CredentialSource
  is documented and reported via HealthStatus/support bundle as admin
  credential provenance specifically, so this misreported security-
  relevant operational state. Track admin-credential file/env
  provenance independently of kev_catalog_url's, matching the
  documented contract. Added
  file_only_kev_catalog_url_does_not_misreport_env_admin_token_as_file_backed.

Not actioned in this commit -- flagging separately for the user's
scope decision: Devin also noted that apply_threat_feed_import's
reconciliation now applies uniformly to all five existing feed
adapters (STIX/MISP/TAXII/OpenCTI/phishing-database), not just KEV,
which changes their prior upsert-only semantics; anyone relying on
incremental/partial imports under a reused feed_id across the other
four adapters would now see earlier entries treated as withdrawn. This
is a cross-adapter behavior/policy question outside what a bug fix can
resolve unilaterally.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 2 new potential issues.

Devin Review

Comment thread src/kev_import.rs Outdated
Comment thread src/credentials.rs
devin-ai-integration[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 3 new potential issues.

Devin Review

Comment thread src/lib.rs Outdated
Comment thread src/kev_import.rs
Comment thread src/lib.rs
devin-ai-integration[bot]

This comment was marked as resolved.

Devin Review: apply_threat_feed_import's operator-ownership skip
(8b7954d) omits operator-owned threats from the upsert loop, but
ThreatFeedImportResult.upserted_threats still reported feed.threats.len()
-- the full submitted set, including entries that were skipped. Import
clients (including KEV's response) received an inflated success count
whenever a feed overlapped operator-managed data.

Count actual upserts in the loop and return that. ThreatFeedStatus.threat_count
is left as feed.threats.len() deliberately -- it represents catalog
membership (what this feed's snapshot claims), not applied mutations,
per Devin's suggested distinction.

Added import_result_excludes_operator_owned_threats_from_upserted_count.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

Devin Review

Comment thread src/lib.rs
@seonghobae

Copy link
Copy Markdown
Contributor Author

Verified on the current PR head c4f4526: apply_threat_feed_import now skips feed upserts for keys present in operator_threat_keys, keeps operator-owned indicators during feed withdrawal, and the branch includes feed_refresh_preserves_operator_managed_indicator_payload plus import_result_excludes_operator_owned_threats_from_upserted_count. That makes the earlier operator-overwrite concern stale on the current head.

Copy link
Copy Markdown
Contributor Author

Bypass-merge rationale (per explicit user authorization)

Merging this via admin bypass rather than waiting on the standard review gate. Status at head c4f4526:

  • CodeQL rust/request-forgery (critical): now completed/success — the first non-neutral, non-failing conclusion across this PR's entire lifecycle, after 6+ remediation attempts (request-body removal → function decoupling → host allowlisting → credential-registry indirection → full removal of runtime URL configurability). Confirmed directly via the Checks API for this exact head SHA, not inferred from an earlier or "neutral" result.
  • CodeRabbit's CWE-306 finding ("admin_authorized returns true when no admin token is configured, so KEV import could run unauthenticated") — verified fixed, not just claimed: import_kev_feed now calls has_write_admin_credential(&state) first and returns 503 if no write-capable credential is configured at all, closing the gap independently of the general auth-disabled fallback.
  • Independently re-ran cargo fmt --check, cargo clippy --locked --workspace --all-targets -- -D warnings, and cargo test --locked --workspace (skipping the pre-existing, unrelated load_surfaces_state_rewrite_failures sandbox flake tracked by test(persistence): replace permission-based fault injection with a deterministic seam #93) against this exact commit myself — all clean, not taken on faith from CI.
  • Devin Review and CodeRabbit PR-level checks: success.
  • Remaining red checks are known non-issues, not defects in this PR: strix is the org-wide ContextualWisdomLab/.github sidecar outage affecting ~15 other open PRs identically (out of this repo's control); opencode-review is fail-closed by design when a fresh commit lands before the bot re-posts a verdict on the new head (confirmed via job-log inspection on a separate PR).

Bypassing the "1 independent approving review" branch-protection gate specifically because no human reviewer has approved and the org's AI reviewers (Devin/CodeRabbit) cannot satisfy that gate — this is done under explicit user authorization to bypass-merge, not a self-approval.


Generated by Claude Code

@seonghobae
seonghobae merged commit b2bcee3 into main Aug 31, 2026
33 of 35 checks passed
@seonghobae
seonghobae deleted the feat/cisa-kev-catalog-ingest branch August 31, 2026 01:01
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.

4 participants