Leaking pins: memcpy != memset

[FlorianUekermann]

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]

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

[szszszsz]

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]

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]

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]

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.