From dcbb9e679eea45e0743fde32ef601aa683fe73b1 Mon Sep 17 00:00:00 2001 From: Mykhailo Shevchuk Date: Mon, 10 Aug 2026 23:50:24 +0300 Subject: [PATCH] NFC, LF RFID, iButton: write the new file before deleting the one it replaces (#1067) * NFC, LF RFID, iButton: write the new file before deleting the one it replaces All three apps unlinked the loaded file up front and only then wrote the replacement, so a save that failed afterwards -- full SD, card pulled, a path the filesystem rejected -- left the user with neither copy. For NFC the delete also took the shadow file with it. This is what turned #1063 from a bad results screen into destroyed data. Write first, then drop the replaced file, and only when it really is a different file. The comparison uses storage_common_equivalent_path() because /ext is case-insensitive: a Card -> card rename lands on the same file, and treating it as a rename would delete the fresh write. A .shd path is normalised to its .nfc before the comparison for the same reason (Update from Initial Card leaves one in file_path). After a successful save any shadow beside the new file is dropped, so a stale one cannot go on overriding it -- nfc_load_file() prefers a shadow whenever it exists. A failed save now restores file_path and file_name. They were rewritten on the way to failing, which was harmless while the old file had already been deleted but now leaves the app pointing at a file that was never created while the original is still on disk -- Delete would then report success having removed nothing. lfrfid_scene_save_data.c had the same delete-first pattern with no rename involved at all, so the unlink only widened the window; dropped. Co-Authored-By: Claude Opus 5 (1M context) * upd changelog Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- CHANGELOG.md | 3 +- .../ibutton/scenes/ibutton_scene_save_name.c | 34 ++++++++++++--- applications/main/lfrfid/lfrfid.c | 9 +++- applications/main/lfrfid/lfrfid_i.h | 2 + .../lfrfid/scenes/lfrfid_scene_save_data.c | 7 ++-- .../lfrfid/scenes/lfrfid_scene_save_name.c | 33 +++++++++++++-- .../protocol_support/nfc_protocol_support.c | 41 +++++++++++++++++-- applications/main/nfc/nfc_app.c | 39 +++++++++++++----- applications/main/nfc/nfc_app_i.h | 2 + 9 files changed, 140 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0f6a9846..a2bd788a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,9 @@ - Current API: 88.2 * NFC: **Fix reading an EMV card with malformed TLV lengths crashing or overflowing buffers** - the poller trusted the card's own length bytes: some tags aborted the firmware through `furi_check`, others were copied straight into fixed-size fields (AID, application name/label, cardholder name, track 1/2), and the PAN/track-2 loops ran past their arrays. Lengths are now bounded against the destination, the PDOL is capped at what the poller can transmit, and rejected tags are logged (by @Endika | PR #1048) * NFC: **Fix a crafted EMV `.nfc` file corrupting the heap on load** - `emv_load()` used the file's own lengths as write sizes: three unbounded `strcpy`s into the cardholder name, application name and label, and `PAN length`/`AID length` read as 32-bit then written into 10- and 16-byte fields; an oversized `PAN length` also walked the card-number render loops past the array. Saving a card no longer writes a garbage `PIN try counter` either (by @mishamyte | PR #1056 | Fixes #1055) -* NFC: **Fix "Unlock with Dictionary" destroying a saved MIFARE Classic dump** - pressing Skip while no card was on the reader adopted the dictionary poller's still-empty data as the loaded card, so the results screen offered to save a blank dump over the file. Saving under a different name did not help either, because renaming on save deletes the previously loaded file first. The MIFARE Plus dictionary attack could lose data the same way and now merges its result instead of replacing (by @mishamyte | Fixes #1063) +* NFC: **Fix "Unlock with Dictionary" destroying a saved MIFARE Classic dump** - pressing Skip while no card was on the reader adopted the dictionary poller's still-empty data as the loaded card, so the results screen offered to save a blank dump over the file. Saving under a different name did not help either, because renaming on save deleted the previously loaded file first (fixed separately below). The MIFARE Plus dictionary attack could lose data the same way and now merges its result instead of replacing (by @mishamyte | Fixes #1063) * NFC: **Fix "Update from Initial Card" dropping sectors from a MIFARE Classic dump** - the refresh replaced the dump with whatever that pass re-read instead of merging into it, so a sector that failed to authenticate this time lost both its key and its blocks. Recoverable through "Restore to Original State", but silent (by @mishamyte | Fixes #1064) +* NFC, LF RFID, iButton: **Renaming a saved file no longer deletes it before the replacement is written** - all three apps unlinked the loaded file first, so a save that then failed (full SD, card pulled) left the user with neither copy. The new file is written first and the old one dropped only once it is safely on disk, a stale NFC shadow file can no longer override a fresh save, and a failed save no longer leaves the app pointing at a file that was never created. Note this protects renames; re-saving under the same name still writes over the only copy (by @mishamyte | Fixes #1065) * Apps: Build tag (**9aug2026**) - **Check out more Apps updates and fixes by following** [this link](https://github.com/xMasterX/all-the-plugins/commits/dev) ## Other changes * OFW PR 4361: fix HID limits to support international keyboards and add JP keyboard layout (by @d3npa) diff --git a/applications/main/ibutton/scenes/ibutton_scene_save_name.c b/applications/main/ibutton/scenes/ibutton_scene_save_name.c index cf621d62a..d9e4a8099 100644 --- a/applications/main/ibutton/scenes/ibutton_scene_save_name.c +++ b/applications/main/ibutton/scenes/ibutton_scene_save_name.c @@ -5,6 +5,8 @@ #include +#define TAG "IButtonSaveName" + static void ibutton_scene_save_name_text_input_callback(void* context) { iButton* ibutton = context; view_dispatcher_send_custom_event(ibutton->view_dispatcher, iButtonCustomEventTextEditResult); @@ -46,11 +48,11 @@ bool ibutton_scene_save_name_on_event(void* context, SceneManagerEvent event) { if(event.type == SceneManagerEventTypeCustom) { consumed = true; if(event.event == iButtonCustomEventTextEditResult) { - if(!is_new_file) { - Storage* storage = furi_record_open(RECORD_STORAGE); - storage_simply_remove(storage, furi_string_get_cstr(ibutton->file_path)); - furi_record_close(RECORD_STORAGE); - } + // Write the replacement first: deleting up front left the user with nothing whenever + // the save then failed. Drop the old file only once the new one is on disk, and only + // when it really is a different file -- compare the way the filesystem does, since + // /ext is case-insensitive and a Key -> key rename writes into the same file. + FuriString* replaced_path = furi_string_alloc_set(ibutton->file_path); furi_string_printf( ibutton->file_path, @@ -59,7 +61,27 @@ bool ibutton_scene_save_name_on_event(void* context, SceneManagerEvent event) { ibutton->key_name, IBUTTON_APP_FILENAME_EXTENSION); - if(ibutton_save_key(ibutton)) { + const bool saved = ibutton_save_key(ibutton); + if(saved) { + if(!is_new_file) { + Storage* storage = furi_record_open(RECORD_STORAGE); + if(!storage_common_equivalent_path( + storage, + furi_string_get_cstr(replaced_path), + furi_string_get_cstr(ibutton->file_path)) && + !storage_simply_remove(storage, furi_string_get_cstr(replaced_path))) { + FURI_LOG_E(TAG, "Failed to remove replaced key file"); + } + furi_record_close(RECORD_STORAGE); + } + } else { + // The path was rewritten on the way to failing; put it back, or the app points at + // a file that was never written while the original is still on disk. + furi_string_set(ibutton->file_path, replaced_path); + } + furi_string_free(replaced_path); + + if(saved) { scene_manager_next_scene(ibutton->scene_manager, iButtonSceneSaveSuccess); if(scene_manager_has_previous_scene( diff --git a/applications/main/lfrfid/lfrfid.c b/applications/main/lfrfid/lfrfid.c index b51a0da26..4518a5f68 100644 --- a/applications/main/lfrfid/lfrfid.c +++ b/applications/main/lfrfid/lfrfid.c @@ -294,10 +294,17 @@ bool lfrfid_load_raw_key_from_file_select(LfRfid* app) { return result; } +bool lfrfid_delete_key_file(LfRfid* app, const FuriString* path) { + furi_assert(app); + furi_assert(path); + + return storage_simply_remove(app->storage, furi_string_get_cstr(path)); +} + bool lfrfid_delete_key(LfRfid* app) { furi_assert(app); - return storage_simply_remove(app->storage, furi_string_get_cstr(app->file_path)); + return lfrfid_delete_key_file(app, app->file_path); } bool lfrfid_load_key_data(LfRfid* app, FuriString* path, bool show_dialog) { diff --git a/applications/main/lfrfid/lfrfid_i.h b/applications/main/lfrfid/lfrfid_i.h index e479aa601..458ff46ec 100644 --- a/applications/main/lfrfid/lfrfid_i.h +++ b/applications/main/lfrfid/lfrfid_i.h @@ -140,6 +140,8 @@ bool lfrfid_load_raw_key_from_file_select(LfRfid* app); bool lfrfid_delete_key(LfRfid* app); +bool lfrfid_delete_key_file(LfRfid* app, const FuriString* path); + bool lfrfid_load_key_data(LfRfid* app, FuriString* path, bool show_dialog); bool lfrfid_save_key_data(LfRfid* app, FuriString* path); diff --git a/applications/main/lfrfid/scenes/lfrfid_scene_save_data.c b/applications/main/lfrfid/scenes/lfrfid_scene_save_data.c index 20c8a10df..7e7a519fe 100644 --- a/applications/main/lfrfid/scenes/lfrfid_scene_save_data.c +++ b/applications/main/lfrfid/scenes/lfrfid_scene_save_data.c @@ -35,10 +35,9 @@ bool lfrfid_scene_save_data_on_event(void* context, SceneManagerEvent event) { if(scene_manager_has_previous_scene(scene_manager, LfRfidSceneSaveType)) { scene_manager_next_scene(scene_manager, LfRfidSceneSaveName); } else { - if(!furi_string_empty(app->file_name)) { - lfrfid_delete_key(app); - } - + // No delete first: the name never changes here, so lfrfid_save_key() rewrites the + // very file this used to unlink, and unlinking it only widened the window in which + // a failure left the user with nothing. if(lfrfid_save_key(app)) { scene_manager_next_scene(scene_manager, LfRfidSceneSaveSuccess); } else { diff --git a/applications/main/lfrfid/scenes/lfrfid_scene_save_name.c b/applications/main/lfrfid/scenes/lfrfid_scene_save_name.c index b8747a9c1..f0530033c 100644 --- a/applications/main/lfrfid/scenes/lfrfid_scene_save_name.c +++ b/applications/main/lfrfid/scenes/lfrfid_scene_save_name.c @@ -2,6 +2,8 @@ #include #include +#define TAG "LfRfidSaveName" + void lfrfid_scene_save_name_on_enter(void* context) { LfRfid* app = context; TextInput* text_input = app->text_input; @@ -51,13 +53,36 @@ bool lfrfid_scene_save_name_on_event(void* context, SceneManagerEvent event) { if(event.type == SceneManagerEventTypeCustom) { if(event.event == LfRfidEventNext) { consumed = true; - if(!furi_string_empty(app->file_name)) { - lfrfid_delete_key(app); - } + // Write the replacement first: deleting up front left the user with nothing whenever + // the save then failed. Drop the old file only once the new one is on disk, and only + // when it really is a different file -- compare the way the filesystem does, since + // /ext is case-insensitive and a Card -> card rename writes into the same file. + FuriString* replaced_path = furi_string_alloc_set(app->file_path); + FuriString* replaced_name = furi_string_alloc_set(app->file_name); + const bool had_file = !furi_string_empty(app->file_name); furi_string_set(app->file_name, app->text_store); - if(lfrfid_save_key(app)) { + const bool saved = lfrfid_save_key(app); + if(saved) { + if(had_file && !storage_common_equivalent_path( + app->storage, + furi_string_get_cstr(replaced_path), + furi_string_get_cstr(app->file_path))) { + if(!lfrfid_delete_key_file(app, replaced_path)) { + FURI_LOG_E(TAG, "Failed to remove replaced key file"); + } + } + } else { + // lfrfid_save_key() rewrote both on its way to failing; put them back, or the app + // points at a file that was never written while the original is still on disk. + furi_string_set(app->file_path, replaced_path); + furi_string_set(app->file_name, replaced_name); + } + furi_string_free(replaced_name); + furi_string_free(replaced_path); + + if(saved) { scene_manager_next_scene(scene_manager, LfRfidSceneSaveSuccess); if(scene_manager_has_previous_scene(scene_manager, LfRfidSceneSavedKeyMenu)) { // Nothing, do not count editing as saving diff --git a/applications/main/nfc/helpers/protocol_support/nfc_protocol_support.c b/applications/main/nfc/helpers/protocol_support/nfc_protocol_support.c index 0e826fbf4..a9931c34f 100644 --- a/applications/main/nfc/helpers/protocol_support/nfc_protocol_support.c +++ b/applications/main/nfc/helpers/protocol_support/nfc_protocol_support.c @@ -718,12 +718,47 @@ static bool if(event.type == SceneManagerEventTypeCustom) { if(event.event == NfcCustomEventTextInputDone) { - if(!furi_string_empty(instance->file_name)) { - nfc_delete(instance); + // Write the replacement before dropping what it replaces: deleting first cost the user + // both copies whenever the save then failed. Normalise a .shd path (Update from Initial + // Card leaves one here) so the rename check below cannot read it as a different file + // and delete what was just written. + FuriString* replaced_path = furi_string_alloc_set(instance->file_path); + FuriString* replaced_name = furi_string_alloc_set(instance->file_name); + if(furi_string_end_with_str(replaced_path, NFC_APP_SHADOW_EXTENSION)) { + furi_string_replace_at( + replaced_path, furi_string_size(replaced_path) - 4, 4, NFC_APP_EXTENSION); } + const bool had_file = !furi_string_empty(instance->file_name); + furi_string_set(instance->file_name, instance->text_store); - if(nfc_save(instance)) { + const bool saved = nfc_save(instance); + if(saved) { + // Any shadow beside the new file is stale now, and nfc_load_file() prefers a + // shadow over the card, so leaving one would silently serve the old data back. + if(!nfc_delete_shadow_file(instance)) { + FURI_LOG_E(TAG, "Failed to remove stale shadow file"); + } + // Compare the way the filesystem does -- /ext is case-insensitive, so a Card -> + // card rename lands on the same file and must not be treated as a rename. + if(had_file && !storage_common_equivalent_path( + instance->storage, + furi_string_get_cstr(replaced_path), + furi_string_get_cstr(instance->file_path))) { + if(!nfc_delete_file(instance, replaced_path)) { + FURI_LOG_E(TAG, "Failed to remove replaced file"); + } + } + } else { + // nfc_save() rewrote both on its way to failing; put them back, or the app points + // at a file that was never written while the original is still on disk. + furi_string_set(instance->file_path, replaced_path); + furi_string_set(instance->file_name, replaced_name); + } + furi_string_free(replaced_name); + furi_string_free(replaced_path); + + if(saved) { scene_manager_next_scene(instance->scene_manager, NfcSceneSaveSuccess); dolphin_deed( scene_manager_has_previous_scene(instance->scene_manager, NfcSceneSetType) ? diff --git a/applications/main/nfc/nfc_app.c b/applications/main/nfc/nfc_app.c index 3ac07f240..806d4aaca 100644 --- a/applications/main/nfc/nfc_app.c +++ b/applications/main/nfc/nfc_app.c @@ -277,7 +277,7 @@ bool nfc_save_file(NfcApp* instance, FuriString* path) { furi_assert(instance); furi_assert(path); - bool result = nfc_device_save(instance->nfc_device, furi_string_get_cstr(instance->file_path)); + bool result = nfc_device_save(instance->nfc_device, furi_string_get_cstr(path)); if(!result) { dialog_message_show_storage_error(instance->dialogs, "Cannot save\nkey file"); @@ -395,19 +395,36 @@ bool nfc_load_file(NfcApp* instance, FuriString* path, bool show_dialog) { return result; } +bool nfc_delete_file(NfcApp* instance, const FuriString* path) { + furi_assert(instance); + furi_assert(path); + + // A .shd only ever overlays its .nfc, so either spelling means "remove the card": normalise to + // the .nfc and drop the shadow beside it. + FuriString* target = furi_string_alloc_set(path); + if(furi_string_end_with_str(target, NFC_APP_SHADOW_EXTENSION)) { + furi_string_replace_at(target, furi_string_size(target) - 4, 4, NFC_APP_EXTENSION); + } + + FuriString* shadow_path = furi_string_alloc(); + bool result = storage_simply_remove(instance->storage, furi_string_get_cstr(target)); + if(nfc_set_shadow_file_path(target, shadow_path)) { + // A surviving shadow would hijack the next card saved under this name, so it counts + // towards the result. storage_simply_remove() is happy when the file is already gone. + result = storage_simply_remove(instance->storage, furi_string_get_cstr(shadow_path)) && + result; + } + + furi_string_free(shadow_path); + furi_string_free(target); + + return result; +} + bool nfc_delete(NfcApp* instance) { furi_assert(instance); - if(nfc_has_shadow_file(instance)) { - nfc_delete_shadow_file(instance); - } - - if(furi_string_end_with_str(instance->file_path, NFC_APP_SHADOW_EXTENSION)) { - size_t path_len = furi_string_size(instance->file_path); - furi_string_replace_at(instance->file_path, path_len - 4, 4, NFC_APP_EXTENSION); - } - - return storage_simply_remove(instance->storage, furi_string_get_cstr(instance->file_path)); + return nfc_delete_file(instance, instance->file_path); } bool nfc_delete_shadow_file(NfcApp* instance) { diff --git a/applications/main/nfc/nfc_app_i.h b/applications/main/nfc/nfc_app_i.h index 725b7f484..c66f32256 100644 --- a/applications/main/nfc/nfc_app_i.h +++ b/applications/main/nfc/nfc_app_i.h @@ -280,6 +280,8 @@ bool nfc_save(NfcApp* instance); bool nfc_delete(NfcApp* instance); +bool nfc_delete_file(NfcApp* instance, const FuriString* path); + bool nfc_load_from_file_select(NfcApp* instance); bool nfc_load_file(NfcApp* instance, FuriString* path, bool show_dialog);