Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 47 additions & 5 deletions libs/core/src/__tests__/enclave.result-sanitization.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@
*
* The sanitizer used to recognize Dates, Maps, Sets and RegExps with `instanceof`, which is false
* for values built in the sandbox realm, so a script's `new Date(0)` came back as `{}`. It now
* checks the builtin's internal slot, as `@enclave-vm/browser` does inside its iframe. (The single
* VM still returns `{}`: its membrane hands the sanitizer a proxy, which the check refuses.)
* checks the builtin's internal slot, as `@enclave-vm/browser` does inside its iframe. The single
* VM's membrane wraps the script's Dates in a proxy, which has no slot, so that adapter hands the
* sanitizer `unwrapSecureProxy` to reach the Date behind it.
*/

import * as vm from 'vm';
import { Enclave } from '../enclave';
import { createSecureProxy, unwrapSecureProxy } from '../secure-proxy';
import { sanitizeValue } from '../value-sanitizer';
import type { CreateEnclaveOptions } from '../types';

describe('sanitizeValue with values from another realm', () => {
const foreign = vm.runInNewContext(`({
Expand Down Expand Up @@ -52,23 +55,62 @@ describe('sanitizeValue with values from another realm', () => {
});
});

describe('script results on the double VM', () => {
describe('unwrapSecureProxy', () => {
it('returns the object a secure proxy wraps', () => {
const date = new Date(0);

expect(unwrapSecureProxy(createSecureProxy(date))).toBe(date);
});

it('returns any other value unchanged', () => {
const plain = { a: 1 };
const foreignProxy = new Proxy(new Date(0), {});

expect(unwrapSecureProxy(plain)).toBe(plain);
expect(unwrapSecureProxy(foreignProxy)).toBe(foreignProxy);
expect(unwrapSecureProxy(null)).toBeNull();
expect(unwrapSecureProxy(1)).toBe(1);
});

it('lets sanitizeValue read a wrapped Date', () => {
const wrapped = { date: createSecureProxy(new Date(0)) };

expect(sanitizeValue(wrapped)).toEqual({ date: {} });
expect(sanitizeValue(wrapped, { unwrap: unwrapSecureProxy })).toEqual({ date: new Date(0) });
});
});

describe.each<[string, CreateEnclaveOptions]>([
['double VM', {}],
['single VM', { doubleVm: { enabled: false } }],
])('script results on the %s', (_name, adapterOptions) => {
let enclave: Enclave;

beforeAll(() => {
jest.spyOn(console, 'warn').mockImplementation(() => undefined);
});

afterAll(() => {
jest.restoreAllMocks();
});

beforeEach(() => {
enclave = new Enclave({ timeout: 5000 });
enclave = new Enclave({ timeout: 5000, ...adapterOptions });
});

afterEach(() => {
enclave.dispose();
});

it('returns a Date as a Date', async () => {
const result = await enclave.run<{ date: Date }>('return { date: new Date(0) };');
const result = await enclave.run<{ date: Date; nested: Date[] }>(
'return { date: new Date(0), nested: [new Date(1)] };',
);

expect(result.error).toBeUndefined();
expect(result.value?.date).toBeInstanceOf(Date);
expect(result.value?.date.getTime()).toBe(0);
expect(result.value?.nested[0].getTime()).toBe(1);
});

it('keeps the other conversions', async () => {
Expand Down
4 changes: 3 additions & 1 deletion libs/core/src/adapters/vm-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import * as vm from 'vm';
import type { SandboxAdapter, ExecutionContext, ExecutionResult, SecurityLevel } from '../types';
import { createSafeRuntime } from '../safe-runtime';
import { createSafeReflect, createSecureProxy } from '../secure-proxy';
import { createSafeReflect, createSecureProxy, unwrapSecureProxy } from '../secure-proxy';
import { createSafeError } from '../safe-error';
import { MemoryTracker, MemoryLimitError } from '../memory-tracker';
import { createHostToolBridge } from '../tool-bridge';
Expand Down Expand Up @@ -1186,6 +1186,8 @@ export class VmAdapter implements SandboxAdapter {
maxProperties: clampedProperties,
allowDates: true,
allowErrors: true,
// The membrane wraps the script's Dates; read them through their targets.
unwrap: unwrapSecureProxy,
});

return {
Expand Down
16 changes: 16 additions & 0 deletions libs/core/src/secure-proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1028,6 +1028,22 @@ export function isSecureProxy(obj: unknown): boolean {
return proxySet.has(obj as object);
}

/**
* The object a secure proxy wraps, or `value` itself when it is not one.
*
* For host code that must read a sandbox value's internal slots, which a proxy does not carry
* (a proxied Date fails `Date.prototype.getTime`). Never hand the result back to the sandbox.
*/
export function unwrapSecureProxy<T>(value: T): T {
let current: unknown = value;
while (current !== null && (typeof current === 'object' || typeof current === 'function')) {
const target = proxyToTarget.get(current as object);
if (target === undefined) break;
current = target;
}
return current as T;
}

/**
* Clear the proxy cache
*
Expand Down
28 changes: 18 additions & 10 deletions libs/core/src/value-sanitizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@ export interface SanitizeOptions {
* @default true
*/
allowErrors?: boolean;

/**
* Returns the object to check for a Date, Map, Set or RegExp in place of a wrapper around it
* (the single VM's secure proxy, which has no internal slots). Only builtins are read from the
* unwrapped object; everything else is still read through `value`.
*/
unwrap?: (value: object) => object;
}

/**
Expand Down Expand Up @@ -270,19 +277,20 @@ export function sanitizeValue(
return result;
}

const isPlain = isPlainObject(value as object);
const inspected = options.unwrap ? options.unwrap(value as object) : (value as object);
const isPlain = isPlainObject(inspected);

// Handle Date objects, including the sandbox realm's.
// SECURITY: read the timestamp via the captured Date.prototype.getTime (Reflect.apply),
// never `value.getTime()`, so the value cannot supply its own method.
if (!isPlain && hasBrand(DateProtoGetTime, value as object)) {
const time = ReflectApply(DateProtoGetTime, value, []) as number;
if (!isPlain && hasBrand(DateProtoGetTime, inspected)) {
const time = ReflectApply(DateProtoGetTime, inspected, []) as number;
if (allowDates) {
// Return a new Date to prevent reference sharing
return new Date(time);
}
// Convert to ISO string if dates not allowed
return ReflectApply(DateProtoToISOString, value, []) as string;
return ReflectApply(DateProtoToISOString, inspected, []) as string;
}

// Handle Error objects
Expand All @@ -305,17 +313,17 @@ export function sanitizeValue(
}

// Handle RegExp objects (convert to their /source/flags string)
if (!isPlain && hasBrand(RegExpProtoSourceGetter, value as object)) {
return regExpToString(value as object);
if (!isPlain && hasBrand(RegExpProtoSourceGetter, inspected)) {
return regExpToString(inspected);
}

// Handle Map objects, including the sandbox realm's.
// SECURITY: enumerate with the captured Map.prototype.entries (Reflect.apply) rather than
// `value.entries()`, so the value cannot hijack the iteration. The returned iterator is a
// genuine host Map Iterator, safe to `for..of`.
if (!isPlain && hasBrand(MapProtoSizeGetter, value as object)) {
if (!isPlain && hasBrand(MapProtoSizeGetter, inspected)) {
const sanitizedMap: Record<string, unknown> = ObjectCreate(null);
const entries = ReflectApply(MapProtoEntries, value, []) as IterableIterator<[unknown, unknown]>;
const entries = ReflectApply(MapProtoEntries, inspected, []) as IterableIterator<[unknown, unknown]>;
for (const [key, val] of entries) {
if (typeof key !== 'string') continue; // Only string keys
if (DANGEROUS_KEYS.has(key)) continue; // Skip dangerous keys
Expand All @@ -328,8 +336,8 @@ export function sanitizeValue(
// Handle Set objects (convert to array), including the sandbox realm's.
// SECURITY: enumerate with the captured Set.prototype.values (Reflect.apply), not
// `Array.from(value)` / the value's own iterator, so the value cannot hijack it.
if (!isPlain && hasBrand(SetProtoSizeGetter, value as object)) {
const iter = ReflectApply(SetProtoValues, value, []) as IterableIterator<unknown>;
if (!isPlain && hasBrand(SetProtoSizeGetter, inspected)) {
const iter = ReflectApply(SetProtoValues, inspected, []) as IterableIterator<unknown>;
const result: unknown[] = [];
for (const item of iter) {
context.propCount.count++;
Expand Down
Loading