Changed default secret length from 80bits to 160bits as recommended by RFC4226

[Mattie112]

This change will improve the default 'security' of this lib.

The TOTP RFC https://www.ietf.org/rfc/rfc6238.txt lists:

   R3: The algorithm MUST use HOTP [RFC4226] as a key building block.

The HTOP RFC https://www.ietf.org/rfc/rfc4226.txt lists:

   R6 - The algorithm MUST use a strong shared secret.  The length of
   the shared secret MUST be at least 128 bits.  This document
   RECOMMENDs a shared secret length of 160 bits.

For example the Android App FreeOTP shows a notice when < 160 bits (see for example https://github.com/freeotp/freeotp-android/issues/307)

[Mattie112]

Before I fix the tests: Is this something that you would be willing to change?

[NicolasCARPi]

Hello,

I have a question: why not go to 256 instead of aiming for the minimum accepted value?

And I vote strongly for this change, on the condition that it doesn't impact existing secrets, because if it does, it is a major breaking change that nobody wants to deal with (imho). But if it only impacts newly created secrets, then it's all good!

[willpower232]

Yeah I don't hate this, whatever the default value ends up being, I would only say that technically it could break backwards compatibility if some users had very strict string lengths in their database

[willpower232]

a question for how spicy @RobThree wants to be I guess :sweat_smile:

[RobThree]

Well, I would like to go "all the way", but in my past experience (it's been a little while though, things probably changed) some authenticators didn't handle anything but the (then) common 80 bits / 6 digits / SHA1 / 30 seconds intervals. So if we're gonna make a change like this I'd love to see the most common authenticators tested and confirmed at the very least that they work without issue. Besides that, we already allow users to go beyond the default 80 bits, they just need to configure it explicitly. Another option would be to put a bit more emphasis on that in the documentation.

To be clear: I'm NOT opposed, I am a bit hesitant though as long as we haven't tested / confirmed. And, finally, while, yes, more bits is more better, I don't think it adds that much more value. Things like replay attacks are much more dangerous IMHO.

Another factor to keep in mind is that people may have a column in their database to store the secrets of type varchar(16) or whatever; when upgrading this lib it will be a breaking change to them, so we need to either up the major version or be very careful and explicit in documenting this change at the very least (and preferably both?).

You know what? I'll dump my 2FA secrets (I have quite a few) and I'll plot their lengths, just for poops and giggles.

---

Here we go:

Secret length	Bits	Count
16	80	52
20	100	1
24	120	1
26	130	3
32	160	47
52	260	3
58	290	1
64	320	3

The majority is 80 bits, closely followed by 160 bits.

Not sure what, if anything, this demonstrates but it's interesting at the very least. I think it shows at the minimum that 80 bits secrets are still incredibly common and, thus, we need to be careful since this will affect possibly half of our users in one way or another.

[willpower232]

That is interesting, I have a similar distribution

Secret length	Bits	Count
16	80	29
24	120	2
26	130	4
32	160	13
52	260	5
64	320	2
103	515	1
128	640	1

(not sure how I have a 103 character secret, I guess I have one broken one somewhere :sweat_smile: )

[Mattie112]

Hello,

I have a question: why not go to 256 instead of aiming for the minimum accepted value?

And I vote strongly for this change, on the condition that it doesn't impact existing secrets, because if it does, it is a major breaking change that nobody wants to deal with (imho). But if it only impacts newly created secrets, then it's all good!

Yes it only affects newly created secrets!

And about why not 256: well that is always the discussion with security. The same story for SSL certificates for example. It does add overhead (either longer computational time, increased storage or network traffic). And if you have a method that for example with current processing power takes 100000 years to brute-force: is it worth making this 1000000 with the extra overhead.

That said: I am in no way an expert on this but I assume the ones responsible for the recommendation in the RFC are.

Yeah I don't hate this, whatever the default value ends up being, I would only say that technically it could break backwards compatibility if some users had very strict string lengths in their database

Yes that indeed is a thing, so this should be a new major release if you look at it that way. (Or document that security defaults can be changed at any time)

Well, I would like to go "all the way", but in my past experience (it's been a little while though, things probably changed) some authenticators didn't handle anything but the (then) common 80 bits / 6 digits / SHA1 / 30 seconds intervals. So if we're gonna make a change like this I'd love to see the most common authenticators tested and confirmed at the very least that they work without issue. Besides that, we already allow users to go beyond the default 80 bits, they just need to configure it explicitly/ Another option would be to put a bit more emphasis on that in the documentation.

To be clear: I'm NOT opposed, I am a bit hesitant though as long as we haven't tested / confirmed. And, finally, while, yes, more bits is more better, I don't think it adds that much more value. Things like replay attacks are much more dangerous IMHO.

Another factor to keep in mind is that people may have a column in their database to store the secrets of type varchar(16) or whatever; when upgrading this lib it will be a breaking change to them, so we need to either up the major version or be very careful and explicit in documenting this change at the very least (and preferably both?).

You know what? I'll dump my 2FA secrets (I have quite a few) and I'll plot their lengths, just for poops and giggles.

Here we go:

Secret length Bits Count 16 80 52 20 100 1 24 120 1 26 130 3 32 160 47 52 260 3 58 290 1 64 320 3 The majority is 80 bits, closely followed by 160 bits.

Not sure what, if anything, this demonstrates but it's interesting at the very least. I think it shows at the minimum that 80 bits secrets are still incredibly common and, thus, we need to be careful since this will affect possibly half of our users in one way or another.

Interesting to see the distribution! If I am correct the 160 bits recommendation was added later so yeah you do have to notice that :)

I did a bit of testing with: Google Authenticator, Authy, Microsoft Authenticator, FreeOTP. They all are ok with the longer secret.

(I was also thinking of suggestion to change the hash to SHA-256 but that is still not widely supported!)

[willpower232]

changing the hash default is definitely very chaotic so probably not ready for that for a long while :sweat_smile:

it might be interesting to throw a deprecation notice with trigger_error() for the developer not specifying the number of bits and see how much noise that makes?

[valerio-bozzolan]

Maybe we can introduce a middleware function array getQRTextArgs() that just returns an associative array. So it's a bit easier and cleaner to extend without doing URL parsing and re-building.

[RobThree]

Maybe we can introduce a middleware function array getQRTextArgs() that just returns an associative array. So it's a bit easier and cleaner to extend without doing URL parsing and re-building.

I don't quite understand this in the context of this issue? The number of bits can be specified in the constructor, no need for parsing the URL and no need for an 'intermediate' function that returns the url parts as associative array? But maybe I'm understanding this wrong?

Having said that, how about we change the default to 160 bits and release as beta/pre-release-something and leave it for a few months to see what comes out of it? It's better than doing nothing and it's better than just going straight for deploying with the defaults. Though I'm also good with @willpower232's suggestion and raising a deprecation of some kind.

[Mattie112]

A beta version seems fine to me. A deprecation can be added (at a later moment). Or do you want to add that the release version? That might also not even be bad.

I also don't understand the comment regarding the middleware.

When I am back from holiday I can finish the PR with working tests and then I can rebase it to whatever branch we want.

[RobThree]

@valerio-bozzolan Could you please elaborate?

[valerio-bozzolan]

My comment no longer has any connection to the code. I was probably root and drunk. Apologies.

[NicolasCARPi]

it could break backwards compatibility if some users had very strict string lengths in their database

if this is the only breaking BC change, then it's fine to include it in 3.0, no? As long as the changelog says: Make sure to store the secret in a column of at least XX size.

[NicolasCARPi]

@RobThree don't let that merge button go cold!!!!! :stuck_out_tongue_winking_eye:

[RobThree]

Whoops, wait, I "resolved" a little too much 🤦