[BUG] MF_CLASSIC_DETECTION (mfkey32) keys wrong

[XazkerBoy]

Environment

Item	Your information
Harware	RevE Rebooted
Firmware	latest (untagged-e909ae723c300d0cc07e) (Iceman: 18007ef)
GUI	latest rebootedGUI (Rebooted Gray)
Slot number	All
Slot configuration	MF_CLASSIC_DETECTION
Dump source	N/A
Reader	PN532 (Arduino)
Flashing environment	Both latest release and self-compiled from latest commit
Flashing method	flash.bat
Flash memory space	4Mbit (512 kB)
Makefile configuration	Default / removed Ultralight and NTAGs on self-compiled

Bug description

Dear chameleon revE rebooted team!
After flashing the latest firmware to the device, I noticed wrong keys being calculated using mfkey32 (reader attack). Tests were done in a controlled environment with a known key by repeatedly authenticating to a single sector. This is the Arduino code:

#include <SoftwareSerial.h>
#include <PN532_SWHSU.h>
#include <PN532.h>

SoftwareSerial SWSerial( 3, 2 ); // RX, TX
PN532_SWHSU pn532hsu(SWSerial);
PN532 nfc(pn532hsu);

void setup(void) {
  Serial.begin(9600);
  nfc.begin();

  uint32_t versiondata = nfc.getFirmwareVersion();
  if (!versiondata) {
    Serial.print("Didn't find PN53x board");
    while (1);
  }

  nfc.SAMConfig();
  Serial.println("Waiting for an ISO14443A Card ...");
}


void loop(void) {
  uint8_t uid[7];
  uint8_t uidLength;
  uint8_t keya[6] = { 0x01, 0x02, 0x03, 0x30, 0x20, 0x10 }; // 0x010203302010

  if (nfc.readPassiveTargetID(PN532_MIFARE_ISO14443A, uid, &uidLength)) { // Select
    nfc.mifareclassic_AuthenticateBlock(uid, uidLength, 240, 0, keya);  // Authenticate to block 240 (sector 39)
    delay(500);
  }
}

Chameleon was fully erased with a CLEARALL command before tests, slot 1 was configured as MF_CLASSIC_DETECTION and DETECTION? command returned all FFs and a checksum on the first run. After collecting nonces, the output changed to following:

52 45 56 45 56 31 2E 34 60 F0 7A 8C 01 20 01 45 4B 46 59 A7 95 7A DF 90 60 F0 7A 8C 01 20 01 45 27 1E 5C 12 B0 21 0F 92 60 F0 7A 8C 01 20 01 45 E9 04 07 25 32 FD 06 2A 60 F0 7A 8C 01 20 01 45 6A 81 02 85 64 2E 92 8A 60 F0 7A 8C 01 20 01 45 59 E1 48 B4 F0 9B 3B 2E 60 F0 7A 8C 01 20 01 45 FE C8 96 CC 10 75 96 90 FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF F0 7C 31 30 30 3A 4F 4B 0D 0A 00 00 00 00 00 00 00 00

After running mfkey32 I got the following result:

[Tag slot 1]
[S39 / B240] KeyA [e5b070c82010]

The expected key was 010203302010, as in the sketch above. I know that 4K configuration isn't available for this mode, I was just experimenting, this is why you see block 240. The following behavior applies to all slots, all keys and all sectors. Only 2 last bytes always don't differ between real and expected outputs. The same function works with the stock firmware and the same newest rebootedGUI (backwards compatibility?). Can you help me to identify the problem? Thanks in advance!

Expected function and references

Working mfkey32 (reader) attack on newest firmware

Bug

Steps to Reproduce

1. Flash newest firmware
2. Set any slot configuration to MIFARE_CLASSIC_DETECTION
3. Obtain enough nonces with a known key
4. Compare mfkey32 execution result with expected output

Resolution paths

Ideas

Please note, that the last 2 bytes are always correct! Given that, I can assume that there is some nonce saving issue, which corrupts first nonces and thus the first part of the key

Possible Implementation

[XazkerBoy]

So, the issue first appeared in the commit 6ec88b4, which was the last one to change MF_DETECTION implementation. Looks like moving it to working memory broke something and it didn't work properly any more. I temporarily fixed it by changing

[CONFIG_MF_CLASSIC_DETECTION]
...
    .CardMemorySize = MFCLASSIC_1K_MEM_SIZE,
    .WorkingMemorySize = DETECTION_MEM_APP_SIZE,
    .ReadOnly = false

to

[CONFIG_MF_CLASSIC_DETECTION]
...
    .CardMemorySize = DETECTION_MEM_APP_SIZE,
    .WorkingMemorySize = MEMORY_NO_MEMORY,
    .ReadOnly = true

in Configuration.c and renaming all AppWorkingMemory commands related to this mode back to AppCardMemory. Just look at the diff of the commit above for better understanding. This is a temporary fix, the real problem is somewhere in working memory

[XazkerBoy]

Okay, found the real problem. In MifareClassic.c you save DetectionCanary into working memory after UID and then read the whole block 0 from the same memory in CommandGetDetection function of Commands.c. The problem is that the UID is stored in card memory and you try to read it from working memory, where it never existed. Thus, on clear run the GUI becomes 8 bytes of FFs instead of the real UID (and SAK, ATQA, CRC). There are 2 solutions:

1. Move canary into card memory. Then you can read block 0 with a single AppCardMemoryRead function and get both UID and canary
2. Leave locations as they are and replace single AppWorkingMemoryRead function in CommandGetDetection (Commands.c) by AppCardMemoryRead to read first 8 bytes (UID, SAK, ATQA) from card memory and then AppWorkingMemoryRead to read the rest of block 0

[herrwiesel]

Can somebody create a pull request with that change, please?