dotnet / dotnet/eShop

Insecure file-serving pattern: `GetItemPictureById` allows path traversal / arbitrary file read via `PictureFileName'

Open
#998 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.