Skip to content
This repository has been archived by the owner on Nov 19, 2024. It is now read-only.

Apps: PicoPass: Fix crash on save with 23+ character default name #487

Closed
wewlad-dev opened this issue Dec 11, 2023 · 4 comments · Fixed by #540
Closed

Apps: PicoPass: Fix crash on save with 23+ character default name #487

wewlad-dev opened this issue Dec 11, 2023 · 4 comments · Fixed by #540
Assignees
Labels
bug Something isn't working release-pending This has been implemented and is waiting to be released publicly

Comments

@wewlad-dev
Copy link

Describe the bug.

The flipper zero Xtreme FW crashes if I scan an NFC HID iClass (picopass) card, and when it prompts to save the scanned profile with the default name (name+date), it crashes and reboot, however, if I rename it to something else before saving, nothing happens, so it must be the name crashes it with memory buffer overflow or similar.

Reproduction

  1. Go to Apps
  2. Got to NFC
  3. Click "HID iClass (picopass)" and read card
  4. once read, save the profile with the default name
  5. Flipper reboot

Target

XFW-0052

Logs

No response

Anything else?

No response

@wewlad-dev wewlad-dev added the bug Something isn't working label Dec 11, 2023
@bmt626
Copy link

bmt626 commented Dec 12, 2023

I am having the same issue I even deleted the apps folder and reflashed the latest stable update to make sure it was not something from the original updat flash.

@Willy-JL
Copy link
Contributor

Willy-JL commented Dec 13, 2023

however, if I rename it to something else before saving, nothing happens, so it must be the name crashes it with memory buffer overflow or similar.

you seem to be correct

here the text input result is copied into the device name buffer (23 chars) using the length of the source text (up to 129 chars), not destination bounds
https://github.com/flipperdevices/flipperzero-good-faps/blob/d9d5c63ddffc64f28ea879bd6dcff99a55b76ce8/picopass/scenes/picopass_scene_save_name.c#L60-L61

previously here the text input was given a max of 22 characters
https://github.com/flipperdevices/flipperzero-good-faps/blob/d9d5c63ddffc64f28ea879bd6dcff99a55b76ce8/picopass/scenes/picopass_scene_save_name.c#L26-L32

this copying wouldn't be too big of an issue if we know for certain that the text buffer will not be longer than the device name buffer. however giving the text input a max length only prevents from adding more after that, instead if the text store already contained more than the maximum length, the limit on the text input is useless.

and in fact, previously the default name was generated with the automatic default generator, using the text buffer size of 129 maximum characters
https://github.com/flipperdevices/flipperzero-good-faps/blob/d9d5c63ddffc64f28ea879bd6dcff99a55b76ce8/picopass/scenes/picopass_scene_save_name.c#L19-L20

im not sure if this problem did not come up previously because on other firmware the default names happen to be shorter than 23 characters, but putting the correct max length on the generator would have fixed this, although cutting off the default timestamp name.

rather @bettse i was thinking that increasing the max name length for picopass devices would be a better solution. i see it is set with #define PICOPASS_DEV_NAME_MAX_LEN 22, from a cursory look at the code it seems like altering this value to 128 (to match the text buffer) for example would not cause other problems, but let me know if im missing something. also the value passed to the text input for max length includes the null terminator, so no need to use PICOPASS_DEV_NAME_MAX_LEN and make the buffer PICOPASS_DEV_NAME_MAX_LEN + 1, simply sizeof() would suffice. from my experience, most Flipper APIs take string sizes including null terminators, just like strlcpy()

EDIT: not sure why github decided to not embed the code snippets, kinda rude lol. i think it doesnt do it across repositories

@Willy-JL
Copy link
Contributor

i have pushed e06837d (update from apps submodule) which should hopefully resolve the issue. let me know if it causes other weird behavior

@Willy-JL Willy-JL added the release-pending This has been implemented and is waiting to be released publicly label Dec 13, 2023
@Willy-JL Willy-JL changed the title Flipperzero crashes and reboot after scanning with NFC HID iClass (picopass) Apps: PicoPass: Fix crash on save with 23+ character default name Dec 13, 2023
@Willy-JL Willy-JL moved this to 👀 Done (In next release) in Xtreme-Firmware Dec 13, 2023
@Willy-JL Willy-JL self-assigned this Dec 13, 2023
@bettse
Copy link
Contributor

bettse commented Dec 13, 2023

i was thinking that increasing the max name length for picopass devices would be a better solution.

I see no problem. CC me on the PR and I'll 👍

@Willy-JL Willy-JL linked a pull request Feb 2, 2024 that will close this issue
@Willy-JL Willy-JL closed this as completed Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working release-pending This has been implemented and is waiting to be released publicly
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants