Skip to content

fix: SSMS extension uses a random temp filename#251

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/ssms-random-tempfile
Apr 21, 2026
Merged

fix: SSMS extension uses a random temp filename#251
erikdarlingdata merged 1 commit into
devfrom
fix/ssms-random-tempfile

Conversation

@erikdarlingdata
Copy link
Copy Markdown
Owner

Summary

PlanViewer.Ssms/AppLauncher.cs:SavePlanToTemp was naming its temp file ssms_plan_{yyyyMMdd_HHmmss}.sqlplan, which is trivially predictable by any other process on the machine. Combined with File.WriteAllText (which follows symlinks), a same-privilege attacker could plant a symlink at the expected path and have the extension's write redirect into an arbitrary file.

Fix

  • Name becomes ssms_plan_<random>.sqlplan using Path.GetRandomFileName for the suffix.
  • Open with FileMode.CreateNew + FileShare.None so the write refuses to land if a file already exists at that path.
  • Extension stays .sqlplan so the app still recognizes the file type.

Test plan

  • Local net8 harness (mirrors the same System.IO calls, which behave identically on net472/net8.0) confirms:
    • File created, content round-trips.
    • Two consecutive saves produce different filenames.
    • Filename no longer matches the old yyyyMMdd_HHmmss pattern.
    • FileMode.CreateNew throws IOException on a pre-existing file and the pre-existing content is preserved (the symlink-plant defense).
  • CI builds the actual VSIX (local dotnet build can't — net472 targeting pack isn't installed on this machine).

🤖 Generated with Claude Code

Swap the predictable `ssms_plan_{yyyyMMdd_HHmmss}.sqlplan` name for a random
suffix produced by Path.GetRandomFileName, and open with FileMode.CreateNew
so we refuse to overwrite a pre-existing file. Closes the race where another
local process could predict the filename and plant a symlink at that path
before the extension's File.WriteAllText landed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@erikdarlingdata erikdarlingdata merged commit fabf59d into dev Apr 21, 2026
2 checks passed
@erikdarlingdata erikdarlingdata deleted the fix/ssms-random-tempfile branch April 21, 2026 22:11
Copy link
Copy Markdown
Owner Author

Post-merge review (PR was merged ~7s after open, so inline comments would be noise — summary only).

What it does
PlanViewer.Ssms/AppLauncher.cs:28-39 replaces the predictable ssms_plan_{yyyyMMdd_HHmmss}.sqlplan name with a Path.GetRandomFileName-derived suffix, and swaps File.WriteAllText for FileStream(FileMode.CreateNew, FileShare.None) + StreamWriter. Closes a same-privilege symlink-plant / TOCTOU vector on the temp path.

Good

  • Targets dev. ✓
  • Path.GetRandomFileName() is documented cryptographically strong — the "cryptographically-random suffix" comment is accurate.
  • FileMode.CreateNew + FileShare.None is the right pair: atomic refuse-if-exists + no concurrent writers. If anything lands at the random path first (symlink or real file), the call throws and the plan is not leaked.
  • using nesting is correct — StreamWriter owns the FileStream and will flush+dispose in order.
  • Scope is surgical: one function, no collateral churn.

Needs attention

  • No committed test. The PR description references a "local net8 harness" but nothing lands in tests/. There's no PlanViewer.Ssms.Tests project, and tests/PlanViewer.Core.Tests/ doesn't cover AppLauncher. Since this is the security-critical surface, worth extracting the temp-file logic to something testable (or adding a net8-targeted test project that references the file directly) and asserting the CreateNew throw-on-existing behavior — otherwise a future refactor can silently regress the fix.
  • Doc-comment framing, AppLauncher.cs:25-26. "FileMode.CreateNew … closing the race further" understates it — CreateNew is the primary defense against the symlink attack. The random name narrows the attack window; CreateNew is what makes the write itself safe. Minor wording nit.
  • Encoding, AppLauncher.cs:34. new StreamWriter(fs) on net472 defaults to UTF-8-no-BOM, which matches the old File.WriteAllText(string, string) behavior, so no regression in practice. Being explicit (new StreamWriter(fs, new UTF8Encoding(false))) would make that invariant resistant to framework-default drift given .sqlplan is XML consumed downstream.
  • CI gap acknowledged in the PR description. Local dotnet build can't target net472; only CI actually builds the VSIX. Worth confirming the VSIX build on dev is green before cutting the next SSMS extension release.

No Avalonia / brush / PlanAnalyzer-sync concerns — this PR doesn't touch UI, theming, or analysis rules.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant