Skip to content

fix(orb): revalidate relay DNS before forwarding#1395

Open
JSONbored wants to merge 3 commits into
mainfrom
codex/fix-dns-based-blind-ssrf-vulnerability
Open

fix(orb): revalidate relay DNS before forwarding#1395
JSONbored wants to merge 3 commits into
mainfrom
codex/fix-dns-based-blind-ssrf-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The relay registration guard only checked URL syntax and the textual hostname, which permits DNS-based rebinding to private/loopback addresses at fetch time and creates a blind SSRF primitive when forwardOrbEvent posts stored relay URLs.
  • Forwarding must therefore revalidate the resolved addresses at send time to prevent attacker-controlled hostnames from resolving to internal addresses.
  • The change preserves the existing best-effort/fail-safe behavior so a forwarding failure never breaks the Orb webhook 202 path.

Description

  • Add forward-time DNS resolution for relay targets using a pluggable resolver and DNS-over-HTTPS lookups to obtain A/AAAA answers (resolveRelayHostname, relayDestinationIsSafe) in src/orb/relay.ts.
  • Validate each resolved address with the existing isSafeHttpUrl logic (via resolvedAddressIsSafe) and reject destinations with no answers or any private/loopback/link-local addresses before attempting the POST.
  • Keep forwardOrbEvent fail-safe by returning "failed" when a destination is unsafe or unresolvable instead of throwing, and keep the HMAC-signing/POST flow unchanged for safe destinations.
  • Add regression tests in test/integration/orb-relay.test.ts to cover DNS revalidation, rejection of loopback/private-address resolutions, no-DNS-answer behavior, and a positive DNS-checked path; make the resolver pluggable for testing.

Testing

  • Ran npx vitest run test/integration/orb-relay.test.ts and the integration suite passed (21 tests passed).
  • Ran npm run typecheck and TypeScript checked clean with no errors.
  • Attempted coverage with npx vitest run test/integration/orb-relay.test.ts --coverage.enabled true --coverage.reporter text, but V8 coverage remapping failed after the tests passed with TypeError: jsTokens is not a function in ast-v8-to-istanbul; the functional tests themselves passed despite the coverage tooling error.

Codex Task

@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jun 25, 2026
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.75000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.34%. Comparing base (8844796) to head (e24bc91).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/orb/relay.ts 68.75% 0 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1395      +/-   ##
==========================================
- Coverage   95.36%   95.34%   -0.02%     
==========================================
  Files         197      197              
  Lines       21464    21480      +16     
  Branches     7759     7768       +9     
==========================================
+ Hits        20470    20481      +11     
  Misses        416      416              
- Partials      578      583       +5     
Files with missing lines Coverage Δ
src/orb/relay.ts 92.30% <68.75%> (-7.70%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 25, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui 92452c4 Commit Preview URL

Branch Preview URL
Jun 25 2026, 11:03 PM

Comment thread src/orb/relay.ts
.first<{ relay_url: string; relay_secret_enc: string; relay_secret_iv: string; relay_secret_salt: string | null }>();
if (!row || !env.TOKEN_ENCRYPTION_SECRET) return "skipped";
try {
if (!(await relayDestinationIsSafe(row.relay_url, resolveHostname))) return "failed";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1: DNS revalidation uses DoH but fetch still resolves hostname, leaving TOCTOU rebinding risk

DoH IP check does not prevent rebinding because fetchImpl re-resolves the hostname at connection time.

Connect to the validated resolved IP directly, preserving Host header for TLS, instead of passing the hostname to fetch.

AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.

<file name="src/orb/relay.ts">
<violation number="1" location="src/orb/relay.ts:184">
<priority>P1</priority>
<title>DNS revalidation uses DoH but fetch still resolves hostname, leaving TOCTOU rebinding risk</title>
<evidence>The relayDestinationIsSafe call resolves the hostname via Cloudflare DoH and checks returned IPs, but the subsequent fetchImpl(row.relay_url, ...) still passes the original hostname string, causing the platform's DNS resolver to re-resolve it at connection time. An attacker controlling a relay hostname can serve a safe public IP to Cloudflare's DoH endpoint while returning a private/loopback IP to the Cloudflare Worker runtime resolver, bypassing the safety check.</evidence>
<recommendation>After DNS validation, connect to the resolved IP directly (e.g., reconstruct the request URL using the validated IP and original port/path, and set the Host header to the original hostname for TLS SNI/certificate verification), or use a fetch implementation that enforces the pre-resolved IP.</recommendation>
</violation>
</file>

@superagent-security superagent-security Bot added the pr:flagged PR flagged for review by security analysis. label Jun 25, 2026
@JSONbored JSONbored self-assigned this Jun 26, 2026
@JSONbored JSONbored added the gittensor:bug Gittensor-scored bug fix - worth 0.5x multiplier. label Jun 26, 2026
@JSONbored JSONbored force-pushed the codex/fix-dns-based-blind-ssrf-vulnerability branch from e8d1523 to 33e2528 Compare June 26, 2026 20:53
Comment thread src/orb/relay.ts
const answers: string[] = [];
for (const type of ["A", "AAAA"]) {
const res = await fetch(`https://cloudflare-dns.com/dns-query?name=${encodeURIComponent(hostname)}&type=${type}`, { headers: { accept: "application/dns-json" }, signal: AbortSignal.timeout(3_000) });
if (!res.ok) throw new Error("dns_resolution_failed");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Relay hostname validation leaks internal hostnames to external DNS resolver

Every relay target hostname is sent to Cloudflare's public DoH resolver, leaking internal infrastructure names to a third party.

Use a configurable or local DNS resolver instead of hardcoding cloudflare-dns.com.

AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.

<file name="src/orb/relay.ts">
<violation number="1" location="src/orb/relay.ts:26">
<priority>P2</priority>
<title>Relay hostname validation leaks internal hostnames to external DNS resolver</title>
<evidence>The resolveRelayHostname function sends every relay target hostname to https://cloudflare-dns.com/dns-query via DNS-over-HTTPS. This exposes potentially private or internal infrastructure hostnames to a third-party service (Cloudflare), creating an information-disclosure risk for enterprise webhook endpoints.</evidence>
<recommendation>Replace the hardcoded public DoH endpoint with a configurable or local DNS resolver so internal hostnames are not leaked to external services, or add an allow-list and documentation noting the Cloudflare query behavior.</recommendation>
</violation>
</file>

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

Labels

aardvark codex gittensor:bug Gittensor-scored bug fix - worth 0.5x multiplier. pr:flagged PR flagged for review by security analysis. size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant