security bug in mifare_crypto_postprocess_data()

[dodgambit]

When a secure channel is present, mifare_crypto_postprocess_data() should insist upon checking the CMAC that is supposed to be present, and finding that the CMAC is missing, report failure.

Otherwise, an attacker can substitute their own false 1-byte response 00 when a 9-byte response (CMAC + status byte) from the card was expected. This causes mifare_crypto_postprocess_data() to see the response is just 1 byte long, so it will skip checking the CMAC that was supposed to be in the response. This yields a false successful response code when calling many of the mifare_desfire_xxx functions. For instance, a write or debit operation could be intercepted and prevented by an attacker but reported as successful by mifare_crypto_postprocess_data().

Given that a secure channel has been established and that we are doing plain or CMAC AS_NEW communication writing to a file, the response from the card to mifare_desfire_write_data() should consist of a CMAC + status = 00. mifare_crypto_postprocess_data() will check the validity of the CMAC to ensure that the card really did send status 00, to confirm that the card really said that the write operation succeeded. If the status is 00 but CMAC does not match, mifare_crypto_postprocess_data() will report crypto failure, indicating that the write operation failed.

However, if an attacker intercepts the write APDU and returns just status byte 00, mifare_crypto_postprocess_data() will see that the response consists of just the fake 1-byte status code and it will return success, causing mifare_desfire_write_data() to falsely report that the write succeeded.

This problem of false CMAC success exists in all functions that call mifare_crypto_postprocess_data().

This problem also affects functions that process encrypted results, such as read_data(), which will return "0 bytes read" instead of "error reading" if an attacker substitutes a false 00 response. In this case, it is of course imperative upon the caller to check the result code from mifare_desfire_read_data() anyhow before accessing the data read, but an attacker can make a file appear to be empty without detection.

[smortex]

@dodgambit, thanks for this detailed report!

I understand you are talking about this: https://github.com/nfc-tools/libfreefare/blob/9d88c18833450a91b97fc6ac91c1d983ed3a8681/libfreefare/mifare_desfire_crypto.c#L435-L437

And indeed, this was added in 8975c60de784e93a1cdcfe0f8698a0c6166ca593 with no much details… I hoped that the commit message would have more information about why this was added: the DESFire sometimes does tricky things and this could be a (bad) workaround… If you have the hardware at hand, it's worth trying to reverse this commit and run the test suite with a DESFire card on a supported device for a quick feedback on success or failure.

If everything is fine, a Pull-Request that revert this commit will be perfect. If some test begin to fail, some heavier refactoring may be needed, but the test suite covers a lot of paths, so you can rely on it to ensure you are not adding regressions 😄

The Hacking document should help you to get on track!

Thanks!

[dodgambit]

Without conclusive proof, I believe that this could be fixed by checking if the status byte is non-zero. Below is not a patch but pseudo-code that I'm making up from my head, and doesn't address AS_LEGACY.

postprocess()
{
    MifareDESFireKey key = MIFARE_DESFIRE(tag)->session_key;
   uint8_t s = data[*nbytes - 1];
   // Is there any time when status byte != 0 but response has a MAC/crypt that we need to check?
   if (s != 0) { return res; } 

    switch (communication_settings & MDCM_MASK) {
    case MDCM_PLAIN:
        if (key == NULL) { return res; }    // No authenticated channel, just return result.
        // Pass through to check CMAC.
        // Do all "plain" responses over authenticated channel have a CMAC?
    case MDCM_MACED:
        if (key == NULL) { return NULL; }      // CMAC required but no authenticated channel active.
        *nbytes -= CMAC_SIZE;
        if (*nbytes < 0) { return NULL; }       // CMAC required but missing. expected condition, not abort()!
        ...

      case MDCM_ENCIPHERED:
        if (key == NULL) { return NULL; }      // Crypt required but no authenticated channel active.
        ...
    }
}

This may cause some existing tests to fail if the authenticated channel is invalidated on the card but libfreefare still believes the authenticated channel is open.

To test this would also require writing some tests where an "attacker" submits false 1-byte success responses, in addition to altered CMAC or crypt.

The postprocess function could avoid searching for the CRC32 if the caller passed in the expected response data length. However, I don't know if it is always possible to know ahead of time the expected length of crypted responses.

[dodgambit]

I'm not sure, but your suggestion that I read the hacking document and that I try reverting some checkins seems to imply that perhaps I should take responsibility for fixing this. I don't want to assume, so can you confirm if that is what you mean?

If so, I think it would be interesting to get involved and address a few other things I've seen reported or requested, such as the AN10922 key diversification bug I reported, adding an API to work with the DESFire SAM (all secret key operations in authenticate, change key, preprocess, postprocess, etc would be done by calling out to a SAM instead of by passing MifareDESFireKey objects to functions), and abstracting libfreefare so it does not depend upon libnfc (either by compilation flags or by wrapping the libnfc calls in a generic API that could dispatch to PC/SC or libnfc).

I appreciate all the work you've done on libfreefare, basically revealing how the DESFire card works.

[smortex]

"taking responsibility" sounds like big words: in fact I am not able to do any test at the moment (I am far away from home for some time), and the problems you spot look quite critical, so I would be pleased to see them fixed! libfreefare is released with a Free/Libre license which offers no warranty to the end user, so if your contribution happen to cause trouble you are not "responsible" beyond the fact that you wrote the code that cause trouble, and made some people sad (so, no big drama I guess).

If you can open Pull-Requests, I will be able to review them / comment on them — so that the changes have been peer-reviewed … and I hope that mean even less probability to cause trouble — and then merge them (in spite of having no access to a NFC device ATM).

Contributors are essential to open-source projects, so if you have the capability of improving the current libfreefare code, it will be a pleasure to integrate these changes!

[darconeous]

I might be able to assist.