Pre-release hardening 1-3: nonce prefix widen + status-text dedup + stale comments
From the four-agent pre-release review (all read-only), the three items that warranted fixing before ship: 1. NONCE PREFIX WIDENED (the one security-relevant finding). The interim counter-nonce used a 32-bit random prefix + 64-bit counter; since a fresh instance restarts the counter at 0, two instances that drew the same prefix would reuse nonces under the same long-lived key (catastrophic for AES-GCM), and 32 bits collides at only ~2^16 instances. Now 48-bit random prefix + 48-bit counter: birthday bound ~2^24 instances while 2^48 packets/session stays far beyond any real session - strictly safer than both the interim scheme AND the original per-packet 96-bit-random nonce. Wire unchanged (receiver reads the nonce off the packet). Comment corrected. 2. STATUS-TEXT DE-DUPLICATED. The CheckedListBox spoken-status builder existed in two copies (CheckedListAccessibility + MainForm) with a comment falsely claiming they were "exact" - and they had already drifted (the remembered-apps empty-state line was in the MainForm copy only). Folded to ONE builder (CheckedListAccessibility.ApplyStatus + EmptyTextFor); MainForm delegates. NVDA wording is now identical in the main window and every dialog by construction. No user-visible change today; removes the silent-desync trap. 3. STALE COMMENTS. RemPacket still documented the pre-5.6 plaintext control wire format (2 bytes / 14 on the wire); it is now sealed (50 on the wire, the 2 bytes are inner plaintext). Fixed that and the MainForm.Peers.cs comment that misplaced CaptureSpecBuilder in Core (it is App by necessity). Docs only. Plus review-flagged test top-ups: NonceSequence layout + two-instance-distinct-prefix assert; ControlReceiveGuard future-dated rejection + 300-command burst (no false replay across the prune threshold); log-rotation INTEGRITY (every line survives exactly once) + a 4-thread concurrency test that actually fails if writeGate is removed; ReleaseSigning now honestly SKIPs (not caveat-PASSes) when the publisher key is absent. Gate 69/69. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Ednunp
Claude Fable 5
@@ -17,7 +17,7 @@ internal static class CheckedListAccessibility
|
||||
var lastIndex = 0;
|
||||
|
||||
void Update(int? overrideIndex = null, bool? overrideChecked = null)
|
||||
=> SetStatus(list, statusLabel, itemKind, overrideIndex, overrideChecked);
|
||||
=> ApplyStatus(list, statusLabel, itemKind, overrideIndex, overrideChecked);
|
||||
|
||||
void RestoreFocus()
|
||||
{
|
||||
@@ -67,12 +67,26 @@ internal static class CheckedListAccessibility
|
||||
Update();
|
||||
}
|
||||
|
||||
// Exact copy of MainForm.UpdateCheckedListStatus so the spoken text is word-for-word identical.
|
||||
private static void SetStatus(CheckedListBox list, Label statusLabel, string itemKind, int? overrideIndex, bool? overrideChecked)
|
||||
/// <summary>The empty-list spoken text for a given item kind. The remembered-applications list
|
||||
/// teaches its own lifecycle here (entries arrive when you TICK an app), since after clearing it
|
||||
/// there is otherwise no cue how it refills; every other list is the plain "none available".
|
||||
/// One home so the main window and the dialogs can never drift (they did once — a MainForm-only
|
||||
/// copy of this branch, 2026-07-27).</summary>
|
||||
public static string EmptyTextFor(string itemKind) =>
|
||||
itemKind == "remembered application"
|
||||
? "No remembered application. Tick an application in the list above and it will be remembered here."
|
||||
: $"No {itemKind}s available.";
|
||||
|
||||
/// <summary>THE single builder+applier for a CheckedListBox's spoken status. MainForm delegates
|
||||
/// here too, so the wording is word-for-word identical in the main window and every dialog by
|
||||
/// construction — not by a comment promising it. Writes the text into the status label and the
|
||||
/// list's AccessibleDescription (and the label's, for a focused non-empty item) so NVDA reads
|
||||
/// the item AND its checked state.</summary>
|
||||
public static void ApplyStatus(CheckedListBox list, Label statusLabel, string itemKind, int? overrideIndex = null, bool? overrideChecked = null)
|
||||
{
|
||||
if (list.Items.Count == 0)
|
||||
{
|
||||
var emptyText = $"No {itemKind}s available.";
|
||||
var emptyText = EmptyTextFor(itemKind);
|
||||
statusLabel.Text = emptyText;
|
||||
list.AccessibleDescription = emptyText;
|
||||
return;
|
||||
|
||||
@@ -12,8 +12,10 @@ namespace RemSound.App;
|
||||
/// The PEER half of the main window — state, discovery/selection reconciliation, arming, naming and
|
||||
/// the remembered-peers plumbing — split out of MainForm.cs verbatim in the 2026-07-26 review's
|
||||
/// god-object shrink (same partial-class pattern Andre's SensorReadout form uses). Pure code motion:
|
||||
/// same class, same members, no behaviour change — the compiler proves it. The shared LOGIC these
|
||||
/// methods lean on (PeerArming, CaptureSpecBuilder, PeerAddress) already lives in Core.
|
||||
/// same class, same members, no behaviour change — the compiler proves it. The shared arming/address
|
||||
/// LOGIC these methods lean on (PeerArming, PeerAddress) lives in Core; the send-spec builder
|
||||
/// (CaptureSpecBuilder) stays in App by necessity — it depends on App's AudioDefaultFollower and the
|
||||
/// Sender's app enumerator — but is likewise shared with the service.
|
||||
/// </summary>
|
||||
public sealed partial class MainForm
|
||||
{
|
||||
|
||||
@@ -9648,31 +9648,12 @@ public sealed partial class MainForm : Form
|
||||
else Restore();
|
||||
}
|
||||
|
||||
// Delegates to the ONE shared builder (CheckedListAccessibility.ApplyStatus) so the main
|
||||
// window and the dialogs speak word-for-word identical status text — including the
|
||||
// remembered-apps empty-state lifecycle line. (These used to be two hand-kept copies that
|
||||
// drifted, 2026-07-27.)
|
||||
private static void UpdateCheckedListStatus(CheckedListBox list, Label statusLabel, string itemKind, int? overrideIndex = null, bool? overrideChecked = null)
|
||||
{
|
||||
if (list.Items.Count == 0)
|
||||
{
|
||||
// The remembered-apps list teaches its own lifecycle when empty (Ed, 2026-07-27: after
|
||||
// clearing it he had no way to know how entries come back — they arrive when you TICK
|
||||
// an app, so the empty state says exactly that, right where the question arises).
|
||||
var emptyText = itemKind == "remembered application"
|
||||
? "No remembered application. Tick an application in the list above and it will be remembered here."
|
||||
: $"No {itemKind}s available.";
|
||||
statusLabel.Text = emptyText;
|
||||
list.AccessibleDescription = emptyText;
|
||||
return;
|
||||
}
|
||||
|
||||
var index = overrideIndex ?? (list.SelectedIndex >= 0 ? list.SelectedIndex : 0);
|
||||
index = Math.Clamp(index, 0, list.Items.Count - 1);
|
||||
var isChecked = overrideChecked ?? list.GetItemChecked(index);
|
||||
var checkedText = isChecked ? "checked" : "not checked";
|
||||
var itemText = list.Items[index]?.ToString() ?? itemKind;
|
||||
var text = $"{checkedText}, {itemText}. Item {index + 1} of {list.Items.Count}. Press Space to toggle.";
|
||||
statusLabel.Text = text;
|
||||
list.AccessibleDescription = text;
|
||||
statusLabel.AccessibleDescription = text;
|
||||
}
|
||||
=> CheckedListAccessibility.ApplyStatus(list, statusLabel, itemKind, overrideIndex, overrideChecked);
|
||||
|
||||
/// <summary>
|
||||
/// Makes a NumericUpDown's text content fully selected whenever the control receives focus,
|
||||
|
||||
@@ -43,6 +43,17 @@ internal static class SelfTest
|
||||
|
||||
private static string Skip(string why) => throw new StepSkipped(why);
|
||||
|
||||
/// <summary>Non-overlapping count of <paramref name="needle"/> in <paramref name="haystack"/> —
|
||||
/// used by the log-rotation integrity checks to prove each line survives exactly once.</summary>
|
||||
private static int CountOccurrences(string haystack, string needle)
|
||||
{
|
||||
if (string.IsNullOrEmpty(needle)) return 0;
|
||||
var count = 0;
|
||||
for (var i = haystack.IndexOf(needle, StringComparison.Ordinal); i >= 0; i = haystack.IndexOf(needle, i + needle.Length, StringComparison.Ordinal))
|
||||
count++;
|
||||
return count;
|
||||
}
|
||||
|
||||
public static int Run(string[] args)
|
||||
{
|
||||
var seconds = int.TryParse(ValueAfter(args, "--seconds"), out var s) && s is > 0 and <= 30 ? s : 3;
|
||||
@@ -2367,16 +2378,40 @@ internal static class SelfTest
|
||||
var skewed = ControlSealing.Seal(key, RemoteControlKind.VolumeDown, 5, nowSecs - 300);
|
||||
Check(new ControlReceiveGuard().TryAccept(key, skewed, now, out _, out _, out _),
|
||||
"a command from a peer whose clock is 5 minutes off must still be accepted");
|
||||
// A FUTURE-dated command (sender clock ahead, or a crafted timestamp) beyond the skew window
|
||||
// must also be rejected — the guard bounds skew in BOTH directions, not just the past.
|
||||
var future = ControlSealing.Seal(key, RemoteControlKind.VolumeUp, 5, nowSecs + 3600);
|
||||
Check(!new ControlReceiveGuard().TryAccept(key, future, now, out _, out _, out var whyFuture) && whyFuture.StartsWith("stale"),
|
||||
"a command dated an hour in the FUTURE must be rejected (skew is bounded both ways)");
|
||||
// Replay cache under a sustained burst: 300 distinct genuine commands (> the 256 prune
|
||||
// threshold) must all be accepted once and the dict must not reject a fresh nonce as a
|
||||
// false replay — i.e. pruning must never evict an entry still inside the skew window.
|
||||
var burstGuard = new ControlReceiveGuard();
|
||||
var accepted = 0;
|
||||
for (var i = 0; i < 300; i++)
|
||||
{
|
||||
// Vary the timestamp by a second each so every packet has a distinct nonce+content.
|
||||
var cmd = ControlSealing.Seal(key, RemoteControlKind.VolumeUp, 1, nowSecs - (i % 60));
|
||||
if (burstGuard.TryAccept(key, cmd, now, out _, out _, out _)) accepted++;
|
||||
}
|
||||
Check(accepted == 300, $"every one of 300 distinct genuine commands must be accepted across the prune threshold (got {accepted})");
|
||||
|
||||
// Nonce discipline: prefix constant per sequence, counter strictly advancing, instances differ.
|
||||
// Nonce discipline: 48-bit random prefix (bytes 0-5) constant within a sequence, 48-bit
|
||||
// little-endian counter (bytes 6-11) advancing, and — the cross-session safety that matters
|
||||
// — two independent instances draw DIFFERENT prefixes so their counter-0 nonces can't collide
|
||||
// under the same long-lived key.
|
||||
var seqA = new RemSoundCrypto.NonceSequence();
|
||||
var n1 = new byte[12]; var n2 = new byte[12];
|
||||
seqA.FillNext(n1); seqA.FillNext(n2);
|
||||
Check(!n1.AsSpan().SequenceEqual(n2), "consecutive nonces from one sequence must differ");
|
||||
Check(n1.AsSpan(0, 4).SequenceEqual(n2.AsSpan(0, 4)), "the 4-byte prefix must stay constant within a sequence");
|
||||
Check(n2[4] == 1 && n1[4] == 0, "the counter half must advance arithmetically (uniqueness by construction)");
|
||||
Check(n1.AsSpan(0, 6).SequenceEqual(n2.AsSpan(0, 6)), "the 6-byte prefix must stay constant within a sequence");
|
||||
Check(n1[6] == 0 && n2[6] == 1, "the counter (byte 6, little-endian) must advance arithmetically");
|
||||
var seqB = new RemSoundCrypto.NonceSequence();
|
||||
var m1 = new byte[12]; seqB.FillNext(m1);
|
||||
Check(!n1.AsSpan(0, 6).SequenceEqual(m1.AsSpan(0, 6)),
|
||||
"two independent NonceSequence instances must draw different prefixes (else same-key counter-0 nonces collide)");
|
||||
|
||||
return "sealed + replay/stale/wrong-key/plaintext all rejected; skew tolerated; nonces counter-based";
|
||||
return "sealed + replay/stale/wrong-key/plaintext all rejected; skew tolerated; nonce prefix 48-bit + per-instance-distinct";
|
||||
}
|
||||
|
||||
/// <summary>Release signing (2026-07-27): the updater refuses any release zip whose detached
|
||||
@@ -2408,7 +2443,12 @@ internal static class SelfTest
|
||||
|
||||
var realKeyPath = Environment.GetEnvironmentVariable("REMSOUND_SIGNING_KEY") ?? @"D:\Dropbox\proj\rsound key\remsound-signing-key.pem";
|
||||
if (!File.Exists(realKeyPath))
|
||||
return "mechanics proven with ephemeral key; embedded key parses (publisher key not on this machine — embed-match check skipped)";
|
||||
// Honest SKIP, not a caveat-PASS: the crypto mechanics above passed, but this step's
|
||||
// headline job — proving the on-disk private key matches the EMBEDDED public key — did
|
||||
// not run without the publisher key, and must show in the SKIPPED line rather than read
|
||||
// as "verified". (The real safety net is build-release.ps1's --sign-update self-check,
|
||||
// which aborts the publish on a mismatch; this step is the early-warning on the dev box.)
|
||||
return Skip("publisher signing key not on this machine — embed-match check did not run (mechanics passed; publish pipeline self-checks regardless)");
|
||||
var realSig = UpdateSignature.SignWithKey(data, File.ReadAllText(realKeyPath));
|
||||
Check(UpdateSignature.Verify(data, realSig),
|
||||
"the on-disk private key MUST match the embedded public key — a mismatch ships a release every updater refuses");
|
||||
@@ -2547,19 +2587,28 @@ internal static class SelfTest
|
||||
/// priority-mode scope decision — levers only while streaming, released after the hold-down.</summary>
|
||||
private static string? LongRunHygiene()
|
||||
{
|
||||
// 1. Log rotation at the cap. Tiny cap via the internal seam; verify multiple real files.
|
||||
// 1. Log rotation at the cap. Tiny cap via the internal seam; verify multiple real files,
|
||||
// that NO line is lost or duplicated across the rolls (integrity — a dropped last-buffered
|
||||
// line would slip past a count-only check), and that concurrent writers can't interleave
|
||||
// (the entire documented reason for writeGate — a single-threaded test never exercises it).
|
||||
var createdLogs = new List<string>();
|
||||
var log = new RemSoundLog { Enabled = true, RollAfterBytes = 400 };
|
||||
try
|
||||
{
|
||||
for (var i = 0; i < 30; i++)
|
||||
const int lineCount = 120;
|
||||
for (var i = 0; i < lineCount; i++)
|
||||
{
|
||||
log.Event($"rotation self-test line {i} — padding padding padding padding");
|
||||
log.Event($"ROTLINE#{i}# padding padding padding padding padding");
|
||||
if (log.Path is { } p && !createdLogs.Contains(p)) createdLogs.Add(p);
|
||||
}
|
||||
Check(log.RollCount >= 2, $"a 400-byte cap over 30 writes must roll at least twice (rolled {log.RollCount})");
|
||||
Check(log.RollCount >= 2, $"a 400-byte cap over {lineCount} writes must roll several times (rolled {log.RollCount})");
|
||||
Check(createdLogs.Count >= 3, "each roll must continue into a NEW timestamped file");
|
||||
Check(createdLogs.All(File.Exists), "every rolled file must exist on disk (the chain, not one giant)");
|
||||
// Integrity: read the whole chain back and confirm every ROTLINE#i# appears exactly once.
|
||||
log.Dispose(); // flush + close so the last file is fully readable
|
||||
var body = string.Concat(createdLogs.Where(File.Exists).Select(File.ReadAllText));
|
||||
var missing = Enumerable.Range(0, lineCount).Where(i => CountOccurrences(body, $"ROTLINE#{i}#") != 1).ToList();
|
||||
Check(missing.Count == 0, $"every logged line must survive rotation exactly once — {missing.Count} lost/duplicated across the roll");
|
||||
}
|
||||
finally
|
||||
{
|
||||
@@ -2567,6 +2616,37 @@ internal static class SelfTest
|
||||
foreach (var f in createdLogs) { try { File.Delete(f); } catch { } }
|
||||
}
|
||||
|
||||
// 1b. Concurrency: many threads writing through ONE log must never interleave a line (the
|
||||
// writeGate guarantee). Delete the lock and this is what fails — a count-only test wouldn't.
|
||||
var concLogs = new List<string>();
|
||||
// Cap high enough that 800 short lines never roll — this case is about the writeGate
|
||||
// interleave guarantee, not rotation, so keep it to one file and capture every path anyway.
|
||||
var clog = new RemSoundLog { Enabled = true, RollAfterBytes = 4 * 1024 * 1024 };
|
||||
try
|
||||
{
|
||||
const int threads = 4, perThread = 200;
|
||||
Parallel.For(0, threads, t =>
|
||||
{
|
||||
for (var i = 0; i < perThread; i++)
|
||||
{
|
||||
clog.Event($"T{t}L{i}-nointerleave-nointerleave-nointerleave");
|
||||
if (clog.Path is { } p && !concLogs.Contains(p)) concLogs.Add(p);
|
||||
}
|
||||
});
|
||||
clog.Dispose();
|
||||
var lines = concLogs.Where(File.Exists).SelectMany(File.ReadAllLines).ToList();
|
||||
var payload = lines.Where(l => l.Contains("-nointerleave")).ToList();
|
||||
Check(payload.Count == threads * perThread,
|
||||
$"every concurrent write must be its own intact line ({threads * perThread} expected, got {payload.Count} — interleave = torn lines)");
|
||||
Check(payload.All(l => CountOccurrences(l, "-nointerleave") == 3),
|
||||
"no line may contain a fragment of another (torn/interleaved write detector)");
|
||||
}
|
||||
finally
|
||||
{
|
||||
clog.Dispose();
|
||||
foreach (var f in concLogs) { try { File.Delete(f); } catch { } }
|
||||
}
|
||||
|
||||
// 2. Crash-report cap: 14 fake reports → newest 10 survive.
|
||||
var tmp = Path.Combine(Path.GetTempPath(), "remsound-selftest-crash-" + Guid.NewGuid().ToString("N"));
|
||||
try
|
||||
|
||||
@@ -12,9 +12,11 @@ public enum RemPacketType : byte
|
||||
/// Remote-control message from one connected peer to another. Currently used to let a
|
||||
/// peer adjust the receiver-side volume slider on a peer it's connected to (so a user
|
||||
/// who's NVDA-Remote'd into another machine can still nudge the listening volume on
|
||||
/// the machine they're physically at). Wire format: 1 byte <see cref="RemoteControlKind"/>
|
||||
/// + 1 byte signed delta (interpreted as signed sbyte; range -128..127, percent points).
|
||||
/// Old peers see "unknown packet type" and silently drop, so adding this is wire-safe.
|
||||
/// the machine they're physically at). Since 5.6 the payload is SEALED (<see cref="RemSound"/>
|
||||
/// .Core ControlSealing): on the wire it is the 12-byte header + a GCM blob of
|
||||
/// ControlSealing.SealedPayloadBytes. The 1-byte kind + 1-byte signed delta
|
||||
/// (<see cref="ControlPayloadSize"/>) is now only the INNER plaintext inside that blob, never
|
||||
/// travelling in the clear. Old peers see "unknown packet type" and silently drop.
|
||||
/// </summary>
|
||||
Control = 5,
|
||||
// 6-9 are the relay's lobby types (hello / roster / full / bye) — relay-side, not modelled here.
|
||||
@@ -112,9 +114,10 @@ public static class RemPacket
|
||||
/// </summary>
|
||||
public const int HeartbeatPayloadSize = 9;
|
||||
/// <summary>
|
||||
/// Control payload: 1 byte <see cref="RemoteControlKind"/> + 1 signed byte delta. Total
|
||||
/// 2 bytes, plus the 12-byte header = 14 bytes on the wire. See <see cref="RemPacketType.Control"/>
|
||||
/// for the rationale.
|
||||
/// Control INNER plaintext: 1 byte <see cref="RemoteControlKind"/> + 1 signed byte delta = 2 bytes.
|
||||
/// Since 5.6 this does NOT travel on the wire on its own — it's sealed by ControlSealing, and the
|
||||
/// on-wire Control payload is ControlSealing.SealedPayloadBytes (nonce+tag over this plaintext plus
|
||||
/// a timestamp). See <see cref="RemPacketType.Control"/>.
|
||||
/// </summary>
|
||||
public const int ControlPayloadSize = 2;
|
||||
/// <summary>
|
||||
|
||||
@@ -164,25 +164,39 @@ public static class RemSoundCrypto
|
||||
return total;
|
||||
}
|
||||
|
||||
/// <summary>The nonce generator for a hot-path encryptor: a random 4-byte prefix chosen at
|
||||
/// construction plus a 64-bit counter — the textbook fix the old per-packet-random comment
|
||||
/// pointed at. Uniqueness within one instance is arithmetic (the counter), not probabilistic;
|
||||
/// across instances (each sender lane, each session) the random prefix keeps streams apart.
|
||||
/// The receiver just reads the nonce from the packet, so this changes nothing on the wire.
|
||||
/// NOT thread-safe — one per encryptor, same ownership rule as the AesGcm itself.</summary>
|
||||
/// <summary>The nonce generator for a hot-path encryptor: a random 48-bit prefix chosen at
|
||||
/// construction plus a 48-bit counter, filling the 96-bit nonce. Two independent guarantees:
|
||||
/// WITHIN one instance uniqueness is arithmetic (the counter, 2^48 packets ≈ tens of thousands
|
||||
/// of years at our rates — effectively unlimited per session); ACROSS instances (every launch,
|
||||
/// profile reload, and the two BothIndependent lanes all rebuild the sequence under the SAME
|
||||
/// long-lived audio key) the random prefix keeps their counter ranges apart, with a birthday
|
||||
/// bound at ~2^24 instances — negligible for any realistic number of app launches on one
|
||||
/// password.
|
||||
///
|
||||
/// Why 48/48 and not the interim 32/64: a fresh instance restarts the counter at 0, so two
|
||||
/// instances that drew the SAME prefix would reuse nonce=prefix‖0,‖1,… under the same key —
|
||||
/// catastrophic for AES-GCM. A 32-bit prefix collides at only ~2^16 instances (a real risk
|
||||
/// over a multi-year, one-password lifetime); widening the prefix to 48 bits pushes that to
|
||||
/// ~2^24 while 48 counter bits stay far beyond any session's packet count. This is strictly
|
||||
/// safer than BOTH the interim scheme and the original per-packet 96-bit-random nonce (whose
|
||||
/// birthday bound a heavy multi-day streamer could actually approach). The receiver just reads
|
||||
/// the nonce off the packet, so the wire is unchanged. NOT thread-safe — one per encryptor,
|
||||
/// same ownership rule as the AesGcm itself.</summary>
|
||||
public sealed class NonceSequence
|
||||
{
|
||||
private readonly byte[] prefix = new byte[4];
|
||||
private ulong counter;
|
||||
private const int PrefixBytes = 6; // 48-bit per-instance random prefix
|
||||
private const int CounterBytes = NonceBytes - PrefixBytes; // 6 → 48-bit counter
|
||||
private readonly byte[] prefix = new byte[PrefixBytes];
|
||||
private ulong counter; // only the low 48 bits are used (see CounterBytes)
|
||||
|
||||
public NonceSequence() => RandomNumberGenerator.Fill(prefix);
|
||||
|
||||
/// <summary>Write the next 12-byte nonce: prefix(4) || counter(8), then advance.</summary>
|
||||
/// <summary>Write the next 12-byte nonce: prefix(6) ‖ counter(6, little-endian), then advance.</summary>
|
||||
public void FillNext(Span<byte> nonce12)
|
||||
{
|
||||
prefix.CopyTo(nonce12);
|
||||
System.Buffers.Binary.BinaryPrimitives.WriteUInt64LittleEndian(nonce12[4..], counter);
|
||||
counter++;
|
||||
var c = counter++;
|
||||
for (var i = 0; i < CounterBytes; i++) nonce12[PrefixBytes + i] = (byte)(c >> (8 * i));
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user