From 2b4cb9dd735976e2a1427f8fa18aa4b267467c32 Mon Sep 17 00:00:00 2001 From: 2ro <17595647+2ro@users.noreply.github.com> Date: Fri, 10 Jul 2026 15:11:23 -0400 Subject: [PATCH] fix(nostr): neutralize bidi/zero-width spoofing in displayed untrusted text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Untrusted strings rendered in the payment/UI strip stripped only ASCII control chars, not Unicode bidi overrides/isolates or zero-width format chars. An attacker could embed a right-to-left override or invisible codepoint in a name, memo, or subject to visually reorder or hide the displayed identity/amount (Trojan-Source style spoof). Funds were never at risk — they always go to the correct npub — but the DISPLAY could lie. Add one shared classifier, nostr::sanitize::is_display_dangerous, that flags all Cc controls (C0/C1/DEL via char::is_control) plus the bidi marks/overrides/isolates (U+200E/200F, U+061C, U+202A-202E, U+2066-2069) and zero-width/BOM chars (U+200B-200D, U+FEFF). Route every untrusted display surface through it: - sanitize_note (DM subject/note): dangerous -> space, was control-only. - validate_memo (pay-URI memo): now filters by char after decode so the multibyte bidi codepoints a byte filter missed are dropped. - escape_for_display (authorize/login prompts): delegates to the shared classifier, completing TODO(audit M5). - nip05::name_by_pubkey (authority @name): sanitized + capped at 64 chars before it becomes a rendered contact handle. Only invisible FORMAT/CONTROL codepoints are neutralized; legitimate letters of every script (Latin/accents, Arabic, Hebrew, CJK, Hangul) and emoji pass through unchanged — proven by tests. --- src/nostr/authuri.rs | 17 ++-- src/nostr/mod.rs | 1 + src/nostr/nip05.rs | 15 +++- src/nostr/payuri.rs | 25 +++--- src/nostr/protocol.rs | 15 +++- src/nostr/sanitize.rs | 186 ++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 230 insertions(+), 29 deletions(-) create mode 100644 src/nostr/sanitize.rs diff --git a/src/nostr/authuri.rs b/src/nostr/authuri.rs index 8630805f..167db945 100644 --- a/src/nostr/authuri.rs +++ b/src/nostr/authuri.rs @@ -531,20 +531,13 @@ pub fn content_preview(content: &str) -> (String, usize) { /// passes through unchanged. Used on ALL requester-controlled strings (content, /// tag values, titles) before they reach a label. pub fn escape_for_display(s: &str) -> String { - // TODO(audit M5): widen escaping to category-based (all C0/C1, bidi controls) - // rather than the current explicit list. + // Category-based, shared with every other untrusted-string surface (completes + // the former TODO(audit M5)): all Cc controls (C0/C1/DEL), the bidi + // overrides/isolates/marks, and the zero-width/BOM format chars. let mut out = String::with_capacity(s.len()); for c in s.chars() { - let code = c as u32; - let dangerous = code < 0x20 - || code == 0x7f - || c == '\u{200E}' // LEFT-TO-RIGHT MARK - || c == '\u{200F}' // RIGHT-TO-LEFT MARK - || c == '\u{061C}' // ARABIC LETTER MARK - || (0x202A..=0x202E).contains(&code) // LRE, RLE, PDF, LRO, RLO - || (0x2066..=0x2069).contains(&code); // LRI, RLI, FSI, PDI - if dangerous { - out.push_str(&format!("\\u{{{code:04X}}}")); + if crate::nostr::sanitize::is_display_dangerous(c) { + out.push_str(&format!("\\u{{{:04X}}}", c as u32)); } else { out.push(c); } diff --git a/src/nostr/mod.rs b/src/nostr/mod.rs index eff18187..70462b8c 100644 --- a/src/nostr/mod.rs +++ b/src/nostr/mod.rs @@ -49,6 +49,7 @@ pub use client::{HeldIdentityKeys, NostrProfile, NostrService, TransportStatus, pub mod avatar; pub mod nip05; +pub mod sanitize; pub mod authuri; pub mod loginuri; diff --git a/src/nostr/nip05.rs b/src/nostr/nip05.rs index 6a2f14fb..5cf0589f 100644 --- a/src/nostr/nip05.rs +++ b/src/nostr/nip05.rs @@ -25,6 +25,11 @@ use crate::nostr::relays::HOME_NIP05_DOMAIN; use crate::tor; use parking_lot::RwLock; +/// Hard cap on a name-authority `@name` before it is shown as a contact handle. +/// A display name has no business being hundreds of characters; this bounds a +/// hostile/compromised authority that returns a wall of text. +const MAX_NAME_CHARS: usize = 64; + /// The active name-authority "home" domain, mirrored here from the wallet config /// once per frame so resolution + display (some on worker threads) can read it /// without threading the config through every call site. `None` = the default @@ -122,10 +127,12 @@ pub async fn name_by_pubkey(domain: &str, pubkey_hex: &str) -> Option { ); let body = tor::http_request("GET", url, None, vec![]).await?; let doc: Value = serde_json::from_str(&body).ok()?; - doc.get("name") - .and_then(|v| v.as_str()) - .filter(|s| !s.is_empty()) - .map(|s| s.to_string()) + let raw = doc.get("name").and_then(|v| v.as_str())?; + // The authority is untrusted display data: strip any control/bidi/zero-width + // codepoints and cap the length before this name becomes a contact handle + // rendered as the payer/peer identity. A hostile or compromised authority + // must not be able to spoof or reorder the displayed name. + crate::nostr::sanitize::sanitize_name(raw, MAX_NAME_CHARS) } /// Verify that a pubkey matches its claimed NIP-05 identifier. diff --git a/src/nostr/payuri.rs b/src/nostr/payuri.rs index 1b441677..59cb8c6c 100644 --- a/src/nostr/payuri.rs +++ b/src/nostr/payuri.rs @@ -242,19 +242,24 @@ fn validate_amount(raw: &str) -> Option { } } -/// Validate a `memo` value: percent-decode, strip ASCII control chars and -/// newlines (untrusted free text — display / tx-message only, never a path or -/// route), then hard-cap at [`MAX_MEMO_BYTES`] on a UTF-8 boundary. Empty → -/// `None`. +/// Validate a `memo` value: percent-decode, then drop every display-dangerous +/// codepoint — ASCII control chars/newlines AND the bidi-override / isolate / +/// zero-width format chars (see +/// [`crate::nostr::sanitize::is_display_dangerous`]) — because a memo is +/// untrusted free text rendered in the payment strip (display / tx-message +/// only, never a path or route). Then hard-cap at [`MAX_MEMO_BYTES`] on a UTF-8 +/// boundary. Empty → `None`. Filtering by CHAR after decoding (not by byte) is +/// what catches the multibyte bidi codepoints a byte filter would miss. fn validate_memo(raw: &str) -> Option { let decoded = percent_decode(raw); - // Drop ASCII control bytes (< 0x20, covering NUL / newline / tab) and DEL. - let cleaned: Vec = decoded - .into_iter() - .filter(|&b| b >= 0x20 && b != 0x7f) + let text = String::from_utf8_lossy(&decoded); + // Drop control chars (NUL / newline / tab / DEL) and bidi/zero-width format + // chars — leaving legitimate letters of every script intact. + let cleaned: String = text + .chars() + .filter(|c| !crate::nostr::sanitize::is_display_dangerous(*c)) .collect(); - let text = String::from_utf8_lossy(&cleaned).into_owned(); - let text = truncate_on_char_boundary(text, MAX_MEMO_BYTES); + let text = truncate_on_char_boundary(cleaned, MAX_MEMO_BYTES); let text = text.trim().to_string(); if text.is_empty() { None } else { Some(text) } } diff --git a/src/nostr/protocol.rs b/src/nostr/protocol.rs index e9525b65..53e0bca5 100644 --- a/src/nostr/protocol.rs +++ b/src/nostr/protocol.rs @@ -50,12 +50,21 @@ static SLATEPACK_RE: LazyLock = LazyLock::new(|| { Regex::new(r"BEGINSLATEPACK\.[\s\S]*?ENDSLATEPACK\.").expect("slatepack regex") }); -/// Sanitize a user note: strip control characters, collapse whitespace, -/// trim and cap the length. Returns `None` when nothing readable remains. +/// Sanitize a user note: neutralize control AND bidi/zero-width format +/// characters (see [`crate::nostr::sanitize::is_display_dangerous`]) to spaces, +/// collapse whitespace, trim and cap the length. Returns `None` when nothing +/// readable remains. Mapping to a space (rather than dropping) preserves word +/// boundaries and keeps a hidden joiner from silently fusing two tokens. pub fn sanitize_note(raw: &str) -> Option { let cleaned: String = raw .chars() - .map(|c| if c.is_control() { ' ' } else { c }) + .map(|c| { + if crate::nostr::sanitize::is_display_dangerous(c) { + ' ' + } else { + c + } + }) .collect(); let collapsed = cleaned.split_whitespace().collect::>().join(" "); let trimmed = collapsed.trim(); diff --git a/src/nostr/sanitize.rs b/src/nostr/sanitize.rs new file mode 100644 index 00000000..5d7ed6e6 --- /dev/null +++ b/src/nostr/sanitize.rs @@ -0,0 +1,186 @@ +// Copyright 2026 The Goblin Developers +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Display sanitization shared by every untrusted-string surface. +//! +//! A single classifier, [`is_display_dangerous`], decides whether a codepoint +//! is unsafe to render raw in the payment/UI strip. It is the one source of +//! truth behind the DM-note sanitizer, the memo/URI sanitizer, the +//! name-authority `@name` cap, and the authorize/login prompt escaper. This +//! completes `TODO(audit M5)`: the set is Unicode-category driven (Cc controls +//! via [`char::is_control`], plus the explicit bidi / zero-width format +//! codepoints) rather than an ad-hoc per-caller list. +//! +//! Threat: a right-to-left override, isolate, or invisible/zero-width codepoint +//! embedded in an attacker-controlled name / memo / subject can visually +//! reorder or hide the surrounding UI text so the DISPLAY lies about identity +//! or amount (a "Trojan Source" style spoof). Funds are unaffected — they +//! always go to the correct npub — but the rendered strip must not be +//! forgeable. +//! +//! Non-goal: mangling legitimate non-Latin text. Only the invisible +//! FORMAT/CONTROL codepoints are neutralized; the visible LETTERS of every +//! script — Latin (incl. accents), Arabic, Hebrew, CJK, Hangul — and +//! standalone emoji pass through untouched. + +/// Whether `c` must never reach a label raw. +/// +/// Neutralized set: +/// - **Cc controls** — C0 `U+0000..=U+001F`, DEL `U+007F`, and C1 +/// `U+0080..=U+009F`. Matched by [`char::is_control`], which is exactly the +/// `Cc` general category. +/// - **Bidi controls / overrides / isolates** (the classic Trojan-Source +/// vector): `U+202A..=U+202E` (LRE, RLE, PDF, LRO, RLO), +/// `U+2066..=U+2069` (LRI, RLI, FSI, PDI), `U+200E`/`U+200F` (LRM/RLM), +/// and `U+061C` (ARABIC LETTER MARK). +/// - **Zero-width / invisible joiners and the BOM**: `U+200B` (ZWSP), +/// `U+200C` (ZWNJ), `U+200D` (ZWJ), `U+FEFF` (ZWNBSP / BOM). +/// +/// Deliberately NOT flagged: ordinary letters of any script and standalone +/// emoji. Arabic and Hebrew LETTERS render right-to-left on their own and are +/// perfectly safe — it is only the explicit override/isolate FORMAT codepoints +/// that are dangerous. +pub fn is_display_dangerous(c: char) -> bool { + if c.is_control() { + // Cc: C0, DEL, and the C1 block. + return true; + } + matches!( + c as u32, + 0x200E | 0x200F // LRM, RLM + | 0x061C // ARABIC LETTER MARK + | 0x202A..=0x202E // LRE, RLE, PDF, LRO, RLO + | 0x2066..=0x2069 // LRI, RLI, FSI, PDI + | 0x200B..=0x200D // ZWSP, ZWNJ, ZWJ + | 0xFEFF // ZWNBSP / BOM + ) +} + +/// Sanitize an attacker-controlled display name (e.g. the `@name` an authority +/// reports for a pubkey): drop every [`is_display_dangerous`] codepoint, trim +/// surrounding whitespace, and hard-cap at `max_chars` characters. Returns +/// `None` when nothing legible remains — a name that was empty, or was made +/// only of control/bidi/zero-width chars, is not a usable handle. +/// +/// A display name has no legitimate need for hundreds of characters, so the cap +/// also bounds a hostile authority that returns a wall of text. +pub fn sanitize_name(raw: &str, max_chars: usize) -> Option { + let cleaned: String = raw.chars().filter(|c| !is_display_dangerous(*c)).collect(); + let trimmed = cleaned.trim(); + if trimmed.is_empty() { + return None; + } + Some(trimmed.chars().take(max_chars).collect()) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The full set of format/control codepoints the classifier must flag. + const DANGEROUS: &[char] = &[ + '\u{0000}', '\u{0007}', '\u{001B}', '\u{007F}', // C0 + DEL + '\u{0085}', '\u{009F}', // C1 + '\u{200E}', '\u{200F}', '\u{061C}', // marks + '\u{202A}', '\u{202B}', '\u{202C}', '\u{202D}', '\u{202E}', // bidi overrides + '\u{2066}', '\u{2067}', '\u{2068}', '\u{2069}', // bidi isolates + '\u{200B}', '\u{200C}', '\u{200D}', // zero-width + '\u{FEFF}', // BOM + ]; + + /// Legitimate, visible text from the locales Goblin ships — must ALL pass. + const SAFE: &[&str] = &[ + "café", // accented Latin + "Grüße", // German umlaut + ß + "日本語", // Japanese / CJK + "中文名字", // Chinese + "한국어", // Korean Hangul + "مرحبا", // Arabic letters (naturally RTL — safe) + "שלום", // Hebrew letters (naturally RTL — safe) + "Ñoño", // Spanish + "🎉", // emoji + "🍺 grin", // emoji + ASCII + ]; + + #[test] + fn flags_every_dangerous_codepoint() { + for &c in DANGEROUS { + assert!( + is_display_dangerous(c), + "{:?} (U+{:04X}) must be flagged", + c, + c as u32 + ); + } + } + + #[test] + fn passes_every_legitimate_letter() { + for s in SAFE { + for c in s.chars() { + assert!( + !is_display_dangerous(c), + "{:?} (U+{:04X}) in {:?} must NOT be flagged", + c, + c as u32, + s + ); + } + } + } + + #[test] + fn sanitize_name_neutralizes_bidi_and_zero_width() { + // A right-to-left override that would visually reverse "1cnp" → "npc1". + assert_eq!( + sanitize_name("Alice\u{202E}eve", 64), + Some("Aliceeve".to_string()) + ); + // Zero-width joiner splicing two look-alike halves. + assert_eq!( + sanitize_name("go\u{200D}blin", 64), + Some("goblin".to_string()) + ); + // BOM + control noise. + assert_eq!( + sanitize_name("\u{FEFF}bob\u{0000}", 64), + Some("bob".to_string()) + ); + } + + #[test] + fn sanitize_name_preserves_non_latin_scripts() { + for s in SAFE { + assert_eq!( + sanitize_name(s, 64).as_deref(), + Some(s.trim()), + "{s:?} must survive sanitization unchanged" + ); + } + } + + #[test] + fn sanitize_name_caps_length() { + let long = "本".repeat(200); // 200 CJK chars, none dangerous + let out = sanitize_name(&long, 64).expect("non-empty"); + assert_eq!(out.chars().count(), 64); + } + + #[test] + fn sanitize_name_rejects_all_noise() { + assert_eq!(sanitize_name("\u{202E}\u{200B}\u{0000}", 64), None); + assert_eq!(sanitize_name(" ", 64), None); + assert_eq!(sanitize_name("", 64), None); + } +}