Skip to content

Commit 8f93c79

Browse files
cursoragentcursoragent
committed
Harden capture against lazy-init tampering of Object.create and bind
Per Bugbot review feedback on PR #2729: even with Object.create(null) as the cache, the construction goes through globalThis.Object.create — which page JS can replace before transport construction runs, because Messaging is lazy on ContentFeature.messaging. Same concern applies to Function.prototype.bind used at capture time. - Switch capturedWebkitHandlers initializer from Object.create(null) to the { __proto__: null } literal, which is a syntactic construct rather than a method dispatch and so cannot be tampered with by page JS. - Stop calling .bind() at capture time. Store the handler object and the raw postMessage function as a { handler, postMessage } pair, and dispatch in wkSend via the captured ReflectApply (already imported from captured-globals.js, which captures Reflect.apply.bind(Reflect) at module-load time before any page JS can run). Adds two regression tests: - 'cache is not derived from a tampered Object.create' replaces Object.create with a tampered factory before the transport is constructed and asserts that the cache still rejects an unknown handler name with MissingHandler (rather than resolving a property injected by the tampered factory). - 'stores handler and postMessage as a separate pair' structurally asserts the capture stores raw references rather than a bound function, proving Function.prototype.bind is not on the capture or send path. (Direct Function.prototype.bind tampering in a Jasmine test breaks Jasmine itself because its spy machinery uses bind, so the structural assertion is the safe equivalent.) - 'dispatches through the captured handler with the correct `this`' asserts the send path preserves host-binding semantics via ReflectApply rather than a bare unbound call.
1 parent de772b6 commit 8f93c79

2 files changed

Lines changed: 98 additions & 13 deletions

File tree

‎injected/unit-test/messaging-webkit.spec.js‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,4 +120,68 @@ describe('WebkitMessagingTransport', () => {
120120
delete Object.prototype.hostilePollutionHandler;
121121
}
122122
});
123+
124+
it('cache is not derived from a tampered `Object.create`', () => {
125+
// Simulate page JS replacing Object.create *before* the transport is
126+
// constructed (which can happen because Messaging is lazy on
127+
// ContentFeature). The cache must still be a true null-prototype
128+
// object, not one synthesised by the tampered Object.create.
129+
const originalCreate = Object.create;
130+
const tampered = jasmine.createSpy('tamperedObjectCreate').and.callFake(() => ({ polluted: true }));
131+
Object.create = /** @type {any} */ (tampered);
132+
133+
try {
134+
env = setupWebkit();
135+
const transport = makeTransport();
136+
137+
// If the cache went through the tampered Object.create, looking up
138+
// `polluted` would resolve to `true` (the tampered factory injected
139+
// it as an own property). With `{ __proto__: null }` literal, it
140+
// doesn't.
141+
expect(() => transport.wkSend('polluted', {})).toThrowMatching(
142+
(e) => e?.name === 'MissingHandler' || /Missing webkit handler/.test(e?.message ?? ''),
143+
);
144+
} finally {
145+
Object.create = originalCreate;
146+
}
147+
});
148+
149+
it('stores handler and postMessage as a separate pair, not a single bound function', () => {
150+
// Storing a `{ handler, postMessage }` pair rather than the result of
151+
// `handler.postMessage.bind(handler)` is what lets wkSend dispatch via
152+
// captured ReflectApply without going through Function.prototype.bind
153+
// — which is critical because page JS can replace
154+
// Function.prototype.bind before lazy transport construction
155+
// (Messaging is lazy on ContentFeature.messaging). The structural
156+
// check below proves the cache holds the raw references rather than a
157+
// bound copy: a bound function would not be reference-equal to the
158+
// original `postMessage`.
159+
env = setupWebkit();
160+
const transport = makeTransport();
161+
162+
const stored = transport.capturedWebkitHandlers.contentScopeScripts;
163+
164+
expect(stored.handler).toBe(env.handlers.contentScopeScripts);
165+
expect(stored.postMessage).toBe(env.handlers.contentScopeScripts.postMessage);
166+
});
167+
168+
it('dispatches through the captured handler with the correct `this` even without `.bind`', () => {
169+
// Replace the host handler's postMessage with one that asserts on its
170+
// `this`. If wkSend dispatched via a bare call (`postMessage(data)`)
171+
// rather than ReflectApply against the captured handler, `this` would
172+
// be wrong (undefined in strict mode) and the call would not preserve
173+
// host-binding semantics.
174+
env = setupWebkit();
175+
const handler = env.handlers.contentScopeScripts;
176+
const observed = { thisValue: /** @type {any} */ (undefined) };
177+
handler.postMessage = function postMessage(/** @type {any} */ _data) {
178+
observed.thisValue = this;
179+
return Promise.resolve('{}');
180+
};
181+
const transport = makeTransport();
182+
183+
transport.wkSend('contentScopeScripts', { hello: 'world' });
184+
185+
expect(observed.thisValue).toBe(handler);
186+
});
123187
});

