Case study: storage containment

CNA snapshot 009d40f5  ·  Development › Human Takeover  ·  source links pinned to 009d40f5

This exercise reconstructs the real path-containment fix in CNA commit abaed98b2 and audits what the storage code does in the TARGET snapshot. It teaches how to review a small-looking change whose failure mode is a recursive delete outside the application's own directory. The defect is historical and fixed; no source change is needed for the exercise, and the destructive storage fixture must not be run before its target directory has been checked.

The historical commit and what followed

Commit abaed98b2a5af1dfb4d5de69b76954c4a52cf153 (2026-07-20, “fix(Task REMED-CONTENT-002): fix fs::path containment at 3 sites, sweep repo for more”) is an ancestor of snapshot 009d40f5 and predates the v0.1.0-alpha.1 tag. It fixed three sites that joined caller- or file-supplied strings onto a base with std::filesystem::path::operator/ and no containment check:

• StorageDevice::DeleteContainer(titleName), which recursively deleted root / titleName;
• ContentReader::ReadExternalReference<T>() via ResolveRelativeAssetPath, where an absolute reference in a crafted .xnb/.cnj passed through unchanged despite the method's own documentation promising it would be rejected;
• PlaylistParser::Parse, which accepted absolute and ..-escaping .m3u/.m3u8 entries.

At the time the code lived in the pre-modularization layout (src/Microsoft/Xna/Framework/Storage/StorageDevice.cpp, include/CNA/Internal/PathContainment.hpp). The same files now live in modules/storage/src/StorageDevice.cpp and modules/core/include/CNA/Internal/PathContainment.hpp, and several later commits (all ancestors of the snapshot) changed the code this exercise reviews:

1. Turn a suspicious expression into a concrete invariant

The historical StorageDevice::DeleteContainer(titleName) called fs::remove_all(fs::path(EnsureStorageRoot()) / titleName) after checking only that titleName was not empty. In C++, joining an absolute right-hand path discards the left-hand base, and .. segments can climb above it. The invariant to state is not “paths look tidy”: the exact recursive-delete target must be a non-root descendant of the selected application storage root.

Ask the questions that turn the expression into a review: who controls titleName; how EnsureStorageRoot chooses the root; whether open and delete use the same root; whether a path component may be a symlink; and whether the string may have been authored on another operating system. The storage internals page gives the owner and the fake-async acquisition path; this case makes the review method concrete.

Caller titleName                                        [untrusted text]
  -> StorageDevice::DeleteContainer                      [empty name rejected]
  -> EnsureStorageRoot                                   [process-static root:
       Android files dir / $XDG_DATA_HOME / %LOCALAPPDATA% / HOME chain / cwd]
  -> CNA::Internal::ResolveContainedPath(root, titleName)
       [separators normalized; rooted and drive-letter text rejected on the string;
        lexical join; weakly_canonical check for symlinks; root itself rejected]
  -> CNA::Internal::PathFromUtf8(resolvedPath)           [UTF-8 to native path]
  -> std::filesystem::remove_all                         [irreversible side effect]

Read the historical commit and the snapshot's StorageDevice.cpp side by side. The commit establishes that no DeleteContainer test existed before the fix. It also records that this behaviour was CNA-introduced, because FNA's DeleteContainer throws NotImplementedException; that is commit evidence for this one method, not a blanket licence to change other storage APIs.

What differs today: the historical root came from SDL_GetPrefPath with an XDG_DATA_HOME/HOME fallback. At the snapshot EnsureStorageRoot is independent of the windowing platform and resolves, in order, the Android package files directory, $XDG_DATA_HOME/<app>, %LOCALAPPDATA%\<app> (read through the wide Windows environment), ~/Library/Application Support/<app> on Apple or ~/.local/share/<app> elsewhere, and finally ./<app>; <app> is game unless SetAppNameEXT was called. The delete target is converted with PathFromUtf8; the source comment notes that the narrow overload would remove nothing on a non-ASCII Windows path, silently. The user-facing chain is documented in the Storage guide.

2. Design an oracle that cannot hide collateral damage

The historical tests did more than assert an exception. They placed a sentinel file under the storage root, called DeleteContainer with an empty name, an absolute name ("/etc"), a parent-escaping name ("../../../../../../../../tmp") and ".", and checked that the sentinel survived; the positive case checked that one named title disappeared while a sibling survived. The sentinel sits outside the authorized delete target: it would vanish if a mis-resolved call deleted the whole root. Note what it cannot see: whether a directory outside the root was touched. For the absolute and escaping cases that part of the claim rests on the thrown std::invalid_argument, which is why the rejection happens before any filesystem call.

The snapshot's StorageDeviceTests.cpp holds ten StorageDeviceDeleteContainerTest cases and four StorageDeviceNotConnectedExceptionSerializationTest cases:

The generalizable technique is an outside-the-authorized-target sentinel, not a test that merely returns without throwing. For a real fix, run a new destructive test for the first time only in a disposable root, with a target whose resolved path has been inspected independently.

3. Follow the helper beyond the original subsystem

The fix introduced the shared header that is now PathContainment.hpp. In July 2026 it had two functions; at the snapshot it has a small family, with one native core that every string variant wraps:

