Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New hashing algorithm? #26

Open
stratacast opened this issue Jul 28, 2016 · 13 comments · May be fixed by #1549
Open

New hashing algorithm? #26

stratacast opened this issue Jul 28, 2016 · 13 comments · May be fixed by #1549

Comments

@stratacast
Copy link

I see this is using SHA1. Any plans for allowing SHA256 or SHA512 also?

@ChristophWurst
Copy link
Member

I have no idea 🙈 @LukasReschke what do you think?

@My1
Copy link
Contributor

My1 commented Nov 26, 2016

well considerin OTP generates just 6 numbers anyway, there is probably no problem with SHA1
also newer sha versions probably arent supported by most of the TOTP apps.

@ChristophWurst
Copy link
Member

no activity and I have no plans to work on this ATM -> close

@My1
Copy link
Contributor

My1 commented Dec 18, 2016

also other hasing algos would totally break compatibility with many apps which only do sha1

@Freeedim
Copy link

Hi guys,

I come from the future (2022), and things have changed a bit.

SHA-1 has been cracked and there are now quite some TOTP apps allowing to select another hashing algorithm.

I would suggest to keep SHA-1 as the default algorithm for compatibility purpose and to offer the user a kind of "advanced settings" option unveiling a drop-down list (or something alike) with other algorithms (SHA-256, SHA-512, SHA-2, ChaCha...).

What do you think?

@My1
Copy link
Contributor

My1 commented Jan 26, 2022

SHA-1 has been cracked and there are now quite some TOTP apps allowing to select another hashing algorithm.

are you referring to https://shattered.it?
back then it was said HMAC wasnt affected.

although having options is always good.

@Freeedim
Copy link

Freeedim commented Jan 26, 2022

I guess leveraging the described attack needs the attacker's knowing the resulting hashing. And few actors have the computational power to conduct the attack in a timely way anyway.

So I would say HMAC is affected, but TOTP over SHA-1 is still quite safe in 99.99% of use cases at least.

Then it depends from whom you want to protect your data and account. Maybe a couple of users out of some millions want a TOTP guaranteed to be able to resist an NSA/FSB attack. And in that case, when you dropped Google, Microsoft etc. for obvious concerns, I guess you want to be reassured by an open-source, independent solution that supports recent and robust cryptography algorithms, just to be ahead rather than behind.

I am not a developer or a cryptologist, so I was picturing supporting a new algo as basically including its library and adding an 'algoType' parameter in the hashing function call, so I thought it was a quick win (even if a tiny win). Apologies if the implementation is actually cumbersome (I know, it's always more complicated than we foresee, even when we are experts).

@My1
Copy link
Contributor

My1 commented Jan 26, 2022

I think the issue is less the implementation, but rather the compatibility, as for example authy doesnt support anything besides standard 30 sec SHA1 6-digit TOTPs, and not sure if for example google authenticator supports other SHA methods.

I am honestly not sure what would be the bigger problem. that you have only 6 digits or that you have HMAC-SHA1.

@Freeedim
Copy link

Freeedim commented Jan 26, 2022

Of course, to make that very clear, I don't suggest to drop SHA-1. I am just suggesting to add other algorithms as options. It could be additional digits too, by the way.

I actually use Aegis Authenticator and it seems to offer the possibility to customise algo and number of digits, which suddenly made me wish the same was possible on Nextcloud's side.

@My1
Copy link
Contributor

My1 commented Jan 26, 2022

I never assumed dropping sha1 just That if it's enabled that the guidance texts might need to change

@Freeedim
Copy link

Oh, yes, I see! Definitely! That's why I suggested to keep SHA-1 as the default and hide the other algorithms (and possibly the number of digits and validity time) under a deterring "advanced settings" thing...

@My1
Copy link
Contributor

My1 commented Jan 26, 2022

Sure. The issue would also be if the setting is done by admin or by user. If by admin the user wouldn't even know that it's different.

@Freeedim
Copy link

In 2022, I think it is too early to put this in admin hands, or at least with a strong warning that most of their users' TOTP mobile apps won't support non-default settings.

Then, when the user sets up the TOTP, there would be two scenarii:

  1. If the algo and number of digits was set to by the admin: indicate them next to the shared secret (and add some info box or so that either says that those are default settings when they are, or that those are non-default settings that might not be supported by some TOTP mobile apps)
  2. fields to allow the user to set the algo and the number of digits, with an info box warning that non-default settings might not be supported by some TOTP mobile apps, and with an easy way to retrieve default values.

@SystemKeeper SystemKeeper linked a pull request Jul 12, 2024 that will close this issue
@ernolf ernolf linked a pull request Jul 22, 2024 that will close this issue
@ernolf ernolf linked a pull request Jul 22, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants