From 4bbcf9cdd7ef0f7c88a7cd6a070f229ce7266518 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 26 Sep 2026 15:15:23 +0200 Subject: [PATCH] fix: check telemetry targets at connect time and fail closed The telemetry sender and the Stalwart version probe checked the target with their own DNS lookup and then fetched with a plain `fetch`, which resolved the name again: a rebinding answer between the two reached an internal host, and a lookup that failed at check time passed the check outright. Both now connect through `fetchPublicUrl`, which validates the address inside the socket's own lookup and fails when the lookup does. The `BULWARK_TELEMETRY_ALLOW_PRIVATE` dev bypass keeps the plain fetch. --- lib/__tests__/telemetry-fetch-target.test.ts | 42 ++++++++++++++++++++ lib/telemetry/endpoint-guard.ts | 27 ++++++++++++- lib/telemetry/payload.ts | 6 +-- lib/telemetry/sender.ts | 9 ++--- 4 files changed, 74 insertions(+), 10 deletions(-) create mode 100644 lib/__tests__/telemetry-fetch-target.test.ts diff --git a/lib/__tests__/telemetry-fetch-target.test.ts b/lib/__tests__/telemetry-fetch-target.test.ts new file mode 100644 index 000000000..49e98b7bb --- /dev/null +++ b/lib/__tests__/telemetry-fetch-target.test.ts @@ -0,0 +1,42 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +// The telemetry sender checked the endpoint with its own DNS lookup, then +// fetched with a plain fetch that resolved again (a rebinding window), and +// the check passed outright when that first lookup failed. + +const guardedFetch = vi.fn(); +vi.mock('@/lib/security/url-guard', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, fetchPublicUrl: (...args: unknown[]) => guardedFetch(...args) }; +}); + +const plainFetch = vi.fn(); + +beforeEach(() => { + guardedFetch.mockReset().mockResolvedValue(new Response('ok')); + plainFetch.mockReset().mockResolvedValue(new Response('ok')); + vi.stubGlobal('fetch', plainFetch); + delete process.env.BULWARK_TELEMETRY_ALLOW_PRIVATE; +}); + +afterEach(() => { + vi.unstubAllGlobals(); + vi.resetModules(); +}); + +describe('fetchTelemetryTarget', () => { + it('connects through the rebinding-safe fetch', async () => { + const { fetchTelemetryTarget } = await import('@/lib/telemetry/endpoint-guard'); + await fetchTelemetryTarget('https://telemetry.example.org/', { method: 'POST', body: '{}' }); + expect(guardedFetch).toHaveBeenCalledWith('https://telemetry.example.org/', { method: 'POST', body: '{}' }); + expect(plainFetch).not.toHaveBeenCalled(); + }); + + it('uses the plain fetch only with the dev bypass, still without redirects', async () => { + process.env.BULWARK_TELEMETRY_ALLOW_PRIVATE = '1'; + const { fetchTelemetryTarget } = await import('@/lib/telemetry/endpoint-guard'); + await fetchTelemetryTarget('http://localhost:4000/', { method: 'GET' }); + expect(plainFetch).toHaveBeenCalledWith('http://localhost:4000/', { method: 'GET', redirect: 'manual' }); + expect(guardedFetch).not.toHaveBeenCalled(); + }); +}); diff --git a/lib/telemetry/endpoint-guard.ts b/lib/telemetry/endpoint-guard.ts index 43d425a83..793a3d4fe 100644 --- a/lib/telemetry/endpoint-guard.ts +++ b/lib/telemetry/endpoint-guard.ts @@ -1,6 +1,7 @@ import { lookup } from 'node:dns/promises'; import { isIP } from 'node:net'; import { isPrivateAddress } from '@/lib/security/ip-ranges'; +import { fetchPublicUrl } from '@/lib/security/url-guard'; export { isPrivateAddress, parseV6Groups } from '@/lib/security/ip-ranges'; @@ -76,7 +77,31 @@ export async function resolveEndpointAllowed(raw: string): Promise; + body?: string; + signal?: AbortSignal; +} + +/** + * Fetch a telemetry target (the collector, the JMAP server's version + * probe). resolveEndpointAllowed runs its own DNS lookup, so on its own it + * is check-then-fetch: the socket resolves again and a rebinding answer, or + * a lookup that failed at check time, lands wherever it points. Unless the + * dev bypass is on, the address is checked inside the socket's own lookup + * instead, and a lookup failure fails the request. Redirects are never + * followed. + */ +export async function fetchTelemetryTarget(url: string, init: TelemetryFetchInit): Promise { + if (bypassEnabled()) { + return fetch(url, { ...init, redirect: 'manual' }); + } + return (await fetchPublicUrl(url, init)) as unknown as Response; +} diff --git a/lib/telemetry/payload.ts b/lib/telemetry/payload.ts index e4d033b1f..fead9b53b 100644 --- a/lib/telemetry/payload.ts +++ b/lib/telemetry/payload.ts @@ -2,7 +2,7 @@ import { readFileSync } from 'node:fs'; import path from 'node:path'; import { configManager } from '@/lib/admin/config-manager'; import { logger } from '@/lib/logger'; -import { resolveEndpointAllowed } from './endpoint-guard'; +import { fetchTelemetryTarget, resolveEndpointAllowed } from './endpoint-guard'; import { getInstanceId } from './state'; import { getLoginCounts } from './login-tracker'; import type { @@ -103,11 +103,9 @@ async function detectStalwartVersion(): Promise { return null; } try { - const res = await fetch(wellKnown, { + const res = await fetchTelemetryTarget(wellKnown, { method: 'GET', signal: AbortSignal.timeout(3000), - // See sender.ts: the guard vetted this URL, not wherever it redirects. - redirect: 'manual', }); const server = res.headers.get('server') ?? ''; const m = server.match(/(\d+\.\d+\.\d+(?:-[\w.]+)?)/); diff --git a/lib/telemetry/sender.ts b/lib/telemetry/sender.ts index 3f008d6f6..1ee4de795 100644 --- a/lib/telemetry/sender.ts +++ b/lib/telemetry/sender.ts @@ -1,7 +1,7 @@ import { logger } from '@/lib/logger'; import { effectiveConsent, endpointEnabled, loadState, saveState } from './state'; import { buildPayload } from './payload'; -import { resolveEndpointAllowed } from './endpoint-guard'; +import { fetchTelemetryTarget, resolveEndpointAllowed } from './endpoint-guard'; import { DEFAULT_ENDPOINT } from './types'; const DAY_MS = 24 * 60 * 60 * 1000; @@ -35,14 +35,13 @@ export async function sendOnce(opts?: { reason?: string }): Promise<{ const payload = await buildPayload(); try { - const res = await fetch(endpoint, { + // Never follows redirects: the endpoint guard only vetted the URL we + // were given, so a 3xx to an internal host would bypass it. + const res = await fetchTelemetryTarget(endpoint, { method: 'POST', headers: { 'content-type': 'application/json' }, body: JSON.stringify(payload), signal: AbortSignal.timeout(5000), - // Never follow redirects: the endpoint guard only vetted the URL we - // were given, so a 3xx to an internal host would bypass it. - redirect: 'manual', }); const ok = res.ok; if (ok) {