pinDigit(uint8_t)

[dogguts]

While scowering the web as how to encode PIN codes (for the eID) in a Verify apdu, I bumped into the two files below

There seem to be two (very) similar bugs related to verifying/changing pin code in these (part of the Mac Os eID middleware?);

https://github.com/Fedict/eid-mw/blob/4fbbc51b463017f4458016077c808561b07f08f9/cardcomm/ctkToken/BEIDToken/TokenSession.m#L164-L176

This bug would only get hit on PIN codes with uneven digits, since AFAICT that's the only case "pinDigit('F')" gets called. https://github.com/Fedict/eid-mw/blob/4fbbc51b463017f4458016077c808561b07f08f9/cardcomm/ctkToken/BEIDToken/TokenSession.m#L229-L231 :warning: :warning: This would result in setting a different PIN than provided (effectively 'locking you out' of your EID).

And very similar in; https://github.com/Fedict/eid-mw/blob/4fbbc51b463017f4458016077c808561b07f08f9/cardcomm/tokend/Tokend-36720/BELPIC/BELPICToken.cpp#L383-L394

[Frederikus]

Hello Bart,

Thank you for reporting this bug, we've merged in your fix.

Though it should never get hit, as we check the PIN length to start padding, https://github.com/Fedict/eid-mw/blob/4fbbc51b463017f4458016077c808561b07f08f9/cardcomm/ctkToken/BEIDToken/TokenSession.m#L229-L231 ( ((ix+1) < PinLength ? [self pinDigit:pin[ix+1]] : [self pinDigit:'F'] ) , it should of course not be there.

Wkr, Frederik

On Tue, Jun 30, 2020 at 7:27 PM Bart Verthé notifications@github.com wrote:

While scowering the web as how to encode PIN codes (for the eID) in a Verify apdu, I bumped into the two files below

There seem to be two (very) similar bugs related to verifying/changing pin code in these (part of the Mac Os eID middleware?);

https://github.com/Fedict/eid-mw/blob/4fbbc51b463017f4458016077c808561b07f08f9/cardcomm/ctkToken/BEIDToken/TokenSession.m#L164-L176

This bug would only get hit on PIN codes with uneven digits, since AFAICT that's the only case "pinDigit('F')" gets called.

https://github.com/Fedict/eid-mw/blob/4fbbc51b463017f4458016077c808561b07f08f9/cardcomm/ctkToken/BEIDToken/TokenSession.m#L229-L231 ⚠️ ⚠️ This would result in setting a different PIN than provided (effectively 'locking you out' of your EID).

And very similar in;

https://github.com/Fedict/eid-mw/blob/4fbbc51b463017f4458016077c808561b07f08f9/cardcomm/tokend/Tokend-36720/BELPIC/BELPICToken.cpp#L383-L394

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Fedict/eid-mw/issues/115, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4FYJC6VS4N3DVROPV7QDLRZIN7HANCNFSM4OMPPOWA .

[dogguts]

Thanks for the merge, I don't even own a Mac ;)

As for the bug getting hit, it would absolutely get hit with odd PIN-lengths, since that triggers the padding. below; translated to c++, without the pinDigit-fix:

#include <iostream>

uint8_t pinDigit(uint8_t digit)
{
    if ('0' <= digit && digit <= '9')
        return digit - '0';
    else if ('A' <= digit && digit <= 'F')
        return digit - 'A' + 0x10;
    else if ('a' <= digit && digit <= 'f')
        return digit - 'a' + 0x10;
    else {
        //os_log_error(OS_LOG_DEFAULT, "BEID impossible PIN digit enterred");
        std::cerr << "BEID impossible PIN digit enterred" << std::endl;
        return 0xFF;
    }
}

void verifytest(const char* pin)
{

    uint8_t PinLength = (uint8_t)strlen(pin);

    //format PIN
    uint8_t apdu[] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
    uint32_t offset = 0;
    apdu[offset++] = 0x20 + PinLength;

    for (uint32_t ix = 0; ix < PinLength; ix += 2)
    {
        apdu[offset++] = (pinDigit(pin[ix]) << 4) + ((ix + 1) < PinLength ? pinDigit(pin[ix + 1]) : pinDigit('F'));
    }
    // display result
    std::cout << "PIN: " << pin << std::endl;
    for (int ix = 0; ix < sizeof(apdu); ix++) {
        std::cout << std::hex << "0x" << (int)apdu[ix] << " ";
    }
    std::cout << std::endl;
}

int main()
{
    verifytest("1234");   // = 0x24 0x12 0x34 0xff 0xff 0xff 0xff 0xff
    verifytest("12345");  // = 0x25 0x12 0x34 0x65 0xff 0xff 0xff 0xff   <== wrong should be 0x25 0x12 0x34 0x5f 0xff ...
    verifytest("123456"); // = 0x26 0x12 0x34 0x56 0xff 0xff 0xff 0xff
}

[yoe]

(PR was merged, so closing this bug)