Skip to content

Commit ea74a4b

Browse files
committed
fix(io-isolated-storage): confine IsolatedStorageFileStream to its store (#2208)
The constructor took a std::filesystem::path and checked nothing: it opened whatever it was handed, anywhere on the filesystem, and created that path's missing parent directories on the way. That is a WIDER hole than the TOCTOU #2207 declared and accepted -- that race needs an attacker who can already write inside the store root plus a window between check and use; this needed neither a race nor a privilege, only the call. It was the one door on a type that exists to confine file access which confined nothing. THE REFERENCE CORRECTED THIS TICKET'S PROPOSED SHAPE, and the correction removed the source break it was priced around. #2208 said "remove the path constructor and take the owning store instead". .NET publishes eight constructors, all beginning (string path, FileMode mode, ...), with the store an OPTIONAL TRAILING parameter (IsolatedStorageFileStream.cs:21-56); its storeless form is NOT unconfined -- a null store means GetUserStoreForDomain(), resolved through isf.GetFullPath(path) exactly as the store-taking form is (:82-118). So the confinement lands without removing an overload .NET publishes and without inventing a leading-store parameter order. The (store, path, mode) order an earlier cut used is now pinned as not compiling, because the wrong order compiles perfectly well as an addition and no behavioural test can see it -- both orders confine correctly. On POSIX there is no source break at all: std::filesystem::path converts to std::string implicitly, so an existing call still compiles and only its MEANING moves, from "this filesystem path" to "this path inside the store". On Windows value_type is wchar_t and the spelling breaks. An absolute path is CONTAINED, not refused -- fullPath() strips leading separators at every door (#2209's rule), and refusing it here alone would make the type inconsistent with itself. Also landed: * mode validation with .NET's own text, before the file is created; * IsolatedStorageFile::GetUserStoreForDomain(), purely additive, .NET's exact scope combination -- the store the storeless form defaults to; * a private four-argument constructor so each door reports its OWN parameter name (#2323's rule): `path` from the constructors, `relativePath` from OpenFile/CreateFile. .NET's `path == "\\"` check is deliberately not reproduced: on POSIX a backslash is an ordinary file-name character, as this module's own isDirectorySeparator() says. The outcome is identical anyway. Eight mutations, seven caught, one a proven equivalence (the default store's scope is unobservable -- both factories call the same GetIsolatedStorageRoot() and the stream never retains the store). Three were invalid as first written and were reformulated rather than counted. Seven of the eight are caught by PRE-EXISTING confinement tests, which is the point: OpenFile now routes through the constructor. The one only the new cases catch is the half-repair. Gate: 17,590 run / 17,590 passed / 0 failed / 0 skipped across 38 executables, recounted from the per-executable logs (+1, IO_IsolatedStorage 62 -> 63). Negative fixture set 46/236 -> 47/240, measured by the checker. Downstream: zero constructions in cna and mobile-eggbert. Build: build/ only, --parallel 2 throughout.
1 parent 22cba0a commit ea74a4b

17 files changed

Lines changed: 557 additions & 15 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

a/b/c/leaf.dat

Whitespace-only changes.

dirlink/written.dat

Whitespace-only changes.
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration: `IsolatedStorageFileStream` is confined to its store (#2208)
5+
6+
**Landed:** 2026-08-19, branch `next`. **Ticket:** #2208.
7+
8+
## What changed
9+
10+
`System::IO::IsolatedStorage::IsolatedStorageFileStream`'s public constructor was
11+
12+
```cpp
13+
IsolatedStorageFileStream(const std::filesystem::path& fullPath, System::IO::FileMode mode);
14+
```
15+
16+
It opened whatever path it was handed, **anywhere on the filesystem**, and created that path's
17+
missing parent directories on the way. It is now
18+
19+
```cpp
20+
IsolatedStorageFileStream(const std::string& path, System::IO::FileMode mode);
21+
IsolatedStorageFileStream(const std::string& path, System::IO::FileMode mode,
22+
const IsolatedStorageFile& store);
23+
```
24+
25+
Both forms resolve `path` against a store's root through that store's own `fullPath()` — the same
26+
resolver `OpenFile`, `CreateFile`, `DeleteFile` and `MoveFile` already used — so a path that
27+
escapes the store is refused **before any filesystem access happens**.
28+
29+
## Why this is more than a tidy-up
30+
31+
The type exists to confine file access, and this was the one door on it that confined nothing.
32+
It was a **wider hole than the TOCTOU #2207 declared and accepted**: that race needs an attacker
33+
who can already write inside the store root, and a window between check and use. This needed
34+
neither a race nor a privilege — only the call.
35+
36+
## The reference corrected this ticket's proposed shape
37+
38+
#2208 was written as *"remove the path constructor and take the owning store instead"*, and that
39+
is **not** what .NET does. .NET publishes **eight** constructors and every one begins
40+
`(string path, FileMode mode, ...)` with the store as an **optional trailing** parameter,
41+
`IsolatedStorageFile? isf` (`IsolatedStorageFileStream.cs:21-56`).
42+
43+
Its storeless form is **not unconfined**. When `isf` is null it takes
44+
`IsolatedStorageFile.GetUserStoreForDomain()` and then resolves through `isf.GetFullPath(path)`
45+
exactly as the store-taking form does (`:82-118`). So the confinement this ticket exists to add
46+
is obtained **without removing an overload .NET publishes** and without inventing a leading-store
47+
parameter order.
48+
49+
That correction removed the source break the ticket was priced around. An earlier cut of this
50+
work used `(store, path, mode)`; it is now pinned as **not compiling**, because the wrong order
51+
would compile perfectly well as an *addition* and would leave this port with two spellings where
52+
.NET has one.
53+
54+
## Is this a source break?
55+
56+
**On POSIX, essentially no.** `std::filesystem::path` converts to `std::string` implicitly there
57+
(`path::operator string_type()`, and `value_type` is `char`), so an existing
58+
`IsolatedStorageFileStream(somePath, FileMode::Open)` still compiles.
59+
60+
**Its meaning changes**, and that is the repair rather than a side effect: the argument used to be
61+
a filesystem path and is now a path relative to the store. An absolute path is **contained, not
62+
refused**`fullPath()` strips leading separators at every door, so `"/etc/passwd"` now names
63+
`<store-root>/etc/passwd`. That rule is older than this ticket (#2209 recorded it) and refusing it
64+
at this one door alone would make the type inconsistent with itself.
65+
66+
**On Windows it is a source break**, because `std::filesystem::path::value_type` is `wchar_t`
67+
there and no implicit conversion to `std::string` exists. Callers pass `p.string()`.
68+
69+
## Migration
70+
71+
Measured on 2026-08-19:
72+
73+
| Consumer | Constructions | Note |
74+
|---|---|---|
75+
| `cna` | 0 | no occurrence of the type at all |
76+
| `mobile-eggbert` | 0 | one `#include` in `WindowsPhoneSpeedyBlupi/Worlds.cpp:60`, no construction |
77+
| first-party | 1 | `IsolatedStorageFile::OpenFile`, which already held the store it now passes |
78+
79+
So the migration is **empty**. Where a call does exist:
80+
81+
```cpp
82+
// before
83+
IsolatedStorageFileStream s(store.getRootDirectoryProperty() / "save.dat", FileMode::Create);
84+
// after -- the store resolves the path
85+
IsolatedStorageFileStream s("save.dat", FileMode::Create, store);
86+
```
87+
88+
## Two behaviours that came with it
89+
90+
- **The mode is validated.** A `FileMode` outside the six defined values is rejected with .NET's
91+
own text, `Invalid mode, see System.IO.FileMode.` (`SR.IsolatedStorage_FileOpenMode`), before
92+
the file is created. Every named `FileMode` is legal, so only a value cast in from outside the
93+
enumeration can reach it.
94+
- **Each door reports its own parameter name.** The public constructors name `path`; `OpenFile`
95+
and `CreateFile` still name `relativePath`, which is what their own parameters are called.
96+
Routing every door through one resolver made it possible for one door's diagnostic to name
97+
another's parameter, which is the shape #2323 rules out.
98+
99+
## What .NET's check this port does not reproduce
100+
101+
.NET additionally rejects a path equal to `"\\"` (`SR.IsolatedStorage_Path`). On POSIX a backslash
102+
is an ordinary file-name character — this module's own `isDirectorySeparator()` says so in a
103+
comment — so reproducing that literal test would reject a legitimate name. The outcome is
104+
nonetheless the same on both platforms: `fullPath()` strips leading separators and rejects what is
105+
left when it is empty, which covers `"/"` on POSIX and both `"/"` and `"\\"` on Windows.
106+
107+
## New public member
108+
109+
`IsolatedStorageFile::GetUserStoreForDomain()` was added, purely additively, because it is the
110+
store .NET's storeless constructor defaults to. It uses .NET's exact scope combination
111+
(`Assembly | Domain | User`, `IsolatedStorageFile.cs:466-469`). Like this port's other two
112+
factories it resolves to the same storage root, so the scope is **recorded rather than reflected
113+
in the directory** — which is why a mutation swapping it for `GetUserStoreForApplication()` is an
114+
unobservable equivalence here. It is written as `ForDomain` for fidelity to the reference, not
115+
because anything in this port can tell the difference.
116+
117+
## Evidence
118+
119+
Eight mutations, seven caught, one a proven equivalence (the store scope, above). Three were
120+
invalid as first written — `-Werror=unused-parameter`, `-Werror=unused-function`, and a
121+
`[[nodiscard]]` on `fullPath()` — and were reformulated rather than counted. The mutation that
122+
only the new cases catch is the **half-repair**: `OpenFile` pre-resolving while the constructor
123+
ignores its store, which leaves the direct door open while every pre-existing confinement test
124+
still passes.
125+
126+
The other seven are caught by **pre-existing** confinement tests, which is the point: `OpenFile`
127+
now routes through the constructor, so the whole shipped confinement suite covers it.
128+
129+
Negative consumer fixture: `test/consumer/io_isolated_storage_stream_confinement_negative.cpp`,
130+
four sites — the store's private `fullPath()`, the private resolving constructor, the private
131+
`Resolve()`, and the store-first parameter order that .NET does not have.

docs/NegativeConsumerFixtureValidation.md

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1555,7 +1555,7 @@ sanitizer was given a deliberate defect built with the same flags:
15551555

15561556
§21.5 records **11 fixtures / 94 sites**, and that has been stale since 2026-08-04: the measured
15571557
total today is **45 fixtures / 231 sites**. #1894 adds no fixture of its own, so the total is
1558-
unchanged by this section.
1558+
unchanged by this section. (§23 later takes it to **47 fixtures / 240 sites**.)
15591559

15601560
**The first version of this subsection claimed the intervening fixtures "were each recorded in
15611561
their own ticket's migration note rather than here, so the record exists". That was asserted rather
@@ -1601,3 +1601,44 @@ the audit re-runs clean: **0 fixtures without a `docs/` record.**
16011601

16021602
Build directories used: `build-asan` (reused) and `build-ubsan` (created), both `--parallel 2`,
16031603
both with `ccache`.
1604+
1605+
---
1606+
1607+
## 23. Ticket #2208 — the isolated-storage stream confinement fixture (2026-08-19)
1608+
1609+
`test/consumer/io_isolated_storage_stream_confinement_negative.cpp`, **4 sites**, component
1610+
`IO.IsolatedStorage`. Running total: **47 fixtures / 240 sites**, measured by running
1611+
`scripts/check_negative_consumer_fixtures.py` rather than derived from §22.4 -- whose 45/231 was
1612+
itself already stale, #1888/#1889 having taken it to 46/236 earlier the same day.
1613+
1614+
### 23.1 What is unusual about this one
1615+
1616+
Almost every fixture in this document pins a **spelling a ticket outlawed**. This one pins
1617+
almost none, and the reason is a premise correction worth recording.
1618+
1619+
#2208 was written as *"remove `IsolatedStorageFileStream`'s path constructor and take the owning
1620+
store instead"*, which would have been a public source break with an outlawed spelling to pin.
1621+
The reference says otherwise: .NET publishes eight constructors, all beginning
1622+
`(string path, FileMode mode, ...)`, with the store an **optional trailing** parameter
1623+
(`IsolatedStorageFileStream.cs:21-56`); its storeless form defaults to
1624+
`GetUserStoreForDomain()` and resolves through `isf.GetFullPath(path)` like every other
1625+
(`:82-118`). So the confinement landed **without removing an overload**, and on POSIX there is
1626+
no source break at all — `std::filesystem::path` converts to `std::string` implicitly there, so
1627+
the old call still compiles and only its *meaning* changes.
1628+
1629+
What is left to pin is therefore not a spelling but a **structure**: the ways a consumer could
1630+
step around the resolver.
1631+
1632+
### 23.2 The four sites
1633+
1634+
| # | id | what it proves |
1635+
|---|---|---|
1636+
| 1 | `store-fullpath-is-not-consumer-reachable` | `IsolatedStorageFile::fullPath()` is private and the stream is its only friend, so the friendship the repair relies on grants a consumer nothing |
1637+
| 2 | `resolving-ctor-is-private` | the four-argument constructor — the only one taking its `paramName` from the caller rather than from the door — is unreachable |
1638+
| 3 | `resolve-is-private` | `Resolve()` is private, so nothing advertises that the check is separable from the construction |
1639+
| 4 | `store-first-overload-does-not-exist` | the `(store, path, mode)` order .NET does not have |
1640+
1641+
**Site 4 is the one this section exists for.** An earlier cut of #2208 used exactly that order,
1642+
before the reference was read. The wrong order is not a compile error waiting to happen — it
1643+
compiles perfectly well as an *addition*, and would leave this port with two spellings where
1644+
.NET has one. A behavioural test cannot see it, because both orders confine correctly.

existing/nested/kept.dat

Whitespace-only changes.

hop2/chained.dat

Whitespace-only changes.

link_dir/through_link.dat

Whitespace-only changes.

modules/io-isolated-storage/include/System/IO/IsolatedStorage/IsolatedStorageFile.hpp

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,10 @@ namespace System::IO::IsolatedStorage
6161
class IsolatedStorageFile : public IsolatedStorage
6262
{
6363
private:
64+
/// #2208: the stream resolves its relative path through this class's own fullPath(), so
65+
/// the confinement has exactly one implementation rather than two that could drift.
66+
friend class IsolatedStorageFileStream;
67+
6468
std::filesystem::path rootDirectory_; ///< Root directory of this isolated storage scope.
6569
bool disposed_ = false; ///< True after Close()/Dispose().
6670

@@ -127,6 +131,16 @@ namespace System::IO::IsolatedStorage
127131
/** Returns an isolated storage scoped to the current assembly (same root as application). */
128132
[[nodiscard]] static IsolatedStorageFile GetUserStoreForAssembly();
129133

134+
/**
135+
* @brief Obtains the user store scoped to the calling assembly and domain.
136+
*
137+
* .NET's `GetStore(Assembly | Domain | User)` (IsolatedStorageFile.cs:466-469). It is the
138+
* store `IsolatedStorageFileStream`'s storeless constructor defaults to, which is why
139+
* #2208 needed it; like this port's other two factories it resolves to the same storage
140+
* root, so the scope is recorded rather than reflected in the directory.
141+
*/
142+
[[nodiscard]] static IsolatedStorageFile GetUserStoreForDomain();
143+
130144
// --- File operations ---
131145

132146
/** Returns true if the specified relative path exists as a file in isolated storage. */

modules/io-isolated-storage/include/System/IO/IsolatedStorage/IsolatedStorageFileStream.hpp

Lines changed: 81 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,18 +4,45 @@
44
#pragma once
55

66
#include <filesystem>
7+
#include <string>
78

89
#include "System/IO/FileMode.hpp"
910
#include "System/IO/FileStream.hpp"
1011

1112
namespace System::IO::IsolatedStorage
1213
{
14+
class IsolatedStorageFile;
15+
1316
/**
1417
* @brief Represents a file stream inside isolated storage.
1518
*
1619
* C++ counterpart of .NET System.IO.IsolatedStorage.IsolatedStorageFileStream, which
1720
* derives from FileStream; this port does the same, inheriting Read/Write/Position/
18-
* Seek/CanSeek/SetLength. Obtain instances via IsolatedStorageFile::OpenFile()/CreateFile().
21+
* Seek/CanSeek/SetLength.
22+
*
23+
* @note **Confined since ticket #2208 (2026-08-19).** The constructor used to take a
24+
* `std::filesystem::path` and check nothing: it opened whatever it was handed, anywhere on
25+
* the filesystem, and created that path's missing parent directories. That was a **wider
26+
* hole than #2207's declared TOCTOU** -- no race and no privilege were needed, just the call.
27+
*
28+
* **The reference corrected this ticket's proposed shape.** #2208 was written as "remove the
29+
* path constructor and take the owning store instead", but .NET publishes eight constructors
30+
* and every one of them begins `(string path, FileMode mode, ...)` with the store as an
31+
* **optional trailing** parameter (`IsolatedStorageFile? isf`,
32+
* `IsolatedStorageFileStream.cs:21-56`). Its storeless form is **not** unconfined: when `isf`
33+
* is null it takes `IsolatedStorageFile.GetUserStoreForDomain()` and resolves through
34+
* `isf.GetFullPath(path)` exactly as the store-taking form does (`:82-118`). So the
35+
* confinement this ticket exists to add is obtained **without** removing an overload .NET
36+
* publishes and without inventing a leading-store parameter order.
37+
*
38+
* Both forms therefore resolve `path` against a store's root through the same `fullPath()`
39+
* every other door on `IsolatedStorageFile` uses; a path that escapes is refused before any
40+
* filesystem access happens.
41+
*
42+
* The parameter type changed with the meaning: `std::filesystem::path` -> `std::string`,
43+
* which is .NET's. On POSIX `std::filesystem::path` converts to `std::string` implicitly, so
44+
* an existing call still compiles and its **meaning** changes -- from "open this filesystem
45+
* path" to "open this path inside the store". That is the repair, not a side effect.
1946
*/
2047
class IsolatedStorageFileStream : public System::IO::FileStream
2148
{
@@ -34,9 +61,61 @@ namespace System::IO::IsolatedStorage
3461
* as a parameter, a public signature change tracked as ticket #2208. Prefer
3562
* IsolatedStorageFile::OpenFile()/CreateFile().
3663
*/
37-
IsolatedStorageFileStream(const std::filesystem::path& fullPath, System::IO::FileMode mode);
64+
/**
65+
* @brief Opens a stream on @p path inside the default user store.
66+
* @param path Path relative to the store. Leading separators are stripped, as at every
67+
* other door, so an absolute path is contained rather than refused.
68+
* @param mode How the file should be opened or created.
69+
* @throws System::ArgumentException if @p path would resolve outside the store, is empty,
70+
* or @p mode is not one of the six defined FileMode values.
71+
*
72+
* The .NET counterpart defaults to `IsolatedStorageFile.GetUserStoreForDomain()` and this
73+
* port does the same.
74+
*/
75+
IsolatedStorageFileStream(const std::string& path, System::IO::FileMode mode);
76+
77+
/**
78+
* @brief Opens a stream on @p path inside @p store.
79+
* @param path Path relative to @p store.
80+
* @param mode How the file should be opened or created.
81+
* @param store The owning isolated store; the path is resolved against its root.
82+
* @throws System::ArgumentException as above.
83+
*
84+
* The store is trailing because .NET's is (`IsolatedStorageFileStream.cs:26`). It is a
85+
* reference rather than .NET's nullable `IsolatedStorageFile?` because passing null there
86+
* is defined to mean "use the default store", which is what the two-argument overload
87+
* above already spells.
88+
*/
89+
IsolatedStorageFileStream(const std::string& path,
90+
System::IO::FileMode mode,
91+
const IsolatedStorageFile& store);
3892

3993
/** Closes the underlying file stream, syncing IDBFS on Emscripten builds. */
4094
void Close() override;
95+
96+
private:
97+
friend class IsolatedStorageFile;
98+
99+
/**
100+
* The resolving constructor every public one funnels through. @p paramName is the name
101+
* the CALLING door gives its own path parameter, so a refusal names the parameter the
102+
* caller actually wrote: `path` from the two public constructors, `relativePath` from
103+
* IsolatedStorageFile::OpenFile()/CreateFile(). Reporting one door's parameter name from
104+
* another door is the shape #2323 rules out.
105+
*/
106+
IsolatedStorageFileStream(const std::string& path,
107+
System::IO::FileMode mode,
108+
const IsolatedStorageFile& store,
109+
const char* paramName);
110+
111+
/**
112+
* Validates @p mode, resolves @p path through @p store's own confinement check, and
113+
* creates the file's missing parents. A member rather than a free function because
114+
* IsolatedStorageFile::fullPath() is private and this class is its only friend.
115+
*/
116+
[[nodiscard]] static std::string Resolve(const std::string& path,
117+
System::IO::FileMode mode,
118+
const IsolatedStorageFile& store,
119+
const char* paramName);
41120
};
42121
}

0 commit comments

Comments
 (0)