Fix base 32 encoding of OTP

[daniellowtw]

Hi, I noticed that the length of the shared secret for OTP does not add up. We generate a 20 byte (160bit) shared key which means that when we encode it in base 32, we should get 160/5 = 32 character long key. Instead, we see a 64 character long key. This is due to the underlying library parsing Buffer and string differently. The previous code was actually encoding the 40 = (160/4) character long hex string as string, resulting in a 40 byte input and hence the 64 character long key.

Although this will work with OTP generator (such as freeOTP) that reads the printed base 32 string, it will not work with generators that read the hexstring, (such as yubikey).

This PR should fix those issues. The impact of this would mean that people with OTP set up needs to redo the pairing.

[frankrousseau]

@babolivier Could you review this PR? It changes your code, I think you are the best person to perform this review.

[babolivier]

Sure thing @frankrousseau, the code seems fine, I'll run a few tests locally and report back then.

[babolivier]

Hi @daniellowtw,

I ran a few tests here. I just couldn't authenticate, using both Google Authenticator and FreeOTP. I see you mentionned Cozy generating a 20 bytes key, but the actual key we generate is 40 bytes. Are you mistaken regarding this point, or did I misread your initial post?

[daniellowtw]

Hi @babolivier,

The confusion here is we treat the 40 character hex string, which should represent a 20 byte key, as a 40 character string, which implies a 40 byte key. It worked before because the verifier was set up to use the 40 character string as a 40 byte key to verify the OTP and the generators were using the 40 byte key to generate OTPs. However, there will be a problem when someone tries to use a different generator. For example, Yubikey only accepts 20 byte keys to seed the OTP generator. This PR fixes it by making the verifier use the 40 character string as a 20 byte key, and also generate the base32 string for the 20 byte key.

If you go into databrowser > user > encryptedOtpKey (I also think this is a misnomer as it is not really encrypted but just a shared key but that's another issue), you'll see a 40 character hex string, which really corresponds to a 20 byte key, and not a 40 byte key, because 2 hex characters (4 bits each) represents a byte.

If you want to use a hardware OTP token like yubikey, you need to give it the 40 character hex string displayed there instead of the base32 string. The specification for that can be found here and here (section 5.3).

However, if you try to give it the 40 characters (20 byte key) from the databrowser and try to generate the OTP using counter mode, you'll realize that the yubikey will generate very different OTP compared to FreeOTP or Google Authenticator when those two are set up with the base32 string.

Unlike the Yubikey, the server verifier, FreeOTP or GoogleAuth have no restriction on the length of the key given (and hence more lenient than the spec) so they will happily generate and accept the OTPs but what we've actually done is we've set up the shared key to be a completely different 80 character hex string from the 40 character hex string in the databrowser.

To give a concrete example:

Inside the databrowser (0): 83b3999dd1d28556c9a4f9c5f1b8a046d6e2c75e Displayed Base32 string (1): HAZWEMZZHE4WIZBRMQZDQNJVGZRTSYJUMY4WGNLGGFRDQYJQGQ3GINTFGJRTONLF Correct Base32 string (2): QOZZTHOR2KCVNSNE7HC7DOFAI3LOFR26

Setting up the Yubikey with (0) and generating HOTP gives: 928130 191094 282136

Using FreeOTP with (1) to generate HOTP gives: 129432 413171 380427

Using FreeOTP with (2) to generate HOTP gives: 928130 191094 282136

What the server currently expects: 129432 413171 380427

What the server should really expect: 928130 191094 282136

[frankrousseau]

@babolivier @daniellowtw Did you find a consensus? Do you need help to decide what's good to do?

[babolivier]

@frankrousseau Not yet, I haven't got all the time I wanted to work around this issue yet.

[nono]

@babolivier just a small reminder. If you have some time what we should do on this issue, it will be really appreciated.

[babolivier]

Hi all. I just received my Yubikey so I'll be available to run some tests on the proposed solutions and to try to find one that works with both mobile apps and Yubikeys.

By the way @daniellowtw, I never took the time to answer this part:

I also think this is a misnomer as it is not really encrypted but just a shared key but that's another issue

A field which name starts with "encrypted" is automatically encrypted in the database by Cozy's Data-System, then decrypted on demand once the user has been authenticated. You don't see it encrypted on Cozy's Databrowser as the decryption already took place while the databrowser was requesting the data, but it actually is encrypted in the database :wink:

[daniellowtw]

@babolivier Let me know your thoughts after you've tried it with the YubiKey.

A field which name starts with "encrypted" is automatically encrypted in the database by Cozy's Data-System

Ahh OK, that makes sense. Thanks for clarifying.

[babolivier]

Okay, I get it now. @daniellowtw, your fix was good, but there was also a fix to be made in the proxy (which handle authentication). I'll send a PR about that within the next hour. The problem was that, although the hex key is correctly handled in the home, the proxy still processes it as a string when checking the authentication. So we couldn't have a successful auth even with the right key. I made some fixes, and was able to authenticate with both my Yubikey and my smartphone. So @frankrousseau I think the PR can be merged once @daniellowtw has resolved all conflicts. Again, sorry for the delay. It has been a busy few weeks, and I didn't get right away why the fix wasn't working.

[nono]

@daniellowtw hello, thanks for your patience. Do you have time to fix the conflicts?

[daniellowtw]

@babolivier @nono PTAL

[babolivier]

@daniellowtw Thanks for the line width.

By "conflicts", we basically meant that there have been some changes between your fork creation and now on Cozy's branch which require you manually fixing merge conflicts, as shown by GitHub's mention:

I don't know how to use Git well enough to help you with that (all the merge conflicts resolutions ended up either magically resolving themselves or by me manually reproducing my changes in another pull request). Maybe @nono can help you with this.

[daniellowtw]

@babolivier Ahh sorry. I was too hasty in reading. I have merge master into my branch and it shouldn't have any more conflicts. I have one assumption in my merging and that is the recovery codes generated are completely independent of the encryptedOtpKey.

[nono]

@daniellowtw Thanks! I'm busy right now, but I'll look at it soon.

[nono]

Looks good to me!

[babolivier]

Awesome. Thanks @daniellowtw for your time, efforts and patience! :pray: I've been making this issue wait way too long, and I apologize for that.