Null-Pointer Dereference in serialize_mecha_XXX()

[hoyong2007]

Commit: a5b1ffc (master) File: libckteec/src/serialize_ck.c Function: serialize_mecha_XXX()

There are lots of potential Null-Pointer deference spots in serialize_ck.c

static CK_RV serialize_mecha_rsa_oaep_param(struct serializer *obj,
                        CK_MECHANISM_PTR mecha)
{
    CK_RSA_PKCS_OAEP_PARAMS *params = mecha->pParameter;
    CK_RV rv = CKR_GENERAL_ERROR;
    size_t params_size = 4 * sizeof(uint32_t) + params->ulSourceDataLen;

    if (mecha->ulParameterLen != sizeof(*params))
        return CKR_ARGUMENTS_BAD;

    ...

    return serialize_buffer(obj, params->pSourceData,
                params->ulSourceDataLen);
}

For example, in the serialize_mecha_rsa_oaep_param() function, mecha->pParameter is referenced directly without proper pointer validation.

Additionally, the params->pSourceData pointer is also directly referenced in the serialize_buffer() function without proper pointer validation.

Directly referencing pointers without proper validation, as shown, can lead to null pointer dereference bugs, which may result in a DoS (Denial of Service).

Credit: @hoyong2007 @jch6637

[etienne-lms]

Thanks for the investigation.

Using null pointers with libckteec will only crash the Linux client application that invokes the TA. It won't affect other Linux applications neither the pkcs11 TA itsleft. So I don't thinks these can lead to DoS attacks.

We could add non-NULL pointer verification for non-zero sized buffer references but that would only help for development/debugging. For example, there is no way to check pointers are well sized. It is the responsibility of the client application to contain programming errors. That said, it should not add much complexity to test some pointer here and there.

[hoyong2007]

Thanks for your reply.

I'm focusing on the bugs via NULL pointer, not non-NULL pointer. I agreed with that the client cannot check the actual buffer size of the pointer, but I believe the client should at least verify whether the pointer is null.

As far as I know, the mechanism pointer is checked in the higher-level functions, but the pointer members of the mechanism are not checked for null. I believe every pointer variable should be checked at least once before being referenced.

[etienne-lms]

If you have some changes to propose, I'll be happy to review them and help.

[hoyong2007]

I made PR for this issue. https://github.com/OP-TEE/optee_client/pull/392

[etienne-lms]

Hi @hoyong2007, thanks. I'll posted some comments.

[hoyong2007]

@etienne-lms I’ve added a few additional comments and committed the code with the comments reflected.