Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mifare_desfire_get_key_settings() discards encryption type bits #135

Open
tlo2357 opened this issue Feb 5, 2021 · 5 comments
Open

mifare_desfire_get_key_settings() discards encryption type bits #135

tlo2357 opened this issue Feb 5, 2021 · 5 comments

Comments

@tlo2357
Copy link

tlo2357 commented Feb 5, 2021

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
Copy link
Contributor

smortex commented Mar 4, 2021

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
Copy link
Contributor

smortex commented Mar 4, 2021

Information is available on page 41 of this PDF:

image

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
Copy link
Author

tlo2357 commented Mar 13, 2021

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
Copy link
Contributor

smortex commented Mar 14, 2021

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
Copy link
Author

tlo2357 commented Mar 14, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants