crypto: allow zero-length secrets and IKM in HKDF (and in webcrypto PBKDF2) by panva · Pull Request #44201 · nodejs/node

panva · 2022-08-10T13:09:43Z

This PR

adds support for zero-length secret KeyObject
allows a zero-length IKM in HKDF (crypto and webcrypto module)
allows a zero-length password in PBKDF2 (webcrypto module, crypto already supports it)

This picks up one of the individual items from #43656 and fixes ~1870 WPTs.

nodejs-github-bot · 2022-08-10T13:09:47Z

Review requested:

@nodejs/crypto

LiviaMedeiros · 2022-08-11T05:46:56Z

If the provided key is a detached ArrayBuffer or a view of that, should it be interpreted as error or as zero-length key?

panva · 2022-08-11T07:38:45Z

@LiviaMedeiros I have to say I don't know enough about detached ArrayBuffer to make the call, would you please explain (or link to explanation of) what a detached ArrayBuffer is, what its unique properties are, and how might I end up with one in Node.js?

LiviaMedeiros · 2022-08-11T08:19:45Z

ArrayBuffers are Transferable objects, which means they can be transfered "by reference" between contexts. Transfering (for example, into of from a worker) makes the content of buffer detached from the source context, so it's no more accessible from it.

Spec-wise, it is done by 25.1.2.3 DetachArrayBuffer.

Code-wise, it can be demonstrated with this:

import assert from 'node:assert';
const view = new Uint8Array([1, 2, 3]);
const buffer = view.buffer;
assert(view.byteLength === 3);
assert(buffer.byteLength === 3);

new MessageChannel().port1.postMessage(buffer, [buffer]); // second argument is an array of objects that are transfered
assert(buffer.byteLength === 0); // detached buffer has zero length
assert(view.byteLength === 0); // any view on detached buffer also has zero length
const viewDetached = new Uint8Array(buffer); // TypeError: Cannot perform Construct on a detached ArrayBuffer

Function to make a view and buffer for testing purposes:

const makeDetachedBuffer = () => {
	const view = new Uint8Array();
	new MessageChannel().port1.postMessage('', [view.buffer]);
	return { view, buffer: view.buffer };
}

Context-wise, detached buffers are a subset of zero-length buffers, and they can be used by mistake in userland code. I'm not sure if webcrypto specs explicitly allows or restricts that, but there is possibility that it will either be silently interpreted as empty key, or throw a TypeError at some point.

panva · 2022-08-11T08:24:32Z

Thank you @LiviaMedeiros, I will do some more testing in browsers.

panva · 2022-08-11T09:30:47Z

I'm not sure if webcrypto specs explicitly allows or restricts that, but there is possibility that it will either be silently interpreted as empty key, or throw a TypeError at some point.

WebCryptoAPI is silent on the matter and the browsers I've tested (Chrome, Safari, Firefox) do not throw on import of detached buffers.

Unify the implementation and perform the same OpenSSL calls regardless of whether the key and/or salt are empty. This simplifies the code and improves coverage. Refs: nodejs#44201 PR-URL: nodejs#44272 Reviewed-By: Filip Skokan <[email protected]>

juanarbol · 2022-10-03T02:36:34Z

This is is causing compile errors.

../src/crypto/crypto_hkdf.cc:134:28: error: no matching member function for call to 'data'
               params.salt.data(),
               ~~~~~~~~~~~~^~~~
../src/crypto/crypto_util.h:228:12: note: candidate template ignored: couldn't infer template argument 'T'
  const T* data() const { return reinterpret_cast<const T*>(get()); }
           ^
1 error generated.

PR-URL: nodejs#44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

PR-URL: nodejs#44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Backport-PR-URL: nodejs#44872

PR-URL: #44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Backport-PR-URL: #44872

Notable changes: assert: add `getCalls` and `reset` to callTracker (Moshe Atlow) #44191 crypto: allow zero-length IKM in HKDF and in webcrypto PBKDF2 (Filip Skokan) #44201 crypto: allow zero-length secret KeyObject (Filip Skokan) #44201 doc: deprecate modp1, modp2, and modp5 groups (Tobias Nießen) #44588 http: throw error on content-length mismatch (sidwebworks) #44378 http: make idle http parser count configurable (theanarkh) #43974 lib: add diagnostics channel for process and worker (theanarkh) #44045 net: add local family (theanarkh) #43975 net,tls: pass a valid socket on `tlsClientError` (Daeyeon Jeong) #44021 os: add machine method (theanarkh) #44416 report: expose report public native apis (Chengzhong Wu) #44255 src: expose environment RequestInterrupt api (Chengzhong Wu) #44362 stream: add `ReadableByteStream.tee()` (Daeyeon Jeong) #44505 test_runner: add before/after/each hooks (Moshe Atlow) #43730 util: add `maxArrayLength` option to Set and Map (Kohei Ueno) #43576

PR-URL: #44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Backport-PR-URL: #44872

Notable changes: assert: add `getCalls` and `reset` to callTracker (Moshe Atlow) #44191 crypto: allow zero-length secret KeyObject (Filip Skokan) #44201 crypto: allow zero-length IKM in HKDF and in webcrypto PBKDF2 (Filip Skokan) #44201 doc: deprecate modp1, modp2, and modp5 groups (Tobias Nießen) #44588 http: make idle http parser count configurable (theanarkh) #43974 http: throw error on content-length mismatch (sidwebworks) #44378 lib: add diagnostics channel for process and worker (theanarkh) #44045 net,tls: pass a valid socket on `tlsClientError` (Daeyeon Jeong) #44021 net: add local family (theanarkh) #43975 report: expose report public native apis (Chengzhong Wu) #44255 src: expose environment RequestInterrupt api (Chengzhong Wu) #44362 stream: add `ReadableByteStream.tee()` (Daeyeon Jeong) #44505 test_runner: add before/after/each hooks (Moshe Atlow) #43730 util: add `maxArrayLength` option to Set and Map (Kohei Ueno) #43576 PR-URL: #44886

