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)#44201panva wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
|
If the provided key is a detached ArrayBuffer or a view of that, should it be interpreted as error or as zero-length key? |
Sorry, something went wrong.
|
@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? |
Sorry, something went wrong.
|
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 ArrayBufferFunction 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. |
Sorry, something went wrong.
|
Thank you @LiviaMedeiros, I will do some more testing in browsers. |
Sorry, something went wrong.
WebCryptoAPI is silent on the matter and the browsers I've tested (Chrome, Safari, Firefox) do not throw on import of detached buffers. |
Sorry, something went wrong.
This comment was marked as outdated.
|
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. |
Sorry, something went wrong.
PR-URL: nodejs#44201 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Backport-PR-URL: nodejs#44872
PR-URL: nodejs#44201 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Backport-PR-URL: nodejs#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
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
This PR
This picks up one of the individual items from #43656 and fixes ~1870 WPTs.