1

I am trying to implement TOTP in C++ using OpenSSL. I know there are vast amounts of existing implementations; however, I would like to implement it myself.

Currently. I have the following code:

bool verifyTOTP(char* code, char* key, int codeLen, int keyLen) {
    if (codeLen != 6 || keylen != 20) {
        return false;
    }
    unsigned long long intCounter = floor(time(NULL)/30);
    char md[20];
    unsigned int mdLen;
    HMAC(EVP_sha1(), key, keylen, (const unsigned char*)&intCounter, sizeof(intCounter), (unsigned char*)&md, &mdLen);
    OPENSSL_cleanse(key, keylen);
    int offset = md[19] & 0x0f;
    int bin_code = (md[offset] & 0x7f) << 24
        | (md[offset+1] & 0xff) << 16
        | (md[offset+2] & 0xff) << 8
        | (md[offset+3] & 0xff);
    bin_code = bin_code % 1000000;
    char correctCode[7];
    snprintf((char*)&correctCode, 7,"%06d", bin_code);
    int compR = compHash(&correctCode, code, 6); // Compares the two char arrays in a way that avoids timing attacks. Returns 0 on success.
    delete[] key;
    delete[] code;
    if (compR == 0) {
        return true;
    }
    std::this_thread::sleep_for(std::chrono::seconds(5));
    return false;
}

This code does not give any error but fails to produce the correct TOTP, and therefore it returns false when the correct TOTP is verified.

For example, when running the below it should return true:

char* newKey = new char[20];
char* key = "aaaaaaaaaaaaaaaaaaaa";
memcpy(newKey, key, 20);
verifyTOTP(newKey, code, 6, 20);

Where code is the token from the TOTP Generator (when using the generator please ensure that the secret key is set to MFQWCYLBMFQWCYLBMFQWCYLBMFQWCYLB).

Can anyone spot where I went wrong? I have looked at how other people implemented it but could not find where the problem is.

Thank you so much for your attention and participation.

mbs9
  • 75
  • 8
  • Can you edit this to give an example of a correct TOTP that should return true, but does not? – NicholasM Aug 17 '22 at 16:50
  • Also, what is `compHash`? That seems pretty crucial. – NicholasM Aug 17 '22 at 16:51
  • @NicholasM I added the example! compHash just compares the two arrays to ensure that they have the same content. Really, it is the same as `==` with the difference that compHash is safer in terms of timing attacks. – mbs9 Aug 17 '22 at 17:19
  • Is your clock reasonably synchronised with the token generator website? – Alan Birtles Aug 17 '22 at 21:37
  • Note that `delete[] key` has undefined behaviour in your example because `key` was not allocated with `new` – Alan Birtles Aug 17 '22 at 21:38
  • @AlanBirtles good point! That example was very problematic. I fixed it. The website should be synchronized since it produces the same result as the TOTP app on my phone. My laptop should also be synchronized because it shows the correct time. – mbs9 Aug 18 '22 at 05:51
  • My next guess would be that the `floor(time(NULL)/30)` is producing the incorrect result, you don't need `floor` as `time()` and `30` are both integers and the integer division already truncates – Alan Birtles Aug 18 '22 at 14:15
  • My other suggestion would be to step through both your code and and known working code in a debugger and see where your code diverges – Alan Birtles Aug 18 '22 at 14:15

1 Answers1

1

After dividing the Unix timestamp by 30, it is necessary to ensure that intCounter is big endian:

unsigned long long endianness = 0xdeadbeef;
if ((*(const uint8_t *)&endianness) == 0xef) {
  intCounter = ((intCounter & 0x00000000ffffffff) << 32) | ((intCounter & 0xffffffff00000000) >> 32);
  intCounter = ((intCounter & 0x0000ffff0000ffff) << 16) | ((intCounter & 0xffff0000ffff0000) >> 16);
  intCounter = ((intCounter & 0x00ff00ff00ff00ff) <<  8) | ((intCounter & 0xff00ff00ff00ff00) >>  8);
};

Credit: I found the solution in this GitHub Gist.

mbs9
  • 75
  • 8