arekinath / PivApplet

PIV applet for JavaCard 2.2.2 and 3.0.4+ with full ECDSA/ECDH support
111 stars 37 forks source link

Fix ykman reset and AES key support #54

Closed mistial-dev closed 3 years ago

mistial-dev commented 3 years ago

When resetting the PIV applet using ykman, the command terminates with an error, because it expects three bytes for the PIN retries.

  File "/Applications/YubiKey Manager.app/Contents/Frameworks/Python.framework/Versions/3.9/lib/python3.9/site-packages/yubikit/piv.py", line 779, in _get_pin_puk_metadata
    attempts[INDEX_RETRIES_REMAINING],

IndexError: index out of range

Looking through the ykman piv code shows the three expected bytes.

    def _get_pin_puk_metadata(self, p2):
        require_version(self.version, (5, 3, 0))
        data = Tlv.parse_dict(self.protocol.send_apdu(0, INS_GET_METADATA, 0, p2))
        pp = pprint.PrettyPrinter(indent=4)
        pp.pprint(data)
        attempts = data[TAG_METADATA_RETRIES]
        return PinMetadata(
            data[TAG_METADATA_IS_DEFAULT] != b"\0",
            attempts[INDEX_RETRIES_TOTAL],
            attempts[INDEX_RETRIES_REMAINING],
        )

This patch adds the retries remaining counter.

Additionally, processReset() failed when DES keys are disabled. This patch fixes reset when DES keys are disabled.

mistial-dev commented 3 years ago

There's a second issue with ykman in that the management slot also has metadata, including whether it is default.

    def get_management_key_metadata(self) -> ManagementKeyMetadata:
        require_version(self.version, (5, 3, 0))
        data = Tlv.parse_dict(
            self.protocol.send_apdu(0, INS_GET_METADATA, 0, SLOT_CARD_MANAGEMENT)
        )
        policy = data[TAG_METADATA_POLICY]
        pp.pprint(policy)
        return ManagementKeyMetadata(
            MANAGEMENT_KEY_TYPE(data.get(TAG_METADATA_ALGO, b"\x03")[0]),
            data[TAG_METADATA_IS_DEFAULT] != b"\0",
            TOUCH_POLICY(policy[INDEX_TOUCH_POLICY]),
        )

Commit 9429de8 adds a flag for whether or not the management key is default, and clears it when the key is changed or a DES key is not present.

Additionally, it sets the touch policy metadata for all slots to "Never", due to the lack of any means to actually perform touch. I can remove this change if desired.

mistial-dev commented 3 years ago

I had another look at the issue, and I was able to get YKMan to work with the default key in. The changes are in 855abf1.

I had to extend the length to AES192 from AES128 (so it would have the correct number of bytes), and had to bump the firmware version to 5.4.0 in order for ykman to recognize non-DES keys as supported.

arekinath commented 3 years ago

Thanks for looking into this one! I double-checked the code in libykcs11 and pivy as well, and it doesn't seem like they were looking for the PIN retry counters in the metadata anyway, which is probably why I didn't notice that was wrong.