mifare_desfire_get_key_settings() discards encryption type bits

[tlo2357]

In mifare_desfire.c line 574 reads

*max_keys = p[1] & 0x0F;

The masking discards the encryption type indicated in the top two bits by an EV1 card.

Rewriting the line as

*max_keys = p[1]; 

would correct that but I suggest also renaming the function parameter to settings2. I don't have an EV1 datasheet to confirm it but I suspect that this byte has been renamed in the spec due to these bits.

[smortex]

Hum, this was changed in 42f9457 which I committed probably after somebody gave me a diff (I do not have the DESFire EV1 datasheet neither, and that's a fairly large amount of code for a single commit. AFAICR, I had an EV1 tag so I could use it to check the code was working after auditing and before committing).

That to say, I have no way to know how should this byte should be interpreted in the latest specifications. Prior to EV1, the full byte was the number of keys.

If someone has access to more recent information, can you enlighten us? And maybe provide a patch?

[smortex]

Information is available on page 41 of this PDF:

Would you like to crate a PR that add a function (e.g. mifare_desfire_get_key_settings()) which also return the key type?

[tlo2357]

Are you suggesting the addition of a new function or a modification of mifare_desfire_get_key_settings() like I described?

Thanks for the PDF.

[smortex]

The older specification has nothing about these bits, so I think the best would be to add a new function that makes this information available through an extra paramter, and rework the older mifare_desfire_get_key_settings() function to rely on the newer to keep backward compatibility. Does that look reasonable?

If so, maybe we can name the new function mifare_desfire_get_key_settings_ev1(), and maybe one day we will have access to ev3 datasheet that might explain how bits at position 0b00110000 are used for something else, and we will be able to add another new function to make this information available.

[tlo2357]

I think it's overkill to create a new function in this case where the function parameters have not changed at the wire level; i.e. two bytes received map to the two parameters. IMHO, a mistake was made by trying to be clever and masking the second byte for the caller's benefit; that put too much intelligence into the library function — based on a correct-at-the-time assumpution that only the lower four bits meant anything — which broke the function for the EV1 when the upper two bits were defined.

I see this type of function as glue between the caller and the card, in this case simply executing the GetKeySettings command and returning the two result bytes with no interpretation of them. That's the caller's responsibility.

Simply removing the masking would be transparent to all but those who are using AES or 3k3DES keys and they're probably annoyed that the function isn't returning the expected result. However, I appreciate that there may be a worry about application breakage for those same AES or 3k3DES users who may not be masking the value themselves out of ignorance. On the other hand, the library is still at major version 0, when breakage is to be expected.

Looking at the two remaining bits: Bit 5 is defined by CreateApplication but it is not returned by GetKeySettings (an EV1 bug?). Bit 4 is marked reserved by CreateApplication but GetKeySettings is silent about it. Again, my suggestion is to leave it to the caller to interpret those bits once they're defined.