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

Review usage of HKDF #908

Open
1 of 3 tasks
fryorcraken opened this issue Aug 25, 2022 · 5 comments
Open
1 of 3 tasks

Review usage of HKDF #908

fryorcraken opened this issue Aug 25, 2022 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers track:restricted-run Restricted run track (Secure Messaging/Waku Product), e.g. filter, WebRTC
Projects

Comments

@fryorcraken
Copy link
Collaborator

fryorcraken commented Aug 25, 2022

Problem

It seems that ethereum-cryptography/pbkdf2does not use the subtle crypto API: https://github.com/status-im/status-web/pull/295/files

While we do not use ethereum-cryptography, we do use the noble libraries for cryptography, by the same author and we do some KDF operation:

function kdf(secret: Uint8Array, outputLength: number): Promise<Uint8Array> {

We need to check whether we could use a crypto subtle API for KDF operations to improve performance over a pure JavaScript implementation.

Acceptance Criteria

  • Review if we use pbkdf2
  • Push patch above upstream if needed
  • Review our usage of KDF and whether subtle crypto could be used
@fryorcraken fryorcraken added this to New in js-waku Aug 25, 2022
@fryorcraken fryorcraken added the track:restricted-run Restricted run track (Secure Messaging/Waku Product), e.g. filter, WebRTC label Aug 25, 2022
@fryorcraken
Copy link
Collaborator Author

Mark as good first issue as it does not require much knowledge of the codebase. It is an advance issue in terms of subject matter (cryptography).

@fryorcraken fryorcraken added enhancement New feature or request good first issue Good for newcomers and removed good first issue Good for newcomers labels Aug 26, 2022
@elijahhampton
Copy link

Hey @fryorcraken I can take this issue and close it out for you if you wish.

@fryorcraken
Copy link
Collaborator Author

Hey @fryorcraken I can take this issue and close it out for you if you wish.

🙏 any help is appreciated.

First step would be an analytical one to review KDF usage. Two questions to answer:

  • do we use kdf from noble libraries?
  • could our hand written kdf function be replaced with subtle crypto kdf?

Note that the test suite run test against another implementation, including asymmetric encryption (where ecies is used to derive an encryption key).
So one could replace the kdf code with a subtle crypto call and see if tests still pass.

Best if I can review the analysis before moving forward with a patch.

Let me know if you need more guidance.

@elijahhampton
Copy link

@fryorcraken Hey looking for a little guidance on this one. I sent you a message on Discord!

@fryorcraken fryorcraken changed the title Review usage of PBKDF2 Review usage of HKDF Mar 3, 2023
@fryorcraken
Copy link
Collaborator Author

fryorcraken commented Mar 3, 2023

I tried to play around to replace our kdf function (link in description) with CryptoSubtle's API:

export function hkdf(
  secret: Uint8Array,
  outputLength: number
): Promise<Uint8Array> {
  return getSubtle()
    .importKey("raw", secret, "HKDF", false, ["deriveKey"])
    .then((cryptoKey) =>
      getSubtle().deriveKey(
        {
          name: "HKDF",
          hash: "SHA-256",
          salt: new Uint8Array(),
          info: new Uint8Array(),
        },
        cryptoKey,
        { name: "AES-CTR", length: outputLength * 8 },
        true,
        ["encrypt", "decrypt"] // TODO: make usage an option
      )
    )
    .then((crytoKey) => getSubtle().exportKey("raw", crytoKey))
    .then((bytes) => new Uint8Array(bytes));
}

The first step was trying to have the function above produce the same output than the old function. Here is a test:

describe("HKDF", function () {
  it("HKDF", async function () {
    await fc.assert(
      fc.asyncProperty(fc.uint8Array({ minLength: 1 }), async (secret) => {
        const oldOutput = await kdf(secret, 32);
        const newOutput = await hkdf(secret, 32);
        console.log(`old [${oldOutput.length}]`, oldOutput.toString());
        console.log(`new [${newOutput.length}]`, newOutput.toString());
        expect(newOutput.length).to.eq(oldOutput.length);
        expect(newOutput.toString()).to.eq(oldOutput.toString());
      })
    );
  });
});

Unfortunately to no avail. The KDF function comes from go-ethereum due to Whisper 1:https://github.com/ethereum/go-ethereum/blob/cd31f2dee2843776e485769ce85e0524716199bc/crypto/ecies/ecies.go#L146

Not sure if it's possible to get Subtle Crypto to do the same KDF algorithm.

The benefit of tackling this would be performance gain and browser API is likely to execute faster than JavaScript runtime.

Also a security benefit if we can the whole stack to use Subtle Crypto so that the user can use subtle crypto to generate and manage keys instead of byte arrays as described here: #984

Happy to do a bounty on this issue to either:

  1. Prove it's not possible to use CryptoSubtle
  2. Replace KDF to use CryptoSubtle (must remain compatible with Waku Message Version 1 standard
  3. Enable usage of CryptoKeys on the API (could help with Provide an API to use crypto.subtle for SymEncoder and AsymEncoder #984 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers track:restricted-run Restricted run track (Secure Messaging/Waku Product), e.g. filter, WebRTC
Projects
Status: To Do
Development

No branches or pull requests

2 participants