The returned form is architectural. PlaylistParser produces song paths that MediaLibrary::songByPath_ looks up. The original commit reports that its first version returned the canonicalized absolute path and broke those lookups, so canonicalization became check-only. A later Windows run found the second half of the same contract: lexically_normal() returned backslashes on Windows while every other producer of the key used /, so playlist lookups missed there; the string variants now return PathToGenericUtf8, and MediaLibraryIndex.cpp keys its index in the same generic form. A “safer” canonical return value would violate a data-flow invariant even if containment stayed correct.

The three original sites have different authority roots, and the snapshot adds more callers. A helper call is not a substitute for deciding what the root means:

The original commit recorded two further findings as out of scope at the time: a duplicated ResolveRelativeFilePath() in the video and song readers with no containment, and eight unchecked .cnj/JSON manifest path fields in ContentManager.cpp (REMED-CONTENT-007/-008). Reading the snapshot shows both now go through the helper (commit c04faaedb). The commit also judged StorageContainer's joins FNA-faithful and left them; since 1af12dac3 they are contained too, documented in the source as an intentional security deviation from FNA's Path.Combine. That is the answer to “are the recorded sites still open?” for these files only; an exhaustive audit of every other path join in the tree was not made for this page.

4. State what the proof does not establish

Lexical and weak-canonical checks reject the tested absolute, traversal and symlink cases at validation time. They do not make the subsequent path-based remove_all atomic against another process changing filesystem entries between check and use. The repository tests prove the specified rejection behaviour in their fixture, not a race-resistant sandbox under a hostile concurrent process. Nor do they cover a concurrent change of the process-static root: storageRoot_, appName_ and storageRootInitialized_ are unsynchronized statics, and SetAppNameEXT changes them.

Host coverage is its own limit. CNA's records for 72d493c7f report that the containment guard did not hold on Windows until then (a path starting with a separator is not is_absolute() there, yet operator/ treats it as rooted), and that the defect was found by running the suite on real Windows, with nine PathContainmentTest failures. Non-ASCII paths need the same kind of native evidence; tools/platform/win32_unicode_paths.ps1 exists for that host. Nothing on this page was run on Windows. Write these as limits in the review memo instead of turning “unit tests pass” into a universal security claim.

A human patch review should answer, in writing:

• Which string is untrusted, and on which OS might it have been authored?
• What is the authority root, and is it the same as the join base?
• Does the check reject every rooted spelling on this host and cross-host text, before any path is built?
• Is canonicalization used for checking only, or also for returned keys?
• Which operations are destructive, and what exactly is their target?
• Which test fixture paths will be created or deleted, and on which environment variables do they depend?
• Which other modules consume this helper, and which output keys do they store?
• Has the code changed since the history being cited?

If those answers are written before editing, the patch can be reasoned about without an AI agent or the original author.

5. Repeat the investigation manually

1. Compare the historical diff with the snapshot: git show abaed98b2 -- src/Microsoft/Xna/Framework/Storage/StorageDevice.cpp include/CNA/Internal/PathContainment.hpp, then git log --oneline abaed98b2..009d40f5 -- modules/storage modules/core/include/CNA/Internal/PathContainment.hpp. Identify which lines moved during modularization, the SDL removal and the Windows path work.
2. Read the snapshot's EnsureStorageRoot, DeleteContainer, the StorageContainer constructor and StorageContainer::ResolveNativePath, then the helper. Draw separate arrows for the authorized root, the join base and the returned UTF-8 or native target.
3. List every call site of the helper family at the snapshot (git grep -n -E 'ResolveContained|ValidateContained|IsDisallowedAbsolutePath' 009d40f5 -- modules) and classify each as read, write or recursive delete. Do not assume one normalization rule suits them all.
4. Read PathContainmentTests.cpp (30 PathContainmentTest cases at the snapshot, against 14 in the original commit, including the Windows drive, drive-relative, rooted-backslash and UNC spellings rejected even on POSIX) and the storage fixture. Before running either, inspect any fixture that writes or deletes real paths.
5. Run isolated, focused filters and record passes, skips and missing hosts, for example XDG_DATA_HOME=$(mktemp -d) ./cmake-build-unit/CnaStorageTests --gtest_filter='StorageDeviceDeleteContainerTest.*' and ./cmake-build-unit/CnaCoreTests --gtest_filter='PathContainmentTest.*' after building those EXCLUDE_FROM_ALL targets (cmake --build cmake-build-unit --target CnaStorageTests CnaCoreTests, or the unit-core build preset). Commands checked against the CMake sources; not executed here.
6. Write a one-page review note with the invariant, call path, owner, exact delete target, other consumers, test side effects and remaining uncertainty. If you cannot explain why playlist and content behaviour stay correct after a helper change, you are not ready to merge it.

This is the pattern for taking over CNA: history explains why an invariant was added; current source proves what executes now; tests provide bounded evidence; the human maintainer supplies the cross-module judgment. The same boundary appears as a blast-radius row in Blast radius and readiness and as a test route in What to test after changing X. The investigation method itself is in How to understand code you did not write.