cSecureOneTimePassword broken

[Jakob Kruse]

Hi

Great library, thank you!

There is an unfortunate TODO left in Procedure UnpackParameterFromStorage: "handle possible issues due to base32 padding". "Unfortunate" because the issues are not "possible" but very real. It doesn't actually work unless you use SHA1, which of course you shouldn't, because it was broken long ago.

This minimal example program illustrates the problem (with SHA256, but the other non-SHA1 algos have the same issue):


Use cApplication.pkg
Use DFSecurity.pkg
Use DFSecurity_CNG.pkg
Use cSecureOneTimePassword.pkg


Object oOneTimePassword is a cSecureOneTimePassword
Set piHashImplementation to C_SEC_HASH_CNG_HMAC_SHA256
Set psIssuer to "Demo"
End_Object


Procedure Test
Handle hoOTP
String sOTPSpec sSecret sBrokenSecret

Get NewOtp of oOneTimePassword C_SEC_OTPTYPE_TIME "guest" to hoOTP
Get PackForStorage of hoOTP to sOTPSpec
Get SecretAsBase32 of hoOTP to sSecret
Send Destroy of hoOTP
Get UnpackFromStorage of oOneTimePassword sOTPSpec to hoOTP
Get SecretAsBase32 of hoOTP to sBrokenSecret

Showln "Generated secret (note padding with '='):"
Showln sSecret
Showln
Showln "OTP Spec (note padding removed):"
Showln sOTPSpec
Showln
Showln "Secret extracted from OTP Spec (note truncation):"
Showln sBrokenSecret
End_Procedure


Send Test
Inkey windowindex


So, what is the problem, exactly? Well, the problem is that the Base32 representation of a hash value can include a number of padding characters at the end (see first output in program).

The padding characters are "=" signs, and those are not good in URI's, so when the cSecureOneTimePassword "packs" an instance into a URI (to use in a QR code), the padding characters are removed from the Base32 encoded secret (see second output in program).

Unfortunately, when the secret from the URI is "unpacked" back into an OTP instance, the missing padding characters are not put back, and as a result the Base322Bin method, expecting a string length that is a multiple of 8, discards the end of the secret.

Result: The secret that was originally created, and is in the URI/QR code, is not identical to the secret you get when unpacking the stored OTP spec from your database.

Only workaround right now is to only use SHA1.

I don't know whether the "proper" fix is in UnpackParameterFromStorage (re-adding missing padding chars), or in Base322Bin (handling missing padding chars).

Also, comment above Base322Bin is wrong (copy/pasted from Bin2Base32). Should probably read something like "Base32 decoding is performed 8 bytes at a time, resulting in 5 bytes decoded."

[Bram]

Hi

Great library, thank you!

There is an unfortunate TODO left in Procedure UnpackParameterFromStorage: "handle possible issues due to base32 padding". "Unfortunate" because the issues are not "possible" but very real. It doesn't actually work unless you use SHA1, which of course you shouldn't, because it was broken long ago.

This minimal example program illustrates the problem (with SHA256, but the other non-SHA1 algos have the same issue):


Use cApplication.pkg
Use DFSecurity.pkg
Use DFSecurity_CNG.pkg
Use cSecureOneTimePassword.pkg


Object oOneTimePassword is a cSecureOneTimePassword
Set piHashImplementation to C_SEC_HASH_CNG_HMAC_SHA256
Set psIssuer to "Demo"
End_Object


Procedure Test
Handle hoOTP
String sOTPSpec sSecret sBrokenSecret

Get NewOtp of oOneTimePassword C_SEC_OTPTYPE_TIME "guest" to hoOTP
Get PackForStorage of hoOTP to sOTPSpec
Get SecretAsBase32 of hoOTP to sSecret
Send Destroy of hoOTP
Get UnpackFromStorage of oOneTimePassword sOTPSpec to hoOTP
Get SecretAsBase32 of hoOTP to sBrokenSecret

Showln "Generated secret (note padding with '='):"
Showln sSecret
Showln
Showln "OTP Spec (note padding removed):"
Showln sOTPSpec
Showln
Showln "Secret extracted from OTP Spec (note truncation):"
Showln sBrokenSecret
End_Procedure


Send Test
Inkey windowindex


So, what is the problem, exactly? Well, the problem is that the Base32 representation of a hash value can include a number of padding characters at the end (see first output in program).

The padding characters are "=" signs, and those are not good in URI's, so when the cSecureOneTimePassword "packs" an instance into a URI (to use in a QR code), the padding characters are removed from the Base32 encoded secret (see second output in program).

Unfortunately, when the secret from the URI is "unpacked" back into an OTP instance, the missing padding characters are not put back, and as a result the Base322Bin method, expecting a string length that is a multiple of 8, discards the end of the secret.

Result: The secret that was originally created, and is in the URI/QR code, is not identical to the secret you get when unpacking the stored OTP spec from your database.

Only workaround right now is to only use SHA1.

I don't know whether the "proper" fix is in UnpackParameterFromStorage (re-adding missing padding chars), or in Base322Bin (handling missing padding chars).

Also, comment above Base322Bin is wrong (copy/pasted from Bin2Base32). Should probably read something like "Base32 decoding is performed 8 bytes at a time, resulting in 5 bytes decoded."

Hi Jakob,

I looked into it; to fix change cSecureOneTimePassword.pkg:167 to " Move (UrlEncode(Parent(Self), sBase32Secret, False)) to asParams[iParam][1]" and then it should solve the issue above. Honestly as far as I can see, I don't see a reason this was done in the first place. I think it is because somebody thought that '=' would interfere with Get parameters but it works just fine.

We will officially patch it in time.

Thanks for bringing it to us.

Sincerely,

Bram Nijenkamp
Data Access Europe

[Anders Ohrt]

Hi Bram,

I've just stumbled onto this as well, and your fix does not work. Microsoft Authenticator is fine with it, but Google Authenticator says the QR code is invalid.

I tried with a secret of 30 and 35 bytes instead, so no padding, and then the Google Authenticator worked, but not the Microsoft one. I've not found a combination with SHA256 where they both work, so I'm switching so SHA1 again.

[Jakob Kruse]

Sorry, I had not seen your reply, Bram, until Anders just posted.

That will indeed not work, as Anders wrote. The problem is an inconsistency between UnpackParameterFromStorage and Base322Bin, and the fix needs to be in either one of them, or possibly both. URL-encoding will break the QR codes.

[Bram]

Sorry, I had not seen your reply, Bram, until Anders just posted.

That will indeed not work, as Anders wrote. The problem is an inconsistency between UnpackParameterFromStorage and Base322Bin, and the fix needs to be in either one of them, or possibly both. URL-encoding will break the QR codes.

I'll take another look soon.