-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 Classic Key Recovery Improvements #3822
base: dev
Are you sure you want to change the base?
Conversation
This PR also resolves |
The accelerated dictionary attack is mostly working. I'm tracking down a minor bug in it and making sure the UI reflects the state of the attack. The fork takes an average of 10 seconds to run dictionary attacks in my tests (1 second of the average is backdoor detection - separate from the dictionary attack). OFW 0.105.0 takes an average of 3 minutes 10 seconds. On real tags the number of unknown keys and the offset in the dictionary are all different, here are five benchmarks on random MIFARE Classic tags:
|
@@ -29,7 +34,9 @@ NfcCommand nfc_dict_attack_worker_callback(NfcGenericEvent event, void* context) | |||
} else if(mfc_event->type == MfClassicPollerEventTypeRequestMode) { | |||
const MfClassicData* mfc_data = | |||
nfc_device_get_data(instance->nfc_device, NfcProtocolMfClassic); | |||
mfc_event->data->poller_mode.mode = MfClassicPollerModeDictAttack; | |||
mfc_event->data->poller_mode.mode = (instance->nfc_dict_context.enhanced_dict) ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see one place where this enhanced_dict
is set to true
on line 175, which means that we will start from MfClassicPollerModeDictAttackEnhanced
. And how MfClassicPollerModeDictAttackStandard
will be performed now? I'm asking because the only place where enhanced_dict
is set to false
is on line 385, in _on_exit
callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flag is to indicate whether nested attacks should be attempted. Nested attacks are disabled during the CUID dictionary attack to force the default behavior (MfClassicPollerModeDictAttackStandard
), as all of the keys in the CUID dictionary match the properties that the enhanced attack searches for. After enhanced_dict
is set to true
, it should no longer be set to false
except upon exit as the CUID dictionary is tested first - the only time it is needed. The variable is necessary to inform the poller how it should behave during initial phase of the dictionary attack. Standard attacks are still attempted in MfClassicPollerModeDictAttackEnhanced
, up until the point the first key is found.
@@ -222,7 +304,27 @@ bool nfc_scene_mf_classic_dict_attack_on_event(void* context, SceneManagerEvent | |||
} else if(event.event == NfcCustomEventDictAttackSkip) { | |||
const MfClassicData* mfc_data = nfc_poller_get_data(instance->poller); | |||
nfc_device_set_data(instance->nfc_device, NfcProtocolMfClassic, mfc_data); | |||
if(state == DictAttackStateUserDictInProgress) { | |||
bool ran_nested_dict = instance->nfc_dict_context.nested_phase != |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, this one is not needed here because we have the same on line 264 which we can move out from if to line 262.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And by the way, maybe it would be better to change type of nested_phase to appropriate enum instead of uint8_t
? I mean this one https://github.com/flipperdevices/flipperzero-firmware/pull/3822/files#r1795343085
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, this one is not needed here because we have the same on line 264 which we can move out from if to line 262.
This exists to reduce computation so we don't do the comparison on every custom event.
And by the way, maybe it would be better to change type of nested_phase to appropriate enum instead of
uint8_t
? I mean this one https://github.com/flipperdevices/flipperzero-firmware/pull/3822/files#r1795343085
Will follow up on your other comment: #3822 (comment)
bool is_nested) { | ||
bool is_nested, | ||
bool backdoor_auth, | ||
bool early_ret) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my point of view it's not very good to put bool vars into function params. This means that this function doesn't respond to single responsibility principle. Also reading and understanding of such functions requires more effort, in my opinion.
Despite the diff, which says that this problem was here even before (is_nested
), we must keep our api as much clean as possible without such things, that's why I spent some time and tried to remove those booleans, it required some efforts, but looks like that it works fine.
Here is the patch with things which I've done: nestednonces_api_patch.patch
Could you please apply it and check whether everything still works as expected?
This is the most important blocker for merging now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gornekich it would be great if you will take a look, after PR will be updated and approve such changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skimming the patch, dict_attack.h duplicates the include on line 6 and backdoor detection uses FURI_LOG_E
(indicating an error) instead of the currently defined log levels. My understanding is that API version 85 in api_symbols.csv is a development artifact. I'm closely reviewing how it handles nested authentication, then I'll test it on each type of card.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in the patch to nfc_scene_mf_classic_dict_attack.c increase total computation performed. #3822 (comment)
mf_classic_poller_send_frame_callback in nfc_test.c needs to have the call updated to match the new prototype of mf_classic_poller_auth
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RebornedBrain As mentioned in my earlier comment, I was closely reviewing how the changes in the patch affected nested authentication. The changes affect a subtle but critical part of the code.
The device must perform almost exactly the same operations in order for the calibration code to have the same runtime as the nested nonce collection. This is a very sensitive operation: the LFSR on the tag (assuming its not static or static encrypted) shifts every ~9.4395 µs. The function call overhead of calling two functions versus one (mf_classic_poller_auth_nested
+mf_classic_poller_get_nt_and_update_auth_context
versus just mf_classic_poller_get_nt_and_update_auth_context
) introduces a timing discrepancy that cannot be detected by the device. During nonce collection, you'll collect nonces but they'll either be offset by an unknown amount (I am not able to get any clocksource on the device to calculate the difference with sufficient precision) or unsolvable.
To verify this issue exists, I used a non-static weak card which I was able to successfully solve with the original PR. I flashed the changes in the patch, it collected multiple invalid nonce pairs during collection (as far as I could tell this affected a majority of collected nonce pairs). Only by flashing back the original changes am I able to find the key immediately.
Further, because the nonce is not located in the correct position, I noted only one third of the possible nonce pairs were collected across the retry attempts (resulting in longer collection times and fewer logged pairs).
I do not believe we should merge this patch if it will disrupt collection for one category of cards: cards with regular weak PRNGs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skimming the patch, dict_attack.h duplicates the include on line 6 and backdoor detection uses
FURI_LOG_E
(indicating an error) instead of the currently defined log levels. My understanding is that API version 85 in api_symbols.csv is a development artifact. I'm closely reviewing how it handles nested authentication, then I'll test it on each type of card.
Oh, sorry for that, I did patch before pulling your latest changes, that's why there were duplications, and Log level I increased for tests only, and I agree that it shouldn't be changed
I will investigate your feedback on patch more precisely today.
@@ -505,6 +557,128 @@ NfcCommand mf_classic_poller_handler_request_read_sector_blocks(MfClassicPoller* | |||
return command; | |||
} | |||
|
|||
NfcCommand mf_classic_poller_handler_analyze_backdoor(MfClassicPoller* instance) { | |||
NfcCommand command = NfcCommandReset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this logic can be simplified and moved to mf_classic api or even to a separate module, smth like mf_classic_backdoor.c
In that module all logic of testing backdoor can be done, and as a public function there can be something like: mf_classic_test_backdoor(MfClassicBackdoor backdoor_type);
or even better MfClassicBackdoor backdoor = mf_classic_find_backdoor();
which will return us value from enum and do all the magic under the hood
This can be placed there: https://github.com/flipperdevices/flipperzero-firmware/pull/3822/files#r1797349885
#define MF_CLASSIC_MAX_BUFF_SIZE (64) | ||
|
||
// Ordered by frequency, labeled chronologically | ||
const MfClassicBackdoorKeyPair mf_classic_backdoor_keys[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be moved to a separate module with backdoor logic
My understanding of where we're at:
|
@noproto sorry for delay, @RebornedBrain and QA being testing this PR against our card library. They found bunch of cards that cannot be read with this PR. They'll bring details later. |
That is unfortunate. I tested it on 110 MIFARE Classic cards here. Very interested to find out the issue so I can remedy it. |
@RebornedBrain Could I have an update on this PR? Which cards no longer read specifically? For example - if it is MIFARE Plus, is it SL1 or SL2? I do not possess a MIFARE Plus card in my collection (all of my cards scan), but I can find members of the community who have certain cards for further testing and to assist us with locating any issue. Please let me know if there's anything I can do. I've paused development while I wait on your feedback (both on the cards which no longer read and on this thread: #3822 (review) ). |
@noproto hold on, I'll bring report and test cards here in next couple hours |
So, @RebornedBrain @doomwastaken did some tests. Here is what they found: Test 1: no plugins, stuck at particular keyTests were performed without plugins (remove folder Plugins at "SD Card/apps_data/nfc") Test 2: MFC 4k, crashThis MFC 4k (not magic) card where first sector was protected with key A "DEADBEAFFFFF" Test 3: MFC 1k, can't find keyThis MFC 1k (magic) card where first sector was also protected with key A "DEADBEAFFFFF". Test 4: Test with pluginsFlipper reads this card totally when there is no plugins (it passes nested attack and etc). Test 5: On this card Flipper stuck On this card Flipper stuck @noproto let us know if you need any additional help/info/etc... |
Awesome, on it and I'll follow up shortly with fixes and feedback. Thank you for testing the PR! |
All issues resolved that I could reproduce, please re-test this PR @skotopes @RebornedBrain @doomwastaken:
Steps taken
ConclusionTwo issues: faulty state machine logic, target sector on 4K cards. Fixed in 4be9e79
Steps taken
ConclusionPossible cache issue? Verify no keys are cached for this tag.
Steps taken
ConclusionInconsistent assignment of known key and known key type/sector led to repeated failed authentication attempts. Keys were provided by specific NFC plugins instead of the dictionary attack. Fixed in 897817a and db26c85
Steps taken
ConclusionLikely related to the other (now fixed) issues. Please re-test. |
Since #3961 (comment) will delay 1.1, is it still possible to squeeze this PR in too if the issues identified in QA are resolved by #3822 (comment) ? These changes would allow me to share an early, significantly easier process with the users as well as limit the scope of each PR versus rolling up more changes into this single PR making it unwieldy to do QA. Additionally, it would be useful to collect feedback from users so we can identify any rare or uncommon issues (thanks to the exceptional QA already done, every issue that was reported to me - even prior to the review - has been resolved). RE: how users could use the nonces collected by this PR, PR 243 to good-faps would make a complete process available: flipperdevices/flipperzero-good-faps#243 Wanted to bring this up for your consideration. I understand if it's not possible. For context, this is what is required for reading cards today. It becomes a two step process with this PR (3822) plus PR 243 to good-faps. |
What's new
Verification
This PR currently contains the minimum necessary code to achieve the intended functionality. It will be superseded with performance improvements.
Checklist (For Reviewer)