panva requested review from bnoordhuis, jasnell and tniessen August 10, 2022 13:09

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 10, 2022

panva force-pushed the crypto-zero-length-hkdf-ikm branch from f0bdeaa to 2999a12 Compare August 10, 2022 13:10

panva mentioned this pull request Aug 10, 2022

crypto: pass all WebCryptoAPI WPTs #43656

Closed

panva added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2022

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Aug 11, 2022

CI Reliability 2022-08-11 nodejs/reliability#343

Open

7 tasks

panva marked this pull request as draft August 11, 2022 08:24

panva force-pushed the crypto-zero-length-hkdf-ikm branch from 2999a12 to 6b9e49e Compare August 11, 2022 09:24

panva marked this pull request as ready for review August 11, 2022 09:31

panva force-pushed the crypto-zero-length-hkdf-ikm branch from 6b9e49e to de3fc6c Compare August 11, 2022 09:41

This comment was marked as outdated.

Sign in to view

panva force-pushed the crypto-zero-length-hkdf-ikm branch from de3fc6c to f5890df Compare August 11, 2022 09:58

juanarbol added the backport-requested-v16.x label Oct 3, 2022

panva mentioned this pull request Oct 3, 2022

[v16.x backport] test,crypto: update WebCryptoAPI WPT #44839

Closed

panva added a commit to panva/node that referenced this pull request Oct 3, 2022

crypto: allow zero-length secret KeyObject

9c66739

PR-URL: nodejs#44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

panva added a commit to panva/node that referenced this pull request Oct 3, 2022

crypto: allow zero-length IKM in HKDF and in webcrypto PBKDF2

68421bc

PR-URL: nodejs#44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

panva mentioned this pull request Oct 3, 2022

[v16.x backport] WPT runner update and crypto followups #44872

Closed

panva added a commit to panva/node that referenced this pull request Oct 3, 2022

crypto: allow zero-length secret KeyObject

e60a0ed

PR-URL: nodejs#44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Backport-PR-URL: nodejs#44872

panva added backport-open-v16.x and removed backport-requested-v16.x labels Oct 3, 2022

panva added a commit to panva/node that referenced this pull request Oct 4, 2022

crypto: allow zero-length secret KeyObject

c47f887

PR-URL: nodejs#44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Backport-PR-URL: nodejs#44872

panva added a commit to panva/node that referenced this pull request Oct 4, 2022

crypto: allow zero-length secret KeyObject

2aae542

PR-URL: nodejs#44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Backport-PR-URL: nodejs#44872

panva added a commit to panva/node that referenced this pull request Oct 5, 2022

crypto: allow zero-length secret KeyObject

57fb475

PR-URL: nodejs#44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Backport-PR-URL: nodejs#44872

panva added a commit to panva/node that referenced this pull request Oct 7, 2022

crypto: allow zero-length secret KeyObject

46b9ff3

PR-URL: nodejs#44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Backport-PR-URL: nodejs#44872

panva added a commit to panva/node that referenced this pull request Oct 8, 2022

crypto: allow zero-length secret KeyObject

8492b7c

PR-URL: nodejs#44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Backport-PR-URL: nodejs#44872

juanarbol pushed a commit that referenced this pull request Oct 10, 2022

crypto: allow zero-length secret KeyObject

6a15dc6

PR-URL: #44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Backport-PR-URL: #44872

juanarbol pushed a commit that referenced this pull request Oct 10, 2022

crypto: allow zero-length IKM in HKDF and in webcrypto PBKDF2

fd53735

PR-URL: #44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Backport-PR-URL: #44872

juanarbol mentioned this pull request Oct 11, 2022

V16.18.0 proposal #44886

Merged

juanarbol pushed a commit that referenced this pull request Oct 11, 2022

crypto: allow zero-length secret KeyObject

e5c9975

PR-URL: #44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Backport-PR-URL: #44872

juanarbol pushed a commit that referenced this pull request Oct 11, 2022

crypto: allow zero-length IKM in HKDF and in webcrypto PBKDF2

317cd05

PR-URL: #44201 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Backport-PR-URL: #44872

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

crypto: allow zero-length secrets and IKM in HKDF (and in webcrypto PBKDF2)#44201

crypto: allow zero-length secrets and IKM in HKDF (and in webcrypto PBKDF2)#44201
panva wants to merge 2 commits intonodejs:mainfrom
panva:crypto-zero-length-hkdf-ikm

panva commented Aug 10, 2022 •

edited

Loading

Uh oh!

nodejs-github-bot commented Aug 10, 2022

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

LiviaMedeiros commented Aug 11, 2022

Uh oh!

panva commented Aug 11, 2022 •

edited

Loading

Uh oh!

LiviaMedeiros commented Aug 11, 2022

Uh oh!

panva commented Aug 11, 2022

Uh oh!

panva commented Aug 11, 2022

Uh oh!

This comment was marked as outdated.

juanarbol commented Oct 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

panva commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 10, 2022

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

LiviaMedeiros commented Aug 11, 2022

Uh oh!

panva commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LiviaMedeiros commented Aug 11, 2022

Uh oh!

panva commented Aug 11, 2022

Uh oh!

panva commented Aug 11, 2022

Uh oh!

This comment was marked as outdated.

juanarbol commented Oct 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

panva commented Aug 10, 2022 •

edited

Loading

panva commented Aug 11, 2022 •

edited

Loading