-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
base: dev
Are you sure you want to change the base?
NFC: Add write support for the password-protected MF ultralight tag #3364
Conversation
201f7cc
to
dcbd51b
Compare
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.
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:
- Is target card password protected? Can we successfully auth to target card?
- Does target card has AUTHLIM?
- Is source card password protected?
- 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.
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)); |
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.
It bricks a target card that has different password and AUTHLIM set not 0.
@gornekich Thanks for the review! I totally understand your concern. I think we can separate this PR into two parts:
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. |
Still thinking about writing to password protected cards. I need some time. |
@@ -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)) { |
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 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
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.
@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.
What's new
mf_ultralight_poller_handler_read_tearing_flags
. Support forMfUltralightFeatureSupportSingleCounter
doesn't imply support for reading tearing flags, as seen in NTAG21x series.auth => read **THEN** write
toauth => read **OR** write
. According to NXP specifications, NTAG commands should maintain the tag inACTIVE
orAUTHENTICATED
state. However, some compatible tags deviate from this spec and accept only one read/write command in theAUTHENTICATED
state. This change addresses write command failures because in the previous logic it would issued after the read commands.Verification
Testing was conducted on an NTAG shipped with an air purifier:
Additional testing on devices without password protection needs reviewer's help due to limited device availability.
Checklist (For Reviewer)