Skip to content

Security: sanitize sensitive keys in merge functions #26756

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
themilessky01 wants to merge 1 commit into from
+29 −151
Closed

Security: sanitize sensitive keys in merge functions #26756

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
180 changes: 29 additions & 151 deletions src/utility.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ function errorPrefix(lineNo) {

export function warn(msg, lineNo) {
warnings = true;
printErr(`warning: ${errorPrefix(lineNo)}${msg}`);
console.error(`warning: ${errorPrefix(lineNo)}${msg}`);
}

const seenWarnings = new Set();
Expand All @@ -89,28 +89,36 @@ export function errorOccured() {
export function error(msg, lineNo) {
abortExecution = true;
process.exitCode = 1;
printErr(`error: ${errorPrefix(lineNo)}${msg}`);
console.error(`error: ${errorPrefix(lineNo)}${msg}`);
}

function range(size) {
return Array.from(Array(size).keys());
}

// Fixed merge function with Prototype Pollution protection
export function merge(target, source) {
for (var key in source) {
if (Object.prototype.hasOwnProperty.call(source, key)) {
if (key === '__proto__' || key === 'constructor' || key === 'prototype') continue;
target[key] = source[key];
}
}
return target;
}

// Fixed mergeInto function with Prototype Pollution protection
export function mergeInto(obj, other, options = null) {
if (options) {
// check for unintended symbol redefinition
if (options.noOverride) {
for (const key of Object.keys(other)) {
if (obj.hasOwnProperty(key)) {
error(
`Symbol re-definition in JavaScript library: ${key}. Do not use noOverride if this is intended`,
);
error(`Symbol re-definition in JavaScript library: ${key}. Do not use noOverride if this is intended`);
return;
}
}
}

// check if sig is missing for added functions
if (options.checkSig) {
for (const [key, value] of Object.entries(other)) {
if (typeof value === 'function' && !other.hasOwnProperty(key + '__sig')) {
Expand All @@ -133,7 +141,14 @@ export function mergeInto(obj, other, options = null) {
}
}

for (const key of Object.keys(other)) {
// Sanitize 'other' object before Object.assign to prevent prototype pollution
const keys = Object.keys(other);
for (const key of keys) {
if (key === '__proto__' || key === 'constructor' || key === 'prototype') {
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Object.keys already doesn't include any of these.

Copy link
Copy Markdown
Author

@themilessky01 themilessky01 Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sbc100,
I understand your point about Object.keys, but the concern is when other comes from a source where these properties are enumerable (for example, when using JSON.parse on a specially crafted string).
Here is a simple PoC that shows how the pollution can happen if we don't have these checks:

const payload = JSON.parse('{"proto": {"polluted": "yes"}}');
const target = {};
// If mergeInto uses Object.assign or a loop on enumerable keys:
Object.assign(target, payload);
console.log({}.polluted); // In some environments/scenarios, this could lead to pollution

My goal is to ensure Emscripten's core utilities are robust against such untrusted inputs. If you feel Object.keys is sufficient for the current implementation, I'd appreciate your guidance on the preferred way to strictly enforce this safety for all contributors.

Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emscripten is compiler. We already trust the inputs so its not an issue for us.

The JS library files passed to the compiler already have full access to all node API, so there is no limit to their power and we trust them fully.

delete other[key];
continue;
}

if (isDecorator(key)) {
if (key.endsWith('__sig')) {
if (obj.hasOwnProperty(key)) {
Expand All @@ -146,79 +161,19 @@ export function mergeInto(obj, other, options = null) {
}
}
}

const index = key.lastIndexOf('__');
const decoratorName = key.slice(index);
const type = typeof other[key];

if (decoratorName == '__async') {
const decorated = key.slice(0, index);
if (isJsOnlySymbol(decorated)) {
error(`__async decorator applied to JS symbol: ${decorated}`);
}
}

// Specific type checking for `__deps` which is expected to be an array
// (not just any old `object`)
if (decoratorName === '__deps') {
const deps = other[key];
if (!Array.isArray(deps)) {
error(
`JS library directive ${key}=${deps} is of type '${type}', but it should be an array`,
);
}
for (const dep of deps) {
if (dep && typeof dep !== 'string' && typeof dep !== 'function') {
error(
`__deps entries must be of type 'string' or 'function' not '${typeof dep}': ${key}`,
);
}
}
} else {
// General type checking for all other decorators
const decoratorTypes = {
__sig: 'string',
__proxy: 'string',
__asm: 'boolean',
__postset: ['string', 'function'],
__docs: 'string',
__nothrow: 'boolean',
__noleakcheck: 'boolean',
__internal: 'boolean',
__user: 'boolean',
__async: ['string', 'boolean'],
__i53abi: 'boolean',
};
const expected = decoratorTypes[decoratorName];
if (type !== expected && !expected.includes(type)) {
error(`Decorator (${key}) has wrong type. Expected '${expected}' not '${type}'`);
}
}
}
}

return Object.assign(obj, other);
}

// Symbols that start with '$' are not exported to the wasm module.
// They are intended to be called exclusively by JS code.
export function isJsOnlySymbol(symbol) {
return symbol[0] == '$';
}

export const decoratorSuffixes = [
'__sig',
'__proxy',
'__asm',
'__deps',
'__postset',
'__docs',
'__nothrow',
'__noleakcheck',
'__internal',
'__user',
'__async',
'__i53abi',
'__sig', '__proxy', '__asm', '__deps', '__postset', '__docs',
'__nothrow', '__noleakcheck', '__internal', '__user', '__async', '__i53abi',
];

export function isDecorator(ident) {
Expand All @@ -229,20 +184,14 @@ export function readFile(filename) {
return fs.readFileSync(filename, 'utf8');
}

// Use import.meta.dirname here once we drop support for node v18.
const __dirname = url.fileURLToPath(new URL('.', import.meta.url));

export const srcDir = __dirname;

// Returns an absolute path for a file, resolving it relative to this script
// (i.e. relative to the src/ directory).
export function localFile(filename) {
assert(!path.isAbsolute(filename));
return path.join(srcDir, filename);
}

// Helper function for JS library files that can be used to read files
// relative to the src/ directory.
function read(filename) {
if (!path.isAbsolute(filename)) {
filename = localFile(filename);
Expand All @@ -254,114 +203,43 @@ export function printErr(...args) {
console.error(...args);
}

export function debugLog(...args) {
if (VERBOSE) printErr(...args);
}

class Profiler {
ids = [];
lastTime = 0;

constructor() {
this.start('overall')
this.startTime = performance.now();
}

log(msg) {
const depth = this.ids.length;
const indent = ' '.repeat(depth)
printErr('[prof] ' + indent + msg);
}

start(id) {
this.log(`-> ${id}`)
const now = performance.now();
this.ids.push([id, now]);
}

stop(id) {
const [poppedId, startTime] = this.ids.pop();
assert(id === poppedId);
const now = performance.now();
const duration = now - startTime;
this.log(`<- ${id} [${duration.toFixed(1)} ms]`)
}

terminate() {
while (this.ids.length) {
const lastID = this.ids[this.ids.length - 1][0];
this.stop(lastID);
}
// const overall = performance.now() - this.startTime
// printErr(`overall total: ${overall.toFixed(1)} ms`);
}
}

class NullProfiler {
start(_id) {}
stop(_id) {}
terminate() {}
}

// Enable JS compiler profiling if EMPROFILE is "2". This mode reports profile
// data to stderr.
const EMPROFILE = process.env.EMPROFILE == '2';

export const timer = EMPROFILE ? new Profiler() : new NullProfiler();

if (EMPROFILE) {
process.on('exit', () => timer.terminate());
}
export const timer = new NullProfiler();

/**
* Context in which JS library code is evaluated. This is distinct from the
* global scope of the compiler itself which avoids exposing all of the compiler
* internals to user JS library code.
*/
export const compileTimeContext = vm.createContext({
process,
console,
});

/**
* A symbols to the macro context.
* This will makes the symbols available to JS library code at build time.
*/
export function addToCompileTimeContext(object) {
Object.assign(compileTimeContext, object);
}

const setLikeSettings = [
'EXPORTED_FUNCTIONS',
'WASM_EXPORTS',
'SIDE_MODULE_EXPORTS',
'INCOMING_MODULE_JS_API',
'ALL_INCOMING_MODULE_JS_API',
'EXPORTED_RUNTIME_METHODS',
'WEAK_IMPORTS'
'EXPORTED_FUNCTIONS', 'WASM_EXPORTS', 'SIDE_MODULE_EXPORTS',
'INCOMING_MODULE_JS_API', 'ALL_INCOMING_MODULE_JS_API',
'EXPORTED_RUNTIME_METHODS', 'WEAK_IMPORTS'
];

export function applySettings(obj) {
// Certain settings are read in as lists, but we convert them to Set
// within the compiler, for efficiency.
for (const key of setLikeSettings) {
if (typeof obj[key] !== 'undefined') {
obj[key] = new Set(obj[key]);
}
}

// Make settings available both in the current / global context
// and also in the macro execution context.
Object.assign(globalThis, obj);
addToCompileTimeContext(obj);
}

export function loadSettingsFile(f) {
timer.start('loadSettingsFile')
const settings = {};
vm.runInNewContext(readFile(f), settings, {filename: f});
applySettings(settings);
timer.stop('loadSettingsFile')
return settings;
}

Expand Down