TOTP generation with bare secret in otp property

[silmaril42]

Summary

This is a little compatibility issue concerning TOTP handling of different KeePass clients.

Examples

The attached ZIP contains a KDBX file, which demonstrates different ways of storing TOTP configurations for default RFC 6238 cases. The DB password is HelloWorld.

There are two entries with the same TOTP secret JBSWY3DPEHPK3PXP stored in the property otp.

The first uses the format from KeePassXC: otpauth://totp/TOTP%20KeePassXC:none?secret=JBSWY3DPEHPK3PXP&period=30&digits=6&issuer=TOTP%20KeePassXC

The second one uses the format from KeeWeb: JBSWY3DPEHPK3PXP

In KeeWeb, both entries show identical (correct) TOTP values.

In KeePassXC, the simple format produces a different (wrong) TOTP value. This is exactly the same value that is produced for the third entry, which also has the otp property, but with an empty value.

Suggested solution

I think there are two things that can be done here:

1. Don't generate TOTP at all if the secret is empty.
2. If the otp property doesn't contain a known format (eg. the correct form of otpauth:// URL), assume the complete property value is the secret to be used with default RFC 6238 parameters.

TOTPProblem.zip

[droidmonkey]

KeeWeb is abandoned fwiw

[silmaril42]

That might be a reason for not putting to much work into this.

I just took a quick look into totp.cpp and found this:

 QVariant secret = Base32::decode(Base32::sanitizeInput(settings->key.toLatin1()));
    if (secret.isNull()) {
        return QObject::tr("Invalid Key", "TOTP");
    }

I guess for an empty string in settings->key this would lead to a zero-length secret?
Maybe we could extend the if condition to return "Invalid Key" for those cases, too.

My main concern is, that currently KeePassXC acts as if it was generating meaningful TOTP values, when it's acutally using an empty secret for the calculation. It's not easy for users to recognize this - which might lead to a lot of confusion (and in a case I wittnessed it led to a server being unreachable for some time because of a security mechanism that reacted to too many authentication failures).

[droidmonkey]

Oh yes, there is a bug here, which is to not just generate a totp without full verification of the input.