chillerlan / php-authenticator

A generator for counter- and time based authentication codes (RFC-4226/RFC-6238, Google Authenticator).
MIT License
43 stars 2 forks source link

Length of secret? #8

Closed jbostoen closed 2 weeks ago

jbostoen commented 2 weeks ago

Am I missing something?

I'm looking to migrate a project from an outdated authenticator library to chillerlan/php-authenticator.

From the read me:

// -> otpauth://totp/my%20label?secret=NKSOQG7UKKID4IXW&issuer=chillerlan.net&digits=6&period=30&algorithm=SHA1

So it seems the secret can still be 16 characters.

However, when I set the secret length to 16; and generate a secret using the createSecret() method; I get an output of 26 characters? E.g. JVD6M43FQR6XBTRB6GTPU27VNM

Am I missing something or am I confused somewhere?

I'd like to re-use the existing secrets for now.

codemasher commented 2 weeks ago

The secret length is the actual length of the binary secret string (which is used internally), not its base32 encoded representation.

jbostoen commented 2 weeks ago

So with the current version of the library, can we still have a base32 encoded representation that results in only 16 characters (as in the example from the read me)?

codemasher commented 2 weeks ago

The minimum length for a secure secret string is 128 bits (with 160 bits recommended), that's why the lower limit is set to 16 bytes - shorter secret strings could compromise the security and are therefore not allowed to be created. However, nothing stops you from feeding a shorter secret to the setSecret() method, as long as it is a valid base32 encoded string.

(I think the secret string in the exampple was shortened and is not a representation for actual values)

jbostoen commented 2 weeks ago

Thanks, good to know.

I'm trying to migrate away from this library. It also seems to use the minimum of 16 bytes. https://github.com/PHPGangsta/GoogleAuthenticator/blob/505c2af8337b559b33557f37cda38e5f843f3768/PHPGangsta/GoogleAuthenticator.php#L24-L53

But there I do end up with a 16-character secret (similar to the demo URI from your read me). I believe it uses the same function to generate the 16 bytes. Is this then related to how your library currently encodes the secret?

https://github.com/chillerlan/php-authenticator/blob/ad87a07a0c6d6fa021c8b8f9db5114f084fb1994/src/Authenticators/AuthenticatorAbstract.php#L70

codemasher commented 2 weeks ago

Yea, the initial version of my library was "inspired" by that one, but over time I found out that several things were badly implemented. The secret phrase and the handling thereof is one of those - it converts the binary to base32 characters (which results in a base32 string with the same length as the secret) instead of properly base32 encoding the whole binary string, which as you have learned, results in a much longer string. So it's highly suggested to somehow migrate away from those short secrets as they're highly insecure. You might notice that both libraries generate different OTP codes with the same base32 secret.

jbostoen commented 2 weeks ago

Thanks for taking your time for the detailed explanation! :)

"You might notice that both libraries generate different OTP codes with the same base32 secret." - I'm surprised; I'll check out then if some of the codes generated by e.g. Microsoft Authenticator or other authenticators are still accepted if I try to validate them with your library after using ´setSecret()´

codemasher commented 2 weeks ago

To be fair, I haven't checked this other library in a long time and it might have been fixed in the meantime, but back then i found out when I added the tests that check against the vectors provided in the RFC documents.

jbostoen commented 2 weeks ago

I can confirm your library does accept the secret key (16 characters) generated by the other library. Internally, your library seems to translate this to a shorter secret length (10). When I then use the getSecret() method, I get the same original secret key; so this confirms there is no difference in base32 encoding.

The other library also generates 16 random bytes; and then seems to create a base32 encoded string of 16 characters based on that. That may indeed be flawed, or perhaps there's just a lot of confusion on this specification? For example, some online TOTP generators also end up displaying a 16 character secret?

Example:

codemasher commented 2 weeks ago

When I then use the getSecret() method, I get the same original secret key; so this confirms there is no difference in base32 encoding.

I checked the other library and it does indeed base32 decode its generated secret internally before handing it over to hash_hmac(), ending up with the same 10 byte string that my library receives from the getSecret() method - the difference however is how it generates the secret, which is not a proper base32 encoding, but a pseudo-base32 and massively reduces the entropy of the original input it receives from the indeed cryptographic secure functions random_bytes(), mcrypt_create_iv() or openssl_random_pseudo_bytes().

perhaps there's just a lot of confusion on this specification?

Neither of the two specifications says anything about base32 encoding - that's a portability feature that Google specified on their own (URI format), and it clearly states that the secret shall be base32 encoded. In their example over here they have a secret phrase Hello! + 4 bytes 0xDE, 0xAD, 0xBE, 0xEF (10 bytes total) that results in a base32 encoded string JBSWY3DPEHPK3PXP - so it's definitely the library that's doing it wrong (perhaps because base32 encoding requires another class/library, idk). Fun fact, there's still that comment over here which is a remnant from previous versions... we're not doing 80 bits anymore haha.

codemasher commented 2 weeks ago

(Updated the above comment to clarify some things)

codemasher commented 2 weeks ago

To conclude this: the secret length as per the specifications (RFCs 4226 and 6238) is the length of the binary string that is given to the HMAC hash function - there is no base32 encoding involved at all. Google's "Key URI format" specification uses base32 encoding in order to make the binary secret string portable (URL safe) - the base32 encoding naturally results in longer strings than the original secret.

However, some of the top used libraries on packagist use some kind of pseudo base32 encoding, with a shorter secret string than requested as a result, which is highly insecure.

Some of the bad examples:

phpgangsta/googleauthenticator https://github.com/PHPGangsta/GoogleAuthenticator/blob/505c2af8337b559b33557f37cda38e5f843f3768/PHPGangsta/GoogleAuthenticator.php#L24-L53

christian-riesen/otp https://github.com/ChristianRiesen/otp/blob/aab865ae6d356993ad9d51f8a7e8f32b8a54730a/src/GoogleAuthenticator.php#L149-L170

silverstripe/totp-authenticator this one is even worse as it uses the same library as I to base32 encode the secret. but then truncates it to the desired length https://github.com/silverstripe/silverstripe-totp-authenticator/blob/4a98f33d141f3cbcaaa47c12a1e70386720866e3/src/RegisterHandler.php#L73-L82

2amigos/2fa-library https://github.com/2amigos/2fa-library/blob/1d858fd79389394b7432c46538e8a40169fcae9c/src/Support/Encoder.php#L95-L102

(I'm gonna stop here, that was enough horrible code for today...)

I'll close this issue here as resolved.