Nitrokey / nitrokey-pro-firmware

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

Authorization mechanism is vulnerable to CRC32 collisions #8

Closed FlorianUekermann closed 7 years ago

FlorianUekermann commented 8 years ago

I have some doubts whether this is an actual problem since the temporary password and pin are clear text anyway on the hid level. An attacker with access to the stuff sent via hid can authorize anything anyway once he observed either pin or password. However, since the protocol requires explicit "authorization" of some commands, I assume that that makes sense somehow. Here is how to circumvent it:

Authorization of commands via the (USER_)AUTHORIZE command requires the CRC of the command that is to be authorized and the temporary password.

Between the authorization command and the authorized command (e.g. GET_CODE), an attacker can execute any other command with a matching CRC without knowing the temporary password or pin. As far as I know CRC32 is not considered cryptographically secure and even reversible. It should be therefore be trivial to create collisions using the unused bytes in the payload.

I verified that GET_CODE works fine with arbitrary values in "unused" payload bytes.

Solutions: Add cryptographically secure hash or password to AUTHORIZE payload.

Question: I am pretty sure I am missing the point here. What is the reason for having temporary password and authorizations? It doesn't look like it is designed to provide security.

jans23 commented 8 years ago

The Nitrokey is not supposed to be secure agains MITM attacker on the USB connection. This is the same as with local smart cards which receive the PIN in cleartext.

AFAIK the idea of the temporary password is to minimize the time the actual PIN needs to be available (stored in RAM) on the client computer. But this would come as a bonus and doesn't provide guaranteed security (in case of malware for instance).

FlorianUekermann commented 8 years ago

I'm still not quite understanding the design here.

Yes, the idea with the temporary password to protect the pin makes a lot of sense. What I find surprising is to have both the authorization via temp password and then getting the code using a command with the authorized CRC as a two step process. Transmitting the password as part of GET_CODE would be sufficient. Why the detour using the CRC32?

The USER_AUTHORIZE detour in the protocol still makes this vulnerable, even without MTIM, because the CRC32 of authorization requests is fairly predictable. The attacker doesn't need to be a MITM.

A possible mitigation strategy would be to fill the GET_CODE command with random data in the unused bytes. That would protect against faked GET_CODE commands with up to 32bit entropy.

However, 32bit is still not all that much, especially given the fact that using the 200bit password directly would provide much more security while simplifying the protocol quite a bit.

FlorianUekermann commented 8 years ago

I would like to note that the audit of the storage firmware notes the same issue (fixed in Storage firmware): "NK-01-007 OTP commands can be used without authorization" The described attack is a little more straightforward than what I had in mind (wish I would have thought of their example...), but the underlying issue is the same (reversibility of CRC32).

The suggested fix is also the same: "It is recommended to send temp_user_password with every report that requires it and remove the CRC32 code."

I think this can be fixed in a backwards compatible manner if the app just does both and USER_AUTHORIZE does nothing in the fixed firmware. Just in case you want to avoid bumping the version (sounds like a bad idea though imo).