Fix a fundamental problem with TypeBuilder.handle by deleting it

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())

Author: plafosse

python/binaryview.py

@@ -5286,8 +5286,8 @@ def define_data_var(
 
 		if isinstance(var_type, str):
 			(var_type, _) = self.parse_type_string(var_type)
-		tc = var_type._to_core_struct()
-		core.BNDefineDataVariable(self.handle, addr, tc)
+		tc = var_type.immutable_copy()
+		core.BNDefineDataVariable(self.handle, addr, tc._to_core_struct())
 
 		if name is not None:
 			if isinstance(name, str):
@@ -5321,8 +5321,11 @@ def define_user_data_var(
 
 		if isinstance(var_type, str):
 			(var_type, _) = self.parse_type_string(var_type)
-		tc = var_type._to_core_struct()
-		core.BNDefineUserDataVariable(self.handle, addr, tc)
+
+		# this var_type temporary is essential! It holds the reference to the immutable type
+		# until after BNDefineUserDataVariable takes the reference.
+		var_type = var_type.immutable_copy()
+		core.BNDefineUserDataVariable(self.handle, addr, var_type._to_core_struct())
 
 		if name is not None:
 			if isinstance(name, str):
@@ -8377,6 +8380,7 @@ def define_type(
 				default_name = new_name
 		assert default_name is not None, "default_name can only be None if named type is derived from string passed to type_obj"
 		name = _types.QualifiedName(default_name)._to_core_struct()
+		type_obj = type_obj.immutable_copy()
 		reg_name = core.BNDefineAnalysisType(self.handle, type_id, name, type_obj.handle)
 		result = _types.QualifiedName._from_core_struct(reg_name)
 		core.BNFreeQualifiedName(reg_name)
@@ -8407,6 +8411,7 @@ def define_user_type(self, name: Optional['_types.QualifiedNameType'], type_obj:
 		if name is None:
 			raise ValueError("name can only be None if named type is derived from string passed to type_obj")
 		_name = _types.QualifiedName(name)._to_core_struct()
+		type_obj = type_obj.immutable_copy()
 		core.BNDefineUserAnalysisType(self.handle, _name, type_obj.handle)
 
 	def define_types(self, types: List[Tuple[str, Optional['_types.QualifiedNameType'], StringOrType]], progress_func: Optional[ProgressFuncType]) -> Mapping[str, '_types.QualifiedName']:
@@ -8685,7 +8690,8 @@ def export_type_to_library(self, lib: typelibrary.TypeLibrary, name: Optional[st
 			(type_obj, new_name) = self.parse_type_string(type_obj)
 			if name is None:
 				_name = new_name
-		if not isinstance(type_obj, (_types.Type, _types.TypeBuilder)):
+		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")
@@ -8719,7 +8725,8 @@ def export_object_to_library(
 			(type_obj, new_name) = self.parse_type_string(type_obj)
 			if name is None:
 				_name = new_name
-		if not isinstance(type_obj, (_types.Type, _types.TypeBuilder)):
+		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")

python/function.py

@@ -1359,6 +1359,7 @@ def return_type(self, value: Optional[StringOrType]) -> None:  # type: ignore
 			type_conf.type = value.handle
 			type_conf.confidence = core.max_confidence
 		else:
+			value = value.immutable_copy()
 			type_conf.type = value.handle
 			type_conf.confidence = value.confidence
 		core.BNSetUserFunctionReturnType(self.handle, type_conf)
@@ -2270,11 +2271,14 @@ def create_graph_immediate(
 	def apply_imported_types(self, sym: 'types.CoreSymbol', type: Optional[StringOrType] = None) -> None:
 		if isinstance(type, str):
 			(type, _) = self.view.parse_type_string(type)
+		if type is not None:
+			type = type.immutable_copy()
 		core.BNApplyImportedTypes(self.handle, sym.handle, None if type is None else type.handle)
 
 	def apply_auto_discovered_type(self, func_type: StringOrType) -> None:
 		if isinstance(func_type, str):
 			(func_type, _) = self.view.parse_type_string(func_type)
+		func_type = func_type.immutable_copy()
 		core.BNApplyAutoDiscoveredFunctionType(self.handle, func_type.handle)
 
 	def set_auto_indirect_branches(
@@ -2440,11 +2444,13 @@ def get_block_annotations(self, addr: int,
 	def set_auto_type(self, value: StringOrType) -> None:
 		if isinstance(value, str):
 			(value, _) = self.view.parse_type_string(value)
+		value = value.immutable_copy()
 		core.BNSetFunctionAutoType(self.handle, value.handle)
 
 	def set_user_type(self, value: StringOrType) -> None:
 		if isinstance(value, str):
 			(value, _) = self.view.parse_type_string(value)
+		value = value.immutable_copy()
 		core.BNSetFunctionUserType(self.handle, value.handle)
 
 	@property
@@ -2462,6 +2468,7 @@ def set_auto_return_type(self, value: StringOrType) -> None:
 			type_conf.type = value
 			type_conf.confidence = core.max_confidence
 		else:
+			value = value.immutable_copy()
 			type_conf.type = value.handle
 			type_conf.confidence = value.confidence
 		core.BNSetAutoFunctionReturnType(self.handle, type_conf)
@@ -2807,14 +2814,14 @@ def set_user_instr_highlight(
 	def create_auto_stack_var(self, offset: int, var_type: StringOrType, name: str) -> None:
 		if isinstance(var_type, str):
 			(var_type, _) = self.view.parse_type_string(var_type)
-		tc = var_type._to_core_struct()
-		core.BNCreateAutoStackVariable(self.handle, offset, tc, name)
+		tc = var_type.immutable_copy()
+		core.BNCreateAutoStackVariable(self.handle, offset, tc._to_core_struct(), name)
 
 	def create_user_stack_var(self, offset: int, var_type: StringOrType, name: str) -> None:
 		if isinstance(var_type, str):
 			(var_type, _) = self.view.parse_type_string(var_type)
-		tc = var_type._to_core_struct()
-		core.BNCreateUserStackVariable(self.handle, offset, tc, name)
+		tc = var_type.immutable_copy()
+		core.BNCreateUserStackVariable(self.handle, offset, tc._to_core_struct(), name)
 
 	def delete_auto_stack_var(self, offset: int) -> None:
 		core.BNDeleteAutoStackVariable(self.handle, offset)
@@ -2827,16 +2834,16 @@ def create_auto_var(
 	) -> None:
 		if isinstance(var_type, str):
 			(var_type, _) = self.view.parse_type_string(var_type)
-		tc = var_type._to_core_struct()
-		core.BNCreateAutoVariable(self.handle, var.to_BNVariable(), tc, name, ignore_disjoint_uses)
+		tc = var_type.immutable_copy()
+		core.BNCreateAutoVariable(self.handle, var.to_BNVariable(), tc._to_core_struct(), name, ignore_disjoint_uses)
 
 	def create_user_var(
 	    self, var: 'variable.Variable', var_type: StringOrType, name: str, ignore_disjoint_uses: bool = False
 	) -> None:
 		if isinstance(var_type, str):
 			(var_type, _) = self.view.parse_type_string(var_type)
-		tc = var_type._to_core_struct()
-		core.BNCreateUserVariable(self.handle, var.to_BNVariable(), tc, name, ignore_disjoint_uses)
+		tc = var_type.immutable_copy()
+		core.BNCreateUserVariable(self.handle, var.to_BNVariable(), tc._to_core_struct(), name, ignore_disjoint_uses)
 
 	def delete_user_var(self, var: 'variable.Variable') -> None:
 		core.BNDeleteUserVariable(self.handle, var.to_BNVariable())
@@ -2955,6 +2962,7 @@ def set_call_type_adjustment(
 			else:
 				confidence = adjust_type.confidence
 			type_conf = core.BNTypeWithConfidence()
+			adjust_type = adjust_type.immutable_copy()
 			type_conf.type = adjust_type.handle
 			type_conf.confidence = confidence
 		else:

python/highlevelil.py

@@ -4816,8 +4816,8 @@ def set_expr_type(self, expr_index: int, expr_type: StringOrType) -> None:
 		"""
 		if isinstance(expr_type, str):
 			(expr_type, _) = self.view.parse_type_string(expr_type)
-		tc = expr_type._to_core_struct()
-		core.BNSetHighLevelILExprType(self.handle, expr_index, tc)
+		ic = expr_type.immutable_copy()
+		core.BNSetHighLevelILExprType(self.handle, expr_index, ic._to_core_struct())
 
 
 class HighLevelILBasicBlock(basicblock.BasicBlock):

python/mediumlevelil.py

@@ -6311,7 +6311,8 @@ def set_expr_type(self, expr_index: int, expr_type: Optional[StringOrType]) -> N
 		if expr_type is not None:
 			if isinstance(expr_type, str):
 				(expr_type, _) = self.view.parse_type_string(expr_type)
-			tc = expr_type._to_core_struct()
+			ic = expr_type.immutable_copy()
+			tc = ic._to_core_struct()
 		else:
 			tc = core.BNTypeWithConfidence()
 			tc.type = None

python/types.py

@@ -435,9 +435,10 @@ class FunctionParameter:
 	location: Optional['variable.VariableNameAndType'] = None
 
 	def __repr__(self):
+		ic = self.type.immutable_copy()
 		if (self.location is not None) and (self.location.name != self.name):
-			return f"{self.type.immutable_copy().get_string_before_name()} {self.name}{self.type.immutable_copy().get_string_after_name()} @ {self.location.name}"
-		return f"{self.type.immutable_copy().get_string_before_name()} {self.name}{self.type.immutable_copy().get_string_after_name()}"
+			return f"{ic.get_string_before_name()} {self.name}{ic.get_string_after_name()} @ {self.location.name}"
+		return f"{ic.get_string_before_name()} {self.name}{ic.get_string_after_name()}"
 
 	def immutable_copy(self) -> 'FunctionParameter':
 		return FunctionParameter(self.type.immutable_copy(), self.name, self.location)
@@ -602,18 +603,8 @@ def __repr__(self):
 	def __str__(self):
 		return str(self.immutable_copy())
 
-	@property
-	def handle(self) -> core.BNTypeHandle:
-		return self.immutable_copy().handle
-
 	def __hash__(self):
-		return hash(ctypes.addressof(self.handle.contents))
-
-	def _to_core_struct(self) -> core.BNTypeWithConfidence:
-		type_conf = core.BNTypeWithConfidence()
-		type_conf.type = self.handle
-		type_conf.confidence = self.confidence
-		return type_conf
+		return hash(ctypes.addressof(self._handle.contents))
 
 	def immutable_copy(self):
 		Types = {
@@ -832,7 +823,8 @@ def child(self) -> 'Type':
 
 	@child.setter
 	def child(self, value: SomeType) -> None:
-		core.BNTypeBuilderSetChildType(self._handle, value.immutable_copy()._to_core_struct())
+		ic = value.immutable_copy()
+		core.BNTypeBuilderSetChildType(self._handle, ic._to_core_struct())
 
 	@property
 	def alternate_name(self) -> Optional[str]:
@@ -978,7 +970,8 @@ def create(
 
 		_const = BoolWithConfidence.get_core_struct(const)
 		_volatile = BoolWithConfidence.get_core_struct(volatile)
-		handle = core.BNCreatePointerTypeBuilderOfWidth(_width, type._to_core_struct(), _const, _volatile, ref_type)
+		ic = type.immutable_copy()
+		handle = core.BNCreatePointerTypeBuilderOfWidth(_width, ic._to_core_struct(), _const, _volatile, ref_type)
 		assert handle is not None, "BNCreatePointerTypeBuilderOfWidth returned None"
 		return cls(handle, platform, confidence)
 
@@ -1099,7 +1092,8 @@ def create(
 	    cls, type: SomeType, element_count: int, platform: Optional['_platform.Platform'] = None,
 	    confidence: int = core.max_confidence
 	) -> 'ArrayBuilder':
-		handle = core.BNCreateArrayTypeBuilder(type._to_core_struct(), element_count)
+		ic = type.immutable_copy()
+		handle = core.BNCreateArrayTypeBuilder(ic._to_core_struct(), element_count)
 		assert handle is not None, "BNCreateArrayTypeBuilder returned None"
 		return cls(handle, platform, confidence)
 
@@ -1127,11 +1121,11 @@ def create(
 	    name_type: 'NameType' = NameType.NoNameType,
 	    pure: Optional[BoolWithConfidence] = None
 	) -> 'FunctionBuilder':
-		param_buf = FunctionBuilder._to_core_struct(params)
+		param_buf, type_list = FunctionBuilder._to_core_struct(params)
 		if return_type is None:
-			ret_conf = Type.void()._to_core_struct()
+			ret_conf = Type.void()
 		else:
-			ret_conf = return_type._to_core_struct()
+			ret_conf = return_type.immutable_copy()
 
 		conv_conf = core.BNCallingConventionWithConfidence()
 		if calling_convention is None:
@@ -1185,7 +1179,7 @@ def create(
 		if params is None:
 			params = []
 		handle = core.BNCreateFunctionTypeBuilder(
-		    ret_conf, conv_conf, param_buf, len(params), vararg_conf, can_return_conf, stack_adjust_conf,
+		    ret_conf._to_core_struct(), conv_conf, param_buf, len(params), vararg_conf, can_return_conf, stack_adjust_conf,
 		    reg_stack_adjust_regs, reg_stack_adjust_values, len(reg_stack_adjust),
 		    return_regs_set, name_type, pure_conf
 		)
@@ -1282,20 +1276,30 @@ def variable_arguments(self) -> BoolWithConfidence:
 	def _to_core_struct(params: Optional[ParamsType] = None):
 		if params is None:
 			params = []
+
+		# type_list is very important as we need to keep a reference to the intermediate type
+		# objects as we're getting their handles if they go out of scope while we're holding
+		# their handles we get a UAF. This is only necessary as we're inside a helper that
+		# has to deal with raw type objects
+		type_list = []
 		param_buf = (core.BNFunctionParameter * len(params))()
 		for i, param in enumerate(params):
 			core_param = param_buf[i]
 			if isinstance(param, (Type, TypeBuilder)):
+				param = param.immutable_copy()
+				type_list.append(param)
 				assert param.handle is not None, "Attempting to construct function parameter without properly constructed type"
 				core_param.name = ""
 				core_param.type = param.handle
 				core_param.typeConfidence = param.confidence
 				core_param.defaultLocation = True
 			elif isinstance(param, FunctionParameter):
 				assert param.type is not None, "Attempting to construct function parameter without properly constructed type"
+				param_type = param.type.immutable_copy()
+				type_list.append(param_type)
 				core_param.name = param.name
-				core_param.type = param.type.handle
-				core_param.typeConfidence = param.type.confidence
+				core_param.type = param_type.handle
+				core_param.typeConfidence = param_type.confidence
 				if param.location is None:
 					core_param.defaultLocation = True
 				else:
@@ -1307,13 +1311,15 @@ def _to_core_struct(params: Optional[ParamsType] = None):
 				name, _type = param
 				if not isinstance(name, str) or not isinstance(_type, (Type, TypeBuilder)):
 					raise ValueError(f"Conversion from unsupported function parameter type {type(param)}")
+				_type = _type.immutable_copy()
+				type_list.append(_type)
 				core_param.name = name
 				core_param.type = _type.handle
 				core_param.typeConfidence = _type.confidence
 				core_param.defaultLocation = True
 			else:
 				raise ValueError(f"Conversion from unsupported function parameter type {type(param)}")
-		return param_buf
+		return param_buf, type_list
 
 	@parameters.setter
 	def parameters(self, params: List[FunctionParameter]) -> None:
@@ -1438,17 +1444,20 @@ def _add_members_to_builder(structure_builder_handle, members: Optional[MembersT
 		for member in members:
 			if isinstance(member, Tuple):
 				_type, _name = member
+				ic = _type.immutable_copy()
 				core.BNAddStructureBuilderMember(
-				    structure_builder_handle, _type._to_core_struct(), _name, MemberAccess.NoAccess, MemberScope.NoScope
+				    structure_builder_handle, ic._to_core_struct(), _name, MemberAccess.NoAccess, MemberScope.NoScope
 				)
 			elif isinstance(member, StructureMember):
+				ic = member.type.immutable_copy()
 				core.BNAddStructureBuilderMemberAtOffset(
-				    structure_builder_handle, member.type._to_core_struct(), member.name, member.offset, False,
+				    structure_builder_handle, ic._to_core_struct(), member.name, member.offset, False,
 				    member.access, member.scope, member.bit_position, member.bit_width
 				)
 			elif isinstance(member, (TypeBuilder, Type)):
+				ic = member.immutable_copy()
 				core.BNAddStructureBuilderMember(
-				    structure_builder_handle, member._to_core_struct(), "", MemberAccess.NoAccess, MemberScope.NoScope
+				    structure_builder_handle, ic._to_core_struct(), "", MemberAccess.NoAccess, MemberScope.NoScope
 				)
 			else:
 				raise ValueError(f"Structure member type {member} not supported")
@@ -1611,8 +1620,9 @@ def index_by_offset(self, offset: MemberOffset) -> Optional[MemberIndex]:
 		return None
 
 	def replace(self, index: int, type: SomeType, name: str = "", overwrite_existing: bool = True):
+		ic = type.immutable_copy()
 		core.BNReplaceStructureBuilderMember(
-		    self.builder_handle, index, type._to_core_struct(), name, overwrite_existing
+		    self.builder_handle, index, ic._to_core_struct(), name, overwrite_existing
 		)
 
 	def remove(self, index: int):
@@ -1622,8 +1632,9 @@ def insert(
 	    self, offset: int, type: SomeType, name: str = "", overwrite_existing: bool = True,
 	    access: MemberAccess = MemberAccess.NoAccess, scope: MemberScope = MemberScope.NoScope, bit_position: int = 0, bit_width: int = 0
 	):
+		ic = type.immutable_copy()
 		core.BNAddStructureBuilderMemberAtOffset(
-		    self.builder_handle, type._to_core_struct(), name, offset, overwrite_existing, access, scope, bit_position,
+		    self.builder_handle, ic._to_core_struct(), name, offset, overwrite_existing, access, scope, bit_position,
 		    bit_width
 		)
 
@@ -1632,16 +1643,18 @@ def append(
 	    scope: MemberScope = MemberScope.NoScope
 	) -> 'StructureBuilder':
 		# appends a member at the end of the structure growing the structure
-		core.BNAddStructureBuilderMember(self.builder_handle, type._to_core_struct(), name, access, scope)
+		ic = type.immutable_copy()
+		core.BNAddStructureBuilderMember(self.builder_handle, ic._to_core_struct(), name, access, scope)
 		return self
 
 	def add_member_at_offset(
 	    self, name: MemberName, type: SomeType, offset: MemberOffset, overwrite_existing: bool = True,
 	    access: MemberAccess = MemberAccess.NoAccess, scope: MemberScope = MemberScope.NoScope, bit_position: int = 0, bit_width: int = 0
 	) -> 'StructureBuilder':
 		# Adds structure member to the given offset optionally clearing any members within the range offset-offset+len(type)
+		ic = type.immutable_copy()
 		core.BNAddStructureBuilderMemberAtOffset(
-		    self.builder_handle, type._to_core_struct(), name, offset, overwrite_existing, access, scope, bit_position,
+		    self.builder_handle, ic._to_core_struct(), name, offset, overwrite_existing, access, scope, bit_position,
 		    bit_width
 		)
 		return self
@@ -3083,7 +3096,7 @@ def create(
 			ret = VoidType.create()
 		if params is None:
 			params = []
-		param_buf = FunctionBuilder._to_core_struct(params)
+		param_buf, type_list = FunctionBuilder._to_core_struct(params)
 		ret_conf = ret._to_core_struct()
 		conv_conf = core.BNCallingConventionWithConfidence()
 		if calling_convention is None: