NVDA hang fix: background the 600k PBKDF2 + Core session key cache
The 5.6 password-strength raise made key derivation 6x slower (~1s on old hardware, run twice for key+fingerprint), and RecomputeAudioCrypto ran it synchronously on the UI thread on profile load / first connect - a full NVDA freeze for a screen-reader user. Fixed two ways: 1. Core session cache (RemSoundCrypto): ForPlainPassword now caches (key,fingerprint) per plaintext password for the process, with Prewarm (derive off-thread without returning) and IsCached (is it a hit?). The slow PBKDF2 runs ONCE per distinct password per process, so a profile switch back to a used password is instant. This also fixes the efficiency-review finding that the SERVICE re-derived un-cached on every audio-device hot-plug - it now hits the same cache. 2. App off-thread derive (MainForm.RecomputeAudioCrypto): the fast cases (password unchanged / already cached / empty / weak - no PBKDF2) still apply synchronously; the ONLY slow case (a strong password's first use this session, with the window live) derives on a worker and applies on the UI thread when done. The key is held null until it lands - audio simply waits (mandatory encryption never streams keyless) rather than the UI freezing - and a generation counter discards a stale result if the password changed again meanwhile. Falls back to synchronous before the window handle exists (startup construction). Also: the weak-password explanation is factored into ExplainWeakPasswordIfNeeded and its one-shot flag now re-arms on a real password change, so a SECOND weak-password profile switched to in one session is still explained (review nit #6). Tests: PasswordRules now pins the cache (miss->hit, same-instance on repeat, Prewarm warms, empty/weak = no work). Gate 69/69. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -8276,27 +8276,85 @@ public sealed partial class MainForm : Form
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>Derive the audio key + fingerprint from the current profile password (cached so
|
/// <summary>Derive the audio key + fingerprint from the current profile password and push them
|
||||||
/// the slow derivation only runs when the password actually changes) and push them down to
|
/// to the sender and receiver. No/weak password → null key → no audio flows (encryption is
|
||||||
/// the sender and receiver. No password → null key → no audio flows (encryption is
|
|
||||||
/// mandatory). Called on password change, when audio is (re)configured, and on the streaming
|
/// mandatory). Called on password change, when audio is (re)configured, and on the streaming
|
||||||
/// gate. 2026-05-31.</summary>
|
/// gate. 2026-05-31.
|
||||||
|
///
|
||||||
|
/// The 5.6 PBKDF2 raise (600k, run twice) costs up to ~1 s on old hardware, so the FIRST use of
|
||||||
|
/// a strong password this session is derived OFF the UI thread — otherwise a blind user gets a
|
||||||
|
/// full NVDA freeze on profile load / first connect (2026-07-27 review). The key is held null
|
||||||
|
/// until it lands (audio simply waits — mandatory encryption means it never streams keyless), a
|
||||||
|
/// generation guard drops a stale result if the password changed again meanwhile, and the fast
|
||||||
|
/// cases (unchanged / cached / empty / weak — no PBKDF2) still apply synchronously so nothing
|
||||||
|
/// races on the common path.</summary>
|
||||||
private void RecomputeAudioCrypto()
|
private void RecomputeAudioCrypto()
|
||||||
{
|
{
|
||||||
if (currentProfilePassword != lastDerivedPassword)
|
var pw = currentProfilePassword;
|
||||||
|
if (pw == lastDerivedPassword)
|
||||||
{
|
{
|
||||||
// Key + fingerprint always together, through the one shared rule (same as the service).
|
PushAudioCrypto(); // no change — re-push the cached key/fp (or null)
|
||||||
(currentAudioKey, currentAudioFingerprint) = RemSoundCrypto.ForPlainPassword(currentProfilePassword);
|
return;
|
||||||
lastDerivedPassword = currentProfilePassword;
|
}
|
||||||
// Since 5.6 that rule also refuses a WEAK password (key comes back null), so a profile
|
lastDerivedPassword = pw;
|
||||||
// that auto-connects at startup with an old guessable password must not just sit
|
var gen = ++cryptoGeneration;
|
||||||
// silently dead — say why, once, and point at the fix. The interactive tick path has
|
// The password genuinely changed — re-arm the one-shot weak-password explanation so a SECOND
|
||||||
// its own guided flow (EnsureStreamingPassword); this catches every other route in.
|
// weak-password profile switched to in the same session is still explained (not just the first).
|
||||||
if (!string.IsNullOrEmpty(currentProfilePassword) && currentAudioKey is null && !weakPasswordExplained)
|
weakPasswordExplained = false;
|
||||||
|
|
||||||
|
// Fast path: null/empty/weak (resolves to (null,null) with no PBKDF2), or an already-cached
|
||||||
|
// strong password. Apply synchronously — no freeze possible. Also covers the pre-window
|
||||||
|
// case where the form has no handle yet (BeginInvoke would throw), so startup stays correct.
|
||||||
|
if (RemSoundCrypto.IsCached(pw) || !IsHandleCreated)
|
||||||
{
|
{
|
||||||
|
(currentAudioKey, currentAudioFingerprint) = RemSoundCrypto.ForPlainPassword(pw);
|
||||||
|
PushAudioCrypto();
|
||||||
|
ExplainWeakPasswordIfNeeded(pw);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Slow path: a strong password not yet derived this session. Hold the key null (audio waits)
|
||||||
|
// and do the PBKDF2 on a worker; apply on the UI thread when it lands, unless superseded.
|
||||||
|
currentAudioKey = null;
|
||||||
|
currentAudioFingerprint = null;
|
||||||
|
PushAudioCrypto();
|
||||||
|
logFile.Event("audio crypto: deriving key off-thread (strong password, first use this session)");
|
||||||
|
var pwLocal = pw;
|
||||||
|
Task.Run(() =>
|
||||||
|
{
|
||||||
|
RemSoundCrypto.Prewarm(pwLocal); // the ~1 s PBKDF2, off the UI thread
|
||||||
|
try
|
||||||
|
{
|
||||||
|
BeginInvoke(() =>
|
||||||
|
{
|
||||||
|
if (gen != cryptoGeneration) return; // a newer password change won; drop this result
|
||||||
|
(currentAudioKey, currentAudioFingerprint) = RemSoundCrypto.ForPlainPassword(pwLocal); // now a cache hit
|
||||||
|
PushAudioCrypto();
|
||||||
|
logFile.Event("audio crypto: key ready");
|
||||||
|
});
|
||||||
|
}
|
||||||
|
catch { /* form closed / no handle — nothing to apply to */ }
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
private void PushAudioCrypto()
|
||||||
|
{
|
||||||
|
sender.AudioKey = currentAudioKey;
|
||||||
|
sender.AudioFingerprint = currentAudioFingerprint;
|
||||||
|
receiver.AudioKey = currentAudioKey;
|
||||||
|
receiver.AudioFingerprint = currentAudioFingerprint;
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>Since 5.6 the derivation rule refuses a WEAK password (key comes back null), so a
|
||||||
|
/// profile that auto-connects at startup with an old guessable password must not just sit
|
||||||
|
/// silently dead — say why, once, and point at the fix. The interactive tick path has its own
|
||||||
|
/// guided flow (EnsureStreamingPassword); this catches every other route in.</summary>
|
||||||
|
private void ExplainWeakPasswordIfNeeded(string? pw)
|
||||||
|
{
|
||||||
|
if (string.IsNullOrEmpty(pw) || currentAudioKey is not null || weakPasswordExplained) return;
|
||||||
weakPasswordExplained = true;
|
weakPasswordExplained = true;
|
||||||
logFile.Event("audio crypto: profile password fails the 5.6 strength rule — no audio until it's changed");
|
logFile.Event("audio crypto: profile password fails the 5.6 strength rule — no audio until it's changed");
|
||||||
var advice = PasswordStrength.Critique(currentProfilePassword) ?? "";
|
var advice = PasswordStrength.Critique(pw) ?? "";
|
||||||
BeginInvoke(() =>
|
BeginInvoke(() =>
|
||||||
{
|
{
|
||||||
var page = new TaskDialogPage
|
var page = new TaskDialogPage
|
||||||
@@ -8313,15 +8371,13 @@ public sealed partial class MainForm : Form
|
|||||||
ForegroundDialog.Show(owner => TaskDialog.ShowDialog(owner, page));
|
ForegroundDialog.Show(owner => TaskDialog.ShowDialog(owner, page));
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
}
|
|
||||||
sender.AudioKey = currentAudioKey;
|
|
||||||
sender.AudioFingerprint = currentAudioFingerprint;
|
|
||||||
receiver.AudioKey = currentAudioKey;
|
|
||||||
receiver.AudioFingerprint = currentAudioFingerprint;
|
|
||||||
}
|
|
||||||
|
|
||||||
// One-shot flag for the weak-password explanation above — the dialog must not re-fire on every
|
// Bumped on every password change so a slow background derive that finishes AFTER a newer change
|
||||||
// profile reapply within a session (the log line still records each derivation refusal).
|
// knows to discard its now-stale result (see RecomputeAudioCrypto).
|
||||||
|
private int cryptoGeneration;
|
||||||
|
// One-shot flag for the weak-password explanation — the dialog must not re-fire on every profile
|
||||||
|
// reapply within a session (the log line still records each derivation refusal). Reset on a
|
||||||
|
// profile switch so a SECOND weak-password profile in one session is still explained.
|
||||||
private bool weakPasswordExplained;
|
private bool weakPasswordExplained;
|
||||||
|
|
||||||
/// <summary>The "you need a password before any audio can flow" gate. Called when the user
|
/// <summary>The "you need a password before any audio can flow" gate. Called when the user
|
||||||
|
|||||||
@@ -2474,7 +2474,21 @@ internal static class SelfTest
|
|||||||
var (goodKey, goodFp) = RemSoundCrypto.ForPlainPassword("kettle9tiger42moon");
|
var (goodKey, goodFp) = RemSoundCrypto.ForPlainPassword("kettle9tiger42moon");
|
||||||
Check(goodKey is { Length: 32 } && goodFp is { Length: 8 }, "a strong password must derive the full key + fingerprint");
|
Check(goodKey is { Length: 32 } && goodFp is { Length: 8 }, "a strong password must derive the full key + fingerprint");
|
||||||
|
|
||||||
return "weak + common refused with concrete advice; derivation choke-point refuses weak on every path";
|
// Session cache (the NVDA-hang fix): a distinct strong password is a cache MISS until derived
|
||||||
|
// or prewarmed, then a HIT — so the ~1s PBKDF2 runs once per password per process and the
|
||||||
|
// UI-thread path can stay synchronous only when it's a hit. A second derive returns the SAME
|
||||||
|
// arrays (proof it wasn't recomputed). Empty/weak always count as "no work" (never block).
|
||||||
|
Check(RemSoundCrypto.IsCached("kettle9tiger42moon"), "a password just derived must read back as cached");
|
||||||
|
var again = RemSoundCrypto.ForPlainPassword("kettle9tiger42moon");
|
||||||
|
Check(ReferenceEquals(again.Key, goodKey), "a cached derive must return the same key instance, not recompute it");
|
||||||
|
var fresh = "prewarm7melon42anchor";
|
||||||
|
Check(!RemSoundCrypto.IsCached(fresh), "an unused strong password must read as a cache miss (would block the UI thread)");
|
||||||
|
RemSoundCrypto.Prewarm(fresh);
|
||||||
|
Check(RemSoundCrypto.IsCached(fresh), "Prewarm must derive off-thread so the later UI-thread lookup is an instant hit");
|
||||||
|
Check(RemSoundCrypto.IsCached("") && RemSoundCrypto.IsCached("Games"),
|
||||||
|
"empty and weak passwords must count as 'no work' (they never trigger a blocking derive)");
|
||||||
|
|
||||||
|
return "weak+common refused with advice; derivation refuses weak everywhere; results cached (no repeat PBKDF2)";
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>The relay address-proof (2026-07-27): an AddrCheck cookie arriving at the receiver
|
/// <summary>The relay address-proof (2026-07-27): an AddrCheck cookie arriving at the receiver
|
||||||
|
|||||||
@@ -71,6 +71,14 @@ public static class RemSoundCrypto
|
|||||||
private static readonly byte[] ObfuscationKey =
|
private static readonly byte[] ObfuscationKey =
|
||||||
Encoding.UTF8.GetBytes("RemSound-profile-password-scramble-v1");
|
Encoding.UTF8.GetBytes("RemSound-profile-password-scramble-v1");
|
||||||
|
|
||||||
|
// Session cache of derived credentials, keyed by plaintext password. The 600k-iteration PBKDF2
|
||||||
|
// (run TWICE per derive, for key + fingerprint) costs up to ~1 s on old hardware; caching means
|
||||||
|
// a given password pays that ONCE per process, so a profile SWITCH back to a used password — or
|
||||||
|
// the service re-deriving on an audio-device hot-plug — is instant. The derived key already lives
|
||||||
|
// in process memory (currentAudioKey), so this doesn't widen exposure; it's cleared on exit like
|
||||||
|
// everything else. Bounded: a user has a handful of distinct passwords, not thousands.
|
||||||
|
private static readonly System.Collections.Concurrent.ConcurrentDictionary<string, (byte[] Key, byte[] Fingerprint)> derivedCache = new(StringComparer.Ordinal);
|
||||||
|
|
||||||
/// <summary>The one rule for turning a PLAIN password into the audio credentials: null/empty →
|
/// <summary>The one rule for turning a PLAIN password into the audio credentials: null/empty →
|
||||||
/// (null, null) → no audio flows (encryption is mandatory); since 5.6 a password that fails
|
/// (null, null) → no audio flows (encryption is mandatory); since 5.6 a password that fails
|
||||||
/// <see cref="PasswordStrength.Critique"/> ALSO yields (null, null) — enforced here, at the single
|
/// <see cref="PasswordStrength.Critique"/> ALSO yields (null, null) — enforced here, at the single
|
||||||
@@ -79,11 +87,34 @@ public static class RemSoundCrypto
|
|||||||
/// explains and walks the user to a stronger one). Otherwise the key AND the fingerprint, always
|
/// explains and walks the user to a stronger one). Otherwise the key AND the fingerprint, always
|
||||||
/// together — the peer verifies the fingerprint before accepting a stream, so a key without its
|
/// together — the peer verifies the fingerprint before accepting a stream, so a key without its
|
||||||
/// fingerprint gets the audio silently rejected at the far end (a divergence that already bit
|
/// fingerprint gets the audio silently rejected at the far end (a divergence that already bit
|
||||||
/// the service once).</summary>
|
/// the service once). Result is cached per password (see <see cref="derivedCache"/>); the slow
|
||||||
public static (byte[]? Key, byte[]? Fingerprint) ForPlainPassword(string? plainPassword) =>
|
/// PBKDF2 runs once per distinct password per process. Callers that must not stall the UI thread
|
||||||
string.IsNullOrEmpty(plainPassword) || PasswordStrength.Critique(plainPassword) is not null
|
/// should <see cref="Prewarm"/> off-thread first (the app does).</summary>
|
||||||
? (null, null)
|
public static (byte[]? Key, byte[]? Fingerprint) ForPlainPassword(string? plainPassword)
|
||||||
: (DeriveKey(plainPassword), Fingerprint(plainPassword));
|
{
|
||||||
|
if (string.IsNullOrEmpty(plainPassword) || PasswordStrength.Critique(plainPassword) is not null)
|
||||||
|
return (null, null);
|
||||||
|
var (k, f) = derivedCache.GetOrAdd(plainPassword, static pw => (DeriveKey(pw), Fingerprint(pw)));
|
||||||
|
return (k, f);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>Derive-and-cache a password's credentials WITHOUT returning them — for calling on a
|
||||||
|
/// background thread so the subsequent <see cref="ForPlainPassword"/> on the UI thread is a cache
|
||||||
|
/// hit and never blocks on PBKDF2. Null/empty/weak passwords are a no-op (nothing to warm).</summary>
|
||||||
|
public static void Prewarm(string? plainPassword)
|
||||||
|
{
|
||||||
|
if (!string.IsNullOrEmpty(plainPassword) && PasswordStrength.Critique(plainPassword) is null)
|
||||||
|
derivedCache.GetOrAdd(plainPassword, static pw => (DeriveKey(pw), Fingerprint(pw)));
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>True when <paramref name="plainPassword"/>'s credentials are already cached, so
|
||||||
|
/// <see cref="ForPlainPassword"/> will return instantly without running PBKDF2. Lets the caller
|
||||||
|
/// decide whether it can derive on the UI thread (cached / weak / empty = fast) or must go
|
||||||
|
/// off-thread (a strong password's first use this session). Empty/weak count as "no work".</summary>
|
||||||
|
public static bool IsCached(string? plainPassword) =>
|
||||||
|
string.IsNullOrEmpty(plainPassword)
|
||||||
|
|| PasswordStrength.Critique(plainPassword) is not null
|
||||||
|
|| derivedCache.ContainsKey(plainPassword);
|
||||||
|
|
||||||
/// <summary>Derive the 256-bit AES key for a password. Cache the result; never call per packet.</summary>
|
/// <summary>Derive the 256-bit AES key for a password. Cache the result; never call per packet.</summary>
|
||||||
public static byte[] DeriveKey(string? password) =>
|
public static byte[] DeriveKey(string? password) =>
|
||||||
|
|||||||
Reference in New Issue
Block a user