validation may be vulnerable to timing attacks

[jawj]

Seems to me that the validate method may be vulnerable to timing attacks, since it uses a naive string equality test.

Can you replace this with crypto.timingSafeEqual, or would you accept a pull request if I have a go at it?

[hectorm]

That's interesting, thanks for sharing this.

I will definitely use crypto.timingSafeEqual for Node.js. For Deno and browsers, do you think this shim would be a good alternative?

[jawj]

Great. The shim looks pretty sane, so yes.

[hectorm]

Let me know if 68f50b6 would be enough to solve the problem.

[jawj]

Buffers must be equal length:

> c.timingSafeEqual(Buffer.from('xyz'), Buffer.from('xy'))
Uncaught [RangeError: Input buffers must have the same byte length] {
  code: 'ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH'
}

Since the length of the OTP is not a secret, it's probably safe to test for equal lengths first, e.g.

if (token.length !== generatedToken.length) return null;

[hectorm]

Done. Thanks again!

[jawj]

I now note that in validate there's an early return on matching tokens, which potentially leaks information about where in the sequence the match occurred.

for (let i = counter - window; i <= counter + window; ++i) {
  const generatedToken = HOTP.generate({
    secret,
    algorithm,
    digits: token.length,
    counter: i
  });

  if (
    token.length === generatedToken.length
    && Crypto.timingSafeEqual(token, generatedToken)
  ) {
    return i - counter;  // <- ** HERE **
  }
}

return null;

Can you fix that so that validate always takes the same amount of time?

[hectorm]

Absolutely correct, thanks for the warning.

Could you confirm that master...constant-time-validation definitely solves the validation in constant time?

[hectorm]

I've also noticed that the length comparison is not really necessary because it will always be true since the HOTP.generate method within the loop is always called with the digits property with the length of the token to be validated.

[jawj]

The fix looks right, I think: thanks.

But isn't that a huge security hole right there in the static variety of generate? If you're validating with that version, can't I can just put in a one-digit code and have a 10% chance of getting it right (or possibly even a zero-digit code and always get it right)?

I see this is mitigated in the instance version by padding the token up to this.digits length, in which case yes, your point about the length comparison seems to hold. But I really wouldn't let anyone call static generate as things stand.

[hectorm]

The use case of the static variant is very limited since it doesn't know the expected token length so the developer must check it first. In most cases it's the instance method the one that should be used. Maybe this should have been part of the private API so that nobody uses it carelessly.

One solution that occurs to me is to add the digits argument to the static method and perform the check myself instead of relying on the developer to do this.

diff --git a/src/otp.js b/src/otp.js
index cad52bd..c9c5ad5 100644
--- a/src/otp.js
+++ b/src/otp.js
@@ -121,6 +121,7 @@ export class HOTP {
         * @param {string} config.token Token value.
         * @param {Secret} config.secret Secret key.
         * @param {string} [config.algorithm='SHA1'] HMAC hashing algorithm.
+        * @param {number} config.digits Token length.
         * @param {number} [config.counter=0] Counter value.
         * @param {number} [config.window=1] Window of counter values to test.
         * @returns {number|null} Token delta, or null if the token is not found.
@@ -129,16 +130,20 @@ export class HOTP {
                token,
                secret,
                algorithm,
+               digits,
                counter = defaults.counter,
                window = defaults.window
        }) {
+               // Return early if the token length does not match the digit number.
+               if (token.length !== digits) return null;
+
                let delta = null;
 
                for (let i = counter - window; i <= counter + window; ++i) {
                        const generatedToken = HOTP.generate({
                                secret,
                                algorithm,
-                               digits: token.length,
+                               digits,
                                counter: i
                        });
 
@@ -164,9 +169,10 @@ export class HOTP {
                window
        }) {
                return HOTP.validate({
-                       token: Utils.pad(token, this.digits),
+                       token,
                        secret: this.secret,
                        algorithm: this.algorithm,
+                       digits: this.digits,
                        counter,
                        window
                });
@@ -292,6 +298,7 @@ export class TOTP {
         * @param {string} config.token Token value.
         * @param {Secret} config.secret Secret key.
         * @param {string} [config.algorithm='SHA1'] HMAC hashing algorithm.
+        * @param {number} config.digits Token length.
         * @param {number} [config.period=30] Token time-step duration.
         * @param {number} [config.timestamp=Date.now] Timestamp value in milliseconds.
         * @param {number} [config.window=1] Window of counter values to test.
@@ -301,6 +308,7 @@ export class TOTP {
                token,
                secret,
                algorithm,
+               digits,
                period = defaults.period,
                timestamp = Date.now(),
                window
@@ -309,6 +317,7 @@ export class TOTP {
                        token,
                        secret,
                        algorithm,
+                       digits,
                        counter: Math.floor(timestamp / 1000 / period),
                        window
                });
@@ -328,9 +337,10 @@ export class TOTP {
                window
        }) {
                return TOTP.validate({
-                       token: Utils.pad(token, this.digits),
+                       token,
                        secret: this.secret,
                        algorithm: this.algorithm,
+                       digits: this.digits,
                        period: this.period,
                        timestamp,
                        window

[hectorm]

I've released a new version which I think solves all your concerns. Thank you for your helpful feedback.