Skip to content

Commit e81a10a

Browse files
raymondginger2018-sudoRaymo
authored andcommitted
fix(security): grant the Windows ACL before stripping inheritance
Stripping the inherited ACEs first could leave the path with no ACE at all: when the following grant failed, the file (or, via directory inheritance, everything under it) kept no entry for the user and the next open threw EPERM (errno -4048). On Windows this crashed `/model` exactly there — `writePrivateFile -> settings.json` — with "operation not permitted". - issue the grant first so a failed grant leaves the inherited ACEs in place and the path stays usable; strip only after the grant landed - carry (OI)(CI) on directory grants so the inheritable ACE reaches existing and future children before /inheritance:r propagates the strip to them (ensurePrivateDirectory previously granted a non-inheritable ACE, which left the files inside with nothing once the strip hit the directory) - route icacls through a default-import childProcess so tests can intercept it with t.mock.method (named imports bind a snapshot the mock cannot reach) - tests: patch process.platform to win32 so the regression runs on every OS CI uses; the positive control asserts grant-before-strip order and the negative control asserts a failed grant never strips inheritance Verified on Windows: `/model` writes settings.json without EPERM again, and the directory/file both end up with a single explicit ACE for the user.
1 parent 1e5d6b7 commit e81a10a

2 files changed

Lines changed: 136 additions & 19 deletions

File tree

‎packages/core/src/common/private-storage.ts‎

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
* current user — matching the 0600 intent.
99
*/
1010

11-
import { execFileSync } from "child_process";
11+
import childProcess from "child_process";
1212
import * as fs from "fs";
1313
import * as os from "os";
1414
import * as path from "path";
@@ -37,7 +37,7 @@ export function windowsIdentity(): string | null {
3737
return `${USERDOMAIN}\\${USERNAME}`;
3838
}
3939
try {
40-
const stdout = execFileSync("whoami", {
40+
const stdout = childProcess.execFileSync("whoami", {
4141
encoding: "utf8",
4242
timeout: 5000,
4343
windowsHide: true,
@@ -53,12 +53,19 @@ export function windowsIdentity(): string | null {
5353
/**
5454
* Restrict an NTFS path to the current user (Windows only; no-op elsewhere).
5555
*
56-
* Two idempotent steps:
57-
* 1. ``icacls /inheritance:r`` removes inherited ACEs so a permissive parent
56+
* Two idempotent steps, ordered fail-safe — grant before strip:
57+
* 1. ``icacls /grant:r`` grants the current user exclusive full control
58+
* (``:r`` replaces, does not append). For a directory the grant carries
59+
* ``(OI)(CI)`` so it is inherited by existing and future children.
60+
* 2. ``icacls /inheritance:r`` removes inherited ACEs so a permissive parent
5861
* (e.g. the profile root granting ``Authenticated Users``) no longer
59-
* applies.
60-
* 2. ``icacls /grant:r <user>:F`` grants the current user exclusive full
61-
* control (``:r`` replaces, does not append).
62+
* applies. For a directory this propagates to children, which is why the
63+
* grant above must already name this user.
64+
*
65+
* The order matters: if the grant is issued first and fails, the inherited
66+
* ACEs are still in place and the path stays usable. Stripping first can
67+
* leave the path (and, via inheritance, its children) with no ACE at all — the
68+
* next open then throws ``EPERM`` and the state file becomes unreachable.
6269
*
6370
* Returns whether the ACL now grants the current user alone. Failures are
6471
* reported, never thrown: callers decide whether an unprotected path is
@@ -67,28 +74,37 @@ export function windowsIdentity(): string | null {
6774
* and leave the ACL untouched, which is how this helper silently no-opped
6875
* before.
6976
*/
70-
export function restrictWindowsAcl(targetPath: string): boolean {
77+
export function restrictWindowsAcl(targetPath: string, isDirectory = false): boolean {
7178
if (process.platform !== "win32") {
7279
return true;
7380
}
7481
const identity = windowsIdentity();
7582
if (!identity) {
7683
return false;
7784
}
78-
for (const args of [
79-
[targetPath, "/inheritance:r"],
80-
[targetPath, "/grant:r", `${identity}:F`],
81-
]) {
85+
const grantee = isDirectory ? `${identity}:(OI)(CI)F` : `${identity}:F`;
86+
const runIcacls = (args: string[]): boolean => {
8287
try {
83-
execFileSync("icacls", args, {
88+
childProcess.execFileSync("icacls", args, {
8489
encoding: "utf8",
8590
timeout: 15000,
8691
windowsHide: true,
8792
stdio: ["ignore", "pipe", "ignore"],
8893
});
94+
return true;
8995
} catch {
9096
return false; // best-effort: keep the caller moving, but do not pretend
9197
}
98+
};
99+
// Grant first: if this fails the inherited ACEs are still in place, so the
100+
// path stays usable. Stripping first could leave a path with no ACE at all.
101+
if (!runIcacls([targetPath, "/grant:r", grantee])) {
102+
return false;
103+
}
104+
// Then drop the inherited ACEs. For a directory this propagates to
105+
// children, which is why the grant above must already name this user.
106+
if (!runIcacls([targetPath, "/inheritance:r"])) {
107+
return false;
92108
}
93109
return true;
94110
}
@@ -97,8 +113,8 @@ export function restrictWindowsAcl(targetPath: string): boolean {
97113
* Write a private file with user-only permissions on every platform.
98114
*
99115
* - POSIX: mode 0600 (applied by the write itself, subject to umask).
100-
* - Windows: mode bits are ignored by the OS, so we remove inherited ACEs
101-
* and grant the current user exclusive full control.
116+
* - Windows: mode bits are ignored by the OS, so we grant the current user
117+
* exclusive full control first and then remove inherited ACEs.
102118
*
103119
* Returns true when the platform's permission model was applied as requested.
104120
*/
@@ -113,7 +129,7 @@ export function writePrivateFile(targetPath: string, contents: string): boolean
113129
*/
114130
export function ensurePrivateDirectory(dirPath: string): boolean {
115131
fs.mkdirSync(dirPath, { recursive: true, mode: PRIVATE_DIRECTORY_MODE });
116-
return process.platform === "win32" ? restrictWindowsAcl(dirPath) : true;
132+
return process.platform === "win32" ? restrictWindowsAcl(dirPath, true) : true;
117133
}
118134

119135
/** Home directory used for DeepCode user state. */

‎packages/core/src/tests/private-storage.test.ts‎

Lines changed: 104 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
1-
import { afterEach, describe, it } from "node:test";
1+
import { afterEach, describe, it, type TestContext } from "node:test";
22
import assert from "node:assert/strict";
3-
import { execFileSync } from "child_process";
3+
import childProcess from "child_process";
44
import * as fs from "fs";
55
import * as os from "os";
66
import * as path from "path";
77
import {
88
PRIVATE_FILE_MODE,
99
PRIVATE_DIRECTORY_MODE,
1010
ensurePrivateDirectory,
11+
restrictWindowsAcl,
1112
windowsIdentity,
1213
writePrivateFile,
1314
} from "../common/private-storage";
@@ -26,6 +27,42 @@ afterEach(() => {
2627
}
2728
});
2829

30+
/**
31+
* Pretend to be Windows and record every execFileSync call. The platform
32+
* property is patched so these regression tests run on every OS upstream CI
33+
* uses — the icacls ordering bug is Windows-only and would otherwise get zero
34+
* coverage off-Windows. ``restore()`` puts platform/env back; the mock itself
35+
* is unwound by node:test at test end.
36+
*/
37+
function pretendWindows(
38+
t: TestContext,
39+
impl: (cmd: string, args: string[]) => string
40+
): { calls: string[][]; restore: () => void } {
41+
const platformDesc = Object.getOwnPropertyDescriptor(process, "platform");
42+
Object.defineProperty(process, "platform", { value: "win32", configurable: true });
43+
const prevDomain = process.env.USERDOMAIN;
44+
const prevUser = process.env.USERNAME;
45+
process.env.USERDOMAIN = "TESTDOM";
46+
process.env.USERNAME = "testuser";
47+
const calls: string[][] = [];
48+
t.mock.method(childProcess, "execFileSync", (cmd: string, args: string[] = []) => {
49+
calls.push([cmd, ...args]);
50+
return impl(cmd, args);
51+
});
52+
return {
53+
calls,
54+
restore: () => {
55+
if (platformDesc) {
56+
Object.defineProperty(process, "platform", platformDesc);
57+
}
58+
if (prevDomain === undefined) delete process.env.USERDOMAIN;
59+
else process.env.USERDOMAIN = prevDomain;
60+
if (prevUser === undefined) delete process.env.USERNAME;
61+
else process.env.USERNAME = prevUser;
62+
},
63+
};
64+
}
65+
2966
describe("private-storage", () => {
3067
it("writes files with 0600 mode on POSIX", () => {
3168
if (process.platform === "win32") {
@@ -65,7 +102,7 @@ describe("private-storage", () => {
65102
return;
66103
}
67104

68-
const out = execFileSync("icacls", [file], {
105+
const out = childProcess.execFileSync("icacls", [file], {
69106
encoding: "utf8",
70107
windowsHide: true,
71108
});
@@ -97,4 +134,68 @@ describe("private-storage", () => {
97134
assert.equal(fs.readFileSync(file, "utf8"), "2", "last write must win");
98135
assert.equal(second, first, "repeated writes must report the same outcome");
99136
});
137+
138+
it("grants the user before stripping inheritance (fail-safe order)", (t) => {
139+
// Regression: when the strip ran first, a failed grant left the path with
140+
// no ACE at all and the next write threw EPERM (errno -4048).
141+
const { calls, restore } = pretendWindows(t, () => "");
142+
try {
143+
assert.equal(restrictWindowsAcl("C:\\state\\settings.json"), true, "clean run must report success");
144+
} finally {
145+
restore();
146+
}
147+
assert.deepEqual(
148+
calls.map((args) => args.slice(1)),
149+
[
150+
["C:\\state\\settings.json", "/grant:r", "TESTDOM\\testuser:F"],
151+
["C:\\state\\settings.json", "/inheritance:r"],
152+
],
153+
"the grant must be issued before the inheritance strip"
154+
);
155+
});
156+
157+
it("keeps the inherited ACEs when the grant fails (no lockout)", (t) => {
158+
const { calls, restore } = pretendWindows(t, (_cmd, args) => {
159+
if (args.includes("/grant:r")) {
160+
throw Object.assign(new Error("icacls: Access is denied."), { status: 5 });
161+
}
162+
return "";
163+
});
164+
try {
165+
assert.equal(restrictWindowsAcl("C:\\state\\settings.json"), false, "a failed grant must report failure");
166+
} finally {
167+
restore();
168+
}
169+
// Negative control: stripping after a failed grant is the lockout. The
170+
// inherited ACEs must stay untouched so the path remains usable.
171+
assert.ok(
172+
calls.some((args) => args.includes("/grant:r")),
173+
"the grant must have been attempted"
174+
);
175+
assert.ok(
176+
calls.every((args) => !args.includes("/inheritance:r")),
177+
`inheritance must not be stripped after a failed grant: ${JSON.stringify(calls)}`
178+
);
179+
});
180+
181+
it("directory grants are inheritable so children keep access", (t) => {
182+
// Root cause companion: /inheritance:r on a directory propagates to its
183+
// children, so the grant must already reach them ((OI)(CI)) or the state
184+
// files inside become unopenable.
185+
const { calls, restore } = pretendWindows(t, () => "");
186+
const dir = tempDir("dc-priv-dir-acl-");
187+
try {
188+
assert.equal(ensurePrivateDirectory(path.join(dir, "deep")), true, "directory must report success");
189+
} finally {
190+
restore();
191+
}
192+
assert.deepEqual(
193+
calls.map((args) => args.slice(1)),
194+
[
195+
[path.join(dir, "deep"), "/grant:r", "TESTDOM\\testuser:(OI)(CI)F"],
196+
[path.join(dir, "deep"), "/inheritance:r"],
197+
],
198+
"directory grants must be inheritable and come first"
199+
);
200+
});
100201
});

0 commit comments

Comments
 (0)