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

fix buffer overflow in mifare ul load #1697

Merged
merged 2 commits into from Sep 5, 2022

Conversation

VVX7
Copy link
Contributor

@VVX7 VVX7 commented Sep 2, 2022

What's new

A buffer overflow exists in nfc_device_load_mifare_ul_data when the pages_total value is greater than MF_UL_MAX_DUMP_SIZE. This occurs because pages_total is parsed from the nfc file and not checked against the max size of the buffer. An nfc file with a page count greater than 2040 will result in an out of bounds write.

This may result in various crashes including a BusFault crash, and a NULL point exception. In some cases, this require re-flashing flipper firmware to recover the device :(

CleanShot 2022-09-02 at 12 55 48@2x

Shout out to https://tmpout.sh/bggp/3/ for giving me a reason to look for bugs!! Maybe more to come ;)

Verification

Upload an nfc file containing more than 2040 pages. An example file (that will produce a null pointer deference) is provided here:
https://gist.github.com/VVX7/c55b122846253e12f1647e2a85ab2775

Steps:

  • Upload the nfc file
  • Navigate to saved nfc submenu and load the file
  • view file info to trigger the crash

Note:
Larger files will result in similar unexpected behaviour and may not require viewing file info to trigger crash.

Impact

An attacker could place a malicious Amiibo file in a repository like this: https://github.com/Gioman101/FlipperAmiibo/

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

@gornekich
Copy link
Contributor

Hello @VVX7 . Am I right that there is no actual NFC tag with so many pages? This file was made by hands?

@VVX7
Copy link
Contributor Author

VVX7 commented Sep 2, 2022

hiya @gornekich , that's right. The file was made by hand. The risk comes from someone inserting a malicious file into database of Amiibos or something similar as I linked above.

Might also be able to emulate an NFC with unusually high page count using a proxmark or hydrabus, idk. I should go poke at that...

@skotopes skotopes merged commit 8d8481b into flipperdevices:dev Sep 5, 2022
8 checks passed
@VVX7 VVX7 deleted the mifare_ul_buffer_overflow branch September 5, 2022 12:10
hedger pushed a commit that referenced this pull request Sep 5, 2022
Co-authored-by: gornekich <n.gorbadey@gmail.com>
Dig03 pushed a commit to Dig03/flipperzero-firmware that referenced this pull request Sep 6, 2022
Co-authored-by: gornekich <n.gorbadey@gmail.com>
litui pushed a commit to litui/flipperzero-firmware that referenced this pull request Sep 10, 2022
Co-authored-by: gornekich <n.gorbadey@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants