Validate 'secret' when instantiating TOTP

[gjuric]

Q	A
Bug report?	yes
Feature request?	no
BC Break report?	no
RFC? / Specification	no
Library version	9.0.2

I tried passing in the secret with mixed characters (alnum and special characters), the object was constructed, the QR code was valid, but the verify method was failing with:

Base32::doDecode() only expects characters in the correct base32 alphabet

I believe that this should fail when instantiating an instance of TOTP.

I also tried testing the library using 'SECRET' for the secret. The rendered QR code and Google Authenticator did not produce valid codes until I took one of the secrets generated by the lib (without passing in the 'secret' parameter) and then setting this manually. I don't know if it has something to do with the length of the secret or something else.

[Spomky]

Hi,

As indicated in the OTP Customization page, the secret must be encoded in Base32.

If you have a secret with mixed characters (e.g. Th1s i5 a nic3 code!), you have to encode it first:

use ParagonIE\ConstantTime\Base32;

$mySecret = Base32::encodeUpper('Th1s i5 a nic3 code!'); //You can remove the '=' padding if any
$otp = TOTP::create($mySecret);

It should work as expected.

[gjuric]

Any specific reason why you are trimming "=" in the example from the Customization page?

[Spomky]

The = is a padding character. The RFC4648 allows to remove it in some circumstances.
It is done here because because the provisioning Uri used to configure the applications is an Uri and the RFC3986 states that the = is a reserved character.
It also indicates that

The pad character "=" is typically percent-encoded when used in an
URI [9], but if the data length is known implicitly, this can be
avoided by skipping the padding; see section 3.2.

If you configure your OTP application by clicking the provisioning Uri (e.g. using a mobile device), you may have troubles.
I never encountered that issue and in general applications are configured using a QR Code, but I prefer not to change that and keep it compliant with those applicable RFCs.

Note: I missed the trim function in my example below.

[fefas]

Isn't it the opportunity to improve this exception message? Maybe something like: The secret must be base32 encoded.

[Spomky]

@fefas This exception is not thrown by this library but a dependency (see https://github.com/paragonie/constant_time_encoding/blob/master/src/Base32.php#L332).
I am not sure it is relevant to catch an exception and then throw another exception.

Another way could be to check if the secret is correctly encoded. But that is not so easy e.g. is ABSOLUTLYSECUREDSECRET encoded or not?

[gjuric]

You should wrap an exception and rethrow one of your own. This is an internal dependency of the library and any calling code should not be aware of internal dependencies, because they can be switched in the future (like you did in v9.0.2 as you state in the README).

Also, I believe you should provide an encoder implementation that belongs to this library so you can swap the encoder provider without braking the code that consumes the library.

But this should probably be discussed in another issue.

[Spomky]

OK that makes sense. I will update the library within the next days and throw a dedicated exception.

[Spomky]

Done. Tagged as v9.0.3.

[marcelsnews]

Hello, i might be very late, but i thing this could be answer here.

In fact, im facing the same issue while using the nugget TwoStepsAuthenticator v1.4.1.
The following code at the end is failing raising the exact error mentionned in this thread.
I wish someone here could help me or guide me on what to do or ultimately redirect may be where relevant ...

Any help on this is welcome please ?

ERROR: ArgumentException: Character is not a Base32 character. (Parameter 'c') TwoStepsAuthenticator.Base32Encoding.CharToValue(char c)

NOTES:

• I'm using the official js library qrcode.min.js to generate the QR-CODE for Register for MFA
• I'm using Microsoft Authenticator App to generate the code. Microsoft Authenticator scans the QR-Code "correctly" but all other Authenticator app like Google is unable to scan it ! But this shouldn't matter i guess
• The string used to generate the MFA keyUri secret is
  private readonly char[] chars ="abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890".ToCharArray();

CODE snippet: Executed when validating the user TOTP read from the Autheticator-APP

private readonly string _totpSecret = "JBSWY3DPEHPK3PXP";
....
var userSecret = await _localUserService.GetUserSecret(subject, "TOTP");
var totpSecret = (userSecret == null) ? _totpSecret : userSecret.Secret;
var authenticator = new TwoStepsAuthenticator.TimeAuthenticator();
//@@@@@THIS CHECK FAILS ALWAYS with any generated TOTP egg.  "053818", "210612"
if (!authenticator.CheckCode(totpSecret, model.Totp, user)) 
{
  ModelState.AddModelError("totp", "TOTP is invalid.");
  return View(model);
}

[Spomky]

abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890

Is this the acceptable chars for Base32? If yes, it looks wrong.
The RFC4648 only has upper case letter from A to Z (26), numbers from 2 to 7 (8) = 32 characters.

By changing the The string used to generate the MFA keyUri secret to the acceptable 32 characters, you should not have any trouble.