search

Nitrokey / nitrokey-pro-firmware

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

Buggy OTP slot range check #4

Closed FlorianUekermann closed 8 years ago

FlorianUekermann commented 8 years ago

I believe the OTP slot range checks are wrong (<= instead of <): The following lines in /src/keyboard/report_protocol.c are affected: 330, 341, 368, 383, 416, 435, 474, 490, 539, 546

Validity: I have verified this by sending a command report with READ_SLOT as command and either 0x13 or 0x2F as payload (reads the non-existent 4th HOTP or 16th TOTP slot). In both cases the command status in the response is CMD_STATUS_SLOT_NOT_PROGRAMMED, when it should be CMD_STATUS_WRONG_SLOT (checked with payloads >0x13 && <0x20). I would have tried writing the slots, but I only have 1 key and don't want to brick it or screw up the OTP keys stored on it. I looked at the code though (see below).

Disclaimer: I don't know much about the hardware, nor have I properly analysed the firmware. The following could be BS.

Severity: I tried to get an idea of how bad this is. My first impression from the rest of the code is that this reads the first 32bit behind the respective (t|h)otp_slot_offsets arrays and uses whatever happens to be there to calculate the memory address for reading and writing the slot data. It doesn't seem like there are any more checks, so I guess the impact of this really depends on what the value of those 32bit behind the array is. The corresponding address range would not only be somewhat readable, but by the looks of it also writeable using HID reports.

Questions: What are the values behind the two arrays, any guarantees on that? Assuming the values are fixed: Where do they point if used to calculate the addresses for reading and writing OTP slots?

Firmware binary: For issues like this it may be important to get the firmware binary on the device. Can you provide either the compiled binary or instructions for reproducible builds and a hash?

Comment: The bounds checks are replicated all over the place. I guess some refactoring wouldn't hurt.

FlorianUekermann commented 8 years ago

Note: This also needs a permanent workaround in the library, since most keys are running old firmware. As hinted at above, this may result in memory corruption, if someone tries to write to the wrong slot, since the firmware will just try to do it instead of sending back the error for non-existing slots.

szszszsz commented 8 years ago

:arrow_up: For information completeness: libnitrokey already checks valid range.