Insecure file-serving pattern: `GetItemPictureById` allows path traversal / arbitrary file read via `PictureFileName'
- Dominant language
- C#
- Stars
- 10.9k
- Forks
- 3.8k
- Avg merge
- 1d 11h
- Merged PRs (30d)
- 4
Description
## Summary
`Catalog.API`'s product-image endpoint builds a filesystem path by concatenating a **database-stored, client-controllable** `PictureFileName` directly into `Path.Combine(..., "Pics", pictureFileName)` and streams the result with `TypedResults.PhysicalFile`, with no containment check. Because `Path.Combine` lets a rooted or `..`-containing final segment escape the `Pics` directory, a caller can make the endpoint return **arbitrary files from the host filesystem** (e.g. `appsettings.json`, project source, `/etc/passwd`).
Since this is a **reference application** that many developers copy patterns from, the file-serving idiom here teaches an insecure practice (CWE-22). The fix is small and self-contained.
## Affected code
`src/Catalog.API/Apis/CatalogApi.cs`
```csharp
// GetItemPictureById (~line 205) — PictureFileName comes from the stored CatalogItem
var path = GetFullPath(environment.ContentRootPath, item.PictureFileName);
...
return TypedResults.PhysicalFile(path, mimetype, lastModified: lastModified);
// ~line 420
public static string GetFullPath(string contentRootPath, string pictureFileName) =>
Path.Combine(contentRootPath, "Pics", pictureFileName); // <-- no normalization / containment
```
`PictureFileName` is bound straight from the request body in `CreateItem`/`UpdateItem` (`product.PictureFileName`), so it is fully attacker-controlled.
## Steps to reproduce
**Observed scope:** any real on-disk file the process can read is returned. (`/proc/*` pseudo-files are *not* returned — `PhysicalFile` reports `Content-Length: 0` for procfs and Kestrel aborts the response — so process environment variables are not exfiltrable through this particular sink.)
## Impact
Unrestricted local file read of the `Catalog.API` host: `appsettings*.json`, mounted secret files, K8s service-account tokens (`/var/run/secrets/...`), data-protection keys, application source, etc. In the default topology `Catalog.API` is an internal service, so realistic exposure requires it to be network-reachable (direct exposure, misconfiguration, or via SSRF/lateral movement) — but the file-read defect itself is unconditional.
## Suggested fix
Constrain the filename to the `Pics` directory. Either reduce it to a bare filename, or verify the resolved path stays inside the base:
```csharp
public static string GetFullPath(string contentRootPath, string pictureFileName)
{
// Reject any directory component supplied by the caller.
var safeName = Path.GetFileName(pictureFileName);
var picsRoot = Path.Combine(contentRootPath, "Pics");
var full = Path.GetFullPath(Path.Combine(picsRoot, safeName));
// Defense in depth: ensure we never escape the Pics directory.
if (!full.StartsWith(picsRoot + Path.DirectorySeparatorChar, StringComparison.Ordinal))
throw new InvalidOperationException("Invalid picture path.");
return full;
}
```
Optionally also validate `PictureFileName` on write (`CreateItem`/`UpdateItem`) to reject path separators and `..`.
## Notes
Related hardening for the same endpoints (lower priority, may be intentional for the sample): the `Catalog.API` write endpoints (`POST/PUT/DELETE /api/catalog/items`) have no authorization, which is what lets the malicious `PictureFileName` be stored. If the reference app intends these to be callable only via an authenticated path, adding `RequireAuthorization` on the write routes would also break the chain. I'm happy to open a PR with the fix above if it's welcome.
Contributor guide
Assessment
This issue has not been assessed yet.