spikerj/spikersoft-issues
1
0
Fork 0
Code Issues 196 Pull Requests Actions Packages Projects Releases Wiki Activity

[Security][Go-live] Replace Assembly.Load of uploaded DLLs with metadata-only inspection #406

New Issue
Closed
opened 2026-07-05 20:24:36 +00:00 by spikerj · 5 comments
spikerj commented 2026-07-05 20:24:36 +00:00
Owner
Copy Link
Copy Source

Problem: The decompiler loads attacker-supplied assemblies and calls GetTypes(), which runs the assembly's module initializer = arbitrary code in the worker. Validation runs AFTER the load, so it never gates. Loaded assemblies never unload (memory-growth DoS), and the upload filename is unsanitized (path traversal).

Evidence:

  • SpikerSoft.Workers.Decompile/Services/DecompilationService.cs:48,51,79
  • DllTypeDetectionService.cs:101,240
  • validation after load: UploadDllCommandHandler.cs:34,43,46; unsanitized filename :166-172

Fix: Inspect metadata only via System.Reflection.Metadata (PEReader/MetadataReader) or ICSharpCode PEFile; never Assembly.Load untrusted bytes; validate before any load; sanitize the filename (Path.GetFileName, reject rooted paths, verify the resolved path stays under the intended root).

Acceptance criteria:

  • No Assembly.Load of uploaded content anywhere in the decompile path
  • Type detection / decompile works from metadata only
  • Path traversal via the upload filename is blocked

Effort: M · Related: CR-3 (untrusted-code doctrine), CR-10 (decompiler currently returns stub source).

**Problem:** The decompiler loads attacker-supplied assemblies and calls `GetTypes()`, which runs the assembly's module initializer = arbitrary code in the worker. Validation runs AFTER the load, so it never gates. Loaded assemblies never unload (memory-growth DoS), and the upload filename is unsanitized (path traversal). **Evidence:** - `SpikerSoft.Workers.Decompile/Services/DecompilationService.cs:48,51,79` - `DllTypeDetectionService.cs:101,240` - validation after load: `UploadDllCommandHandler.cs:34,43,46`; unsanitized filename `:166-172` **Fix:** Inspect metadata only via `System.Reflection.Metadata` (`PEReader`/`MetadataReader`) or ICSharpCode `PEFile`; never `Assembly.Load` untrusted bytes; validate before any load; sanitize the filename (`Path.GetFileName`, reject rooted paths, verify the resolved path stays under the intended root). **Acceptance criteria:** - No `Assembly.Load` of uploaded content anywhere in the decompile path - Type detection / decompile works from metadata only - Path traversal via the upload filename is blocked **Effort:** M · Related: CR-3 (untrusted-code doctrine), CR-10 (decompiler currently returns stub source).
spikerj added the agentic label 2026-07-05 20:24:36 +00:00
spikerj commented 2026-07-06 16:05:49 +00:00
Author
Owner
Copy Link
Copy Source

Concrete location + interim mitigation shipped: PR #141.

The untrusted-Assembly.Load this epic is about is live at SpikerSoft.Workers.Decompile/Services/DecompilationService.cs:48 — Assembly.Load(file.GetContentBytes()) on a user-uploaded DLL, reached via POST decompile/upload on DecompileController (class-level [Authorize], so any authenticated user, not just admin). It's the same code as #412's fabricated-decompiler output.

PR #141 removes the Assembly.Load and gates the feature to an honest failure (it was returning fabricated skeleton source anyway), which closes this exposure as a safe, reversible interim. The full ask of this epic — replace with metadata-only inspection — is the natural way to restore the feature: CSharpDecompiler.DecompileWholeModuleAsString over a PEFile reads the assembly's metadata without loading it into the AppDomain, giving real decompiled source and satisfying "metadata-only, no Assembly.Load." That's the follow-up repair; I can implement it on a go-ahead (it's bounded and unit-testable, unlike the verification-gated items).

Any other Assembly.Load/LoadFrom/LoadFile of uploaded content should be swept too — this ticket can track that sweep; #141 handles the decompiler instance.

**Concrete location + interim mitigation shipped: PR #141.** The untrusted-`Assembly.Load` this epic is about is live at **`SpikerSoft.Workers.Decompile/Services/DecompilationService.cs:48`** — `Assembly.Load(file.GetContentBytes())` on a user-uploaded DLL, reached via `POST decompile/upload` on `DecompileController` (class-level `[Authorize]`, so **any authenticated user**, not just admin). It's the *same code* as #412's fabricated-decompiler output. PR #141 removes the `Assembly.Load` and gates the feature to an honest failure (it was returning fabricated skeleton source anyway), which **closes this exposure as a safe, reversible interim**. The full ask of this epic — replace with **metadata-only inspection** — is the natural way to *restore* the feature: `CSharpDecompiler.DecompileWholeModuleAsString` over a `PEFile` reads the assembly's metadata without loading it into the AppDomain, giving real decompiled source **and** satisfying "metadata-only, no `Assembly.Load`." That's the follow-up repair; I can implement it on a go-ahead (it's bounded and unit-testable, unlike the verification-gated items). Any other `Assembly.Load`/`LoadFrom`/`LoadFile` of uploaded content should be swept too — this ticket can track that sweep; #141 handles the decompiler instance.
spikerj commented 2026-07-06 16:35:07 +00:00
Author
Owner
Copy Link
Copy Source

Real metadata-only implementation shipped — PR #143 (supersedes the #141 interim gate for the decompiler path).

Delivered exactly the recipe from my earlier comment: DecompileDllAsync now reads the upload with CSharpDecompiler over a UniversalAssemblyResolver (temp file, deleted in finally) and builds the tree from decompiler.TypeSystem — no Assembly.Load of the untrusted upload anywhere. Per-type source is DecompileTypeAsString (real, not fabricated). API was locked with a throwaway compile+run spike; 65/65 Decompile tests pass.

Remaining #406 scope: the broader sweep for any other Assembly.Load/LoadFrom/LoadFile of uploaded/untrusted content elsewhere in the codebase. The decompiler instance — the one concrete case identified — is now resolved. If the sweep turns up nothing else, this epic can close on #143's merge.

**Real metadata-only implementation shipped — PR #143** (supersedes the #141 interim gate for the decompiler path). Delivered exactly the recipe from my earlier comment: `DecompileDllAsync` now reads the upload with `CSharpDecompiler` over a `UniversalAssemblyResolver` (temp file, deleted in `finally`) and builds the tree from `decompiler.TypeSystem` — **no `Assembly.Load` of the untrusted upload anywhere**. Per-type source is `DecompileTypeAsString` (real, not fabricated). API was locked with a throwaway compile+run spike; 65/65 Decompile tests pass. **Remaining #406 scope:** the broader sweep for any *other* `Assembly.Load`/`LoadFrom`/`LoadFile` of uploaded/untrusted content elsewhere in the codebase. The decompiler instance — the one concrete case identified — is now resolved. If the sweep turns up nothing else, this epic can close on #143's merge.
spikerj commented 2026-07-06 16:42:08 +00:00
Author
Owner
Copy Link
Copy Source

Codebase-wide Assembly.Load* sweep done. Results:

Site Loads untrusted upload? Action
DecompilationService (decompiler) Yes ✅ fixed — PR #143 (metadata-only)
DllTypeDetectionService :101, :240 Yes — same upload flow, run before decompile ✅ fixed — PR #144 (this sweep caught it; #143 missed it)
RoslynCodeExecutor :115, :235 No — executes compiled student code by design Out of scope (sandboxing concern for the code-execution epic, not "inspect an uploaded DLL")
Test-project Assembly.Load (curriculum snippet tests, etc.) No — loads code the tests just compiled N/A

PR #144 converts DllTypeDetectionService to PEReader.HasMetadata + MetadataReader (BCL, no new dep), reading version/runtime/company/product/copyright straight from metadata. 6/6 tests pass.

With #143 + #144 merged, the DLL upload flow no longer Assembly.Loads the untrusted upload anywhere — the entirety of #406's "replace Assembly.Load of uploaded DLLs with metadata-only inspection." Recommend closing this epic once both merge (RoslynCodeExecutor sandboxing, if wanted, belongs on its own code-execution ticket).

**Codebase-wide `Assembly.Load*` sweep done.** Results: | Site | Loads untrusted upload? | Action | |------|------------------------|--------| | `DecompilationService` (decompiler) | Yes | ✅ fixed — PR #143 (metadata-only) | | **`DllTypeDetectionService` :101, :240** | **Yes** — *same upload flow, run before decompile* | ✅ **fixed — PR #144** (this sweep caught it; #143 missed it) | | `RoslynCodeExecutor` :115, :235 | No — executes compiled **student** code by design | Out of scope (sandboxing concern for the code-execution epic, not "inspect an uploaded DLL") | | Test-project `Assembly.Load` (curriculum snippet tests, etc.) | No — loads code the tests just compiled | N/A | PR #144 converts `DllTypeDetectionService` to `PEReader.HasMetadata` + `MetadataReader` (BCL, no new dep), reading version/runtime/company/product/copyright straight from metadata. 6/6 tests pass. **With #143 + #144 merged, the DLL upload flow no longer `Assembly.Load`s the untrusted upload anywhere** — the entirety of #406's "replace `Assembly.Load` of uploaded DLLs with metadata-only inspection." Recommend closing this epic once both merge (RoslynCodeExecutor sandboxing, if wanted, belongs on its own code-execution ticket).
spikerj commented 2026-07-06 16:49:38 +00:00
Author
Owner
Copy Link
Copy Source

Third AC — path traversal — now fixed: PR #146. I'd nearly recommended closing after #144, but re-reading the ACs caught that the filename-traversal criterion (evidence UploadDllCommandHandler.cs:166-172) was still open. SaveToStagingForAuditAsync composed the staging path from the unsanitized client FileName, so ../../.. was an arbitrary-write. #146 adds a unit-tested BuildStagingFileName sanitizer (single-segment, both separators + invalid chars stripped, dot-only fallback) plus a resolved-path-under-root guard. 18 new cases pass.

Acceptance criteria — all three met (pending merges):

  • ✅ No Assembly.Load of uploaded content anywhere in the decompile path — #143 (decompiler) + #144 (type detection)
  • ✅ Type detection / decompile works from metadata only — #143 + Visual Bug: Space Game Text (#144)
  • ✅ Path traversal via the upload filename blocked — #146

Recommend closing this epic once #146 merges (#143/#144 already merged to master). The RoslynCodeExecutor Assembly.Load remains intentionally out of scope (executes compiled student code by design — a code-execution-sandbox concern, worth its own ticket if desired).

**Third AC — path traversal — now fixed: PR #146.** I'd nearly recommended closing after #144, but re-reading the ACs caught that the filename-traversal criterion (evidence `UploadDllCommandHandler.cs:166-172`) was still open. `SaveToStagingForAuditAsync` composed the staging path from the unsanitized client `FileName`, so `../../..` was an arbitrary-write. #146 adds a unit-tested `BuildStagingFileName` sanitizer (single-segment, both separators + invalid chars stripped, dot-only fallback) plus a resolved-path-under-root guard. 18 new cases pass. **Acceptance criteria — all three met (pending merges):** - ✅ No `Assembly.Load` of uploaded content anywhere in the decompile path — #143 (decompiler) + #144 (type detection) - ✅ Type detection / decompile works from metadata only — #143 + #144 - ✅ Path traversal via the upload filename blocked — #146 Recommend closing this epic once #146 merges (#143/#144 already merged to master). The `RoslynCodeExecutor` `Assembly.Load` remains intentionally out of scope (executes compiled student code by design — a code-execution-sandbox concern, worth its own ticket if desired).
spikerj commented 2026-07-06 16:55:52 +00:00
Author
Owner
Copy Link
Copy Source

All three fixing PRs are merged to master — closing.

  • #143 — decompiler reads metadata-only via CSharpDecompiler/MetadataFile; no Assembly.Load, real source instead of stubs.
  • #144 — DllTypeDetectionService reads managed-check + version/company/product/copyright via PEReader.HasMetadata/MetadataReader; no Assembly.Load.
  • #146 — upload filename sanitized (BuildStagingFileName + resolved-path-under-root guard); path traversal blocked.

Acceptance criteria met: ✅ no Assembly.Load of uploaded content in the decompile path · ✅ detection/decompile from metadata only · ✅ filename traversal blocked. Verified by unit tests across all three (decompiler real-source, metadata-only detection, traversal payloads); full SpikerSoft.Tests.Unit green.

Out of scope (by design): RoslynCodeExecutor.Assembly.Load executes compiled student code — a code-execution-sandbox concern; file a separate ticket if hardening there is wanted.

All three fixing PRs are merged to `master` — closing. - **#143** — decompiler reads metadata-only via `CSharpDecompiler`/`MetadataFile`; no `Assembly.Load`, real source instead of stubs. - **#144** — `DllTypeDetectionService` reads managed-check + version/company/product/copyright via `PEReader.HasMetadata`/`MetadataReader`; no `Assembly.Load`. - **#146** — upload filename sanitized (`BuildStagingFileName` + resolved-path-under-root guard); path traversal blocked. Acceptance criteria met: ✅ no `Assembly.Load` of uploaded content in the decompile path · ✅ detection/decompile from metadata only · ✅ filename traversal blocked. Verified by unit tests across all three (decompiler real-source, metadata-only detection, traversal payloads); full `SpikerSoft.Tests.Unit` green. Out of scope (by design): `RoslynCodeExecutor.Assembly.Load` executes compiled *student* code — a code-execution-sandbox concern; file a separate ticket if hardening there is wanted.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
agentic
Actionable surfaced by the agentic architecture & security review
 bug
Something is not working
 duplicate
This issue or pull request already exists
 enhancement
New feature
 help wanted
Need some help
 invalid
Something is wrong
 question
More information is needed
 sonarqube
Code quality issue surfaced by SonarQube (bug / code smell)
 wontfix
This won't be fixed
No labels agentic
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
spikerj (Joseph Spiker)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: spikerj/spikersoft-issues#406