Handling of sensitive data in HID transactions

[FlorianUekermann]

I have some doubts about whether the data in commands and responses is handled properly or not.

Example from device.cpp l.1667ff :

int Device::firstAuthenticate(uint8_t cardPassword[], uint8_t tempPasswrod[]) {
  int res;
  uint8_t data[50];
  uint32_t crc;
  memcpy(data, cardPassword, 25);
  memcpy(data + 25, tempPasswrod, 25);
  if (isConnected) {
    Command *cmd = new Command(CMD_FIRST_AUTHENTICATE, data, 50);
    res = sendCommand(cmd);
    crc = cmd->crc;
    // remove the card password from memory
    delete cmd;

C++ can be complicated, maybe I missed something. But it seems like the sendCommand function doesn't clear the contents of Command and I couldn't find a Destructor. How do you clear the data in the command struct?

[Belatronix]

I 've already pointed to a similar problem in issue 84

[FlorianUekermann]

Yes, I saw your report. I added this because I wanted to draw attention to HID reports specifically (commands and responses). This goes a little beyond leaking the pin. Even CRCs can be fairly sensitive in some contexts.
Once a command is sent, or any report goes out of scope they should all be zeroed imo. This can for example be done by using variables with automatic storage duration instead of dynamic ones. Or in the case of commands, just inside the sendCommand function. That would eliminate whole classes of potential problems, while reducing the amount of code and the amount of things the programmer/reader needs to remember.

[szszszsz]

This is fixed in v1.0. HID reports are now cleared on destruction.
https://github.com/Nitrokey/libnitrokey/blob/master/include/device_proto.h#L167