Skip to content

fix(security): SSRF-guard gate webhooks (Phase C, C4)#216

Merged
yairfalse merged 2 commits into
mainfrom
feature/monster-c-ssrf-sandbox
May 25, 2026
Merged

fix(security): SSRF-guard gate webhooks (Phase C, C4)#216
yairfalse merged 2 commits into
mainfrom
feature/monster-c-ssrf-sandbox

Conversation

@yairfalse
Copy link
Copy Markdown
Collaborator

@yairfalse yairfalse commented May 25, 2026

Monster Phase C — item C4 from docs/audit-2026-05-22.md.

A pipeline-declared gate webhook_url was POSTed with no destination validation, so it could be pointed at the cloud metadata endpoint (169.254.169.254) or internal services from inside the trust boundary — a classic SSRF / metadata-exfiltration sink.

Fix

  • Extracted the existing notification-webhook SSRF guard into a shared Sykli.HTTP.check_ssrf/1 (resolves the host; blocks loopback, link-local incl. 169.254.0.0/16, and RFC1918 — IPv4 and IPv6).
  • Applied it in gate_service's wait_webhook (blocked → {:denied, "webhook_url blocked: ..."}).
  • notification_service now delegates to the same helper (DRY — both webhook paths enforce identical blocking).

Verification

mix format / mix credo (clean) / mix test (1757, 0 failures, +5 SSRF tests) / mix escript.build; black-box GATE cases pass.

Phase C remaining

  • C5 Shell-runtime trust ADR + Add fluent Go SDK API #4 rewrite (decision: document Shell = trusted) — own PR (establishes docs/adr/)
  • C3 mask file/OIDC-resolved secrets — own PR
  • C2 per-team coordinator authz (decision: enforce per-team tokens) — own PR (auth-model change)

🤖 Generated with Claude Code

@yairfalse @claude
fix(security): SSRF-guard gate webhooks via shared Sykli.HTTP.check_s…
3414de9 
…srf/1

Monster Phase C, item C4 (audit). A pipeline-declared gate webhook_url could be
pointed at loopback/link-local/private addresses (e.g. 169.254.169.254 cloud
metadata) from inside the trust boundary. Extracted the existing notification
webhook SSRF guard into shared Sykli.HTTP.check_ssrf/1 (resolves host, blocks
loopback/link-local/RFC1918 + IPv6 equivalents) and applied it to gate webhooks;
notification_service now delegates to the same helper.

Verification: mix format/credo/test (1757, 0 failures, +5 SSRF tests),
escript.build, black-box GATE cases pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
yairfalse
yairfalse commented May 25, 2026
View reviewed changes
Comment thread core/lib/sykli/http.ex Outdated
defp private_ip?(_), do: false

defp private_ip6?({0, 0, 0, 0, 0, 0, 0, 1}), do: true
defp private_ip6?({0xFE80, _, _, _, _, _, _, _}), do: true
Copy link
Copy Markdown
Collaborator Author

@yairfalse yairfalse May 25, 2026

Choose a reason for hiding this comment

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

This IPv6 matcher only blocks the exact first hextets fe80, fc00, and fd00, but the blocked ranges are broader: link-local is fe80::/10 (fe80 through febf) and unique-local is fc00::/7 (fc00 through fdff). I reproduced locally that Sykli.HTTP.check_ssrf("http://[fd12:3456::1]/hook"), http://[fc01::1], and http://[fe90::1] all currently return :ok. That leaves an IPv6 private/link-local SSRF bypass. Please compare the first 16-bit segment with masks/ranges, e.g. (h &&& 0xffc0) == 0xfe80 for link-local and (h &&& 0xfe00) == 0xfc00 for ULA, and add tests for non-boundary addresses like fd12::1 and fe90::1.

yairfalse
yairfalse commented May 25, 2026
View reviewed changes
Comment thread core/lib/sykli/http.ex Outdated
host ->
host_charlist = String.to_charlist(host)

case :inet.getaddr(host_charlist, :inet) do
Copy link
Copy Markdown
Collaborator Author

@yairfalse yairfalse May 25, 2026

Choose a reason for hiding this comment

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

For DNS names, checking a single :inet.getaddr/2 result is weaker than the request path: a hostname can resolve to multiple A/AAAA records, and this guard accepts the URL if the one sampled address is public even if another returned address is private/link-local. :httpc resolves the host again when sending, so it may connect to a different record than the one checked. For an SSRF guard this should reject if any resolved address is private, using :inet.getaddrs(host, :inet) and :inet.getaddrs(host, :inet6) and checking the full returned sets (ideally treating mixed public/private answers as blocked).

@yairfalse
Copy link
Copy Markdown
Collaborator Author

yairfalse commented May 25, 2026

Review summary: the shared SSRF helper is the right direction and CI is green, but I found two security gaps that should be fixed before merge:

  • IPv6 range matching is incomplete: fe80::/10 and fc00::/7 are broader than the exact first-hextet checks currently implemented, so addresses like fd12:3456::1, fc01::1, and fe90::1 are allowed.
  • DNS names should be checked across all resolved A/AAAA records, not a single getaddr/2 result, because httpc resolves again and may connect to a different address.

I left inline comments with suggested fixes/tests.

@yairfalse @claude
fix(security): correct IPv6 ranges + multi-record DNS in SSRF guard
6b9d05a 
Addresses review on #216 (both findings were real and pre-existed in the
notification guard this was extracted from):

- IPv6 matcher only blocked exact hextets fe80/fc00/fd00, missing the full
  fe80::/10 (fe80-febf) and fc00::/7 (fc00-fdff) ranges — fd12::1, fc01::1,
  fe90::1 bypassed it. Now mask-compares the first hextet ((h &&& 0xffc0) ==
  0xfe80 for link-local, (h &&& 0xfe00) == 0xfc00 for ULA), and also blocks ::,
  ::1, and IPv4-mapped (::ffff:a.b.c.d) addresses.
- DNS check sampled a single :inet.getaddr result, so a host resolving to both
  public and private records (or re-resolved differently by :httpc on send)
  could bypass. Now resolves via :inet.getaddrs across both families and blocks
  if ANY resolved address is private.

Fixes notification_service too (it delegates). Tests cover the non-boundary
IPv6 and IPv4-mapped cases.

Verification: mix format/credo/test (1759, 0 failures), escript.build.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@yairfalse
Copy link
Copy Markdown
Collaborator Author

yairfalse commented May 25, 2026

Both findings were real (and pre-existed in the notification guard this was extracted from) — fixed in 6b9d05a.

IPv6 ranges (http.ex): the exact-hextet match missed most of fe80::/10 and fc00::/7. Now mask-compares the first hextet — (h &&& 0xffc0) == 0xfe80 (link-local) and (h &&& 0xfe00) == 0xfc00 (ULA) — and also blocks ::, ::1, and IPv4-mapped ::ffff:a.b.c.d (checking the embedded v4). Added tests for the exact bypasses you reproduced (fd12:3456::1, fc01::1, fe90::1, plus febf::1/fdff::1) and the mapped cases; confirmed they now return {:error, ...}.

Multi-record DNS (http.ex): switched from a single :inet.getaddr to :inet.getaddrs across both families, blocking if any resolved address is private — so a mixed public/private answer is rejected rather than accepted on the sampled record.

Residual caveat (not addressed here): this is still check-then-connect, so a DNS-rebinding attacker controlling the authoritative server could return a public record at check time and a private one when :httpc re-resolves on send. Fully closing that needs pinning the validated IP and connecting to it directly — I'd do that as a separate hardening item rather than expand this PR. Flag if you want it folded in.

Verification: mix format/credo/test (1759, 0 failures, +2 IPv6 tests)/escript.build.

@yairfalse yairfalse merged commit e0f12ab into main May 25, 2026
12 checks passed
@yairfalse yairfalse deleted the feature/monster-c-ssrf-sandbox branch May 25, 2026 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@yairfalse