[SmartcardServices-Users] Submitting patches for PIVToken.cpp bugs [Yubikey Neo]

[david.lloyd at fsmail.net]

Hi,

I have raised an issue against the Yubico-Piv-tool https://github.com/Yubico/yubico-piv-tool/issues/33 
- looking at the posts it seems like the consensus is to "Add an initialize parameter that sets everything up".  There is no need for error detection or PIV specification compliance testing at 
this stage, although it would obviously be a nice-to-have feature for OpenSC in future.


I have also tried to raise a ticket against SmartcardServices/tokend, but it looks as though although the website is now back up, the "New Ticket" feature is still broken.  My text is below - I 
have highlighted a list of potential issues with the patch that should be reviewed be the PIV/TokenD experts although I don't think any of them are particularly important.



Title:  PIVToken.cpp needs to be more flexible if the CCC is not on a PIV Card [Yubikey NEO]

The Yubikey NEO PIV tool is not including the "Card Capability Container" file, which is in violation of the PIV specification.  This causes the PIV.tokend tool to fail as the PIVToken::probe() 
function (line 265)  assumes that it can access this to structure to generate the Unique name (PIV-<hexstring>) based on the card identifier.


Current behaviour:  the function uses GetDataCore() that throws an exception when the file is missing, causing Probe() to fail and the card is ignored.

Proposed behaviour:  In the case that the CCC is not available the code uses the CHUID to generate a unique name.  This is actually what Windows appears to do in its PIV driver.


I am able to compile and test any code changes.  I also have access to the NIST PIV card set for regression (if needed).  

Here is my proposed patch.  Note that:
  (1) although the CCC object is cached, it does not appear to be used anywhere else in the code.  Basic functionality in Safari has been confirmed.
  (2) existing cards with a CCC have unchanged behaviour (beyond the extra step of checking for the CCC first).
  (3) an alternative is to catch the exception when the CCC is missing, rather than to probe for it
  (4) the patch does not parse the CHUID to extract the GUID, it uses the whole file as a hex string.  This needs to be checked as the FASN would be revealed.  A SHA512 may be an 
alternative to writing a parser.
  (5) If both CCC and CHUID are missing the card will not work (similar to Windows), it would be possible to hardcode a "PIV-Unknown" UID in this case.


		if (!identify())
			doDisconnect = true;
		else
		{	
#ifndef _USEFALLBACKTOKENUID
            const size_t sz = sizeof(oidCardCapabilityContainer);
            if (getDataExists(oidCardCapabilityContainer, sz, sDescripCardCapabilityContainer))
            {
                secdebug( "probe", "Look up Card Capability Container");

                byte_string cccOid((const unsigned char *)oidCardCapabilityContainer, oidCardCapabilityContainer + sizeof(oidCardCapabilityContainer));
                byte_string cccdata;
                /*
                 Since probe is called before establish, securityd has not passed us
                 the cache directory yet, so we don't try to cache anything right now
                 */
            
                const bool allowCaching = false;
                getDataCore(cccOid, "CCC", false, allowCaching, cccdata);
                PIVCCC ccc(cccdata);
                snprintf(tokenUid, TOKEND_MAX_UID, "PIV-%s", ccc.hexidentifier().c_str());
            }
            else
            {
                secdebug( "probe", "Look up CHUID");
                byte_string chuidOid((const unsigned char *)oidCardHolderUniqueIdentifier,
                                     oidCardHolderUniqueIdentifier + sizeof(oidCardHolderUniqueIdentifier));
                byte_string chuidData;

                const bool allowCaching = false;
                getDataCore(chuidOid, "CHUID", false, allowCaching, chuidData);
                
                CssmData data;
                data.Data = &chuidData[0];
                data.Length = MAX(chuidData.size(), TOKEND_MAX_UID/2 - 6);
                snprintf(tokenUid, TOKEND_MAX_UID, "PIV-%s", data.toHex().c_str());
            }
            
            secdebug( "probe", "tokenUid is %s", tokenUid);
#else



There is a discussion of the tokens issue here:  https://lists.macosforge.org/pipermail/smartcardservices-users/2015-October/000539.html

I have also opened an issue against the Yubikey PIV tool here:https://github.com/Yubico/yubico-piv-tool/issues/33

The general sentiment is (1) that the Yubikey PIV tool should have an "initialize" option that installs all the mandatory files for novice users.  (2) As the tokend code is only using this for a 
card identifier, it would be highly desirable for it to be more flexible in the case that the file is missing.



Regards,

David L