Skip to content

Conversation

@GMMan
Copy link
Contributor

@GMMan GMMan commented Dec 30, 2024

What's new

  • Fixes sector overrun that occurs when running nested dictionary attack on MIFARE Plus X 2k variants in SL1.

Verification

  1. Provision a MIFARE Plus X 2k variants in SL1
  2. Write a Vingcard credential to the card
  3. Read the card as MIFARE Classic

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

I'm not sure what the actual intended flow is supposed to be when the dictionary attack runs off the end of the card. I assume it would loop back and continue to check the first part of the dictionary, but it could also just move on to nonce collection, although that would involve some refactoring to free up the loaded dictionaries. Probably something for @noproto to clarify.

Edit: I've discovered that the test card in question is actually a MIFARE Plus EV1. Flipper's detection of this is not quite working, and because the card gave the same historical bytes as MIFARE Plus X, it was incorrectly assumed to be that chip.

@GMMan GMMan force-pushed the nfc-fix-mfc-mfplus-crash branch from 892a19b to bb27473 Compare December 30, 2024 02:02
@noproto
Copy link
Contributor

noproto commented Dec 30, 2024

Possible regression with this PR or 3822 for Hardnested. Waiting on a card I ordered 2 weeks ago to isolate a crash.

edit: 2K cards aren't recognized, which seems like the root cause for any sector overrun?

@GMMan
Copy link
Contributor Author

GMMan commented Dec 31, 2024

edit: 2K cards aren't recognized, which seems like the root cause for any sector overrun?

Potentially addressed in #4053

@noproto
Copy link
Contributor

noproto commented Jan 4, 2025

This should be the correct fix for this issue (rather than fixing a byproduct of the crash): noproto/xero-firmware@56fe7b0

Tested and would welcome feedback.

@RebornedBrain
Copy link
Contributor

Hi @GMMan , thanks for your contribution, and sorry for such a long waiting. Could you please take a look at @noproto proposal about this fix and test, whether it works in your case?

@GMMan
Copy link
Contributor Author

GMMan commented Feb 13, 2025

Hi @GMMan , thanks for your contribution, and sorry for such a long waiting. Could you please take a look at @noproto proposal about this fix and test, whether it works in your case?

That code does not crash, but it also causes some keys to not be detected. I've previously mentioned to @noproto that he can open another PR with the new code and I can close this one whenever that PR is made.

@RebornedBrain
Copy link
Contributor

@GMMan nice, thank you! @noproto, could you please open a pr with your fix?

@hedger hedger self-requested a review September 25, 2025 19:23
@noproto
Copy link
Contributor

noproto commented Sep 25, 2025

@hedger this is the fix, the PR is out of date: #4048 (comment)

Edit: Opened #4288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants