waku-org / js-waku

JavaScript implementation of Waku v2
https://js.waku.org
Apache License 2.0
162 stars 41 forks source link

Review usage of HKDF #908

Open fryorcraken opened 2 years ago

fryorcraken commented 2 years ago

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: https://github.com/waku-org/js-waku/blob/b33306655a0b6da4b294bef37f0131afda4fb476/packages/message-encryption/src/crypto/ecies.ts#L8

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

fryorcraken commented 2 years ago

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).

elijahhampton commented 1 year ago

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

fryorcraken commented 1 year ago

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:

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 commented 1 year ago

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

fryorcraken commented 1 year ago

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: https://github.com/waku-org/js-waku/issues/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 https://github.com/waku-org/js-waku/issues/984 )