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 · 6 comments
Open

Memory Leak in serialize_indirect_attribute() #387

hoyong2007 opened this issue Sep 12, 2024 · 6 comments
Labels

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 <[email protected]>
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 <[email protected]>
Reviewed-by: Etienne Carriere <[email protected]>
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 <[email protected]>
Reviewed-by: Etienne Carriere <[email protected]>
@hoyong2007
Copy link
Contributor Author

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

Copy link

This issue has been marked as a stale issue because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this issue will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants