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

NFC: Add write support for the password-protected MF ultralight tag #3364

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

nekolab
Copy link

@nekolab nekolab commented Jan 14, 2024

What's new

  • Add write capability for password-protected MF Ultralight tags.
  • Fix the check condition in mf_ultralight_poller_handler_read_tearing_flags. Support for MfUltralightFeatureSupportSingleCounter doesn't imply support for reading tearing flags, as seen in NTAG21x series.
  • Revised MF Ultralight poller logic from auth => read **THEN** write to auth => read **OR** write. According to NXP specifications, NTAG commands should maintain the tag in ACTIVE or AUTHENTICATED state. However, some compatible tags deviate from this spec and accept only one read/write command in the AUTHENTICATED state. This change addresses write command failures because in the previous logic it would issued after the read commands.
  • More debug logs to provide users with better NFC status insights without needing to rebuild the firmware.

Verification

Testing was conducted on an NTAG shipped with an air purifier:

  • Read, unlock, and save the tag first. Modified a sector and wrote it back to the NFC tag without issues. The altered sector was correctly read in subsequent actions.
  • Attempted to write with an incorrect password resulted in expected write failure.

Additional testing on devices without password protection needs reviewer's help due to limited device availability.

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

@nekolab nekolab force-pushed the feat/mf-ultralight-write-with-password branch from 201f7cc to dcbd51b Compare January 14, 2024 11:57
@gornekich gornekich self-assigned this Jan 23, 2024
@nekolab nekolab requested a review from gsurkov as a code owner January 23, 2024 19:34
Copy link
Member

@gornekich gornekich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for PR!

While your solution works for the case you mentioned, it could be dangerous in other cases. For instance, if we want to write data on a tag with different password and AUTHLIM set, we might brick card after several AUTH attempts.

Personally I think that writing to NTAG/Ultralight can be done in separate application. In general to write data from one card to another we should think about:

  1. Is target card password protected? Can we successfully auth to target card?
  2. Does target card has AUTHLIM?
  3. Is source card password protected?
  4. Does user want to write full card dump, including configuration paged and password + pack, or just user data?

The consideration of all possible scenarios will result in quite big logic diagram. Also I think during writing logic we should address mentioned issues in UI, which we may not want to implement in NFC application. That's why I think this Ultraight/NTAG writing would be a good separate application.

The bottom line: we can't accept this PR because it may bricks some users cards, and I can't think about how to modify it without writing too much code. Please let me know your thoughts on it and what do you think about separate application.

Comment on lines +28 to +32
uint8_t pwd_page_idx = mf_ultralight_get_pwd_page_num(mfu_ref_data->type);
memcpy(
mfu_event->data->auth_context.password.data,
mfu_ref_data->page[pwd_page_idx].data,
sizeof(MfUltralightAuthPassword));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It bricks a target card that has different password and AUTHLIM set not 0.

@nekolab
Copy link
Author

nekolab commented Jan 28, 2024

@gornekich Thanks for the review! I totally understand your concern. I think we can separate this PR into two parts:

  1. Can we consider merging the bug fix parts first? This should include the flag correction and the read or write fix. It's not related to the password but only the logic and bug fix.
  2. For the write support to password-protected card part. IMO, for the NFC app, the basic scenario would be "I dumped a card and edit the nfc file, then I want to write it back to the original card". So what if we just rename the "write" to "write back to origin", validate the UID before writing, and add a new warning screen display something like "A full card write will be performed, please make sure the password is correct or it may break your card"? The write operation only happens with the full user concentration.

Also, I totally agree if the user wants to write a single sector or tune a feature with interactive UI, it should not be done in the NFC app.

@gornekich
Copy link
Member

Still thinking about writing to password protected cards. I need some time.

@gornekich gornekich marked this pull request as draft February 12, 2024 09:45
@@ -374,8 +374,7 @@ static NfcCommand mf_ultralight_poller_handler_read_tearing_flags(MfUltralightPo
NfcCommand command = NfcCommandContinue;

if(mf_ultralight_support_feature(
instance->feature_set,
MfUltralightFeatureSupportCheckTearingFlag | MfUltralightFeatureSupportSingleCounter)) {
instance->feature_set, MfUltralightFeatureSupportCheckTearingFlag)) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gornekich Sorry for the disturbing but would you mind if I create a new PR that only contains changes to this file? These changes correct the wrong feature detection behavior and avoid performing an unnecessary read before write

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nekolab Sorry for late response. Yes, it would be better to make this fix in separate PR.

Regarding writing to original card, we have this task in backlog and will implement this soon.

@hedger hedger added NFC NFC-related New Feature Contains an IMPLEMENTATION of a new feature labels May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Feature Contains an IMPLEMENTATION of a new feature NFC NFC-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants