Skip to content

Commit a5a0f5c

Browse files
authored
fix(pass-style): Relax passable error criteria in hardenTaming unsafe mode. (#2990)
This change increases test coverage over lockdown with hardenTaming unsafe. The increase in test coverage revealed the need to relax certain cases, but moreof adjust `passStyleOf` to repair unfrozen V8 error objects in `unsafe` `hardenTaming` mode.
2 parents 60190cd + 39f8386 commit a5a0f5c

File tree

15 files changed

+257
-38
lines changed

15 files changed

+257
-38
lines changed

packages/check-bundle/package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,9 +75,9 @@
7575
]
7676
},
7777
"sesAvaConfigs": {
78-
"shims": "../../ava-endo-shims-only.config.mjs",
7978
"lockdown": "../../ava-endo-lockdown.config.mjs",
80-
"unsafe": "../../ava-endo-lockdown-unsafe.config.mjs"
79+
"unsafe": "../../ava-endo-lockdown-unsafe.config.mjs",
80+
"shims": "../../ava-endo-shims-only.config.mjs"
8181
},
8282
"typeCoverage": {
8383
"atLeast": 86.95

packages/common/package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
]
7474
},
7575
"sesAvaConfigs": {
76-
"lockdown": "../../ava-endo-lockdown.config.mjs"
76+
"lockdown": "../../ava-endo-lockdown.config.mjs",
77+
"unsafe": "../../ava-endo-lockdown-unsafe.config.mjs"
7778
}
7879
}

packages/common/test/object-meta-map.test.js

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@ import { objectMetaMap } from '../object-meta-map.js';
44

55
const { getOwnPropertyDescriptors, getPrototypeOf } = Object;
66

7+
// @ts-expect-error isFake is not advertised by the type of harden.
8+
const hardenIsFake = !!harden.isFake;
9+
710
test('test objectMetaMap', async t => {
811
const mapped = objectMetaMap(
912
{ a: 1, b: 2, c: 3 },
@@ -21,15 +24,15 @@ test('test objectMetaMap', async t => {
2124
t.deepEqual(getOwnPropertyDescriptors(mapped), {
2225
a: {
2326
value: 2,
24-
writable: false,
27+
writable: hardenIsFake,
2528
enumerable: false,
26-
configurable: false,
29+
configurable: hardenIsFake,
2730
},
2831
c: {
2932
value: 6,
30-
writable: false,
33+
writable: hardenIsFake,
3134
enumerable: false,
32-
configurable: false,
35+
configurable: hardenIsFake,
3336
},
3437
});
3538
t.is(getPrototypeOf(mapped), null);

packages/exo/package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,8 @@
7171
]
7272
},
7373
"sesAvaConfigs": {
74-
"lockdown": "../../ava-endo-lockdown.config.mjs"
74+
"lockdown": "../../ava-endo-lockdown.config.mjs",
75+
"unsafe": "../../ava-endo-lockdown-unsafe.config.mjs"
7576
},
7677
"typeCoverage": {
7778
"atLeast": 94.22

packages/exo/test/heap-classes.test.js

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@ import {
99
makeExo,
1010
} from '../index.js';
1111

12+
// @ts-expect-error isFake is not advertised by the type of harden.
13+
const hardenIsFake = !!harden.isFake;
14+
1215
const NoExtraI = M.interface('NoExtra', {
1316
foo: M.call().returns(),
1417
});
@@ -231,14 +234,14 @@ const PassableGreeterI = M.interface(
231234
test('passable guards', t => {
232235
const greeter = makeExo('greeter', PassableGreeterI, {
233236
sayHello(immutabe) {
234-
t.is(Object.isFrozen(immutabe), true);
237+
t.true(Object.isFrozen(immutabe));
235238
return 'hello';
236239
},
237240
});
238241

239242
const mutable = {};
240243
t.is(greeter.sayHello(mutable), 'hello', `passableGreeter can sayHello`);
241-
t.is(Object.isFrozen(mutable), true, `mutable is frozen`);
244+
t.true(Object.isFrozen(mutable), `mutable is frozen`);
242245
t.throws(() => greeter.sayHello(makeBehavior()), {
243246
message:
244247
/In "sayHello" method of \(greeter\): Remotables must be explicitly declared/,
@@ -280,37 +283,50 @@ test('raw guards', t => {
280283
return 'hello';
281284
},
282285
rawIn(obj) {
283-
t.is(Object.isFrozen(obj), false);
286+
// Object is never actually frozen, but isFrozen will always say true
287+
// when hardenIsFake.
288+
t.is(hardenIsFake, Object.isFrozen(obj));
284289
return obj;
285290
},
286291
rawOut(obj) {
287-
t.is(Object.isFrozen(obj), true);
292+
// Object is implicitly frozen by harden as a side-effect of passing to
293+
// an M.any guard, or unfrozen because harden is fake, but isFrozen lies.
294+
t.true(Object.isFrozen(obj));
288295
return { ...obj };
289296
},
290297
passthrough(obj) {
291-
t.is(Object.isFrozen(obj), false);
298+
// The object is not frozen, but isFrozen lies when hardenTaming is
299+
// unsafe.
300+
t.is(hardenIsFake, Object.isFrozen(obj));
292301
return obj;
293302
},
294303
tortuous(hardA, softB, hardC, optHardD, optSoftE = {}) {
304+
// Recall that isFrozen lies with hardenTaming: unsafe
295305
// Test that `M.raw()` does not freeze the arguments, unlike `M.any()`.
296-
t.is(Object.isFrozen(hardA), true);
297-
t.is(Object.isFrozen(softB), false);
306+
t.true(Object.isFrozen(hardA));
307+
t.is(hardenIsFake, Object.isFrozen(softB));
298308
softB.b = 2;
299-
t.is(Object.isFrozen(hardC), true);
300-
t.is(Object.isFrozen(optHardD), true);
301-
t.is(Object.isFrozen(optSoftE), false);
309+
t.true(Object.isFrozen(hardC));
310+
t.true(Object.isFrozen(optHardD));
311+
// optSoftE is never frozen. It's either explicitly passed raw, or
312+
// defaults in the argument to an unfrozen object.
313+
// But, in unsafe harden taming, isFrozen misreports that soft objects
314+
// are frozen.
315+
t.is(hardenIsFake, Object.isFrozen(optSoftE));
302316
return {};
303317
},
304318
});
305319
t.deepEqual(greeter2[GET_INTERFACE_GUARD]?.(), Greeter2I);
306320
testGreeter(t, greeter, 'explicit raw');
307321

308-
t.is(Object.isFrozen(greeter2.rawIn({})), true);
309-
t.is(Object.isFrozen(greeter2.rawOut({})), false);
310-
t.is(Object.isFrozen(greeter2.passthrough({})), false);
322+
t.true(Object.isFrozen(greeter2.rawIn({})));
323+
// These both return actually unfrozen objects because of the M.raw return
324+
// guard, but isFrozen says true anyway if hardenTaming is unsafe.
325+
t.is(hardenIsFake, Object.isFrozen(greeter2.rawOut({})));
326+
t.is(hardenIsFake, Object.isFrozen(greeter2.passthrough({})));
311327

312-
t.is(Object.isFrozen(greeter2.tortuous({}, {}, {}, {}, {})), true);
313-
t.is(Object.isFrozen(greeter2.tortuous({}, {}, {})), true);
328+
t.true(Object.isFrozen(greeter2.tortuous({}, {}, {}, {}, {})));
329+
t.true(Object.isFrozen(greeter2.tortuous({}, {}, {})));
314330

315331
t.throws(
316332
() => greeter2.tortuous(makeBehavior(), {}, {}),

packages/marshal/package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,8 @@
9191
"timeout": "2m"
9292
},
9393
"sesAvaConfigs": {
94-
"lockdown": "../../ava-endo-lockdown.config.mjs"
94+
"lockdown": "../../ava-endo-lockdown.config.mjs",
95+
"unsafe": "../../ava-endo-lockdown-unsafe.config.mjs"
9596
},
9697
"typeCoverage": {
9798
"atLeast": 85.3

packages/ocapn/package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
]
7474
},
7575
"sesAvaConfigs": {
76-
"lockdown": "../../ava-endo-lockdown.config.mjs"
76+
"lockdown": "../../ava-endo-lockdown.config.mjs",
77+
"unsafe": "../../ava-endo-lockdown-unsafe.config.mjs"
7778
}
7879
}

packages/pass-style/NEWS.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@ User-visible changes in `@endo/pass-style`:
33
# Next release
44

55
- deprecates `assertChecker`. Use `Fail` in the confirm/reject pattern instead, as supported by `@endo/errors/rejector.js`.
6+
- Enables `passStyleOf` to make errors passable as a side-effect when SES locks
7+
down with `hardenTaming` set to `unsafe`, which impacts errors on V8 starting
8+
with Node.js 21, and similar engines, that own a `stack` getter and setter
9+
that would otherwise be repaired as a side-effect of `harden`.
610

711
# 1.6.3 (2025-07-11)
812

packages/pass-style/package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,8 @@
7070
]
7171
},
7272
"sesAvaConfigs": {
73-
"lockdown": "../../ava-endo-lockdown.config.mjs"
73+
"lockdown": "../../ava-endo-lockdown.config.mjs",
74+
"unsafe": "../../ava-endo-lockdown-unsafe.config.mjs"
7475
},
7576
"typeCoverage": {
7677
"atLeast": 84.24

packages/pass-style/src/error.js

Lines changed: 164 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,159 @@ import { Fail, q, hideAndHardenFunction } from '@endo/errors';
88
* @import {PassStyle} from './types.js';
99
*/
1010

11-
const { getPrototypeOf, getOwnPropertyDescriptors, hasOwn, entries } = Object;
11+
const {
12+
defineProperty,
13+
getPrototypeOf,
14+
getOwnPropertyDescriptors,
15+
getOwnPropertyDescriptor,
16+
hasOwn,
17+
entries,
18+
freeze,
19+
} = Object;
20+
21+
const { apply } = Reflect;
22+
23+
const hardenIsFake = () => {
24+
// We do not trust isFrozen because lockdown with unsafe hardenTaming replaces
25+
// isFrozen with a version that is in cahoots with fake harden.
26+
const subject = harden({ __proto__: null, x: 0 });
27+
const desc = getOwnPropertyDescriptor(subject, 'x');
28+
return desc?.writable === true;
29+
};
30+
31+
/**
32+
* Pass-style must defend its own integrity under a number of configurations.
33+
*
34+
* In all environments where we use pass-style, we can in principle rely on the
35+
* globalThis.TypeError and globalThis.Error to be safe.
36+
* We have similar code in SES that stands on the irreducible risk that an
37+
* attacker may run before SES, so the application must either ensure that SES
38+
* initializes first or that all prior code is benign.
39+
* For all other configurations, we rely to some degree on SES lockdown and a
40+
* Compartment for any measure of safety.
41+
*
42+
* Pass-style may be loaded by the host module system into the primary realm,
43+
* which the authors call the Start Compartment.
44+
* SES provides no assurances that any number of guest programs can be safely
45+
* executed by the host in the start compartment.
46+
* Such code must be executed in a guest compartment.
47+
* As such, it is irrelevant that the globalThis is mutable and also holds all
48+
* of the host's authority.
49+
*
50+
* Pass-style may be loaded into a guest compartment, and the globalThis of the
51+
* compartment may or may not be frozen.
52+
* We typically, as with importBundle, run every Node.js package in a dedicated
53+
* compartment with a gratuitiously frozen globalThis.
54+
* In this configuration, we can rely on globalThis.Error and
55+
* globalThis.TypeError to correspond to the realm's intrinsics, either because
56+
* the Compartment arranged for a frozen globalThis or because the pass-style
57+
* package provides no code that can arrange for a change to the compartment's
58+
* globalThis.
59+
*
60+
* Running multiple guests in a single compartment with an unfrozen globalThis
61+
* is incoherent and provides no assurance of mutual safety between those
62+
* guests.
63+
* No code, much less Pass-style, should be run in such a compartment.
64+
*
65+
* Although we can rely on the globalThis.Error and globalThis.TypeError
66+
* bindings, we can and do use `makeTypeError` to produce a TypeError instance
67+
* that is guaranteed to be an instance of the realm intrinsic by dint of
68+
* construction from language syntax.
69+
* The idiom "belt and suspenders" is well-known among the authors and means
70+
* gratuitous or redundant safety measures.
71+
* In this case, we wear both belt and suspenders *on our overalls*.
72+
*
73+
* @returns {TypeError}
74+
*/
75+
const makeTypeError = () => {
76+
try {
77+
// @ts-expect-error deliberate TypeError
78+
null.null;
79+
throw TypeError('obligatory'); // To convince the type flow inferrence.
80+
} catch (error) {
81+
return error;
82+
}
83+
};
84+
85+
export const makeRepairError = () => {
86+
if (!hardenIsFake()) {
87+
return undefined;
88+
}
89+
90+
const typeErrorStackDesc = getOwnPropertyDescriptor(makeTypeError(), 'stack');
91+
const errorStackDesc = getOwnPropertyDescriptor(Error('obligatory'), 'stack');
92+
93+
if (
94+
typeErrorStackDesc === undefined ||
95+
typeErrorStackDesc.get === undefined
96+
) {
97+
return undefined;
98+
}
99+
100+
if (
101+
errorStackDesc === undefined ||
102+
typeof typeErrorStackDesc.get !== 'function' ||
103+
typeErrorStackDesc.get !== errorStackDesc.get ||
104+
typeof typeErrorStackDesc.set !== 'function' ||
105+
typeErrorStackDesc.set !== errorStackDesc.set
106+
) {
107+
// We have own stack accessor properties that are outside our expectations,
108+
// that therefore need to be understood better before we know how to repair
109+
// them.
110+
// See https://github.com/endojs/endo/blob/master/packages/ses/error-codes/SES_UNEXPECTED_ERROR_OWN_STACK_ACCESSOR.md
111+
throw TypeError(
112+
'Unexpected Error own stack accessor functions (PASS_STYLE_UNEXPECTED_ERROR_OWN_STACK_ACCESSOR)',
113+
);
114+
}
115+
116+
// We should otherwise only encounter this case on V8 and possibly immitators
117+
// like FaceBook's Hermes because of its problematic error own stack accessor
118+
// behavior, which creates an undeniable channel for communicating arbitrary
119+
// capabilities through the stack internal slot of arbitrary frozen objects.
120+
// Note that FF/SpiderMonkey, Moddable/XS, and the error stack proposal
121+
// all inherit a stack accessor property from Error.prototype, which is
122+
// great. That case needs no heroics to secure.
123+
124+
// In the V8 case as we understand it, all errors have an own stack accessor
125+
// property, but within the same realm, all these accessor properties have
126+
// the same getter and have the same setter.
127+
// This is therefore the case that we repair.
128+
//
129+
// Also, we expect tht the captureStackTrace proposal to create more cases
130+
// where error objects have own "stack" getters.
131+
// https://github.com/tc39/proposal-error-capturestacktrace
132+
133+
const feralStackGetter = freeze(errorStackDesc.get);
134+
135+
/** @param {unknown} error */
136+
const repairError = error => {
137+
// Only pay the overhead if it first passes this cheap isError
138+
// check. Otherwise, it will be unrepaired, but won't be judged
139+
// to be a passable error anyway, so will not be unsafe.
140+
const stackDesc = getOwnPropertyDescriptor(error, 'stack');
141+
if (
142+
stackDesc &&
143+
stackDesc.get === feralStackGetter &&
144+
stackDesc.configurable
145+
) {
146+
// Can only repair if it is configurable. Otherwise, leave
147+
// unrepaired, in which case it will not be judged passable,
148+
// avoiding a safety problem.
149+
defineProperty(error, 'stack', {
150+
// NOTE: Calls getter during harden, which seems dangerous.
151+
// But we're only calling the problematic getter whose
152+
// hazards we think we understand.
153+
value: apply(feralStackGetter, error, []),
154+
});
155+
}
156+
};
157+
harden(repairError);
158+
159+
return repairError;
160+
};
161+
harden(makeRepairError);
162+
163+
export const repairError = makeRepairError();
12164

13165
// TODO: Maintenance hazard: Coordinate with the list of errors in the SES
14166
// whilelist.
@@ -178,6 +330,16 @@ export const confirmRecursivelyPassableError = (
178330
reject`Passable Error must inherit from an error class .prototype: ${candidate}`
179331
);
180332
}
333+
if (repairError !== undefined) {
334+
// This point is unreachable unless the candidate is mutable and the
335+
// platform is V8 or like V8 creates errors with an own "stack" getter or
336+
// setter, which would otherwise make them non-passable.
337+
// This should only occur with lockdown using unsafe hardenTaming or an
338+
// equivalent fake, non-actually-freezing harden.
339+
// Under these circumstances only, passStyleOf alters an object as a side
340+
// effect, converting the "stack" property to a data value.
341+
repairError(candidate);
342+
}
181343
const descs = getOwnPropertyDescriptors(candidate);
182344
if (!('message' in descs)) {
183345
return (
@@ -197,9 +359,7 @@ export const confirmRecursivelyPassableError = (
197359
};
198360
harden(confirmRecursivelyPassableError);
199361

200-
/**
201-
* @type {PassStyleHelper}
202-
*/
362+
/** @type {PassStyleHelper} */
203363
export const ErrorHelper = harden({
204364
styleName: 'error',
205365

0 commit comments

Comments
 (0)