From b8d7d2e124b5dbf8729db57722decbfb29ec0592 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Fri, 22 May 2026 21:49:08 +0000 Subject: [PATCH 1/2] fix(security): address open CodeQL alerts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - workflows: add `permissions: contents: read` to website-rebuild.yml so the rebuild job no longer inherits the repo's default GITHUB_TOKEN scope (cs-actions/missing-workflow-permissions, alert #21). - webhooks: sanitize the raw route value via SanitizeWebhookId before logging the route_not_found case — ASP.NET URL-decodes path segments, so a caller could otherwise inject CR/LF into log lines (cs/log-forging, alert #22). - lifecycle: add a SanitizeReason helper to DaemonLifecycleNotifier that strips control chars and caps length at 200, and apply it in NotifyShutdown (HTTP-tainted via ShutdownDaemonRequest.Reason) and NotifyCrashing (cs/log-forging, alert #18). - reminders: harden ReminderDefinitionStore.GetPath with an explicit base-directory containment check on top of the existing Uri.EscapeDataString encoding, and add a regression test covering traversal, backslash, and absolute-path ids (cs/path-injection, alert #17). --- .github/workflows/website-rebuild.yml | 3 ++ .../Reminders/ReminderDefinitionStoreTests.cs | 16 +++++++ .../Reminders/ReminderDefinitionStore.cs | 16 ++++++- .../Services/DaemonLifecycleNotifier.cs | 48 ++++++++++++++++--- .../WebhookEndpointRouteBuilderExtensions.cs | 5 +- 5 files changed, 80 insertions(+), 8 deletions(-) diff --git a/.github/workflows/website-rebuild.yml b/.github/workflows/website-rebuild.yml index 7de2f6fae..6c9a45b06 100644 --- a/.github/workflows/website-rebuild.yml +++ b/.github/workflows/website-rebuild.yml @@ -12,6 +12,9 @@ on: types: [published] workflow_dispatch: +permissions: + contents: read + jobs: rebuild: runs-on: ubuntu-latest diff --git a/src/Netclaw.Actors.Tests/Reminders/ReminderDefinitionStoreTests.cs b/src/Netclaw.Actors.Tests/Reminders/ReminderDefinitionStoreTests.cs index 35ba2e976..0a6b6728f 100644 --- a/src/Netclaw.Actors.Tests/Reminders/ReminderDefinitionStoreTests.cs +++ b/src/Netclaw.Actors.Tests/Reminders/ReminderDefinitionStoreTests.cs @@ -210,6 +210,22 @@ public void ReminderDefinition_id_serializes_as_bare_json_string() Assert.Equal(new ReminderId("reminder-byte-eq"), loaded!.Id); } + [Theory] + [InlineData("../../etc/passwd")] + [InlineData("..\\..\\windows\\system32\\config")] + [InlineData("/etc/passwd")] + [InlineData("C:\\Windows\\System32")] + public void Exists_with_traversal_id_does_not_escape_reminders_directory(string maliciousId) + { + var store = new ReminderDefinitionStore(_paths); + + // Uri.EscapeDataString neutralizes path separators, so the canonical + // path stays inside _basePath and Exists() simply returns false. The + // explicit containment check in GetPath would throw if that invariant + // ever regressed. + Assert.False(store.Exists(new ReminderId(maliciousId))); + } + public void Dispose() { if (Directory.Exists(_basePath)) diff --git a/src/Netclaw.Actors/Reminders/ReminderDefinitionStore.cs b/src/Netclaw.Actors/Reminders/ReminderDefinitionStore.cs index 88ec8a1b6..bbb720463 100644 --- a/src/Netclaw.Actors/Reminders/ReminderDefinitionStore.cs +++ b/src/Netclaw.Actors/Reminders/ReminderDefinitionStore.cs @@ -150,8 +150,22 @@ public bool Delete(ReminderId id) private string GetPath(ReminderId id) { + // Uri.EscapeDataString escapes path separators and absolute-path + // markers (/, \, :, control chars), so the encoded value cannot encode a + // traversal. The post-canonicalization containment check below is the + // belt-and-suspenders that CodeQL also recognizes as a path sanitizer. + // (cs/path-injection) var encoded = Uri.EscapeDataString(id.Value); - return Path.Combine(_directory, $"{encoded}.json"); + var baseDir = Path.GetFullPath(_directory); + var candidate = Path.GetFullPath(Path.Combine(baseDir, $"{encoded}.json")); + var baseWithSep = baseDir.EndsWith(Path.DirectorySeparatorChar) + ? baseDir + : baseDir + Path.DirectorySeparatorChar; + if (!candidate.StartsWith(baseWithSep, StringComparison.Ordinal)) + throw new ArgumentException( + $"Reminder id '{id.Value}' resolves outside the reminders directory.", + nameof(id)); + return candidate; } private void PruneInvalidDefinitions() diff --git a/src/Netclaw.Daemon/Services/DaemonLifecycleNotifier.cs b/src/Netclaw.Daemon/Services/DaemonLifecycleNotifier.cs index 5dcd26554..3fcbcf7ea 100644 --- a/src/Netclaw.Daemon/Services/DaemonLifecycleNotifier.cs +++ b/src/Netclaw.Daemon/Services/DaemonLifecycleNotifier.cs @@ -4,6 +4,7 @@ // // ----------------------------------------------------------------------- using System.Globalization; +using System.Text; using Microsoft.Extensions.Logging; using Netclaw.Configuration; @@ -60,12 +61,17 @@ public void NotifyStarted() public void NotifyShutdown(string reason, IReadOnlyDictionary? additionalContext = null) { var pid = Environment.ProcessId; - _logger.LogInformation("Netclaw daemon stopping (PID {Pid}, reason: {Reason})", pid, reason); + // `reason` reaches this method straight from the HTTP query string + // (ShutdownDaemonRequest in LifecycleEndpointRouteBuilderExtensions), so a + // caller can inject CR/LF into log lines. Sanitize before logging or + // propagating into the alert context. (cs/log-forging) + var safeReason = SanitizeReason(reason); + _logger.LogInformation("Netclaw daemon stopping (PID {Pid}, reason: {Reason})", pid, safeReason); var context = new Dictionary { ["pid"] = pid.ToString(CultureInfo.InvariantCulture), - ["reason"] = reason, + ["reason"] = safeReason, }; if (additionalContext is not null) @@ -78,7 +84,7 @@ public void NotifyShutdown(string reason, IReadOnlyDictionary? a _timeProvider, type: "daemon.stopping", category: AlertType.DaemonStopping, - summary: $"Netclaw daemon stopping: {reason}", + summary: $"Netclaw daemon stopping: {safeReason}", severity: AlertSeverity.Info, source: pid.ToString(CultureInfo.InvariantCulture), context: context)); @@ -99,18 +105,22 @@ public void NotifyCrashing( var exceptionMessage = string.IsNullOrWhiteSpace(exception.Message) ? "(no exception message)" : exception.Message; + // Today every caller passes a code-side constant for `reason`, but + // sanitizing here keeps the contract symmetric with NotifyShutdown and + // forecloses on future callers that might forward HTTP input. + var safeReason = SanitizeReason(reason); _logger.LogCritical( exception, "Netclaw daemon crashing (PID {Pid}, reason: {Reason}, exceptionType: {ExceptionType})", pid, - reason, + safeReason, exceptionType); var context = new Dictionary(StringComparer.Ordinal) { ["pid"] = pid.ToString(CultureInfo.InvariantCulture), - ["reason"] = reason, + ["reason"] = safeReason, ["exceptionType"] = exceptionType, ["exceptionMessage"] = Trim(exceptionMessage, 400), }; @@ -130,7 +140,7 @@ public void NotifyCrashing( _timeProvider, type: "daemon.crashing", category: AlertType.DaemonCrashed, - summary: $"Netclaw daemon crashing: {reason} ({exceptionType})", + summary: $"Netclaw daemon crashing: {safeReason} ({exceptionType})", severity: AlertSeverity.Critical, source: pid.ToString(CultureInfo.InvariantCulture), context: context)); @@ -148,4 +158,30 @@ private static string Trim(string value, int maxChars) return value[..maxChars]; } + + private const int MaxReasonChars = 200; + + private static string SanitizeReason(string reason) + { + if (string.IsNullOrEmpty(reason)) + return string.Empty; + + var hasControl = false; + foreach (var ch in reason) + { + if (char.IsControl(ch)) + { + hasControl = true; + break; + } + } + + if (!hasControl) + return Trim(reason, MaxReasonChars); + + var buf = new StringBuilder(reason.Length); + foreach (var ch in reason) + buf.Append(char.IsControl(ch) ? ' ' : ch); + return Trim(buf.ToString(), MaxReasonChars); + } } diff --git a/src/Netclaw.Daemon/Webhooks/WebhookEndpointRouteBuilderExtensions.cs b/src/Netclaw.Daemon/Webhooks/WebhookEndpointRouteBuilderExtensions.cs index 4ef910d1d..da53d62f8 100644 --- a/src/Netclaw.Daemon/Webhooks/WebhookEndpointRouteBuilderExtensions.cs +++ b/src/Netclaw.Daemon/Webhooks/WebhookEndpointRouteBuilderExtensions.cs @@ -44,9 +44,12 @@ public static IEndpointRouteBuilder MapWebhookEndpoints(this IEndpointRouteBuild if (!routeCatalog.TryGetRoute(route, out var registeredRoute)) { WebhookTelemetry.RecordRouteNotFound(route); + // Route comes from the URL path and is URL-decoded by ASP.NET routing, + // so a caller can inject CR/LF and other control chars. Sanitize before + // logging to defeat log forging. (cs/log-forging) logger.LogWarning( "Webhook rejected route={Route} reason={Reason} remote_ip={RemoteIp} delivery_id={DeliveryId} event_type={EventType}", - route, "route_not_found", remoteIp, (string?)null, (string?)null); + SanitizeWebhookId(route), "route_not_found", remoteIp, (string?)null, (string?)null); return TypedResults.NotFound(); } From a9ecd1c1598abec1d430e5363dca6cffd322f0f5 Mon Sep 17 00:00:00 2001 From: Aaron Stannard Date: Fri, 22 May 2026 22:15:41 +0000 Subject: [PATCH 2/2] fix(security): tighten log/telemetry sanitization from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups from the post-CodeQL code review: - webhooks: sanitize `route` once and pass the safe value to both WebhookTelemetry.RecordRouteNotFound and the log line, not just the log. The metric tag was the same log-forging surface and additionally an unbounded high-cardinality vector against the `route` dimension. - lifecycle: widen the SanitizeReason predicate from `char.IsControl` to also strip U+2028 (LINE SEPARATOR) and U+2029 (PARAGRAPH SEPARATOR); these are categories Zl/Zp (not Cc) so IsControl misses them, but JSON- line readers and many log shippers still split on them. - lifecycle: strip sanitized chars rather than replacing them with space, so an attacker-controlled `reason=ok\nlevel=critical` collapses to `reason=oklevel=critical` instead of `reason=ok level=critical` — a space would have been a plausible field separator to key=value log parsers. - lifecycle: when truncating reason at 200 chars, back off one position if the cut would orphan a high surrogate, so downstream UTF-8 encoders don't emit U+FFFD or throw. - tests: cover the four behaviors above with a Theory across CR/LF, NUL, U+2028, U+2029, and a surrogate-pair-boundary truncation case. --- .../Services/DaemonLifecycleNotifierTests.cs | 32 ++++++++++++++ .../Services/DaemonLifecycleNotifier.cs | 42 +++++++++++++++---- .../WebhookEndpointRouteBuilderExtensions.cs | 11 +++-- 3 files changed, 74 insertions(+), 11 deletions(-) diff --git a/src/Netclaw.Daemon.Tests/Services/DaemonLifecycleNotifierTests.cs b/src/Netclaw.Daemon.Tests/Services/DaemonLifecycleNotifierTests.cs index 79899a3b8..c51bdb0fb 100644 --- a/src/Netclaw.Daemon.Tests/Services/DaemonLifecycleNotifierTests.cs +++ b/src/Netclaw.Daemon.Tests/Services/DaemonLifecycleNotifierTests.cs @@ -82,6 +82,38 @@ public void NotifyCrashing_EmitsCriticalAlert() Assert.Equal("C123/171313.123", alert.Context["latest_session_id"]); } + [Theory] + [InlineData("ok\r\nlevel=critical attacker=admin", "oklevel=critical attacker=admin")] + [InlineData("ok\nfake-line", "okfake-line")] + [InlineData("ok\u2028fake-line", "okfake-line")] // U+2028 LINE SEPARATOR + [InlineData("ok\u2029fake-line", "okfake-line")] // U+2029 PARAGRAPH SEPARATOR + [InlineData("ok\0NUL", "okNUL")] + public void NotifyShutdown_strips_log_line_breakers_from_reason(string raw, string expected) + { + _sut.NotifyShutdown(raw); + + var alert = Assert.Single(_sink.Alerts); + Assert.Equal(expected, alert.Context!["reason"]); + Assert.Equal($"Netclaw daemon stopping: {expected}", alert.Summary); + } + + [Fact] + public void NotifyShutdown_truncates_reason_at_200_chars_without_splitting_surrogate_pair() + { + // 99 emoji = 198 UTF-16 chars (2 each), then 1 emoji at positions + // 198–199, then plain ASCII tail. Position 200 would split the 100th + // emoji's surrogate pair if we naively cut at char index 200. + var emoji = "😀"; // U+1F600 GRINNING FACE + var raw = string.Concat(Enumerable.Repeat(emoji, 100)) + "TAIL"; + + _sut.NotifyShutdown(raw); + + var reason = Assert.Single(_sink.Alerts).Context!["reason"]; + Assert.True(reason.Length <= 200); + Assert.False(char.IsHighSurrogate(reason[^1]), + "truncated reason ended on a high surrogate — dangling surrogate would break UTF-8 encoders downstream"); + } + private sealed class RecordingSink : IOperationalNotificationSink { public List Alerts { get; } = []; diff --git a/src/Netclaw.Daemon/Services/DaemonLifecycleNotifier.cs b/src/Netclaw.Daemon/Services/DaemonLifecycleNotifier.cs index 3fcbcf7ea..3faf78a7d 100644 --- a/src/Netclaw.Daemon/Services/DaemonLifecycleNotifier.cs +++ b/src/Netclaw.Daemon/Services/DaemonLifecycleNotifier.cs @@ -161,27 +161,55 @@ private static string Trim(string value, int maxChars) private const int MaxReasonChars = 200; + // Returns true for chars that, if left in a log line or alert summary, can + // act as a line terminator for at least one downstream consumer: + // - char.IsControl covers Cc (CR/LF/NUL/ESC/etc.) + // - U+2028 (LINE SEPARATOR, Zl) and U+2029 (PARAGRAPH SEPARATOR, Zp) are NOT + // in Cc and so are not caught by IsControl, but JSON-line readers, many + // log shippers, and pre-ES2019 JSON parsers still split on them. + private static bool IsLogLineBreak(char c) + => char.IsControl(c) || c is '\u2028' or '\u2029'; + private static string SanitizeReason(string reason) { if (string.IsNullOrEmpty(reason)) return string.Empty; - var hasControl = false; + var hasBreak = false; foreach (var ch in reason) { - if (char.IsControl(ch)) + if (IsLogLineBreak(ch)) { - hasControl = true; + hasBreak = true; break; } } - if (!hasControl) - return Trim(reason, MaxReasonChars); + if (!hasBreak) + return TrimAtCharBoundary(reason, MaxReasonChars); + // Strip — don't space-replace. Space would let a CR/LF payload still + // pass through as a plausible field separator to key=value structured + // log parsers ("reason=ok\nlevel=critical" → "reason=ok level=critical"). var buf = new StringBuilder(reason.Length); foreach (var ch in reason) - buf.Append(char.IsControl(ch) ? ' ' : ch); - return Trim(buf.ToString(), MaxReasonChars); + { + if (!IsLogLineBreak(ch)) + buf.Append(ch); + } + return TrimAtCharBoundary(buf.ToString(), MaxReasonChars); + } + + private static string TrimAtCharBoundary(string value, int maxChars) + { + if (value.Length <= maxChars) + return value; + + // Don't split a surrogate pair at the truncation boundary — a dangling + // high surrogate makes downstream UTF-8 encoders emit U+FFFD or throw. + var cut = maxChars; + if (char.IsHighSurrogate(value[cut - 1])) + cut -= 1; + return value[..cut]; } } diff --git a/src/Netclaw.Daemon/Webhooks/WebhookEndpointRouteBuilderExtensions.cs b/src/Netclaw.Daemon/Webhooks/WebhookEndpointRouteBuilderExtensions.cs index da53d62f8..fef42fe00 100644 --- a/src/Netclaw.Daemon/Webhooks/WebhookEndpointRouteBuilderExtensions.cs +++ b/src/Netclaw.Daemon/Webhooks/WebhookEndpointRouteBuilderExtensions.cs @@ -43,13 +43,16 @@ public static IEndpointRouteBuilder MapWebhookEndpoints(this IEndpointRouteBuild if (!routeCatalog.TryGetRoute(route, out var registeredRoute)) { - WebhookTelemetry.RecordRouteNotFound(route); // Route comes from the URL path and is URL-decoded by ASP.NET routing, - // so a caller can inject CR/LF and other control chars. Sanitize before - // logging to defeat log forging. (cs/log-forging) + // so a caller can inject CR/LF and other control chars — both as a + // log-forging vector and as an unbounded metric-tag cardinality vector + // against the `route` dimension on WebhookTelemetry counters. Sanitize + // once and use the safe value for both surfaces. (cs/log-forging) + var safeRoute = SanitizeWebhookId(route); + WebhookTelemetry.RecordRouteNotFound(safeRoute); logger.LogWarning( "Webhook rejected route={Route} reason={Reason} remote_ip={RemoteIp} delivery_id={DeliveryId} event_type={EventType}", - SanitizeWebhookId(route), "route_not_found", remoteIp, (string?)null, (string?)null); + safeRoute, "route_not_found", remoteIp, (string?)null, (string?)null); return TypedResults.NotFound(); }