generateTOTP generates insecure OTPs with digits over 6 and alphabetic charSet

[ironbyte]

I'm not sure if this is intentional but it seems that this particular case is not covered in the tests.

So here's the main issue:

I'm using generateTOTP from the @epic-web/totp library to generate OTP codes. I'm trying to create secure and unique tokens using the SHA256 algorithm with 20 digits and a custom character set for a password reset flow in Remix. However, the generated tokens have repetitive and insecure patterns. This seems to happen only when I include alphabetic letters in the charSet

Here's the code to reproduce:

// index.ts

import { generateTOTP } from "@epic-web/totp";

async function main() {
  const totpPayload = await generateTOTP({
    algorithm: "SHA256",
    digits: 20,
    charSet: "ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567980",
  });

  console.log("totpPayload.otp", totpPayload.otp);
}

await main().catch((e) => {
  console.error(e);
});

/*
Output (after running the code...)

totpPayload AAAAAAAAAAAAAAJKQ)C0
totpPayload AAAAAAAAAAAAAAE!JV6N
totpPayload AAAAAAAAAAAAAAITNPHA
...

*/

Environment:

Node.js - v18.17.1 (Runtime)
Bun - v1.0.3 (Runtime) 
OS: Fedora 38 - 6.4.15-200.fc38.x86_64

Deps:

@epic-web/totp - 1.1.1

[kentcdodds]

I'm afraid this is as far as my knowledge goes with this. I'm not sure whether this is expected or a bug. Sorry.

[ironbyte]

Hey, understood. No problem. This type-safe OTP library is exactly what we need. Thanks for it!

I might try to figure out how to fix this bug over the weekend.

[kentcdodds]

Thank you!

[moishinetzer]

https://chat.openai.com/share/e96f8b80-8b65-4f46-a75e-8eb68142c40b

This is what ChatGPT had to say about this, can anyone confirm this is an ok change to make? I can confirm it generates less biased HOTP's

@ChristianBoehlke linking you in here as you seem to have some experience with this.

[kentcdodds]

I would definitely appreciate any change be accompanied by tests. Especially those that involve some sort of time mocking so we can verify things work over time.

[ChristianBoehlke]

Here is what I know about this bug:

1. I think the solution suggested by your ChatGPT link will fix this bug but break compatibility with the authenticator apps as far as I can tell.
2. To stay compatible with authenticator apps, the algorithm that converts (truncates) the long SHA1 hash into the six digits must follow a certain pattern. This is why the hotpVal is calculated in that way right now. I don't know if rfc6238 (time-based) and rfc4226 (counter-based) are the "original" specifications, but they describe this and have a reference algorithm does exactly that.
3. This hotpValue is something like 886295804 and the resulting TOTP would be 295804. As you can see, it's just the last 6 digits. That's why before the loop was introduced in Implement charSet options #4, the resulting number was converted into a string with a base of 10 and just cut off. The new loop does the same generally, but converts the number to a string with a different base (charSet.length).
4. The problem now with the longer TOTPs is that the current spec-compliant implementation of hotpValue creates values like 886295804 that are way to small to create long strings. That's why it's filled up with As after hotpValue becomes zero and the first character is picked again and again from the charset within the loop.

The spec says in section 5.4.:

The purpose of the dynamic offset truncation technique is to extract a 4-byte dynamic binary code from a 160-bit (20-byte) HMAC-SHA-1 result.

So, based on this, to me it sounds like adding more characters to the charset will not make the resulting string any more secure (once the 4 bytes are "processed") or longer (in fact, adding more characters will make it shorter).

[kentcdodds]

In that case I think it would make sense to throw a descriptive error if anyone provides more digits than is supported. If you really want long codes, then you can concat multiple together.

Anyone up for a pull request?

[kentcdodds]

Unless I misunderstand what's being said and we should remove the charSet option as well to restore spec compliance?

[kentcdodds]

Closing due to inactivity. Please feel free to open up a PR if anyone is interested in working on this. Thanks!

[chrisvariety]

As far as I can tell this happens even without charSet:

import { generateTOTP } from '@epic-web/totp';

async function main() {
  for (let i = 0; i < 10; i++) {
    const totpPayload = generateTOTP({
      algorithm: 'SHA256',
      digits: 20
    });

    console.log('totpPayload.otp', totpPayload.otp);
  }
}

await main().catch((e) => {
  console.error(e);
});

Output will be something like:

totpPayload.otp 00000000001141418029
totpPayload.otp 00000000001590695200
totpPayload.otp 00000000000902504412
totpPayload.otp 00000000000541207217
totpPayload.otp 00000000001805339550
totpPayload.otp 00000000000009022076
totpPayload.otp 00000000000412695627
totpPayload.otp 00000000000385536067
totpPayload.otp 00000000000753914133
totpPayload.otp 00000000001611436767

Seems like digits should be capped at ~6?

[kentcdodds]

Seems reasonable to go up to 8 or 10 🤔