Skip to content
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

hkdf API is inconsistent with other supported KDFs #39471

Closed
tniessen opened this issue Jul 20, 2021 · 5 comments
Closed

hkdf API is inconsistent with other supported KDFs #39471

tniessen opened this issue Jul 20, 2021 · 5 comments
Labels
crypto Issues and PRs related to the crypto subsystem. security Issues and PRs related to security. wontfix Issues that will not be fixed.

Comments

@tniessen
Copy link
Member

Node.js supports three KDFs:

const password = '123';
const salt = '123';
const keyLength = 10;
const digest = 'sha256';
const iterations = 1000;
const info = 'info';

const pbkdf2 = crypto.pbkdf2Sync(password, salt, iterations, keyLength, digest);
const scrypt = crypto.scryptSync(password, salt, keyLength);
const hkdf = crypto.hkdfSync(digest, password, salt, info, keyLength);

console.log({ pbkdf2, scrypt, hkdf });

Output:

{
  pbkdf2: <Buffer 00 1c 95 97 43 5c 16 7a 03 6d>,
  scrypt: <Buffer 92 8a 6a ad 7d 2b 8c f8 49 20>,
  hkdf: ArrayBuffer {
    [Uint8Contents]: <bb b5 51 4b 6b c0 57 34 ea e0>,
    byteLength: 10
  }
}

Inconsistencies:

  • Unlike the two more established key derivation functions, HKDF produces an ArrayBuffer, not a Node.js Buffer. In the async case, the callback even has the same signature and same argument names (err, derivedKey), but they derived key is not a Buffer when using HKDF.
  • The arguments are in a different order than the arguments to pbkdf2.

And this can absolutely lead to security issues. The output is computationally indistinguishable from a random bit sequence and all three KDFs are collision-resistant and preimage-resistant, therefore, the comparison of outputs does not necessarily need to be timing-safe. While not advisable, it is possible (depending on the application) to compare the outputs in a timing-unsafe manner without compromising the password

a.toString('hex') === b.toString('hex')

to compare the output of the KDF to a known correct value, e.g., as part of a login procedure. Now change a and b to ArrayBuffer each, and suddenly, the condition is always true, since

hkdf.toString('hex') === '[object ArrayBuffer]'

When scrypt was added, we had a lengthy discussion about designing KDF APIs. Quoting #21766 (comment):

cryptography API design is very important to application security in a lot of ways that aren't always immediately obvious. Node.js cryptography API security is, therefore, emphatically a security issue that affects Node.js.


Is there anything we can do about it now? Or simply label it wontfix and close the issue?

Refs: #21766
Refs: #35093
Refs: #39453

@tniessen tniessen added crypto Issues and PRs related to the crypto subsystem. security Issues and PRs related to security. labels Jul 20, 2021
@panva
Copy link
Member

panva commented Jul 20, 2021

The only non-breaking way out of it I see is

  1. deprecate now and later remove hkdf
  2. expose 'crypto/kdf' module => { hkdf, scrypt, pbkdf2 } with scrypt and pbhkf2 being aliases? while hkdf having Buffer as a return type and well-ordered arguments

KDFs would having their own module is then the remaining inconsistency with the rest of the crypto module?

@jasnell
Copy link
Member

jasnell commented Jul 20, 2021

I disagree with it being an actual issue but, I won't block if someone wants to change things so long as hkdf is not removed.

That said, there's no reason to expose a different module or make any breaking changes to the existing APIs. We can simply expose a single crypto.kdf(options) function...

const pbkdf2 = crypto.kdfSync({
  type: 'pbkdf2',
  password,
  salt,
  iterations,
  keyLength,
  digest,
  arrayBuffer: [true|false],
});

const scrypt = crypto.kdfSync({
  type: 'scrypt',
  password,
  salt,
  keyLength,
  arrayBuffer: [true|false],
});

const hkdf = crypto.kdfSync({
  type: 'hkdf',
  digest,
  password,
  salt,
  info,
  keyLength,
  arrayBuffer: [true|false],
);

@tniessen
Copy link
Member Author

I don't have a strong opinion. I do believe that the current API is not ideal and should have aimed for consistency with Node.js and not with WebCrypto, but I don't see a way to solve that problem now without breaking existing code. Adding more APIs makes things more complicated and does not actually solve the problem at hand. I opened this issue mostly so that, when someone does come across this problem, they will find a resolution, even if that resolution is wontfix.

@panva
Copy link
Member

panva commented Jul 21, 2021

Adding more APIs makes things more complicated and does not actually solve the problem at hand.

Why does replacing the current API not solve the problem at hand?

jasnell pushed a commit that referenced this issue Jul 26, 2021
PR-URL: #39474
Refs: #39471
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this issue Aug 2, 2021
PR-URL: #39474
Refs: #39471
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@tniessen tniessen added the wontfix Issues that will not be fixed. label Apr 19, 2023
@tniessen
Copy link
Member Author

I still think it's not ideal that we are being inconsistent in when we return Buffer, ArrayBuffer, or Uint8Array, but I don't see this improving, so I am closing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. security Issues and PRs related to security. wontfix Issues that will not be fixed.
Projects
None yet
Development

No branches or pull requests

3 participants