search

Nitrokey / nitrokey-pro-firmware

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

Leaking pins: memcpy != memset #13

Closed FlorianUekermann closed 8 years ago

FlorianUekermann commented 8 years ago 
memcpy (card_password, 0, sizeof (card_password));

I suspect that this is supposed to zero the card_password (memset?). It doesn't. This is undefined behavior. If you are lucky the compiler doesn't generate any code for this.

A quick regex search finds 10 of these just in report_protocol.c. I didn't look further. Also, if this is used with a nonzero second argument the compiler will actually have to generate code for this afaik, so that might be better, or even worse, depending on the situation. Since it is a bit harder to search for, I didn't. Make sure to look for that as well.

The compiler issues a warning every time this pattern is used!

szszszsz commented 8 years ago

Hi @MaVo159 ! I agree - this looks like obvious typo, it should be memset instead. Reference: memset memcpy.

szszszsz commented 8 years ago

Note: clearing is already correctly implemented in Storage firmware (checking same file, that is report_protocol.c) Edit: Other files seems OK in both projects. Searched with memcpy.*0 regexp.

FlorianUekermann commented 8 years ago

Just checking that my logic isn't off. The nk hsm doesn't use this protocol at all, so these bugs don't matter on the temporary hsm branch. Actually, someone could still use it, but we would never expect a sane user to actually generate reports with sensitive info on an hsm... Correct? Otherwise we should remove the corresponding USB callbacks on the hsm branch to make sure this code is dead on hsm.

szszszsz commented 8 years ago

The functions listed in report_protocol.c (HID protocol) are not used anywhere else. I believe HSM is officially not supporting HID, so we can safely skip porting these changes to hsm branch.

FlorianUekermann commented 8 years ago

Well, I believe the protocol is still exposed on an hsm, we just don't advertise it in the descriptor. So if any illegitimate use of the protocol can lead to sensitive leaks, we need to deactivate it. But I don't have a scenario. Well... even if it is a problem, which I doubt, the protocol will be properly deactivated once the hsm branch goes away.