dotnet / dotnet/aspnetcore

Use-after-free: ServerErrorHandler retains a borrowed reference to owner-managed response content

Open
#68,342 0 comments 0 reactions 0 assignees View on GitHub
area-networking feature-iis
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 10h
Merged PRs (30d)
281

Description

## Summary

`ServerErrorHandler` stores its response content as a reference to a `std::string` owned by another object (`ServerErrorApplication` or `StartupExceptionApplication`) instead of owning its own copy. If that owning object is destroyed while the handler is still in use, the handler reads from freed memory (use-after-free).

## What is wrong

* `ServerErrorHandler` binds its `m_ExceptionInfoContent` member directly to the caller's `std::string` argument rather than copying or taking ownership of it. The handler has no mechanism to keep the owning object alive for as long as it needs the content.
* The owning application objects (`ServerErrorApplication`, `StartupExceptionApplication`) are independently reference-counted and can be destroyed through normal IIS native-module lifecycle events (for example, application shutdown/recycle or app_offline processing) at any point after a handler has been created from them, including while a handler created earlier is still executing or has response bytes queued with IIS.
* Two other request-handler implementations in the same codebase (in-process and out-of-process request handlers) already solve this correctly by explicitly retaining a reference-counted pin on their owning application object for the handler's lifetime. A third existing handler (`AppOfflineHandler`) solves the same class of problem by copying the content it needs into its own member instead of aliasing the owner. `ServerErrorHandler` does neither.

## Why it matters (defense in depth)

* This is a correctness/memory-safety defect independent of any attacker: any code path that destroys the owning application object while a previously created `ServerErrorHandler` is still alive triggers undefined behavior (stale-but-valid read in the common case, potential crash, and in principle a read of unrelated heap content depending on allocator reuse).
* Fixing it removes an object-lifetime hazard and brings this handler in line with the ownership discipline already established and enforced elsewhere in this codebase for every other request-handler type, closing a gap between "correct by convention" and "correct by construction."

## Affected code

* src/Servers/IIS/AspNetCoreModuleV2/CommonLib/ServerErrorHandler.h:12-19 - constructor takes `std::string& responseContent` and initializes `m_ExceptionInfoContent` as a reference to it
* src/Servers/IIS/AspNetCoreModuleV2/CommonLib/ServerErrorHandler.h:60 - `std::string& m_ExceptionInfoContent;` member declaration; the aliasing root cause
* src/Servers/IIS/AspNetCoreModuleV2/CommonLib/ServerErrorApplication.h:12-27 - owns `std::string m_responseContent` by value and passes it by reference into every `ServerErrorHandler` it constructs (no change expected here once the handler owns its own copy)
* src/Servers/IIS/AspNetCoreModuleV2/InProcessRequestHandler/StartupExceptionApplication.h:13-36 - duplicates the identical borrowed-reference pattern for its own `m_error` content (no change expected here once the handler owns its own copy)
* src/Servers/IIS/AspNetCoreModuleV2/CommonLib/AppOfflineHandler.h:9-24 - existing, unaffected handler in this codebase that already copies owner content into a handler-owned member; useful as the idiom to mirror

## Recommended fix

Change `ServerErrorHandler::m_ExceptionInfoContent` from `std::string&` to an owned `std::string`. Take the constructor parameter by value (`std::string responseContent`) instead of by reference, and initialize the member by moving the parameter into it (`m_ExceptionInfoContent(std::move(responseContent))`) rather than binding a reference. This mirrors the existing `AppOfflineHandler` idiom already present in this codebase. Add `#include ` for `std::move` since it is not directly included today.

Because this constructor is the only place the aliasing occurs, this change is confined to a single header file. Both current call sites (`ServerErrorApplication::CreateHandler` and `StartupExceptionApplication::CreateHandler`) already pass an lvalue `std::string` member into the constructor; once the parameter is by value, that lvalue simply copies into the new by-value parameter at the call boundary instead of binding a reference, so neither call site needs to change. A third call site in the out-of-process forwarding handler constructs `ServerErrorHandler` from a function-local `static std::string` and is likewise unaffected and requires no change, but should still be rebuilt to confirm it compiles against the updated signature.

Alternatives considered:

* Retain the owning application object via the reference-counted "pin" pattern already used by this codebase's in-process and out-of-process request handlers. Rejected as the primary fix: it requires new constructor plumbing and release-on-destroy logic in three classes, and extends the owning object's lifetime until the in-flight response completes - a larger behavioral change than a value copy for content of this size, with no additional correctness benefit.
* Share ownership of the content via a reference-counted string wrapper between the owner and the handler. Rejected: requires changing the member type in both owning classes and every assignment site, for no behavioral benefit over a direct value copy of a small response string.

Compatibility, migration, and versioning: this is an internal C++ object-lifetime change with no public API, configuration, or wire-format surface. It only touches `ServerErrorHandler.h`. No changes are needed or expected to `global.json`, `package.json`/`package-lock.json`, or `NuGet.config`.

## Acceptance criteria

* [ ] The handler's response content is independent of the lifetime of the object that originally supplied it: destroying that object after the handler is created must not change the handler's observed status, substatus, or response bytes.
* [ ] `ServerErrorHandler`'s content member is owned by the handler (by value), not a reference to another object's storage.
* [ ] A focused, deterministic native test exists (no thread-timing dependency) that: constructs the owning application object with a distinctive response string, obtains a handler from it, releases/destroys the owning application object, then executes the handler and asserts the original status/substatus/content bytes are still produced correctly.
* [ ] The existing native test project (`CommonLibTests`) continues to build and its existing tests continue to pass unmodified.
* [ ] `ServerErrorApplication.h` and `StartupExceptionApplication.h` build successfully against the updated constructor signature with no source changes required in either file.

Contributor guide

Open the contributing guide

Research direction

Start with src/Servers/IIS/AspNetCoreModuleV2/CommonLib/ServerErrorHandler.h and compare its content member with AppOfflineHandler.h; then inspect the ServerErrorApplication::CreateHandler and StartupExceptionApplication::CreateHandler call sites. Add a deterministic lifetime-focused native test in CommonLibTests, build the existing native test project, and verify the original status, substatus, and response bytes remain after the owner is destroyed.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.