Skip to content

Conversation

@metsma
Copy link
Contributor

@metsma metsma commented Apr 6, 2025

IB-8171

Signed-off-by: Raul Metsma [email protected]

@metsma metsma force-pushed the thales branch 3 times, most recently from a47ad42 to 77fdd42 Compare April 7, 2025 07:09
@metsma metsma force-pushed the thales branch 3 times, most recently from af2527a to 9d0c75f Compare May 29, 2025 09:37
@metsma metsma force-pushed the thales branch 4 times, most recently from 1ddfbbb to e517837 Compare June 30, 2025 20:33
@metsma metsma force-pushed the thales branch 6 times, most recently from c0a5df5 to 1b0e367 Compare July 18, 2025 08:05
@mrts mrts self-requested a review July 25, 2025 18:30
@metsma metsma force-pushed the thales branch 9 times, most recently from 9fff0ce to 38f9454 Compare July 27, 2025 21:34

void getResponseWithLE(ResponseApdu& response, byte_vector command) const
{
size_t pos = command.size() <= 5 ? 4 : 5 + command[4];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's follow defensive programming principles and protect against out of bounds writes.

Consider the following two cases:

Short command:

std::vector<uint8_t> cmd = {0x00,0x84,0x00,0x00};  // size()==4
size_t pos = cmd.size() <= 5 ? 4 : 5+cmd[4];  // pos==4
cmd[pos] = 0x08;  // write past end

Invalid Lc:

std::vector<uint8_t> cmd = {0x00,0xD0,0x00,0x00,0x10,  // Lc = 16
                            0xAA,0xBB,0xCC,0xDD,0xEE};  // only 5 data bytes, size()==10
size_t pos = 5 + cmd[4];  // 5 + 0x10 = 21
cmd[pos] = 0x08;  // write past end

We should throw cleanly and not write past end in these cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metsma correctly pointed out that the command byte vector is always converted from a CommandApdu that internally guarantees that Lc is valid.

So the issue is actually more about type safety - getResponseWithLE() and transitively transmitBytes() should use a CommandApdu parameter, not a raw byte vector, something like this:

    void getResponseWithLE(ResponseApdu& response, const CommandApdu& command) const
    {
        ...
    }

    ResponseApdu transmitBytes(const CommandApdu& command) const
    {
        byte_vector commandBytes = command;
        byte_vector responseBytes(ResponseApdu::MAX_SIZE, 0);
        auto responseLength = DWORD(responseBytes.size());

        SCard(Transmit, cardHandle, &_protocol, commandBytes.data(), DWORD(commandBytes.size()),
              nullptr, responseBytes.data(), &responseLength);

        auto response = toResponse(std::move(responseBytes), responseLength);

        if (response.sw1 == ResponseApdu::WRONG_LE_LENGTH) {
            getResponseWithLE(response, command);
        }
        ...
    }

@metsma metsma force-pushed the thales branch 2 times, most recently from 79ceae6 to e7050c0 Compare August 1, 2025 07:42
@metsma metsma force-pushed the thales branch 14 times, most recently from f02342c to a40cdf0 Compare August 4, 2025 06:02
@metsma metsma force-pushed the thales branch 2 times, most recently from 87e9b98 to 2849ce1 Compare August 11, 2025 11:55
IB-8171

Signed-off-by: Raul Metsma <[email protected]>

#include <algorithm>

using namespace std::string_literals;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be top level as it leaks everywhere the header is used, let's move it into readBinary() scope where it used.

@mrts mrts merged commit dfb29b8 into web-eid:main Aug 24, 2025
9 checks passed
@metsma metsma deleted the thales branch August 25, 2025 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants