From f7db29e793d4ce40a7b7ceabab12dc1ca3d609d9 Mon Sep 17 00:00:00 2001 From: Avi Date: Fri, 21 Aug 2026 16:06:18 -0500 Subject: [PATCH] Add SSRF guard, signer queue cap, secret echo, request timeout --- frontend/electron/main.ts | 84 +++++++++++++++++++++++++- src/signer.rs | 123 +++++++++++++++++++++++++++++++++++++- 2 files changed, 204 insertions(+), 3 deletions(-) diff --git a/frontend/electron/main.ts b/frontend/electron/main.ts index 55e18ea..6f3de62 100644 --- a/frontend/electron/main.ts +++ b/frontend/electron/main.ts @@ -1,8 +1,10 @@ import { app, BrowserWindow, clipboard, dialog, ipcMain, protocol, shell } from 'electron'; +import { lookup } from 'node:dns/promises'; import { spawn, type ChildProcess } from 'node:child_process'; import { randomBytes } from 'node:crypto'; import { readFileSync } from 'node:fs'; import { createInterface } from 'node:readline'; +import * as net from 'node:net'; import * as path from 'node:path'; const MIME: Record = { @@ -135,6 +137,13 @@ function startBackend(): void { }); } +/** + * Upper bound for one backend round-trip. Generous on purpose: a publish can + * wait for each relay in turn (10s connect + 15s send each). A hung backend + * still gets reaped instead of leaking promises forever. + */ +const BACKEND_TIMEOUT_MS = 120_000; + async function backendRequest(method: string, params: Record): Promise { startBackend(); const stdin = backend?.stdin; @@ -143,11 +152,19 @@ async function backendRequest(method: string, params: Record): } const id = nextId++; const payload = { id, method, ...params }; + let timer!: ReturnType; const response = new Promise((resolve, reject) => { - pending.set(id, { resolve, reject }); + pending.set(id, { + resolve, + reject, + }); + timer = setTimeout(() => { + pending.delete(id); + reject(new Error('The background service did not respond in time.')); + }, BACKEND_TIMEOUT_MS); }); stdin.write(`${JSON.stringify(payload)}\n`); - return response; + return response.finally(() => clearTimeout(timer)); } /** @@ -219,6 +236,66 @@ const LINK_PREVIEW_TIMEOUT_MS = 10_000; /** Cap on how much HTML we parse for meta tags. */ const LINK_PREVIEW_MAX_BYTES = 1_000_000; +/** Whether an IPv4 address is loopback, private, or otherwise non-routable. */ +function ipv4IsPrivate(ip: string): boolean { + const parts = ip.split('.').map(Number); + if (parts.length !== 4 || parts.some((n) => Number.isNaN(n) || n < 0 || n > 255)) { + return true; // Malformed: treat as unsafe. + } + const [a, b] = parts; + return ( + a === 0 || + a === 10 || + a === 127 || + (a === 100 && b >= 64 && b <= 127) || + (a === 169 && b === 254) || + (a === 172 && b >= 16 && b <= 31) || + (a === 192 && b === 168) + ); +} + +/** Whether an IPv6 address is loopback, link-local, unique-local, or v4-mapped private. */ +function ipv6IsPrivate(ip: string): boolean { + const addr = ip.toLowerCase(); + if (addr === '::' || addr === '::1') { + return true; + } + const mapped = addr.startsWith('::ffff:') ? addr.slice(7) : null; + if (mapped) { + return net.isIPv4(mapped) ? ipv4IsPrivate(mapped) : true; + } + // fc00::/7 (unique local) and fe80::/10 (link local). + return /^f[cd]/.test(addr) || /^fe[89ab]/.test(addr); +} + +/** Whether `ip` points at the local machine or a private network. */ +function isPrivateAddress(ip: string): boolean { + return net.isIPv4(ip) ? ipv4IsPrivate(ip) : ipv6IsPrivate(ip); +} + +/** + * Resolve `url`'s host and refuse loopback/private targets, so a crafted note + * link cannot make the app probe the user's localhost or LAN ("SSRF"). Hosts + * are checked at their resolved addresses, not just by name. + */ +async function resolvesToPrivateAddress(url: URL): Promise { + const host = url.hostname.replace(/^\[|\]$/g, '').toLowerCase(); + if (host === 'localhost' || host.endsWith('.localhost') || host.endsWith('.local')) { + return true; + } + let addresses: string[]; + if (net.isIP(host)) { + addresses = [host]; + } else { + try { + addresses = (await lookup(host, { all: true, verbatim: true })).map((a) => a.address); + } catch { + return true; // Unresolvable: nothing useful to preview anyway. + } + } + return addresses.some(isPrivateAddress); +} + /** A best-effort MIME type derived from the file name. */ function mimeForPath(filePath: string): string { switch (path.extname(filePath).toLowerCase()) { @@ -344,6 +421,9 @@ async function fetchLinkPreview(rawUrl: string): Promise { if (url.protocol !== 'https:' && url.protocol !== 'http:') { return null; } + if (await resolvesToPrivateAddress(url)) { + return null; + } const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), LINK_PREVIEW_TIMEOUT_MS); diff --git a/src/signer.rs b/src/signer.rs index 0873027..086f0b3 100644 --- a/src/signer.rs +++ b/src/signer.rs @@ -36,6 +36,12 @@ const CONNECT_TIMEOUT: Duration = Duration::from_secs(10); /// How long a request may wait for the user to approve it before it expires. const APPROVAL_TIMEOUT: Duration = Duration::from_secs(300); +/// Maximum number of requests kept waiting for approval at once. A malicious +/// relay flooding `sign_event` requests cannot grow the queue unboundedly or +/// bury one genuine prompt under thousands of fakes; further requests are +/// dropped until the user clears the queue (or entries time out). +const MAX_PENDING_APPROVALS: usize = 20; + /// Lifecycle of the remote signer, for display in the GUI. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] #[serde(rename_all = "snake_case")] @@ -188,6 +194,11 @@ impl Signer { let (sender, receiver) = oneshot::channel(); { let mut inner = self.inner.lock().expect("signer mutex poisoned"); + if inner.pending.len() >= MAX_PENDING_APPROVALS { + // Queue is full: drop the request instead of parking it. The + // caller answers with the standard "no decision" error. + return None; + } inner.pending.insert( id.clone(), PendingApprovalInner { @@ -404,7 +415,21 @@ fn handle_request( signer.set_connected(); match request.method.as_str() { - "connect" => Some(response_ok(&request.id, "ack".to_string())), + "connect" => { + // NIP-46: when the nostrconnect:// link carried a secret, the + // client must echo it back in its connect request, proving the + // link was delivered unmodified. A mismatched echo is refused. + if let Some(expected) = &uri.secret { + if !request.params.iter().any(|param| param == expected) { + return Some(response_err( + &request.id, + "The connect acknowledgement did not include the expected secret." + .to_string(), + )); + } + } + Some(response_ok(&request.id, "ack".to_string())) + } "get_public_key" => Some(response_ok(&request.id, keys.public_key().to_hex())), "ping" => Some(response_ok(&request.id, "pong".to_string())), "sign_event" => approved_response(keys, request), @@ -829,6 +854,62 @@ mod tests { assert!(!requires_approval("logout")); } + #[test] + fn connect_without_secret_is_acked() { + let signer = Signer::new(); + let keys = Keys::generate(); + let uri = ConnectUri { + peer: Keys::generate().public_key(), + relays: vec![RelayUrl::parse("wss://relay.example.com").unwrap()], + secret: None, + }; + let request = RawRequest { + id: "1".into(), + method: "connect".into(), + params: vec![keys.public_key().to_hex()], + }; + let response = handle_request(&signer, &keys, &uri, &request).unwrap(); + assert!(response.contains("ack")); + } + + #[test] + fn connect_must_echo_the_link_secret() { + let signer = Signer::new(); + let keys = Keys::generate(); + let uri = ConnectUri { + peer: Keys::generate().public_key(), + relays: vec![RelayUrl::parse("wss://relay.example.com").unwrap()], + secret: Some("shared-quiet-secret".to_string()), + }; + + // Missing echo: refused. + let request = RawRequest { + id: "1".into(), + method: "connect".into(), + params: vec![], + }; + let response = handle_request(&signer, &keys, &uri, &request).unwrap(); + assert!(response.contains("expected secret")); + + // Wrong echo: refused. + let request = RawRequest { + id: "2".into(), + method: "connect".into(), + params: vec!["something-else".to_string()], + }; + let response = handle_request(&signer, &keys, &uri, &request).unwrap(); + assert!(response.contains("expected secret")); + + // Correct echo: acknowledged. + let request = RawRequest { + id: "3".into(), + method: "connect".into(), + params: vec!["shared-quiet-secret".to_string()], + }; + let response = handle_request(&signer, &keys, &uri, &request).unwrap(); + assert!(response.contains("ack")); + } + #[tokio::test] async fn sign_event_waits_for_approval_then_signs() { let signer = Signer::new(); @@ -892,6 +973,46 @@ mod tests { assert!(signer.approve("no-such-id", true).is_err()); } + #[tokio::test] + async fn pending_approvals_are_capped() { + let signer = Signer::new(); + let keys = Keys::generate(); + let mk_request = |id: usize| RawRequest { + id: id.to_string(), + method: "sign_event".into(), + params: vec![r#"{"kind":1,"created_at":1714078911,"tags":[],"content":"x"}"#.into()], + }; + + // Flood with more gated requests than the cap allows. + let mut tasks = Vec::new(); + for i in 0..MAX_PENDING_APPROVALS + 5 { + let s1 = signer.clone(); + let keys_for_task = keys.clone(); + let request = mk_request(i); + tasks.push(tokio::spawn(async move { + gated_response(&s1, &keys_for_task, &request).await + })); + } + tokio::time::sleep(Duration::from_millis(50)).await; + + assert_eq!( + signer.pending_count(), + MAX_PENDING_APPROVALS, + "queue must never exceed the cap" + ); + + // Approve one; the queue drains by exactly one (overflow requests were + // already dropped, not parked). + let pending_id = signer.status().pending[0].id.clone(); + signer.approve(&pending_id, false).unwrap(); + tokio::time::sleep(Duration::from_millis(20)).await; + assert_eq!(signer.pending_count(), MAX_PENDING_APPROVALS - 1); + + for task in tasks { + task.abort(); + } + } + #[test] fn sign_event_summary_previews_the_event() { let request = RawRequest {