Skip to content

crypto: fix cross-realm ArrayBuffer validation #57828

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.

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

Merged
merged 1 commit into from
Apr 22, 2025

Conversation

fforbeck
Copy link
Contributor

@fforbeck fforbeck commented Apr 11, 2025

Problem

The Web Crypto API in Node.js v22+ fails to properly validate ArrayBuffer instances
created in different JavaScript realms. When an ArrayBuffer from a different realm
is passed to SubtleCrypto.digest(), it fails with:

TypeError: Failed to execute 'digest' on 'SubtleCrypto': 2nd argument is not instance of ArrayBuffer, Buffer, TypedArray, or DataView.

This is a common issue when:

  1. Using TypedArray.buffer with ArrayBuffers created in worker threads
  2. Using TypedArray.buffer with ArrayBuffers created in VM contexts
  3. In some cases, when libraries load in different contexts in bundled applications

The problem is in isNonSharedArrayBuffer() which checks using prototype
inheritance (ObjectPrototypeIsPrototypeOf(ArrayBufferPrototype, V)) which fails
for cross-realm objects, even though structurally they are valid ArrayBuffers.

Solution

This PR modifies isNonSharedArrayBuffer() to use isArrayBuffer from internal/util/types to properly check for cross-realm objects.

Testing

Added a new test that verifies that ArrayBuffers created in different VM contexts
are correctly recognized as valid inputs to SubtleCrypto.digest().

Workaround

Until this fix is merged, users can work around this issue by wrapping cross-realm ArrayBuffers in a TypedArray view before passing them to WebCrypto functions. Here's a simple example demonstrating the workaround:

'use strict';

// Create a cross-realm ArrayBuffer
const vm = require('vm');
const context = vm.createContext({});
const crossRealmBuffer = vm.runInContext('new ArrayBuffer(16)', context);

// Fill it with some data
const view = new Uint8Array(crossRealmBuffer);
for (let i = 0; i < view.length; i++) {
  view[i] = i % 256;
}

// This will fail without the fix:
// crypto.subtle.encrypt({ name: 'AES-GCM', iv }, key, crossRealmBuffer);

// Workaround: Use the TypedArray view instead
const ciphertext = await crypto.subtle.encrypt(
  { name: 'AES-GCM', iv },
  key,
  view  // Use the view instead of the cross-realm buffer
);

Ref: storacha/w3up#1591

@fforbeck fforbeck requested a review from a team as a code owner April 11, 2025 00:41
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions
  • @nodejs/releasers
  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Apr 11, 2025
@fforbeck fforbeck changed the base branch from main to v22.x April 11, 2025 00:41
@legendecas
Copy link
Member

In general, a patch should land on the main branch first. The same patch can apply to the main branch:

function isNonSharedArrayBuffer(V) {
return ObjectPrototypeIsPrototypeOf(ArrayBufferPrototype, V);
}
.

Would you mind re-target the PR to the main branch? Thank you

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

This PR should target the main branch not the nodejs:v22.x branch.

@fforbeck fforbeck force-pushed the fix/webcrypto-cross-realm-arraybuffer branch from 941520a to 4a2a300 Compare April 14, 2025 14:03
@fforbeck fforbeck changed the base branch from v22.x to main April 14, 2025 14:03
@fforbeck fforbeck force-pushed the fix/webcrypto-cross-realm-arraybuffer branch from 4a2a300 to 982d482 Compare April 14, 2025 14:06
@fforbeck
Copy link
Contributor Author

Thanks for the review @jasnell, @ljharb, and @legendecas. This is ready for another round.

@nodejs-github-bot
Copy link
Collaborator

@@ -194,7 +195,7 @@ converters.object = (V, opts) => {
};

function isNonSharedArrayBuffer(V) {
return ObjectPrototypeIsPrototypeOf(ArrayBufferPrototype, V);
return isArrayBuffer(V);
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively this could be:

const isNonSharedArrayBuffer = isArrayBuffer;

Also, should isSharedArrayBuffer(...) also be similarly updated here?

Copy link
Member

Choose a reason for hiding this comment

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

I would actually just remove the method and use isArrayBuffer() directly instead. It should be clear that it's not shared.

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused - a SharedArrayBuffer is also an ArrayBuffer, in terms of internal slots - what actual check does isArrayBuffer perform?

Copy link
Member

Choose a reason for hiding this comment

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

@legendecas legendecas removed the request for review from a team April 15, 2025 09:30
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

(with or without "remove isNonSharedArrayBuffer and use isArrayBuffer" directly)

Copy link

codecov bot commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.27%. Comparing base (29af9ce) to head (607a044).
Report is 53 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57828      +/-   ##
==========================================
+ Coverage   90.24%   90.27%   +0.03%     
==========================================
  Files         630      630              
  Lines      185670   186140     +470     
  Branches    36405    36481      +76     
==========================================
+ Hits       167555   168045     +490     
+ Misses      10998    10971      -27     
- Partials     7117     7124       +7     
Files with missing lines Coverage Δ
lib/internal/crypto/webidl.js 98.48% <100.00%> (-0.01%) ⬇️

... and 75 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aduh95
Copy link
Contributor

aduh95 commented Apr 15, 2025

There's a linter and a test error that would need to be adressed, let us know if you need help.

@fforbeck
Copy link
Contributor Author

There's a linter and a test error that would need to be adressed, let us know if you need help.

can you point out which test it that? thank you!

@aduh95
Copy link
Contributor

aduh95 commented Apr 15, 2025

There's a linter and a test error that would need to be adressed, let us know if you need help.

can you point out which test it that? thank you!

You can find it in the CI output, here it is:

=== release test-crypto-subtle-cross-realm ===
Path: parallel/test-crypto-subtle-cross-realm
Error: --- stderr ---
node:internal/modules/cjs/loader:1408
  throw err;
  ^

Error: Cannot find module 'internal/util/types'
Require stack:
- /home/runner/work/node/node/node/test/parallel/test-crypto-subtle-cross-realm.js
    at Function._resolveFilename (node:internal/modules/cjs/loader:1405:15)
    at defaultResolveImpl (node:internal/modules/cjs/loader:1061:19)
    at resolveForCJSWithHooks (node:internal/modules/cjs/loader:1066:22)
    at Function._load (node:internal/modules/cjs/loader:1215:37)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:235:24)
    at Module.require (node:internal/modules/cjs/loader:1491:12)
    at require (node:internal/modules/helpers:135:16)
    at Object.<anonymous> (/home/runner/work/node/node/node/test/parallel/test-crypto-subtle-cross-realm.js:10:27)
    at Module._compile (node:internal/modules/cjs/loader:1734:14) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/runner/work/node/node/node/test/parallel/test-crypto-subtle-cross-realm.js'
  ]
}

Node.js v24.0.0-pre
Command: out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/runner/work/node/node/node/test/parallel/test-crypto-subtle-cross-realm.js

===
=== 1 tests failed
===

@fforbeck
Copy link
Contributor Author

@aduh95 - I've made the change to enable the internal modules, but I can't trigger the test workflow. Can you help? Thanks

@nodejs-github-bot
Copy link
Collaborator

@fforbeck fforbeck force-pushed the fix/webcrypto-cross-realm-arraybuffer branch from 32bd471 to 3f31959 Compare April 21, 2025 12:33
@fforbeck
Copy link
Contributor Author

@aduh95 I've fixed the test and fixed the commit message. Can you approve the workflow execution again? Thank you!

@fforbeck fforbeck changed the title crypto: fix cross-realm ArrayBuffer validation in WebCrypto crypto: fix cross-realm ArrayBuffer validation Apr 21, 2025
This patch modifies the isNonSharedArrayBuffer function in the WebIDL
implementation for the SubtleCrypto API to properly handle ArrayBuffer
instances created in different JavaScript realms.

Before this fix, when a TypedArray.buffer from a different realm
(e.g., from a VM context or worker thread) was passed to
SubtleCrypto.digest(), it would fail with:

"TypeError: Failed to execute 'digest' on 'SubtleCrypto': 2nd argument
is not instance of ArrayBuffer, Buffer, TypedArray, or DataView."

The fix use the isArrayBuffer function from internal/util/types to detect
cross-realm ArrayBuffer instances when the prototype chain check fails.

This ensures compatibility with TypedArray.buffer across JavaScript realms.

