Open ThisIsMissEm opened 7 months ago
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.
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.
By bailing out early if
expected.length !== actual.length
you create a timing attack. The correct implementation would be: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.