-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
crypto: fix webcrypto derive(Bits|Key) resolve values and docs #38148
Conversation
This comment has been minimized.
This comment has been minimized.
@@ -122,7 +122,7 @@ async function pbkdf2DeriveBits(algorithm, baseKey, length) { | |||
return new Promise((resolve, reject) => { | |||
pbkdf2(raw, salt, iterations, byteLength, hash, (err, result) => { | |||
if (err) return reject(err); | |||
resolve(result); | |||
resolve(result.buffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, pbkdf2
is only required to give us a Buffer
, which does not necessarily have the same byte length as the backing ArrayBuffer
:
> Buffer.from([1, 2, 3]).buffer.byteLength
8192
> crypto.randomFillSync(Buffer.allocUnsafe(1024)).buffer.byteLength
8192
That's one of many reasons why WebCrypto doesn't align well with Node.js. However, we know that the implementation of PBKDF2 always returns a Buffer
that has the same byteLength
as the backing ArrayBuffer
. And, hopefully, tests will catch it if that ever changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, i've checked that they do. Alternatively we can do new Uint8Array(result).buffer
> const foo = Buffer.from('foo')
> foo.buffer.byteLength
8192
> new Uint8Array(foo).buffer
ArrayBuffer { [Uint8Contents]: <66 6f 6f>, byteLength: 3 }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these would be places where adding assertions about matching lengths and byteOffsets might make sense :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we can do
new Uint8Array(result).buffer
That would create an unnecessary copy of sensitive data (the result of PBKDF2 is considered secret). And yes, I realize that arguing about memory safety is moot in JavaScript :)
Landed in 896dc39 |
Couple of issues discovered in #38115 addressed by this PR
PBKDF2
deriveBits resolve value typeNODE-SCRYPT
deriveBits resolve value typesubtle.deriveKey
resolve value type### Deriving bits and keys
example does not work withderiveBits
ad resolve value types see SubtleCrypto interface definition.