NFC: MF Classic parsers read() fix (dictionary attack skip)

[Leptopt1los]

Issue

All implementations of existing MF Classic parsers that implement the read() function use is_read=true after reading the card. Since it is not checked whether the card was actually successfully read, it is possible that the validator returned a false positive result and the card began to be read by a parser that was not intended for it. in this case, the card will be read by the parser keys, after which the card reading will be completed and the dictionary attack will not be performed

Issue verification: write Washcity_issue.nfc.txt to magic card and try to read it

What's new

The issue described above is fixed

Verification

Same as issue verification. Dict attack must start

Checklist (For Reviewer)

• PR has description of feature/bug or link to Confluence/Jira task
• Description contains actions to verify feature/bugfix
• I've built this code, uploaded it to the device and verified feature/bugfix

[gornekich]

Thanks for PR.

For some supported cards it's OK to read a few sectors to successfully emulate and parse data. I think it should be plugin's responsibility to identify if the card was read enough or not.

Meanwhile, the return code from mf_classic_poller_sync_read() may be misleading. I think it's better to introduce new error like MfClassicErrorPartialRead and return this value if the tag wasn't read fully. And "read" plugin method should process this error.

If you agree, you can make this changes in nfc lib and rework the plugins you added. But please, revert changes in other plugins for now, since your changed might break them

[Leptopt1los]

Thanks for PR.

For some supported cards it's OK to read a few sectors to successfully emulate and parse data. I think it should be plugin's responsibility to identify if the card was read enough or not.

Meanwhile, the return code from mf_classic_poller_sync_read() may be misleading. I think it's better to introduce new error like MfClassicErrorPartialRead and return this value if the tag wasn't read fully. And "read" plugin method should process this error.

If you agree, you can make this changes in nfc lib and rework the plugins you added. But please, revert changes in other plugins for now, since your changed might break them

This is a great idea. I just implemented it, but during the implementation I noticed the following:
All MF Classic parsers use the following code:

error = mf_classic_poller_sync_read(nfc, &keys, data);
if(error != MfClassicErrorNone) {
    FURI_LOG_W(TAG, "Failed to read data");
    break;
}

nfc_device_set_data(device, NfcProtocolMfClassic, data);

With the new changes (MfClassicErrorPartialRead), this will mean that the keys found during the reading process by this parser will not be updated if the reading occurred partially. I'm not sure if this is a nice behavior for the average user. For example, we have a transport card "X", and there is a parser for it. The data important for the parser is stored in sector 8. At some point, new cards "X" begin to be released, in which one key from sector 10 is changed. The card can still be parsed, but now after read() 0 keys will be found, and the user you will have to perform a dictionary attack in order to read this card
Is this behavour acceptable?

And one more question: I'm not sure I understand how changes from the original commit can break any parser? After all, after replacing is_read = true to is_read = mf_classic_is_card_read(), at best, nothing will change, and at worst, after reading by the parser, a dictionary attack will be started to find the missing keys, which does not in any way affect the logic of the parser itself. Moreover, a dictionary attack can be skipped by the user

[gornekich]

I want to take some time to think about it. I agree that after partial read from plugin we should continue read with dictionary attack. I will think about solution and share it here later

[gornekich]

I finally come to a solution. I think that mf_classic_poller_sync_read() function should return 3 type of errors:

1. MfClassicErrorNotPresent - when mfc poller couldn't find any keys
2. MfClassicErrorPartialRead - when mfc poller found at least 1 key
3. MfClassicErrorNone - when mfc poller found all keys and read all sectors

The nfc plugins at the same time should correctly process these errors. We should always update data with nfc_device_set_data() if we received MfClassicErrorPartialRead or MfClassicErrorNone errors. After that plugins should decide which value it returns in read() function:

1. If error is MfClassicErrorNotPresent - the return value is false
2. If error is MfClassicErrorNone - the return value is true
3. If error is MfClassicErrorPartialRead it's up to plugin to decide weather read data is enough to be parsed and successfully emulated, or further dict attack should be run. In first case return code is true, otherwise - false.

Please have a look at my implementation in the patch attached. If it works good for you - you can commit it in this PR and we will merge it.
mfc_poller_sync_read_patch.txt