See storacha/w3up#1591 for more details.
@fforbeck fforbeck force-pushed the fix/webcrypto-cross-realm-arraybuffer branch from 3f31959 to 607a044 Compare April 21, 2025 12:50
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 22, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2025
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 merged commit 6bf7fd7 into nodejs:main Apr 22, 2025
58 of 59 checks passed
@aduh95
Copy link
Contributor

aduh95 commented Apr 22, 2025

Landed in 6bf7fd7

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
This patch modifies the `isNonSharedArrayBuffer` function in the WebIDL
implementation for the SubtleCrypto API to properly handle `ArrayBuffer`
instances created in different JavaScript realms.

Before this fix, when a `TypedArray.buffer` from a different realm
(e.g., from a VM context or worker thread) was passed to
`SubtleCrypto.digest()`, it would fail with:

> TypeError: Failed to execute 'digest' on 'SubtleCrypto': 2nd argument
> is not instance of ArrayBuffer, Buffer, TypedArray, or DataView."

The fix use the `isArrayBuffer` function from `internal/util/types` to
detect cross-realm `ArrayBuffer` instances when the prototype chain
check fails.

PR-URL: #57828
Refs: storacha/w3up#1591
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
This patch modifies the `isNonSharedArrayBuffer` function in the WebIDL
implementation for the SubtleCrypto API to properly handle `ArrayBuffer`
instances created in different JavaScript realms.

Before this fix, when a `TypedArray.buffer` from a different realm
(e.g., from a VM context or worker thread) was passed to
`SubtleCrypto.digest()`, it would fail with:

> TypeError: Failed to execute 'digest' on 'SubtleCrypto': 2nd argument
> is not instance of ArrayBuffer, Buffer, TypedArray, or DataView."

The fix use the `isArrayBuffer` function from `internal/util/types` to
detect cross-realm `ArrayBuffer` instances when the prototype chain
check fails.

PR-URL: #57828
Refs: storacha/w3up#1591
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 6, 2025
This patch modifies the `isNonSharedArrayBuffer` function in the WebIDL
implementation for the SubtleCrypto API to properly handle `ArrayBuffer`
instances created in different JavaScript realms.

Before this fix, when a `TypedArray.buffer` from a different realm
(e.g., from a VM context or worker thread) was passed to
`SubtleCrypto.digest()`, it would fail with:

> TypeError: Failed to execute 'digest' on 'SubtleCrypto': 2nd argument
> is not instance of ArrayBuffer, Buffer, TypedArray, or DataView."

The fix use the `isArrayBuffer` function from `internal/util/types` to
detect cross-realm `ArrayBuffer` instances when the prototype chain
check fails.

PR-URL: #57828
Refs: storacha/w3up#1591
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 6, 2025
This patch modifies the `isNonSharedArrayBuffer` function in the WebIDL
implementation for the SubtleCrypto API to properly handle `ArrayBuffer`
instances created in different JavaScript realms.

Before this fix, when a `TypedArray.buffer` from a different realm
(e.g., from a VM context or worker thread) was passed to
`SubtleCrypto.digest()`, it would fail with:

> TypeError: Failed to execute 'digest' on 'SubtleCrypto': 2nd argument
> is not instance of ArrayBuffer, Buffer, TypedArray, or DataView."

The fix use the `isArrayBuffer` function from `internal/util/types` to
detect cross-realm `ArrayBuffer` instances when the prototype chain
check fails.

PR-URL: #57828
Refs: storacha/w3up#1591
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
This patch modifies the `isNonSharedArrayBuffer` function in the WebIDL
implementation for the SubtleCrypto API to properly handle `ArrayBuffer`
instances created in different JavaScript realms.

Before this fix, when a `TypedArray.buffer` from a different realm
(e.g., from a VM context or worker thread) was passed to
`SubtleCrypto.digest()`, it would fail with:

> TypeError: Failed to execute 'digest' on 'SubtleCrypto': 2nd argument
> is not instance of ArrayBuffer, Buffer, TypedArray, or DataView."

The fix use the `isArrayBuffer` function from `internal/util/types` to
detect cross-realm `ArrayBuffer` instances when the prototype chain
check fails.

PR-URL: #57828
Refs: storacha/w3up#1591
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 16, 2025
This patch modifies the `isNonSharedArrayBuffer` function in the WebIDL
implementation for the SubtleCrypto API to properly handle `ArrayBuffer`
instances created in different JavaScript realms.

