fix: address CodeQL findings (file service + VNC secret at rest)#11
Merged
Conversation
- FileService: on error, log the detail server-side and return a generic text/plain body instead of echoing the exception message (cs/web/xss in context + no info leak). Strip CR/LF from audited paths (cs/log-forging). - VncProvisioningService: encrypt vnc.secret at rest with DPAPI (machine scope; the agent runs as SYSTEM), migrating legacy plaintext files transparently (cs/cleartext-storage). Remaining CodeQL alerts are by design and dismissed separately: full-filesystem path access in the authenticated, token-gated, loopback file manager; and the fixed-key DES of the VNC password interop format. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThree security-focused changes in ChangesFileService Security Hardening
VNC DPAPI Encryption
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/RemoteAgent/Services/VncProvisioningService.cs (1)
107-117: ⚡ Quick winPrefer the shared
RemoteAgent.Security.Dpapiwrapper instead of directProtectedDatacalls.This keeps crypto usage consistent with the existing repo abstraction and prevents drift in scope/entropy handling across files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/RemoteAgent/Services/VncProvisioningService.cs` around lines 107 - 117, Replace the direct ProtectedData.Unprotect call in the try block and the ProtectedData.Protect call in the WriteSecret method with the shared RemoteAgent.Security.Dpapi wrapper instead. This ensures consistent crypto handling across the codebase and prevents potential drift in scope and entropy handling. Use the appropriate Dpapi wrapper methods for both the unprotect operation and the protect operation in WriteSecret.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/RemoteAgent/Services/VncProvisioningService.cs`:
- Around line 107-113: The ReadSecret method currently treats any
ProtectedData.Unprotect failure as legacy plaintext, which can silently corrupt
encrypted blobs that are tampered or corrupted by decoding them as text and
re-writing them. Instead of catching all exceptions from Unprotect, validate
that the raw data actually represents valid plaintext before treating it as
legacy; consider checking if the data decodes to reasonable text content or if
there are specific indicators that distinguish legitimate legacy plaintext from
corrupted encrypted data, and only fall back to plaintext handling when you are
confident the data is not a corrupted encrypted blob.
---
Nitpick comments:
In `@src/RemoteAgent/Services/VncProvisioningService.cs`:
- Around line 107-117: Replace the direct ProtectedData.Unprotect call in the
try block and the ProtectedData.Protect call in the WriteSecret method with the
shared RemoteAgent.Security.Dpapi wrapper instead. This ensures consistent
crypto handling across the codebase and prevents potential drift in scope and
entropy handling. Use the appropriate Dpapi wrapper methods for both the
unprotect operation and the protect operation in WriteSecret.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9721e5f-6142-4cdd-8e1f-61878267b60a
📒 Files selected for processing (3)
src/RemoteAgent/Files/FileService.cssrc/RemoteAgent/RemoteAgent.csprojsrc/RemoteAgent/Services/VncProvisioningService.cs
Comment on lines
+107
to
+113
| try { return Encoding.UTF8.GetString(ProtectedData.Unprotect(raw, null, DataProtectionScope.LocalMachine)).Trim(); } | ||
| catch | ||
| { | ||
| var plain = Encoding.UTF8.GetString(raw).Trim(); // legacy plaintext | ||
| try { WriteSecret(file, plain); } catch { /* best-effort migration */ } | ||
| return plain; | ||
| } |
There was a problem hiding this comment.
Differentiate legacy plaintext from decryption failure to avoid silent secret corruption.
ReadSecret currently treats any Unprotect failure as legacy plaintext. If an encrypted blob is corrupted/tampered, it gets decoded as text, re-written, and used as the active password. That can silently rotate to garbage and break the persisted secret contract.
Proposed hardening
+ private static ReadOnlySpan<byte> SecretHeader => "DPAPI1:"u8;
+
private static string? ReadSecret(string file)
{
if (!File.Exists(file)) return null;
var raw = File.ReadAllBytes(file);
if (raw.Length == 0) return null;
- try { return Encoding.UTF8.GetString(ProtectedData.Unprotect(raw, null, DataProtectionScope.LocalMachine)).Trim(); }
- catch
+ // New encrypted format: header + DPAPI blob
+ if (raw.AsSpan().StartsWith(SecretHeader))
{
- var plain = Encoding.UTF8.GetString(raw).Trim(); // legacy plaintext
- try { WriteSecret(file, plain); } catch { /* best-effort migration */ }
- return plain;
+ var blob = raw.AsSpan(SecretHeader.Length).ToArray();
+ return Encoding.UTF8.GetString(
+ ProtectedData.Unprotect(blob, null, DataProtectionScope.LocalMachine)).Trim();
}
+
+ // Legacy plaintext migration path only.
+ var plain = Encoding.UTF8.GetString(raw).Trim();
+ try { WriteSecret(file, plain); } catch { /* best-effort migration */ }
+ return plain;
}
private static void WriteSecret(string file, string password) =>
- File.WriteAllBytes(file, ProtectedData.Protect(Encoding.UTF8.GetBytes(password), null, DataProtectionScope.LocalMachine));
+ File.WriteAllBytes(
+ file,
+ SecretHeader.ToArray()
+ .Concat(ProtectedData.Protect(Encoding.UTF8.GetBytes(password), null, DataProtectionScope.LocalMachine))
+ .ToArray());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/RemoteAgent/Services/VncProvisioningService.cs` around lines 107 - 113,
The ReadSecret method currently treats any ProtectedData.Unprotect failure as
legacy plaintext, which can silently corrupt encrypted blobs that are tampered
or corrupted by decoding them as text and re-writing them. Instead of catching
all exceptions from Unprotect, validate that the raw data actually represents
valid plaintext before treating it as legacy; consider checking if the data
decodes to reasonable text content or if there are specific indicators that
distinguish legitimate legacy plaintext from corrupted encrypted data, and only
fall back to plaintext handling when you are confident the data is not a
corrupted encrypted blob.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Address CodeQL findings
Triage of the 15 code-scanning alerts: real fixes here, by-design/false-positive ones dismissed with rationale.
Fixed
cs/web/xss+ info-leak (FileService) — on error, log the detail server-side and return a generictext/plainbody instead of echoingex.Message. No request-controlled data is reflected.cs/log-forging(FileService) — strip CR/LF from the request-controlled path before it enters the audit log.cs/cleartext-storage×5 (VncProvisioningService) — encryptvnc.secretat rest with DPAPI (machine scope; the agent runs as SYSTEM), consistent with how the client PFX is protected. Legacy plaintext files migrate transparently on first read, so existing devices keep working.Dismissed (by design / interop)
cs/path-injection×7 (FileService) — it's a file manager: full-filesystem access is the feature, gated by a per-session token + loopback-only binding + the authenticated reverse tunnel + operator grant + consent (token check precedes all path use). No base dir to confine to.cs/ecb-encryption×1 (VncPassword) — VNC fixed-key DES is TightVNC's password interop format, not a protection mechanism; the real boundary is the SSH tunnel + loopback.Agent bumped to 1.7.1. Build only, no rollout yet.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes - v1.7.1.0