‎messaging/lib/webkit.js‎

Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,13 @@
66
import { MessagingTransport, MissingHandler } from '../index.js';
77
import { isResponseFor, isSubscriptionEventFor } from '../schema.js';
88
import { ensureNavigatorDuckDuckGo } from '../../injected/src/navigator-global.js';
9-
import { JSONparse, Error as _Error, ReflectDeleteProperty, objectDefineProperty } from '../../injected/src/captured-globals.js';
9+
import {
10+
JSONparse,
11+
Error as _Error,
12+
ReflectApply,
13+
ReflectDeleteProperty,
14+
objectDefineProperty,
15+
} from '../../injected/src/captured-globals.js';
1016

1117
/**
1218
* @example
@@ -25,9 +31,16 @@ export class WebkitMessagingTransport {
2531
/**
2632
* Null-prototype cache so a hostile page that pollutes `Object.prototype`
2733
* cannot supply a callable from there if `capture` ever misses a handler.
28-
* @type {Record<string, any>}
34+
*
35+
* Uses the `{ __proto__: null }` literal rather than `Object.create(null)`
36+
* because the latter is a method dispatch through `globalThis.Object`, which
37+
* page JS could replace before this class field runs if transport
38+
* construction is deferred (`Messaging` is lazy on `ContentFeature.messaging`).
39+
* The `__proto__: null` literal is a syntactic construct, not method
40+
* dispatch, so it always yields a true null-prototype object.
41+
* @type {Record<string, { handler: any, postMessage: Function }>}
2942
*/
30-
capturedWebkitHandlers = Object.create(null);
43+
capturedWebkitHandlers = /** @type {any} */ ({ __proto__: null });
3144

3245
/**
3346
* @param {WebkitMessagingConfig} config
@@ -56,10 +69,12 @@ export class WebkitMessagingTransport {
5669
*/
5770
wkSend(handler, data = {}) {
5871
const captured = this.capturedWebkitHandlers[handler];
59-
if (typeof captured !== 'function') {
72+
if (!captured || typeof captured.postMessage !== 'function') {
6073
throw new MissingHandler(`Missing webkit handler: '${handler}'`, handler);
6174
}
62-
return captured(data);
75+
// Use the captured ReflectApply so the send path doesn't go through
76+
// any page-mutable function (`.call`, `.bind`, etc.).
77+
return ReflectApply(captured.postMessage, captured.handler, [data]);
6378
}
6479

6580
/**
@@ -108,20 +123,26 @@ export class WebkitMessagingTransport {
108123
* `window.webkit.messageHandlers` (e.g. by privacy hardening that nullifies
109124
* the namespace for site JS to reduce fingerprinting surface).
110125
*
126+
* Stores the handler object and its `postMessage` function as a pair so
127+
* `wkSend` can dispatch via the captured `ReflectApply` rather than calling
128+
* `.bind()` here. `.bind` is a method on the page-mutable
129+
* `Function.prototype` — if transport construction is deferred (`Messaging`
130+
* is lazy on `ContentFeature.messaging`) page JS could replace
131+
* `Function.prototype.bind` first and have the cache store an attacker-
132+
* controlled function. Storing the unbound pair sidesteps that.
133+
*
111134
* @param {string[]} handlerNames
112135
*/
113136
captureWebkitHandlers(handlerNames) {
114137
const handlers = window.webkit.messageHandlers;
115138
if (!handlers) throw new MissingHandler('window.webkit.messageHandlers was absent', 'all');
116139
for (const webkitMessageHandlerName of handlerNames) {
117-
if (typeof handlers[webkitMessageHandlerName]?.postMessage === 'function') {
118-
/**
119-
* `bind` is used here to ensure future calls to the captured
120-
* `postMessage` have the correct `this` context
121-
*/
122-
const original = handlers[webkitMessageHandlerName];
123-
const bound = handlers[webkitMessageHandlerName].postMessage?.bind(original);
124-
this.capturedWebkitHandlers[webkitMessageHandlerName] = bound;
140+
const handler = handlers[webkitMessageHandlerName];
141+
if (typeof handler?.postMessage === 'function') {
142+
this.capturedWebkitHandlers[webkitMessageHandlerName] = {
143+
handler,
144+
postMessage: handler.postMessage,
145+
};
125146
}
126147
}
127148
}

0 commit comments

Comments
 (0)