CommandAPDU reuse data object to avoid PIN copy #80
Conversation
WE2-1007 Signed-off-by: Raul Metsma <[email protected]>
WE2-1007 Signed-off-by: Raul Metsma <[email protected]>
| @@ -140,53 +140,37 @@ struct ResponseApdu | |||
| }; | |||
|
|
|||
| /** Struct that wraps command APDUs. */ | |||
| struct CommandApdu | |||
| struct CommandApdu : public byte_vector | |||
There was a problem hiding this comment.
Inheriting from STL containers is usually frowned upon, see isocpp/CppCoreGuidelines#1232
But I see your point and like this creative approach. Note that std::vector destructor is non-virtual, so deallocating CommandApdu through a byte_vector pointer can lead to undefined behavior, but I think only if we add fields to CommandApdu (possibly in the future). At minimum, both the rationale and undefined behavior risk must be clearly documented.
| { | ||
| } | ||
|
|
||
| CommandApdu(const CommandApdu& other, byte_vector d) : | ||
| CommandApdu(other.cla, other.ins, other.p1, other.p2, std::move(d), other.le) | ||
| // Case 2 |
There was a problem hiding this comment.
Should we disable copying and assignment and only allow moving?
There was a problem hiding this comment.
There are still many cases where we need make copy. Eg hash inputs are all const ref.
| CommandApdu(byte_type cls, byte_type ins, byte_type p1, byte_type p2, byte_vector data, | ||
| byte_type le) : CommandApdu {cls, ins, p1, p2, std::move(data)} | ||
| { | ||
| push_back(le); | ||
| } | ||
| }; |
There was a problem hiding this comment.
We should add a destructor that erases memory with std::fill(data.begin(), data.end(), 0)
There was a problem hiding this comment.
We cannot have destructor because std::vector does not have virtual destructor.
We could do another PR to change all const predefined APDU-s to runtime generated and move ownerships to transfer() where we can clear all buffers.
This change does not clear all PIN copy-s only minimises.
| + bytes2hexstr(command.toBytes()) + "' - expected '"s | ||
| + bytes2hexstr(expectedResponseBytes) + "', got '"s + bytes2hexstr(response.toBytes()) | ||
| + " in " + removeAbsolutePathPrefix(file) + ':' + std::to_string(line) + ':' | ||
| + bytes2hexstr(command) + "' - expected '"s + bytes2hexstr(expectedResponseBytes) |
There was a problem hiding this comment.
Maybe we should add a SecureCommandApdu that outputs *** when passed to bytes2hexstr()?
There was a problem hiding this comment.
Again out of scope this change. Only minimise copy-s of PIN memories. The issue already is in master branch
| } | ||
| insert(begin(), {cls, ins, p1, p2, static_cast<byte_type>(size())}); |
There was a problem hiding this comment.
Isn't it so that when using the T=0 protocol, the Le field should be omitted? Shouldn't you handle protocol differences here?
There was a problem hiding this comment.
We have many cases where we send with T=0 Le field and do not have any issues reported:
https://github.com/web-eid/libelectronic-id/blob/main/src/electronic-ids/pcsc/EIDIDEMIA.cpp#L146-L152
https://github.com/web-eid/libelectronic-id/blob/main/src/electronic-ids/pcsc/FinEID.cpp#L161-L162
If this is issue we should handle this in transfer()
WE2-1007
Signed-off-by: Raul Metsma [email protected]