search

Nitrokey / libnitrokey

Communicate with Nitrokey devices in a clean and easy manner
https://nitrokey.com/
GNU Lesser General Public License v3.0
65 stars 34 forks source link

Increase user password buffer in IsAESSupported command #158

Open d-e-s-o opened 5 years ago

d-e-s-o commented 5 years ago

The IsAESSupported command struct has a 20 byte buffer size for storing the user password. That is in contrast to, say, the EnablePasswordSafe struct which uses a 30 byte buffer. Such a smaller buffer can cause string length errors to be emitted for a legitimate user PIN as has been found as part of the investigation for d-e-s-o/nitrocli#85. That is, the nitrokey allows for setting a user PIN of 21 characters. Retrieving an OTP using such a PIN works fine, whereas inquiring the PWS status does not, as it first tries to cram the supplied password into 20 characters, which fails. This change increases the buffer size in the IsAESSupported command struct to 30 bytes.

szszszsz commented 5 years ago

Hi! Thank you for the PR! I was sure this is the same size as in the Pro's and Storage's firmwares, but turned out it should be 25 bytes (in Pro case), not 20. In Storage case it seems there is no handling at all for this command (DETECT_SC_AES, ==0x6a), so the call in libnitrokey could be removed for this device. Will correct that to 25 bytes to maintain correct mapping with the Pro firmware. What's more, I need to confirm that yet with the historical data (not sure if the v0.7 was the first released ever), but I think that this command call is redundant and could be removed altogether.

As for the size choice, the 25 bytes PIN size used in commands was selected to allow packing both old and new PIN inside one USB 1.1 HID command 64 bytes buffer, for case of its update through HID (to keep things simple, and avoid more complicated transport protocol). Some of the commands indeed allow 30 bytes PIN length, and this is inconsistent (though might be usable in case, when the longer PIN is set via the CCID interface).

Our use-case rule for the maximum PIN size is 20 bytes (link to FAQ), and usual size is expected to be about 10 characters (or less), since smart card does not allow more than 3 guess attempts anyway, and thus brute-forcing here could not apply.

Edit: added link to FAQ

d-e-s-o commented 5 years ago

Thanks for the explanation! Sounds good. If there is nothing you need from my side then feel free to close this pull request.

Some of the commands indeed allow 30 bytes PIN length, and this is inconsistent (though might be usable in case, when the longer PIN is set via the CCID interface).

Okay, but can we somehow at least make the command for changing the PIN accept the lowest of the sizes of the other command's buffers? Otherwise we can change the PIN successfully and then have other commands fail with errors when really they must not.

szszszsz commented 5 years ago

If there is nothing you need from my side then feel free to close this pull request.

I plan to merge it with a modification in a separate commit, to keep your work included.

Okay, but can we somehow at least make the command for changing the PIN accept the lowest of the sizes of the other command's buffers? Otherwise we can change the PIN successfully and then have other commands fail with errors when really they must not.

You are right with that case of course. I will decrease it to 20 bytes, with a 5 bytes padding to keep the structure size. Perhaps I will treat the other PIN related structures similarly, to make this consistent.

Thank you for the idea!