From d04d1c789dea6518a5ac0dcd192345129480fcc7 Mon Sep 17 00:00:00 2001 From: jwinterm Date: Wed, 9 Sep 2026 11:00:31 -0400 Subject: [PATCH] fix(nip98): widen the clock window and say what actually failed The register, login and link handlers rejected any proof signed more than 30 seconds from server time, measured against a clock the client owns and we do not. Ordinary desktop drift exceeds that, so a user at a new location failed every attempt with 'Invalid Nostr proof': a cryptographic-sounding message that names nothing the user can act on. The window is now 120 seconds and symmetric, so a fast clock is bounded like a slow one, and the message points at the device clock. Erasure moves with it: left at 30 it would have let a device register and log in while silently unable to delete or export its own data, which is harder to diagnose than a uniform failure. 120 seconds is a mitigation, not a cure. Nobody has measured the reported machine's offset, and whole-hour errors from a wrong timezone or an RTC kept in local time are common. The durable fix is to return the server's time with the challenge so an honest client stamps from it; the client already accepts an injected timestamp. --- src/api/auth.rs | 166 ++++++++++++++++++++++++++++++++++++--- src/api/erasure.rs | 6 +- src/core/crypto/nip98.rs | 10 ++- 3 files changed, 169 insertions(+), 13 deletions(-) diff --git a/src/api/auth.rs b/src/api/auth.rs index a081d90..9b30389 100644 --- a/src/api/auth.rs +++ b/src/api/auth.rs @@ -62,14 +62,33 @@ use crate::AppState; // ── shared constants ──────────────────────────────────────────────────────── -/// Replay window for a NIP-98 LOGIN token (seconds). A captured login token is a -/// bearer credential within this window, so keep it tight — matched to the -/// state-change grade below. Still ample for real client clock skew. -const NIP98_LOGIN_MAX_AGE_SECS: i64 = 30; +/// Replay window for a NIP-98 LOGIN token (seconds), applied symmetrically to a +/// clock that is slow OR fast. A captured login token is a bearer credential +/// within this window and login binds no server nonce, so this window IS the +/// replay bound here; 120s keeps that bound short while absorbing the drift a +/// user cannot see (see the incident note on the action window below). +const NIP98_LOGIN_MAX_AGE_SECS: i64 = 120; /// Replay window for a NIP-98 STATE-CHANGE (signed action) token (seconds). -/// Deliberately tighter than login. -const NIP98_ACTION_MAX_AGE_SECS: i64 = 30; +/// +/// 2026-09-09: a user creating a wallet on a machine whose clock was off by more +/// than 30s failed EVERY registration, identically on the desktop app and the +/// extension (both sign with the one machine clock), and the rejection surfaced +/// as a cryptographic-sounding error that pointed nowhere. Replay protection on +/// this grade comes from the server-issued single-use nonce that +/// `verify_signed_action` binds and the handler atomically consumes, not from +/// the tightness of this window, so widening it to cover ordinary consumer drift +/// (sleep/resume, no working NTP) costs essentially nothing. +const NIP98_ACTION_MAX_AGE_SECS: i64 = 120; + +/// One opaque rejection message for every NIP-98 failure on every grade. It is +/// deliberately identical across login/link/register and across failure modes, +/// so a rejection is never an oracle (it says nothing about whether an account +/// exists, or which check failed). It names the device clock because that is the +/// only cause a user can act on: in the 2026-09-09 incident "Invalid Nostr +/// proof" sent people hunting a corrupt wallet instead of a wrong time. +const NIP98_REJECTED_MSG: &str = + "Nostr proof rejected. Check that your device date and time are correct, then try again."; /// TTL for a Nostr-link nonce (seconds): long enough for the wallet to sign and /// POST, short enough to bound an unused nonce's lifetime. @@ -1442,7 +1461,7 @@ pub async fn nostr_login( Utc::now().timestamp(), NIP98_LOGIN_MAX_AGE_SECS, ) - .map_err(|_| AppError::AuthError("Invalid Nostr auth".into()))?; + .map_err(|_| AppError::AuthError(NIP98_REJECTED_MSG.into()))?; // Resolve to an existing user only. Never create. let user = state @@ -1620,7 +1639,7 @@ pub async fn nostr_link( Utc::now().timestamp(), NIP98_ACTION_MAX_AGE_SECS, ) - .map_err(|_| AppError::AuthError("Invalid Nostr proof".into()))?; + .map_err(|_| AppError::AuthError(NIP98_REJECTED_MSG.into()))?; // 4. Persist. UNIQUE collision -> 409 CONFLICT (handled in set_nostr_pubkey). state.db.set_nostr_pubkey(user_id, &pubkey).await?; @@ -1751,7 +1770,7 @@ pub async fn nostr_register( Utc::now().timestamp(), NIP98_ACTION_MAX_AGE_SECS, ) - .map_err(|_| AppError::AuthError("Invalid Nostr proof".into()))?; + .map_err(|_| AppError::AuthError(NIP98_REJECTED_MSG.into()))?; // 3. Returning npub bypasses the abuse gates, exactly like a returning // pubkey_hash on the BTC path. @@ -2018,4 +2037,133 @@ mod tests { serde_json::from_str(with_rot).expect("rotation envelope deserializes"); assert_eq!(req.rotation_signature.as_deref(), Some("oldsig")); } + + // ── NIP-98 clock skew (2026-09-09 regression) ────────────────────────── + // A client signs its NIP-98 event with the LOCAL machine clock, so a device + // whose clock has drifted fails verification deterministically, on every + // client at once. These assert the PROPERTY, never the constant: drift a + // user would not notice must verify in BOTH directions (slow clock and fast + // clock), and a wildly wrong clock must still be rejected. + mod clock_skew { + use super::super::{NIP98_ACTION_MAX_AGE_SECS, NIP98_LOGIN_MAX_AGE_SECS}; + use crate::core::crypto::nip98::{ + descriptor_sha256, request_descriptor, verify_nip98, verify_signed_action, + }; + use base64::engine::general_purpose::STANDARD; + use base64::Engine; + use k256::schnorr::SigningKey; + use sha2::{Digest, Sha256}; + + /// Drift an ordinary consumer machine reaches without its owner noticing + /// (sleep/resume, no working NTP). Chosen independently of the windows + /// under test so tightening one back down fails here. + const ORDINARY_DRIFT_SECS: i64 = 60; + /// Skew no honest client produces. + const ABSURD_SKEW_SECS: i64 = 6 * 60 * 60; + + const KIND: u32 = 27235; + const URL: &str = "https://api.test/api/v1/auth/nostr/register"; + const NOW: i64 = 1_800_000_000; + + /// Sign a kind-27235 event at `created_at` with a fixed test key. + fn sign(tags: Vec>, created_at: i64) -> String { + let sk = SigningKey::from_bytes(&[9u8; 32]).expect("valid scalar"); + let pk = hex::encode(sk.verifying_key().to_bytes()); + let serial = + serde_json::to_string(&serde_json::json!([0, pk, created_at, KIND, tags, ""])) + .expect("serialize"); + let id = hex::encode(Sha256::digest(serial.as_bytes())); + let sig = sk + .sign_raw(&hex::decode(&id).expect("hex id"), &[0u8; 32]) + .expect("sign"); + let ev = serde_json::json!({ + "id": id, "pubkey": pk, "created_at": created_at, "kind": KIND, + "tags": tags, "content": "", "sig": hex::encode(sig.to_bytes()) + }); + format!( + "Nostr {}", + STANDARD.encode(serde_json::to_vec(&ev).expect("encode")) + ) + } + + #[test] + fn register_proof_survives_ordinary_drift_both_ways() { + let payload = descriptor_sha256(&request_descriptor( + "POST", + "/api/v1/auth/nostr/register", + "", + b"", + )); + let tags = |p: &str| -> Vec> { + vec![ + vec!["u".to_string(), URL.to_string()], + vec!["method".to_string(), "POST".to_string()], + vec!["purpose".to_string(), "nostr_register".to_string()], + vec!["challenge".to_string(), "nonce-1".to_string()], + vec!["payload".to_string(), p.to_string()], + ] + }; + let verify = |signed_at: i64| { + verify_signed_action( + &sign(tags(&payload), signed_at), + URL, + "POST", + "nostr_register", + "nonce-1", + &payload, + None, + None, + NOW, + NIP98_ACTION_MAX_AGE_SECS, + ) + }; + for signed_at in [NOW - ORDINARY_DRIFT_SECS, NOW, NOW + ORDINARY_DRIFT_SECS] { + assert!( + verify(signed_at).is_ok(), + "a proof signed {}s from server time must still register", + signed_at - NOW + ); + } + for signed_at in [NOW - ABSURD_SKEW_SECS, NOW + ABSURD_SKEW_SECS] { + assert!( + verify(signed_at).is_err(), + "a proof signed {}s from server time must be rejected", + signed_at - NOW + ); + } + } + + #[test] + fn login_proof_survives_ordinary_drift_both_ways() { + let tags = || -> Vec> { + vec![ + vec!["u".to_string(), URL.to_string()], + vec!["method".to_string(), "POST".to_string()], + ] + }; + let verify = |signed_at: i64| { + verify_nip98( + &sign(tags(), signed_at), + URL, + "POST", + NOW, + NIP98_LOGIN_MAX_AGE_SECS, + ) + }; + for signed_at in [NOW - ORDINARY_DRIFT_SECS, NOW, NOW + ORDINARY_DRIFT_SECS] { + assert!( + verify(signed_at).is_ok(), + "a login token signed {}s from server time must still sign in", + signed_at - NOW + ); + } + for signed_at in [NOW - ABSURD_SKEW_SECS, NOW + ABSURD_SKEW_SECS] { + assert!( + verify(signed_at).is_err(), + "a login token signed {}s from server time must be rejected", + signed_at - NOW + ); + } + } + } } diff --git a/src/api/erasure.rs b/src/api/erasure.rs index 94fc8a5..d517733 100644 --- a/src/api/erasure.rs +++ b/src/api/erasure.rs @@ -32,7 +32,11 @@ use crate::models::db::{NewAdminAudit, User}; use crate::AppState; const CHALLENGE_TTL_SECS: i64 = 300; -const ACTION_MAX_AGE_SECS: i64 = 30; +const // Matches auth's window. Left at 30 it produced the exact failure this work +// set out to remove: after the auth fix, a device 60s off could register and +// log in but silently could not delete or export its own data, which is +// harder to diagnose than a uniform failure. +ACTION_MAX_AGE_SECS: i64 = 120; const PURPOSE_REQUEST: &str = "erasure_request"; const PURPOSE_CONFIRM: &str = "erasure_confirm"; const PURPOSE_CANCEL: &str = "erasure_cancel"; diff --git a/src/core/crypto/nip98.rs b/src/core/crypto/nip98.rs index 4ce7d61..3d0eee9 100644 --- a/src/core/crypto/nip98.rs +++ b/src/core/crypto/nip98.rs @@ -182,9 +182,13 @@ pub fn descriptor_sha256(descriptor: &str) -> String { /// an optional explicit target id and instance id — each as a tag that must /// appear exactly once, compared constant-time. Returns the verified pubkey. /// -/// Pass a tight `max_age_secs` (e.g. 30). The caller MUST also atomically -/// consume `expected_nonce` from the challenge store; this proves only that the -/// signed event commits to it. +/// `max_age_secs` is a symmetric window (a fast client clock is bounded exactly +/// like a slow one), so it must cover ordinary consumer clock drift: the client +/// stamps `created_at` from its own machine clock, and too tight a window +/// rejects every honest request from a skewed device (2026-09-09 incident). +/// Replay is prevented by the nonce, which the caller MUST also atomically +/// consume from the challenge store; this proves only that the signed event +/// commits to it. #[allow(clippy::too_many_arguments)] pub fn verify_signed_action( header: &str,