-
Notifications
You must be signed in to change notification settings - Fork 261
Fix UAF when passing TypeBuilder objects into some APIs #7566
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
base: dev
Are you sure you want to change the base?
Conversation
TypeBuilder.handle creates an immutable_type gets the handle and then deletes the object to which the handle belonged to. This was fundamentally a UAF. immutable_copy is an error prone api as its very easy to misuse. We should consider a re-architecture of this API. `TypeBuilder.immutable_copy().<anything>` or `Type.mutable_copy().<anything>` should be considered sketchy and immutable_copy().handle is just a straight up UAF. Prior to this commit the following would cause a crash: current_data_variable.type = PointerBuilder.create(FunctionType.create())
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses a use-after-free (UAF) vulnerability by ensuring that immutable copies of Type and TypeBuilder objects are created before passing their handles to core functions. The changes prevent type objects from being garbage collected while their handles are still in use.
- Extracts
immutable_copy()calls into local variables to maintain references throughout the lifetime of handle usage - Adds a
type_listtoFunctionBuilder._to_core_struct()to preserve type object references - Updates
_to_core_struct()return signature to include the preserved type list - Applies immutable copies consistently across all type-related API calls
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| python/types.py | Core changes to prevent UAF by storing immutable copies before extracting handles; removed unused handle property and _to_core_struct from TypeBuilder; updated FunctionBuilder._to_core_struct to return type_list |
| python/mediumlevelil.py | Applied immutable_copy() before calling _to_core_struct() to prevent UAF |
| python/highlevelil.py | Applied immutable_copy() before calling _to_core_struct() to prevent UAF |
| python/function.py | Applied immutable_copy() before accessing handles to prevent UAF across multiple function methods |
| python/binaryview.py | Applied immutable_copy() before accessing handles; updated isinstance checks to only check for Type after conversion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type_obj = type_obj.immutable_copy() | ||
| if not isinstance(type_obj, _types.Type): | ||
| raise TypeError("type_obj must be a Type object") | ||
| if _name is None: | ||
| raise ValueError("name can only be None if named type is derived from string passed to type_obj") |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isinstance check at line 8694 is now redundant since immutable_copy() always returns a Type object. This check should be moved before the immutable_copy() call to validate the input parameter, or the error message should be updated to reflect that the conversion failed. The same pattern occurs at lines 8728-8730.
| type_obj = type_obj.immutable_copy() | |
| if not isinstance(type_obj, _types.Type): | |
| raise TypeError("type_obj must be a Type object") | |
| if _name is None: | |
| raise ValueError("name can only be None if named type is derived from string passed to type_obj") | |
| elif not isinstance(type_obj, _types.Type): | |
| raise TypeError("type_obj must be a Type object or a string") | |
| elif not isinstance(type_obj, _types.Type): | |
| raise TypeError("type_obj must be a Type object or a string") | |
| type_obj = type_obj.immutable_copy() |
| type_obj = type_obj.immutable_copy() | ||
| if not isinstance(type_obj, _types.Type): | ||
| raise TypeError("type_obj must be a Type object") |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isinstance check at line 8729 is now redundant since immutable_copy() always returns a Type object. This check should be moved before the immutable_copy() call to validate the input parameter, or the error message should be updated to reflect that the conversion failed. This is the same issue as in export_type_to_library method.
| type_obj = type_obj.immutable_copy() | |
| if not isinstance(type_obj, _types.Type): | |
| raise TypeError("type_obj must be a Type object") | |
| elif not isinstance(type_obj, _types.Type): | |
| raise TypeError("type_obj must be a Type object or a type string") | |
| type_obj = type_obj.immutable_copy() |
This is a fundamental rewrite of how TypeBuilder objects are handled in the python API.