Cleanup + security narrowing + manual updates (post-review)
Security (narrow the no-admin grants to one account): - The service's no-admin start/stop and bin-write grants went to Authenticated Users / BUILTIN\Users - together that was a one-step local privilege escalation for ANY account (overwrite the SYSTEM-run binary, then stop/start it). Now both grants go to the INSTALLING user's SID only (the elevated install runs as that interactive user). Same effortless workflow for that user; the any-account escalation surface is gone. AddUserStartStopAce takes the SID; self-test asserts it's scoped, not AU. Dead-code removal: - --probe-apploopback diagnostic verb + ProbeAppLoopback.cs + the ProcessLoopbackCapture .Diagnostic hook (all scaffolding for the now-fixed activation bug). - --update-service verb + ServiceControl.DoUpdate (the "Update service" menu item is gone; auto-update via ServiceUpdate.RestartSelf replaced it). Verb gate now lists five. Manual (readme.html): - New "Sending specific applications" section (the How-to-send chooser + the two app lists) and the Alt+6/8/9 shortcuts - the whole per-app feature was undocumented. - Documented the two "Clear remembered ... list" buttons on Preferences > General. Gate: 42/42. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Ednunp
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
984bcd042e
commit
d002130402
@@ -1,66 +0,0 @@
|
||||
using System.Diagnostics;
|
||||
using RemSound.Sender;
|
||||
|
||||
namespace RemSound.App;
|
||||
|
||||
/// <summary>Focused diagnostic probe for the process-loopback activation hang (per-app send). Run via
|
||||
/// `RemSound.exe --probe-apploopback [pid]`. Traces the whole activation path (hr, callback delivery,
|
||||
/// timing) for the given pid — or the FIRST process named in the REMSOUND_PROBE_APP env var, else self.
|
||||
/// Prints to the console; not part of the gate. Temporary investigation aid.</summary>
|
||||
internal static class ProbeAppLoopback
|
||||
{
|
||||
public static int Run(string[] args)
|
||||
{
|
||||
var outPath = Path.Combine(Path.GetTempPath(), "remsound-apploopback-probe.txt");
|
||||
var sw0 = new StreamWriter(outPath, append: false) { AutoFlush = true };
|
||||
void Line(string m) => sw0.WriteLine(m);
|
||||
ProcessLoopbackCapture.Diagnostic = m => Line($" [{DateTime.Now:HH:mm:ss.fff}] {m}");
|
||||
|
||||
int pid;
|
||||
var explicitPid = args.FirstOrDefault(a => int.TryParse(a, out _));
|
||||
if (explicitPid is not null) pid = int.Parse(explicitPid);
|
||||
else
|
||||
{
|
||||
var name = Environment.GetEnvironmentVariable("REMSOUND_PROBE_APP");
|
||||
var proc = string.IsNullOrWhiteSpace(name)
|
||||
? null
|
||||
: Process.GetProcessesByName(name).FirstOrDefault();
|
||||
pid = proc?.Id ?? Environment.ProcessId;
|
||||
}
|
||||
|
||||
Line($"process-loopback probe: target pid={pid} "
|
||||
+ $"({(pid == Environment.ProcessId ? "SELF (silent)" : SafeName(pid))}), IsSupported={ProcessLoopbackCapture.IsSupported}");
|
||||
|
||||
var sw = Stopwatch.StartNew();
|
||||
long bytes = 0;
|
||||
Exception? stopError = null;
|
||||
var stopped = new ManualResetEventSlim(false);
|
||||
var capture = new ProcessLoopbackCapture(pid);
|
||||
capture.DataAvailable += (_, e) => Interlocked.Add(ref bytes, e.BytesRecorded);
|
||||
capture.RecordingStopped += (_, e) => { stopError = e.Exception; stopped.Set(); };
|
||||
|
||||
Line("starting capture...");
|
||||
capture.StartRecording();
|
||||
|
||||
// Give activation up to 5s to complete or fail, then observe ~2s of data flow.
|
||||
var settled = stopped.Wait(5000);
|
||||
var afterActivate = sw.ElapsedMilliseconds;
|
||||
if (!settled) Thread.Sleep(2000); // capture is live — let some audio flow
|
||||
var seen = Interlocked.Read(ref bytes);
|
||||
|
||||
Line($"--- after {afterActivate}ms: stopped={settled}, bytesCaptured={seen}, stopError={stopError?.GetType().Name}: {stopError?.Message}");
|
||||
|
||||
var disposeSw = Stopwatch.StartNew();
|
||||
capture.Dispose();
|
||||
disposeSw.Stop();
|
||||
Line($"dispose took {disposeSw.ElapsedMilliseconds}ms (~2000ms = capture thread was still stuck in activation)");
|
||||
Line(seen > 0 ? "RESULT: capture DELIVERED audio" : "RESULT: NO audio captured (activation failed)");
|
||||
sw0.Dispose();
|
||||
return 0;
|
||||
}
|
||||
|
||||
private static string SafeName(int pid)
|
||||
{
|
||||
try { return Process.GetProcessById(pid).ProcessName; } catch { return "unknown"; }
|
||||
}
|
||||
}
|
||||
@@ -22,7 +22,7 @@ internal static class Program
|
||||
/// never reached.</summary>
|
||||
internal static bool IsServiceInvocation(string[] args) =>
|
||||
HasArg(args, ServiceControl.RunVerb) || HasArg(args, ServiceControl.InstallVerb)
|
||||
|| HasArg(args, ServiceControl.UninstallVerb) || HasArg(args, ServiceControl.UpdateVerb)
|
||||
|| HasArg(args, ServiceControl.UninstallVerb)
|
||||
|| HasArg(args, ServiceControl.StartVerb) || HasArg(args, ServiceControl.StopVerb);
|
||||
|
||||
// Writes an otherwise-fatal exception to a timestamped crash file in the logs folder, so a
|
||||
@@ -87,14 +87,6 @@ internal static class Program
|
||||
return;
|
||||
}
|
||||
|
||||
// --probe-apploopback [pid]: temporary focused diagnostic for the per-app-send activation hang.
|
||||
// Runs the process-loopback activation with full tracing and exits. Not part of the gate.
|
||||
if (args.Length > 0 && Array.Exists(args, a => string.Equals(a, "--probe-apploopback", StringComparison.OrdinalIgnoreCase)))
|
||||
{
|
||||
Environment.ExitCode = ProbeAppLoopback.Run(args);
|
||||
return;
|
||||
}
|
||||
|
||||
// --config-dir <folder> (test / portable isolation): redirect ALL user state - config,
|
||||
// profiles, logs, cue sounds - to an explicit folder for THIS process only. Applied first,
|
||||
// before the layout migration and sound consolidation below read or write the default
|
||||
|
||||
@@ -760,15 +760,18 @@ internal static class SelfTest
|
||||
Check(createArgs.Contains("\\\"" + ServiceStore.BinExePath + "\\\" " + ServiceControl.RunVerb),
|
||||
"the create command must register the ProgramData bin exe as the service binary");
|
||||
|
||||
// 2. AddUserStartStopAce inserts the AU start/stop ACE into the DACL, ahead of the SACL, and is idempotent.
|
||||
// 2. AddUserStartStopAce inserts the user's start/stop ACE into the DACL, ahead of the SACL, idempotently.
|
||||
const string sample = "D:(A;;CCLCSWRPWPDTLOCRRC;;;SY)(A;;CCLCSWLOCRRC;;;IU)S:(AU;FA;CCDCLCSWRPWPDTLOCRSDRCWDWO;;;WD)";
|
||||
var amended = ServiceControl.AddUserStartStopAce(sample);
|
||||
Check(amended is not null && amended.Contains(ServiceControl.UserStartStopAce), "the AU start/stop ACE must be added");
|
||||
Check(amended!.IndexOf(ServiceControl.UserStartStopAce, StringComparison.Ordinal) < amended.IndexOf("S:", StringComparison.Ordinal),
|
||||
const string sid = "S-1-5-21-111-222-333-1001"; // a specific user SID (the installing user, scoped grant)
|
||||
var ace = ServiceControl.UserStartStopAceFor(sid);
|
||||
var amended = ServiceControl.AddUserStartStopAce(sample, sid);
|
||||
Check(amended is not null && amended.Contains(ace), "the user's start/stop ACE must be added");
|
||||
Check(amended!.IndexOf(ace, StringComparison.Ordinal) < amended.IndexOf("S:", StringComparison.Ordinal),
|
||||
"the ACE must sit inside the DACL, before the SACL");
|
||||
Check(amended.StartsWith("D:", StringComparison.Ordinal), "the result must still be a valid DACL-first SDDL");
|
||||
Check(ServiceControl.AddUserStartStopAce(amended) == amended, "adding the ACE twice must be a no-op (idempotent)");
|
||||
Check(ServiceControl.AddUserStartStopAce("garbage") is null, "a non-DACL SDDL must be rejected");
|
||||
Check(!amended.Contains(";;;AU)"), "the grant must be scoped to the specific user SID, not Authenticated Users");
|
||||
Check(ServiceControl.AddUserStartStopAce(amended, sid) == amended, "adding the ACE twice must be a no-op (idempotent)");
|
||||
Check(ServiceControl.AddUserStartStopAce("garbage", sid) is null, "a non-DACL SDDL must be rejected");
|
||||
|
||||
// 2b. The app-source path (which the SYSTEM service watches for auto-updates) round-trips, and drives
|
||||
// the update check: unknown/empty source => no update, so the service never acts on uncertainty.
|
||||
@@ -811,7 +814,7 @@ internal static class SelfTest
|
||||
Check(File.Exists(Path.Combine(dst, "default sounds", "connect.wav")), "bundled default sounds must be copied");
|
||||
Check(!Directory.Exists(Path.Combine(dst, "user settings and logs")), "user settings/logs must NOT be copied");
|
||||
Check(!Directory.Exists(Path.Combine(dst, "logs")), "stray logs folder must NOT be copied");
|
||||
return "runs from own ProgramData bin; AU start/stop ACE added idempotently; program copy excludes user state";
|
||||
return "runs from own ProgramData bin; user-scoped start/stop ACE added idempotently; program copy excludes user state";
|
||||
}
|
||||
finally { try { Directory.Delete(root, recursive: true); } catch { /* temp */ } }
|
||||
}
|
||||
@@ -1417,7 +1420,7 @@ internal static class SelfTest
|
||||
foreach (var verb in new[]
|
||||
{
|
||||
ServiceControl.RunVerb, ServiceControl.InstallVerb, ServiceControl.UninstallVerb,
|
||||
ServiceControl.UpdateVerb, ServiceControl.StartVerb, ServiceControl.StopVerb,
|
||||
ServiceControl.StartVerb, ServiceControl.StopVerb,
|
||||
})
|
||||
{
|
||||
Check(Program.IsServiceInvocation(new[] { verb }), $"'{verb}' must be recognised as a service invocation");
|
||||
@@ -1435,7 +1438,7 @@ internal static class SelfTest
|
||||
if (!loadedBefore)
|
||||
Check(!IsAssemblyLoaded(svcAsm), "deciding a normal launch must not load the Windows-service assembly");
|
||||
|
||||
return "normal launches stay load-safe; all six service verbs recognised (case-insensitive)";
|
||||
return "normal launches stay load-safe; all five service verbs recognised (case-insensitive)";
|
||||
}
|
||||
|
||||
/// <summary>The Service menu's "View service log" opens the newest diagnostic log — the log that says
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
using System.ComponentModel;
|
||||
using System.Diagnostics;
|
||||
using System.IO;
|
||||
using System.Security.Principal;
|
||||
using System.ServiceProcess;
|
||||
using RemSound.Core;
|
||||
|
||||
@@ -33,7 +34,6 @@ public static class ServiceControl
|
||||
/// the Program.cs dispatcher agree.</summary>
|
||||
public const string InstallVerb = "--install-service";
|
||||
public const string UninstallVerb = "--uninstall-service";
|
||||
public const string UpdateVerb = "--update-service";
|
||||
public const string StartVerb = "--start-service";
|
||||
public const string StopVerb = "--stop-service";
|
||||
public const string RunVerb = "--run-service";
|
||||
@@ -129,20 +129,31 @@ public static class ServiceControl
|
||||
return 0;
|
||||
}
|
||||
|
||||
/// <summary>Grant Authenticated Users Modify rights on the service's bin folder (via icacls), so a
|
||||
/// <summary>The SID to grant the no-admin service rights to: the account that installed it (the
|
||||
/// elevated install runs as the same interactive user with an elevated token, so its SID is that user).
|
||||
/// Scoping the grants to ONE account instead of all Users/Authenticated-Users keeps the effortless
|
||||
/// stop/update workflow for that user while removing the "any account on this PC could replace a
|
||||
/// SYSTEM-run binary" escalation surface. Falls back to BUILTIN\Users only if the SID can't be read.</summary>
|
||||
private static string InstallingUserSid()
|
||||
{
|
||||
try { return WindowsIdentity.GetCurrent().User?.Value ?? "S-1-5-32-545"; }
|
||||
catch { return "S-1-5-32-545"; }
|
||||
}
|
||||
|
||||
/// <summary>Grant the installing user Modify rights on the service's bin folder (via icacls), so a
|
||||
/// stopped service's binaries can be refreshed without administrator rights. Best-effort.</summary>
|
||||
private static void GrantUsersWriteToBin()
|
||||
{
|
||||
try
|
||||
{
|
||||
// *S-1-5-32-545 = BUILTIN\Users (locale-independent) — the group the interactive user is in.
|
||||
// (OI)(CI) = inherit to files + subfolders; (M) = Modify. /T applies to the existing contents too
|
||||
// (the bin was just populated), /C keeps going past any single-file error. Capture stderr so a
|
||||
// real failure is logged rather than swallowed.
|
||||
// Grant the installing user (see InstallingUserSid) Modify. (OI)(CI) = inherit to files +
|
||||
// subfolders; (M) = Modify. /T applies to the existing contents too (the bin was just
|
||||
// populated), /C keeps going past any single-file error. Capture stderr so a real failure is
|
||||
// logged rather than swallowed.
|
||||
var psi = new ProcessStartInfo
|
||||
{
|
||||
FileName = "icacls.exe",
|
||||
Arguments = $"\"{ServiceStore.BinDirectory}\" /grant \"*S-1-5-32-545:(OI)(CI)(M)\" /T /C",
|
||||
Arguments = $"\"{ServiceStore.BinDirectory}\" /grant \"*{InstallingUserSid()}:(OI)(CI)(M)\" /T /C",
|
||||
UseShellExecute = false,
|
||||
CreateNoWindow = true,
|
||||
RedirectStandardOutput = true,
|
||||
@@ -195,7 +206,7 @@ public static class ServiceControl
|
||||
try
|
||||
{
|
||||
var sddl = RunScCapture($"sdshow {ServiceName}").Trim();
|
||||
var newSddl = AddUserStartStopAce(sddl);
|
||||
var newSddl = AddUserStartStopAce(sddl, InstallingUserSid());
|
||||
if (newSddl is null || string.Equals(newSddl, sddl, StringComparison.Ordinal)) return;
|
||||
var rc = RunSc($"sdset {ServiceName} {newSddl}");
|
||||
if (rc != 0) ServiceStore.AppendServiceEvent($"install: sdset (user start/stop) returned {rc}");
|
||||
@@ -203,21 +214,22 @@ public static class ServiceControl
|
||||
catch (Exception ex) { ServiceStore.AppendServiceEvent($"install: grant user start/stop failed: {ex.GetType().Name}: {ex.Message}"); }
|
||||
}
|
||||
|
||||
/// <summary>The ACE granting Authenticated Users start (RP) + stop (WP) + query status (LC) + read
|
||||
/// control (RC). Public-ish for the self-test.</summary>
|
||||
internal const string UserStartStopAce = "(A;;RPWPLCRC;;;AU)";
|
||||
/// <summary>The ACE granting <paramref name="sid"/> start (RP) + stop (WP) + query status (LC) + read
|
||||
/// control (RC) on the service.</summary>
|
||||
internal static string UserStartStopAceFor(string sid) => $"(A;;RPWPLCRC;;;{sid})";
|
||||
|
||||
/// <summary>Pure, testable: insert <see cref="UserStartStopAce"/> into a service SDDL's DACL (right
|
||||
/// after "D:" and any DACL flags, ahead of the first ACE and the SACL). Returns null for an SDDL that
|
||||
/// doesn't start with a DACL, and the input unchanged if the ACE is already present.</summary>
|
||||
internal static string? AddUserStartStopAce(string? sddl)
|
||||
/// <summary>Pure, testable: insert the start/stop ACE for <paramref name="sid"/> into a service SDDL's
|
||||
/// DACL (right after "D:" and any DACL flags, ahead of the first ACE and the SACL). Returns null for an
|
||||
/// SDDL that doesn't start with a DACL, and the input unchanged if the ACE is already present.</summary>
|
||||
internal static string? AddUserStartStopAce(string? sddl, string sid)
|
||||
{
|
||||
if (string.IsNullOrEmpty(sddl) || !sddl.StartsWith("D:", StringComparison.Ordinal)) return null;
|
||||
if (sddl.Contains(UserStartStopAce, StringComparison.OrdinalIgnoreCase)) return sddl; // already granted
|
||||
var ace = UserStartStopAceFor(sid);
|
||||
if (sddl.Contains(ace, StringComparison.OrdinalIgnoreCase)) return sddl; // already granted
|
||||
var firstAce = sddl.IndexOf('(');
|
||||
var sacl = sddl.IndexOf("S:", StringComparison.Ordinal);
|
||||
var insertAt = firstAce >= 0 && (sacl < 0 || firstAce < sacl) ? firstAce : (sacl >= 0 ? sacl : sddl.Length);
|
||||
return sddl.Insert(insertAt, UserStartStopAce);
|
||||
return sddl.Insert(insertAt, ace);
|
||||
}
|
||||
|
||||
/// <summary>The sc.exe "failure" args that make the service auto-restart on a crash. Pure, so a
|
||||
@@ -239,30 +251,6 @@ public static class ServiceControl
|
||||
return rc;
|
||||
}
|
||||
|
||||
/// <summary>Refreshes the service's OWN copy of the program with the CURRENTLY-running build, without
|
||||
/// an uninstall/reinstall: stop → overwrite the binaries in <see cref="ServiceStore.BinDirectory"/>
|
||||
/// from this exe's folder → start. Must be run elevated (writing into the admin-only service bin
|
||||
/// folder, and stop/start). Returns 0 on success. Used by the Service menu's "Update service" so a new
|
||||
/// build reaches the service in one UAC prompt.</summary>
|
||||
public static int DoUpdate()
|
||||
{
|
||||
if (!IsInstalled()) return DoInstall(); // not installed yet — a plain install does the copy too
|
||||
var exe = Environment.ProcessPath;
|
||||
var sourceDir = string.IsNullOrEmpty(exe) ? null : Path.GetDirectoryName(exe);
|
||||
if (string.IsNullOrEmpty(sourceDir)) return 2;
|
||||
|
||||
try { DoStop(); } catch { /* best-effort — copy may still fail if files stay locked, handled below */ }
|
||||
try { CopyProgramTo(sourceDir, ServiceStore.BinDirectory); }
|
||||
catch (Exception ex)
|
||||
{
|
||||
ServiceStore.AppendServiceEvent($"update: copy program failed: {ex.GetType().Name}: {ex.Message}");
|
||||
try { DoStart(); } catch { /* leave it stopped rather than half-updated */ }
|
||||
return 5;
|
||||
}
|
||||
ServiceStore.SaveAppSourcePath(sourceDir); // keep the auto-update watch pointed at the current app
|
||||
return DoStart();
|
||||
}
|
||||
|
||||
/// <summary>Starts the service. Must be run elevated. Returns 0 on success.</summary>
|
||||
public static int DoStart()
|
||||
{
|
||||
|
||||
@@ -36,7 +36,6 @@ internal static class ServiceEntry
|
||||
|
||||
var verb = Has(args, ServiceControl.InstallVerb) ? "install"
|
||||
: Has(args, ServiceControl.UninstallVerb) ? "uninstall"
|
||||
: Has(args, ServiceControl.UpdateVerb) ? "update"
|
||||
: Has(args, ServiceControl.StartVerb) ? "start"
|
||||
: Has(args, ServiceControl.StopVerb) ? "stop"
|
||||
: null;
|
||||
@@ -53,7 +52,6 @@ internal static class ServiceEntry
|
||||
{
|
||||
"install" => ServiceControl.DoInstall(),
|
||||
"uninstall" => ServiceControl.DoUninstall(),
|
||||
"update" => ServiceControl.DoUpdate(),
|
||||
"start" => ServiceControl.DoStart(),
|
||||
"stop" => ServiceControl.DoStop(),
|
||||
_ => 0,
|
||||
|
||||
Reference in New Issue
Block a user