Skip to content
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

Rename 'Detect Reader' to 'Extract MF Keys' #3874

Merged
merged 5 commits into from
Sep 8, 2024

Conversation

bettse
Copy link
Contributor

@bettse bettse commented Sep 4, 2024

What's new

  • Rename 'Detect Reader' to 'Extract MF Keys'

The inaccurate name of this has been a major headache for those trying to help people with NFC. It also tempts unfamiliar users to try it, which can in turn get them into trouble.

Verification

  • Launch NFC app, look at menu, see new name

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

@nvx
Copy link
Contributor

nvx commented Sep 4, 2024

Does "Collect MFC Nonces" fit in the UI?

@bettse
Copy link
Contributor Author

bettse commented Sep 4, 2024

Does "Collect MFC Nonces" fit in the UI?

I didn't try, I was more concerned with getting something out ASAP since I'd been so delinquent in doing it before. I felt the new name would certainly fit where the old name did. Always possible someone does a follow up PR to further improve the name (or the code references)

@skotopes
Copy link
Member

skotopes commented Sep 7, 2024

@bettse @noproto @nvx

@zhovner proposed idea that Collect Nonces is a part of Extract reader keys/Sniff reader keys/Sniff MF keys process.
And it make sense to ship MFKey with firmware and integrate into NFC flow so this function will be complete.

We can make it happen in next sprint(starting next week). Do you guys have any thoughts about it?

@noproto
Copy link
Contributor

noproto commented Sep 7, 2024

@bettse @noproto @nvx

@zhovner proposed idea that Collect Nonces is a part of Extract reader keys/Sniff reader keys/Sniff MF keys process. And it make sense to ship MFKey with firmware and integrate into NFC flow so this function will be complete.

We can make it happen in next sprint(starting next week). Do you guys have any thoughts about it?

Integrating MFKey's functionality into the NFC app would be the friendliest process for users (it would be completely opaque: NFC app->Read->Done, NFC app->Collect keys from reader->Done). The biggest obstacle I see to achieving it is the available memory in the NFC app. The NFC app is using almost all of the available memory, even when it is on the main menu. There is 20% of the available RAM remaining - about 30 KB. To integrate it with the NFC app we'll need 60KB at least (6 min avg - slow) up to 110KB (3 min avg - fast). I haven't analyzed the memory usage of the NFC app but I imagine some of the functionality could be exported into FALs and loaded on demand to drop total memory usage. I wouldn't expect the main menu by itself to consume 100 KB of RAM.

For specifically this feature, on the nonce collection side, we can eliminate duplicate nonces from the current implementation of Collect Nonces/Extract reader keys/Sniff reader keys/Sniff MF keys so the user is not prompted to recover keys for nonces which do not result in new keys. MFKey already provides this function: key_already_found_for_nonce_in_dict, so we'd just move it up earlier in the process. The scene where nonces are collected can inform the user how many duplicate nonces were collected ("Duplicate:"), and how many are new ("New:"), so users understand if they are making progress.

Regardless, I'm sure @bettse and @nvx 's concerns are the label itself. There is an NFC/RFID Field Detector application which seems to closer align with the label of "Detect Reader", but without any further protocol analysis or fingerprinting.

@bettse
Copy link
Contributor Author

bettse commented Sep 7, 2024 via email

@skotopes
Copy link
Member

skotopes commented Sep 7, 2024

@noproto we'll stop nfc app before opening mfkey. memory shouldn't be a problem.

@bettse agree, we'll merge this PR first. The only change we propose is to change name from Collect Nonces to something that will be understood by users without help(Extract reader keys/Sniff reader keys/Sniff MF keys process).

@bettse
Copy link
Contributor Author

bettse commented Sep 7, 2024

The only change we propose is to change name from Collect Nonces to something that will be understood by users without help(Extract reader keys/Sniff reader keys/Sniff MF keys process).

Done!

@nvx
Copy link
Contributor

nvx commented Sep 8, 2024

@noproto we'll stop nfc app before opening mfkey. memory shouldn't be a problem.

@bettse agree, we'll merge this PR first. The only change we propose is to change name from Collect Nonces to something that will be understood by users without help(Extract reader keys/Sniff reader keys/Sniff MF keys process).

It's probably worth avoiding the term sniff as that usually means passively sniffing an interaction with a card/reader, while doing the reader part of a mfkey32 attack is an active process. "Extract reader keys" seems workable, although some people seem to think that the media access keys is the key to open a door but not sure what better wording could be on that front that's both self explanatory while avoiding people jumping to incorrect conclusions.

@skotopes
Copy link
Member

skotopes commented Sep 8, 2024

@nvx Extract MF Keys then?

@nvx
Copy link
Contributor

nvx commented Sep 8, 2024

@nvx Extract MF Keys then?

Ooh I like it!

@bettse bettse force-pushed the rename_detect_reader branch from 1b2a622 to aa6fded Compare September 8, 2024 18:12
@bettse
Copy link
Contributor Author

bettse commented Sep 8, 2024

new name pushed

skotopes
skotopes previously approved these changes Sep 8, 2024
@skotopes skotopes changed the title Rename 'Detect Reader' to 'Collect Nonces' Rename 'Detect Reader' to 'Extract MF Keys' Sep 8, 2024
@skotopes skotopes merged commit 75f4782 into flipperdevices:dev Sep 8, 2024
11 checks passed
@bettse bettse deleted the rename_detect_reader branch September 8, 2024 22:46
@mishamyte
Copy link
Contributor

Meaningful rename, but IMO Extract MFC keys is more conceptually right, cause that feature isn't related to other Mifare family chips

@bettse
Copy link
Contributor Author

bettse commented Sep 11, 2024

Unfortunately a bit late to re-litigate. Perhaps open a new PR with that change? I agree it does improve the accuracy of the label.

@mishamyte
Copy link
Contributor

Yeah, unfortunately I completely missed that topic in time.
It could be created separately, but docs will need to be updated one more time

ofabel pushed a commit to ofabel/flipperzero-firmware that referenced this pull request Sep 26, 2024
* Rename 'Detect Reader' to 'Collect Nonces'
* Updated name
* Updated name
* Format Sources

Co-authored-by: Aleksandr Kutuzov <alleteam@gmail.com>
@noproto noproto mentioned this pull request Oct 13, 2024
3 tasks
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.

5 participants