Generates incorrect TOTP codes when using seed of less than 10 bytes

[raxod502]

Okay, this one is a doozy. But, I root-caused the whole thing so we can go straight to the fix. As the title says, when your TOTP seed is less than 10 bytes (so, less than 16 characters in base32), then Browserpass will generate incorrect TOTP codes. This is due to a bug in the popular but unmaintained dependency https://github.com/yeojz/otplib which Browserpass uses, where keys of less than 10 bytes are padded incorrectly before being passed to SHA1-HMAC. Here are the relevant RFCs to read:

• https://datatracker.ietf.org/doc/html/rfc6238 "TOTP: Time-Based One-Time Password Algorithm"
• https://datatracker.ietf.org/doc/html/rfc4226 "HOTP: An HMAC-Based One-Time Password Algorithm"
• https://datatracker.ietf.org/doc/html/rfc2104 "HMAC: Keyed-Hashing for Message Authentication"
• https://datatracker.ietf.org/doc/html/rfc4648 "The Base16, Base32, and Base64 Data Encodings"

The relevant text is that of RFC2104:

    (1) append zeros to the end of K to create a B byte string
        (e.g., if K is of length 20 bytes and B=64, then K will be
         appended with 44 zero bytes 0x00)

Now, RFC2104 says that for SHA1 (the default for TOTP) the minimum recommended key length is 20 bytes - so, obviously, using a 5-byte key is a terrible idea from a security perspective. But, the behavior is nonetheless well-defined: you right-pad with zeroes to the block size.

Here is what the code in otplib does instead:

/**
 * Pads the secret to the expected minimum length
 * and returns a hex representation of the string.
 */
export const totpPadSecret = (
  secret: SecretKey,
  encoding: KeyEncodings,
  minLength: number
): HexString => {
  const currentLength = secret.length;
  const hexSecret = Buffer.from(secret, encoding).toString('hex');

  if (currentLength < minLength) {
    const newSecret = new Array(minLength - currentLength + 1).join(hexSecret);
    return Buffer.from(newSecret, 'hex')
      .slice(0, minLength)
      .toString('hex');
  }

  return hexSecret;
};

If you run this on a 5-byte key with SHA-1 for TOTP, it will compute currentLength as 5 and minLength as 20, calculate minLength - currentLength + 1 as 16, create an empty array of length 16, then perform a string join(?!) that creates 15 consecutive copies of the 5-byte key for a total length of 75 bytes, then slices to get the first 20 bytes of that.

I have literally no clue how on earth this implementation came to be, but it obviously gives completely garbled results for any key that "needs padding". In fact this entire function could be deleted entirely, because it is not even handling the padding from the RFC, because that is asking for padding to the block size of 64, which is already taken care of later, inside the SHA1-HMAC implementation from crypto.js - why are we trying to add our own padding on top of that?

The initial version of this logic was added net-new without explanation in commit 57eb106e460395b50c0ae6b0f4949cf18dc27988 of Apr 9, 2017, alongside test cases that do not appear to exercise it at all, so its introduction remains a mystery to me. There is also a mention of special padding behavior to 20 bytes (rather than 64 bytes) in the README, added in commit f554516a5418f646604e43d9f0da9903f2ff7232 of Aug 25, 2019, with the enigmatic commit message chore: update package deps, so it seems like this may have been intended behavior rather, but the behavior is assuredly wrong and I cannot work out what specification it is supposed to be based on.

Anyway, what this means for Browserpass is that if you have an otpauth:// URL in your password entry, and you have OTP support enabled in Browserpass settings, and you look at the OTP codes generated by Browserpass, they will not be the same as the ones you generate using pass-otp or oathtool, both of which use the correct RFC-compliant OTP generation that is done by all other OTP generation libraries.

Unfortunately, this is not as simple as fixing the bug upstream since otplib is apparently abandoned - but since OTP functionality is extraordinarily simple I'd suggest we just pull in the transitive crypto.js dependency ourselves and generate OTP codes using that directly. I'm happy to submit a PR.