keepassxreboot / keepassxc

KeePassXC is a cross-platform community-driven port of the Windows application “Keepass Password Safe”.
https://keepassxc.org/
Other
21.27k stars 1.47k forks source link

TOTP: custom settings for SHA-1/SHA-256/SHA-512 #873

Closed marcaurele closed 5 years ago

marcaurele commented 7 years ago

Would it be possible to add an extra customization option for TOTP to choose the hash method based to generate the token:

Those extra hash functions are described in the RFC for TOTP: https://tools.ietf.org/html/rfc6238

ccoenen commented 7 years ago

Are they being used anywhere?

weslly commented 7 years ago

This one is very unlikely to be implemented. I never found any website/service actually using this parameter and even Google Authenticator lacks support for it.

I'm closing this, but I will reopen if anyone finds an example of a website that requires this parameter.

marcaurele commented 7 years ago

www.kraken.com does let you choose for example (not very explicit in their support doc: https://support.kraken.com/hc/en-us/articles/203395513-How-do-I-set-up-two-factor-authentication-)

ccoenen commented 7 years ago

In case you're afraid of SHA-1 collisions, this will really not be an issue in TOTP. For one thing, the token is usually a second factor after a password (so someone would have to have both), and furthermore the input is mutated by the current time, so any collision that would ever have occurred would only ever have been valid for 30 seconds and the changes of another collision are astronomically small.

So, in contrast to TLS-certificates (where SHA-1 is discouraged and should be replaced as soon as possible), TOTP does not face the same threats.

phoerious commented 7 years ago

The same is true for HMAC-SHA1 and HOTP BTW. It's not time-based, but collisions aren't an immediate thread. Bruteforcing the secret is usually a better way to attack them.

weslly commented 7 years ago

Quoting this StackExchange answer:

The implementation calls for the result of the HMAC to be truncated and the initial HMAC uses a very short secret: using a different hash algorithm here wouldn't improve security but it would make it incompatible with existing applications and tokens.

marcaurele commented 7 years ago

I'm not afraid of the collision at all. I was just trying to use another hashing function and see if tools were compatible. Some are, some don't. I don't think it's a huge change neither to propose such option in the customization settings. Saying that you're not going to implement it because Google Authenticator lacks support of it is sad from a developer point of view.

phanimahesh commented 7 years ago

It can be interpreted as 'We won't implement it because one of the most popular TOTP apps doesn't, and that indicates that it isn't widespread in its use, and so we won't spend time on it.' And that is a reasonable argument IMO.

I suspect a PR would be welcome if it doesn't look like it'll add to maintenance burden ( code fits with rest of project's style and philosophy and is relatively self contained, etc. )

weslly commented 7 years ago

Saying that you're not going to implement it because Google Authenticator lacks support of it is sad from a developer point of view.

It's not because Google Authenticator lacks support for this, we added support for a couple of parameters that it doesn't support either (digits and period), but these are actually used in a few situations.

The algorithm parameter, however, isn't adopted nor required anywhere (that I'm aware of) and doesn't provide any visible improvement on security, so it wouldn't make a lot of sense to implement that.

I suspect having this parameter on the interface would only make the TOTP setup dialog more confusing for some users.

droidmonkey commented 7 years ago

Precisely what weslly said. We are very cognizant of scope creep, especially in an open source project. Even if a PR was fashioned, the likely complication on the gui side would make the program less user friendly for zero gain.

octiron commented 6 years ago

I've discovered that Red Hat defaults to using sha256 (because sha1 is deprecated?), and they maintain the FreeOTP android play store app to support that.

woodychuck commented 5 years ago

For what it's worth, Sophos UTM Firewalls are using SHA-256 in their TOTP implementation, so Google Authenticator doesn't work with them (neither does KeepassXC, nor KeeOTP in the last official release 1.3.9, though beta 1.3.12 supports both SHA-256 and SHA-512 with a simple 3 radio button GUI). Consequently Sophos provide their own App which otherwise is very similar to Google's.

So while it is true that SHA-1 is most widespread, there are existing applications/services using SHA-2 instead, which might not add any substantial security but leave users stranded in terms of Keepass integration, so maybe it's at least worth reconsidering.

BryanJacobs commented 5 years ago

Australia's "MyGov" system used for accessing government services such as the tax office uses TOTP with a SHA-512 hash. The current behavior of keepassxc is that it generates an incorrect TOTP (it uses SHA-1, and ignores the "otpHashMode" parameter in the OTP URL). KeePass 2 generates the correct code.

If what you're worried about is UI complexity, there's no need to put it in the UI, but it'd be nice to have some way to get the program to generate correct codes for the sites that need them. I think it's worth pointing at Steam code integration in the TOTP generator as a comparable feature - one, LITERALLY one, site uses them - and that appears to have been accepted without issue.

ccoenen commented 5 years ago

grafik

Well, having this setting could be 30;6;SHA-512 in TOTP Settings. Paired with some documentation this would make it possible and would not require going through this in every single case while setting up the mostly non-512-entries.

Given that we now have actual use cases (RedHat, Sophos, MyGov) for this, maybe the decision to close the issue should be revisited? After all, there's a special case for Steam and that's not even easy to set up.

droidmonkey commented 5 years ago

IMO this was a big misstep on the TOTP RFC to allow multiple hash algorithms. The permutation of options is ridiculous for something that is supposed to be quick and easy. The Steam integration was resisted until someone built a PR to integrate it. Even then it required me to spend several hours refactoring the whole TOTP mess to make it more effective. Please don't down play the significance of adding features.

octiron commented 5 years ago

For reference, the keycloak documentation: https://www.keycloak.org/docs/3.3/server_admin/topics/authentication/otp-policies.html

For my.gov.au, they are following DTA advice: https://www.dta.gov.au/our-projects/digital-identity/join-identity-federation/accreditation-and-onboarding/trusted-digital-identity-framework

The DTA advice is straight from NIST https://pages.nist.gov/800-63-3/sp800-63b.html

The NIST guidance is to use TOTP codes of at least 6 digits, valid for no longer than two minutes, and accepted only once. For hashes, they want at least 112 bits, i.e. half of 224. For SHA-1 this is now much less than 80 bits. It is a bit vague if this recommendation applies to TOTP. A number of Australian Govt guidance documents (e.g. https://acsc.gov.au/publications/ism/ISM_22_Guidelines_for_Using_Cryptography.pdf page 7) say 'must not' use SHA-1 in a general sense, and you can't expect policy people with no crypto understanding to make a distinction.

https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-107r1.pdf

I guess what I'm getting at is that SHA-256 is just going to get more common.

droidmonkey commented 5 years ago

This is all great information, but the hash in totp is simply a means to an end. It has zero relevance to using a hash for a signature or similar purpose. It was simply inappropriate for the RFC to be such an open door.

pepa65 commented 8 months ago

A 6-digit TOTP is huge on collisions of course, there is a 1 in a million chance to guess it right, it's a less than 20-bit space. SHA1 or SHA2 is just not relevant when simplifying towards a 6 (or even 8) digit TOTP. Just agreeing with @droidmonkey here.