OP-TEE / optee_client

Normal World Client side of the TEE
Other
188 stars 235 forks source link

Memory Leak in serialize_indirect_attribute() #387

Open hoyong2007 opened 1 month ago

hoyong2007 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

My pleasure :) I made PR at https://github.com/OP-TEE/optee_client/pull/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 commented 1 month ago

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 commented 4 weeks ago

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