Before this fix, when a `TypedArray.buffer` from a different realm
(e.g., from a VM context or worker thread) was passed to
`SubtleCrypto.digest()`, it would fail with:

> TypeError: Failed to execute 'digest' on 'SubtleCrypto': 2nd argument
> is not instance of ArrayBuffer, Buffer, TypedArray, or DataView."

The fix use the `isArrayBuffer` function from `internal/util/types` to
detect cross-realm `ArrayBuffer` instances when the prototype chain
check fails.

PR-URL: #57828
Refs: storacha/w3up#1591
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
This patch modifies the `isNonSharedArrayBuffer` function in the WebIDL
implementation for the SubtleCrypto API to properly handle `ArrayBuffer`
instances created in different JavaScript realms.

Before this fix, when a `TypedArray.buffer` from a different realm
(e.g., from a VM context or worker thread) was passed to
`SubtleCrypto.digest()`, it would fail with:

> TypeError: Failed to execute 'digest' on 'SubtleCrypto': 2nd argument
> is not instance of ArrayBuffer, Buffer, TypedArray, or DataView."

The fix use the `isArrayBuffer` function from `internal/util/types` to
detect cross-realm `ArrayBuffer` instances when the prototype chain
check fails.

PR-URL: #57828
Refs: storacha/w3up#1591
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
This patch modifies the `isNonSharedArrayBuffer` function in the WebIDL
implementation for the SubtleCrypto API to properly handle `ArrayBuffer`
instances created in different JavaScript realms.

Before this fix, when a `TypedArray.buffer` from a different realm
(e.g., from a VM context or worker thread) was passed to
`SubtleCrypto.digest()`, it would fail with:

> TypeError: Failed to execute 'digest' on 'SubtleCrypto': 2nd argument
> is not instance of ArrayBuffer, Buffer, TypedArray, or DataView."

The fix use the `isArrayBuffer` function from `internal/util/types` to
detect cross-realm `ArrayBuffer` instances when the prototype chain
check fails.

PR-URL: #57828
Refs: storacha/w3up#1591
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
This patch modifies the `isNonSharedArrayBuffer` function in the WebIDL
implementation for the SubtleCrypto API to properly handle `ArrayBuffer`
instances created in different JavaScript realms.

Before this fix, when a `TypedArray.buffer` from a different realm
(e.g., from a VM context or worker thread) was passed to
`SubtleCrypto.digest()`, it would fail with:

> TypeError: Failed to execute 'digest' on 'SubtleCrypto': 2nd argument
> is not instance of ArrayBuffer, Buffer, TypedArray, or DataView."

The fix use the `isArrayBuffer` function from `internal/util/types` to
detect cross-realm `ArrayBuffer` instances when the prototype chain
check fails.

PR-URL: #57828
Refs: storacha/w3up#1591
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 18, 2025
This patch modifies the `isNonSharedArrayBuffer` function in the WebIDL
implementation for the SubtleCrypto API to properly handle `ArrayBuffer`
instances created in different JavaScript realms.

Before this fix, when a `TypedArray.buffer` from a different realm
(e.g., from a VM context or worker thread) was passed to
`SubtleCrypto.digest()`, it would fail with:

> TypeError: Failed to execute 'digest' on 'SubtleCrypto': 2nd argument
> is not instance of ArrayBuffer, Buffer, TypedArray, or DataView."

The fix use the `isArrayBuffer` function from `internal/util/types` to
detect cross-realm `ArrayBuffer` instances when the prototype chain
check fails.

PR-URL: #57828
Refs: storacha/w3up#1591
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 19, 2025
This patch modifies the `isNonSharedArrayBuffer` function in the WebIDL
implementation for the SubtleCrypto API to properly handle `ArrayBuffer`
instances created in different JavaScript realms.

Before this fix, when a `TypedArray.buffer` from a different realm
(e.g., from a VM context or worker thread) was passed to
`SubtleCrypto.digest()`, it would fail with:

> TypeError: Failed to execute 'digest' on 'SubtleCrypto': 2nd argument
> is not instance of ArrayBuffer, Buffer, TypedArray, or DataView."

The fix use the `isArrayBuffer` function from `internal/util/types` to
detect cross-realm `ArrayBuffer` instances when the prototype chain
check fails.

PR-URL: #57828
Refs: storacha/w3up#1591
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants