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

[wewlad-dev]

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

[bmt626]

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]

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]

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

[bettse]

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 👍