Conversation
|
Isn't it better to use a separate dedicated class for VERIFY PIN? See #84 |
It has separate struct? |
WE2-1055 Signed-off-by: Raul Metsma <raul@metsma.ee>
| using CommandApdu::CommandApdu; | ||
| PCSC_CPP_DISABLE_COPY(VerifyApdu); | ||
| PCSC_CPP_DEFAULT_MOVE(VerifyApdu); | ||
| constexpr ~VerifyApdu() noexcept final { std::fill(d.begin(), d.end(), byte_type(0)); } |
There was a problem hiding this comment.
Can we use memset_explicit() to ensure that the compiler will not optimize the memory overwrite away? See this and this. However, it is officially available since C23, so I'm unsure if our toolchain supports this on all platforms. In Windows we could use SecureZeroMemory(). Note that neither memset_explicit() or SecureZeroMemory() is constexpr.
We should use memset_explicit() here as well:
- https://github.com/web-eid/web-eid-app/blob/main/src/controller/utils/erasedata.hpp#L32-L34
- https://github.com/web-eid/web-eid-app/blob/main/src/ui/webeiddialog.cpp#L252-L254
(and elsewhere where we erase the pin).
| 0x00, 0x20, 0x00, p2, pcsc_cpp::byte_vector(pinMinMax.second, paddingChar)}; | ||
| response = session.transmitCTL(verifyPin, 0, pinMinMax.first); | ||
| response = session.transmitCTL( | ||
| pcsc_cpp::CommandApdu::verify(p2, std::move(pin), pinMinMax.second, paddingChar), 0, |
There was a problem hiding this comment.
If the caller accidentally provides a non-empty PIN for the pin-pad reader case, this change causes it to be embedded into the APDU. Previous code with the dummy padding-only PIN was safer. Or am I mistaken here?
| pcsc_cpp::byte_type paddingChar) | ||
| { | ||
| if (!pin.empty() && pin.capacity() < paddingLength + 5) { | ||
| throw std::invalid_argument( |
There was a problem hiding this comment.
Let's use the THROW macro to include source location metadata:
| throw std::invalid_argument( | |
| THROW(ProgrammingError, |
| * Lc and Data field = Empty or verification data | ||
| * Le = Empty | ||
| */ | ||
| static PCSC_CPP_CONSTEXPR_VECTOR VerifyApdu verify(byte_type p2, byte_vector&& pin, |
There was a problem hiding this comment.
Regarding PCSC_CPP_CONSTEXPR_VECTOR, can and should polymorphic types (with a virtual destructor) be constexpr? CommandApdu and VerifyApdu are polymorphic now and I'm not sure constexpr really applies to them any more.
On the other hand, we could avoid making CommandApdu destructor virtual as the temporary VerifyApdu object is deleted inside verifyPin() directly, not through a CommandApdu pointer or reference.
| constexpr ~VerifyApdu() noexcept final { std::fill(d.begin(), d.end(), byte_type(0)); } | ||
| }; | ||
|
|
||
| PCSC_CPP_CONSTEXPR_VECTOR VerifyApdu CommandApdu::verify(byte_type p2, byte_vector&& pin, |
| size_t paddingLength, | ||
| pcsc_cpp::byte_type paddingChar) | ||
| { | ||
| if (!pin.empty() && pin.capacity() < paddingLength + 5) { |
There was a problem hiding this comment.
This will fail if pin.size() > paddingLength. You can argue that this will not happen, but we should consider edge cases.
For example, assume pin.size() == 12 and paddingLength == 8, so no padding, payload is 12 and required capacity is 12 + 5 = 17. But if you only check for paddingLength + 5 = 13, there will be a reallocation.
And by ES.45: Avoid “magic constants”; use symbolic constants I would recommend replacing 5 with APDU_HEADER_AND_LC_SIZE or something like that. If you run clang-tidy, you will see a warning.
2 participants
WE2-1055
Signed-off-by: Raul Metsma raul@metsma.ee