Quote Originally Posted by Jakob Kruse View Post
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):

Code:
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