Check the screen frame size before copying it - #16
Open
munzzyy wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
expansion_process_screen_streaming()readsScreenFrame.data->bytesand passes it toframe_parse_data(), which memcpy's a fixed 1024 bytes (sizeof(frame_t)) out of it. Nothing checks the pointer or the length first.ScreenFrame.datais a pointer field — flipperzero-protobuf'sgui.optionshasPB_Gui.ScreenFrame.data type:FT_POINTER. On that path nanopb allocates exactly as many bytes as arrived on the wire, and leaves the pointer NULL when the field isn't present at all. Themax_size:1024line next to it only constrains statically allocated fields,pb_dec_bytes()never applies it to pointer fields. So a frame that omits the data field, or carries a short one, gets us either a NULL dereference or a read off the end of the allocation.I checked this against real generated code rather than reasoning about it. I ran the repo's own
gui.protoandgui.optionsthrough the nanopb generator, decoded hand-built wire bytes with the realpb_decode.c, and repeated the same access pattern under ASan.Field omitted:
Field present but only 4 bytes long:
The other two callers of
frame_parse_data()both hand it a fixed 1024-byte buffer, so the streaming path is the only one that needs the check. Breaking out of the loop on a bad frame matches how the rest of the function already handles a frame it can't use.