DESFire key diversification does not agree with AN10922

[dodgambit]

mifare_key_deriver does not compute the right diversified key when the input is less than the key block size.

This problem will be very common for something like UID-based diversification, since the UID will always be less than the key block size.

For example, AN10922 specifies that the input data for AES128 key diversification input must consist of exactly 32 bytes: 0x01 | M | Padding.

However, if M is, say, 7 bytes long, mifare_key_deriver_end_raw() will only pass 8 bytes (0x01 | M) to the libfreefare cmac() function, which will pad that input to 16 bytes and compute the CMAC of just those 16 bytes.

According to AN10922, the input should have been padded to 32 bytes, so that 2 encryptions are done, flowing through the IV, presumably producing a "more encrypted" or "more difficult to reverse" result.

There is no way to tell the existing cmac() function to pad to 32 bytes in this case when the input is less than 16 bytes. This is only a problem with key diversification -- cmac() pads just fine when computing the CMAC for a secure channel.

• This problem also exists for DES key derivation when M is < 8 bytes.
• Conversely if M is >= 32 bytes (AES) or M >= 16 bytes (DES), then too many bytes will be sent to the cmac() function.

Solutions:

• Document that this is not actually AN10922 key diversification.
• Document that it is the caller's responsibility to ensure that for AES, M is at least 16 bytes, so that the mifare_key_deriver will pad to 32 bytes, and similarly for DES.
• Fix the implementation to bring it into compliance with AN10922.

[dodgambit]

:( It is unfortunately too late for me, since I already have cards deployed with the wrong (possibly less secure?) derivation function.

[dodgambit]

Untested code for correcting cmac(), so mifare_derive_key can pass buffer manually padded to correct length for key but the crypto commands can still use default padding.

void
cmac(const MifareDESFireKey key, uint8_t *ivect, const uint8_t *data, size_t len, bool padded, uint8_t *cmac)
{
    int kbs = key_block_size(key);
    int size = padded_data_length(len, kbs)
    uint8_t *buffer = malloc(size);

    if (!buffer)
	abort();

    memset(buffer, 0, size);
    memcpy(buffer, data, len);

    if ((!len) || (len % kbs)) {
	buffer[len] = 0x80;
        len = size;
        padded = true;
    } 
    if (padded) { 
        // caller provided padded buffer, or cmac() determined buffer needed to be padded
	xor(key->cmac_sk2, buffer + len - kbs, kbs);
    } else {
	xor(key->cmac_sk1, buffer + len - kbs, kbs);
    }

    mifare_cypher_blocks_chained(NULL, key, ivect, buffer, len, MCD_SEND, MCO_ENCYPHER);

    memcpy(cmac, ivect, kbs);

    free(buffer);
}

[smortex]

@dodgambit, thank you for this details report!

I guess the best response to be done is to fix this problem is:

1. Fix the problem into master (pull-requests are welcome 😉)
2. Tag a new major version of libfreefare (i.e. 1.0.0, would help Version not changing #81)
3. Start doing semver (see Provide versioning information nfc-tools.github.io#3)

[darconeous]

Well crap. I implemented all of the relevant test vectors from AN10922, and they all passed. It seems that AN10922 didn't include any test vectors with less than one block. The absence of test vectors for 8 bytes or less of diversification data is going to make it difficult to positively confirm that a new version of the diversification is, in fact, correct.

CMAC, when used with AES-128, is secure with only a single message block. I have no idea why they expand it to two for AN10922. CMAC comes with a robust security proof, AN10922 doesn't, so if anything I'd call what was implemented more secure, not less secure. That being said, I don't see any reason why the AN10922 version would actually be less secure, but not using cryptographic primitives exactly as specified is never a good idea.

In other words, I don't think you should worry that what you are using is less secure. It is as secure as CMAC, which has a security proof. Doing an extra round of crypto on padding doesn't increase the security.

I designed the API so that it would be possible to have different constructors for different diversification algorithms. We could simply have a mifare_key_deriver_new_an10922_real() if we want to maintain backward compatibility... Or we could change the behavior of the existing constructor and simply add a mifare_key_deriver_new_an10922_bogus() to ensure that any deployed apps can continue to derive their keys consistently.

What a shame. This could have all been avoided with better test vectors in AN10922.

[darconeous]

My initial plan is to deprecate mifare_key_deriver_new_an10922() and introduce mifare_key_deriver_new_an10922_real() and mifare_key_deriver_new_cmac(). This will ensure that code that was building against the previous version will fail to compile and force whoever is compiling to investigate why. If they were using less than 8 bytes of diversification data, they could switch to mifare_key_deriver_new_cmac(). Otherwise, they would switch to mifare_key_deriver_new_an10922_real(). Any new code would simply use mifare_key_deriver_new_an10922_real().

But there is a bigger problem, and I've already mentioned it:

The absence of test vectors for 8 bytes or less [in AN10922] of diversification data is going to make it difficult to positively confirm that a new version of the diversification is, in fact, correct.

This seems like a big deal. I could add support for this and just go with the numbers I end up getting and hope I'm correct, but I think @smortex would frown on that.

I suppose I could just try to be super careful. It's a shame that AN10922 didn't include a larger test vector corpus.

@dodgambit, if you don't mind me asking, how did you come to determine this deficiency? Was it a code review, or did you actually encounter a case where the keys didn't match up?

[darconeous]

Actually, thinking more about it, adding some sort of flags parameter to mifare_key_deriver_new_an10922() might be a better option.

[darconeous]

It looks like there might be a suitable "official" test vector for the short case in AN10957, pages 13-14:

• Master Key : 0xf3f9377698707b688eaf84abe39e3791
• UID : 0x04deadbeeffeed
• Diversified Key: 0x0bb408baff98b6ee9f2e1585777f6a51

This gives at least allows us to know if the fix is correct.

[darconeous]

Fixed by #118.