DarkFlippers / unleashed-firmware

Flipper Zero Unleashed Firmware

https://flipperunleashed.com

GNU General Public License v3.0

17.66k stars 1.46k forks source link

NFC: Fix Mifare DESFire reading #757

Closed Willy-JL closed 1 month ago

Willy-JL commented 6 months ago

What's new

Previous fix for EMV read crash in 75ece9b697d1e3b66a75f2d423e51f369bc387f1 caused DESFire to never read
Was comparing sizes of same buffer, and also logic should have compared size and capacity, destination buffer is always empty in beginning so size is 0
Changed check to use same parameters as the furi_check()s that cause the crash in this original bugfix from 75ece9b697d1e3b66a75f2d423e51f369bc387f1
Fixes #756

Verification

Read any Mifare DESFire card
Read EMV cards that used to cause crashes before 75ece9b697d1e3b66a75f2d423e51f369bc387f1

Checklist (For Reviewer)

[x] PR has description of feature/bug
[x] Description contains actions to verify feature/bugfix
[ ] I've built this code, uploaded it to the device and verified feature/bugfix

Willy-JL commented 6 months ago

cc @Leptopt1los for

Read EMV cards that used to cause crashes before 75ece9b697d1e3b66a75f2d423e51f369bc387f1

Leptopt1los commented 5 months ago

hi @Willy-JL! sorry for delay. i tested this PR and unfortunately it beaks emv read. i tried to come to a different solution again, but today my buggy emv card died. i will order new one. will ping you here when i get it, ok?

Willy-JL commented 5 months ago

Sounds good to me! I think it's worth noting that some emv card do still read with this PR, it seems to be an exception rather than the rule with the one you had. It might be worth considering merging as a temporary fix since the trade-off is no desfire at all, or only some emv cards break, but that decision is for your team to make. I'll gladly try to find the root cause when you receive the new card :D

xMasterX commented 5 months ago

Sounds good to me! I think it's worth noting that some emv card do still read with this PR, it seems to be an exception rather than the rule with the one you had. It might be worth considering merging as a temporary fix since the trade-off is no desfire at all, or only some emv cards break, but that decision is for your team to make. I'll gladly try to find the root cause when you receive the new card :D

Since we are currently in situation when we have options

Break desfire
Break emv (when this PR is merged) (@Leptopt1los haven't explained that this fix actually breaks reading all of his cards)
and return all to previous state where we had paritally broken emv reader (some cards only) and have desfire working at same time

I chosen 3rd option and did this https://github.com/DarkFlippers/unleashed-firmware/commit/1db05ed2c6a8393bef9df1a4b59f3635b76d8d6d

Ill keep PR open until we found solution to fully fix that, which is impossible without test card at the moment sadly

Willy-JL commented 1 month ago

no longer required, fixed by 180d1f04711dee208fa18160ab9ab69d277dfd35