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

Memory Leak in serialize_indirect_attribute() #387

Open
hoyong2007 opened this issue Sep 12, 2024 · 5 comments
Open

Memory Leak in serialize_indirect_attribute() #387

hoyong2007 opened this issue Sep 12, 2024 · 5 comments

Comments

@hoyong2007
Copy link
Contributor

hoyong2007 commented Sep 12, 2024

Commit: a5b1ffc (master)
File: libckteec/src/serialize_ck.c#L104
Function: serialize_indirect_attribute()

static CK_RV serialize_indirect_attribute(struct serializer *obj,
					  CK_ATTRIBUTE_PTR attribute)
{
	CK_ATTRIBUTE_PTR attr = NULL;
	CK_ULONG count = 0;
	CK_RV rv = CKR_GENERAL_ERROR;
	struct serializer obj2 = { 0 };

	...

	/* Create a serialized object for the content */
	rv = serialize_ck_attributes(&obj2, attr, count);
	if (rv)
		return rv;

	...

	rv = serialize_buffer(obj, obj2.buffer, obj2.size);
	if (rv)
		return rv;

	obj->item_count++;

	return rv;
}

In the serialize_indirect_attribute() function, serialize_ck_attributes() function call allocate memory to obj2, but the allocated memory is not properly freed, leading to a memory leak.

This can degrade performance over time, particularly in long-running processes where the function is frequently called.

Credit: @hoyong2007 @jch6637

@etienne-lms
Copy link
Contributor

When serialize_ck_attributes() fails, it internally calls release_serial_object() to release the related object attributes memory blob before reporting the error status.
In libckteec, all calls to serialize_ck_attributes() in src/pkcs11_processing.c are balanced by a call to release_serial_object(), usually on exit point of the functions that make the calls.
All in one, I think that there is no leakage of memory allocation in libckteec. That said, I may have missed something so if you have found explicit occurrences, please feel free to share pointers, some code examples or fix proposal.

@etienne-lms
Copy link
Contributor

My mistake, you're right, there misses a call to release_serial_object() for obj2 in serialize_indirect_attribute() before the function returns.
Do you want to create a P-R to fix this? Unless you prefer I do so.

@hoyong2007
Copy link
Contributor Author

hoyong2007 commented Sep 13, 2024

My pleasure :)
I made PR at #390

Since memory leakage can lead to security issues, such as DoS (Denial of Service), could you assign a CVE for this bug?

@etienne-lms
Copy link
Contributor

I don't think this issues falls in the scope of a security vulnerability. @jbech-linaro, any thought?

@hoyong2007, we thank you for your contributions in OP-TEE, however that please note that if you think you have found a security flaw in OP-TEE, we would prefer you follow the vulnerability reporting procedure detailed here:
https://optee.readthedocs.io/en/latest/general/contact.html#vulnerability-reporting.

hoyong2007 added a commit to hoyong2007/optee_client that referenced this issue Sep 13, 2024
Fix memory allocation leakage with a call to release_serial_object()
to release obj2 before serialize_indirect_attribute() returns.

Link: OP-TEE#387
Fixes: e88c264 ("libckteec: helper function to serialize a attribute template")
Signed-off-by: Hoyong Jin <hoyong2007@naver.com>
hoyong2007 added a commit to hoyong2007/optee_client that referenced this issue Sep 19, 2024
Fix memory allocation leakage with a call to release_serial_object()
to release obj2 before serialize_indirect_attribute() returns.

Link: OP-TEE#387
Fixes: e88c264 ("libckteec: helper function to serialize a attribute template")
Signed-off-by: Hoyong Jin <hoyong2007@naver.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
jforissier pushed a commit that referenced this issue Sep 24, 2024
Fix memory allocation leakage with a call to release_serial_object()
to release obj2 before serialize_indirect_attribute() returns.

Link: #387
Fixes: e88c264 ("libckteec: helper function to serialize a attribute template")
Signed-off-by: Hoyong Jin <hoyong2007@naver.com>
Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com>
@hoyong2007
Copy link
Contributor Author

I believe that persistent memory leaks can exhaust system-wide memory, potentially compromising the availability of other applications.

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

No branches or pull requests

2 participants