Nitrokey / nitrokey-pro-firmware

Firmware for the Nitrokey Pro device
GNU General Public License v3.0
119 stars 22 forks source link

User command authorization works only once #7

Closed FlorianUekermann closed 7 years ago

FlorianUekermann commented 8 years ago

How I think the protocol is supposed to work:

  1. The USER_AUTHENTICATE authenticate command sets a temporary password, which I assume is supposed to be used as authorization secret during a session.
  2. The USER_AUTHORIZE command informs the device about an upcoming command CRC and needs to provide the correct temporary password.
  3. Get OTP using GET_CODE
  4. Repeat from step 2.

What actually happens:

First time works fine. Second time: The response to GET_CODE reports the status NOT_AUTHORIZED (payload 0), even though the response to USER_AUTHORIZE looks fine.

To successfully use GET_CODE repeatedly, both the USER_AUTHENTICATE and USER_AUTHORIZE command have to be used every time.

What I didn't test: There might be a similar issue with the other temporary password (FIRST_AUTHENTICATE). Or not.

szszszsz commented 8 years ago

Hi @MaVo159 ! Are your codes PIN protected? I do not have to authorize getting HOTP codes otherwise AFAIR. Please check this simple example code

FlorianUekermann commented 8 years ago

I'm testing with TOTP. I'm not sure what you mean with "Are your codes PIN protected?". The first step (USER_AUTHENTICATE) involves sending the pin if I remember correctly, so that would mean they are PIN protected, I guess. I'm fairly certain not doing the USER_AUTHENTICATE & USER_AUTHORIZE thing first gives an error even on the first GET_CODE.

Actually, I assumed you were aware of this bug since it seems familiar from nitrokey-app#82. But that is just a guess.

Your test code only does HOTP from what I can see, so I wouldn't be surprised if the bug doesn't show up there. I'll get to checking the HOTP stuff in detail at some point. So far I have only tested TOTP thoroughly.

If you can't reproduce this with TOTP, comment again. In about a week I am back from vacation and may have time to deal with this.

szszszsz commented 8 years ago

I have asked about PIN protection since this switch possibly could force user to authenticate each code request (it does in application and from your description it looks like it is checked on device). When I have this one checked the test code I have linked was not working. The option state is visible in Nitrokey App's Safe settings as in mentioned nitrokey-app#82.

As for HOTP & TOTP, the GET_CODE should be the same for both, since they differ only with slot number as far as I remember. If in doubt, you can always check how App handles this (the commands sequence) with running nitrokey-app -d and selecting Debug from context menu. Let me know has this worked for you. Have a nice holidays!

FlorianUekermann commented 8 years ago

Ah. Makes sense. I'll check.

FlorianUekermann commented 8 years ago

Yes you were right. It is checked. Unchecking it allow multiple OTP requests. However, now it doesn't require any PIN at all.

Just so I get this straight... This does not work as intended, right? The only way I can wrap my head around all these different commands is that the idea behind the temporary password is: Do authentication via PIN once and then authorize the generation of OTPs many times via temporary password without reentering the PIN or keeping it in memory.

szszszsz commented 8 years ago

Just to leave a solution - the culprit is here: https://github.com/Nitrokey/nitrokey-pro-firmware/blob/master/src/keyboard/report_protocol.c#L757 https://github.com/Nitrokey/nitrokey-pro-firmware/blob/master/src/keyboard/report_protocol.c#L733

cmd_authorize should not clear temporary password, but it does. Reseting temporary password on authorization cancels whole purpose of using it - protecting OTP codes with PIN needs providing PIN from user each time, while temporary password should be used.

This is already fixed on NK Storage in same places.

FlorianUekermann commented 7 years ago

The fix for this should be done together with #8 since the same code will be touched.