Security Vulnerability: Timing Attack

[ThisIsMissEm]

I just happened across your code whilst looking for a definition of x-hub-signature header and how it should be parsed, and I noticed that you've a timing attack in your code.

You tried to address this through #7 but accidentally didn't correctly address it.

  verify (expectedSignature, requestBody) {
    const expected = this.#encoder.encode(expectedSignature)
    const actual = this.#encoder.encode(this.sign(requestBody))
    if (expected.length !== actual.length) {
      return false
    }
    return crypto.timingSafeEqual(expected, actual)
  }

By bailing out early if expected.length !== actual.length you create a timing attack. The correct implementation would be:

  verify (expectedSignature, requestBody) {
    const expected = this.#encoder.encode(expectedSignature)
    const actual = this.#encoder.encode(this.sign(requestBody))

    return crypto.timingSafeEqual(expected, actual)
  }

This ensures that a timing safe comparison is always used, meaning that mismatching string lengths for signatures and mismatching signatures would happen in the exact same time expense.

I'm not a user of your module, so can't manage a PR for you.

[ThisIsMissEm]

Looks like my initial ticket actually had an issue since the crypto.timingSafeEqual will throw an Error if the inputs aren't the same byte length, in which case you'd actually want:

 verify (expectedSignature, requestBody) {
    const expected = this.#encoder.encode(expectedSignature)
    const actual = this.#encoder.encode(this.sign(requestBody))

    if (expected.byteLength !== actual.byteLength) {
      return false;
    }

    return crypto.timingSafeEqual(expected, actual)
  }

Given that the algorithm for generating signatures is widely known (HMAC SHA-256 hex digest on utf-8 data), the comparison of the actual and expected byte lengths wouldn't disclose additional information.

The reason for using byteLength instead of length is because length gives the:

The length accessor property of TypedArray instances returns the length (in elements) of this typed array.

Where as crypto.timingSafeEqual is expecting to values of equal byte length.