OP-TEE / optee_os

Trusted side of the TEE
Other
1.51k stars 1.03k forks source link

Question about TEE_Panic #6901

Closed kyoWNC closed 1 week ago

kyoWNC commented 1 week ago

Why does TEE_ERROR_BAD_PARAMETERS trigger for some APIs but not for others?

For example (OPTEE 3.2.0): 1)

TEE_Result TEE_AsymmetricDecrypt(...) {

    if (res != TEE_SUCCESS &&
        res != TEE_ERROR_SHORT_BUFFER &&
        res != TEE_ERROR_BAD_PARAMETERS)                                                                                                                                                     
        TEE_Panic(res);

    return res;
}

2)

TEE_AsymmetricVerifyDigest(...) {
...

    __utee_from_attr(ua, params, paramCount);
    res = utee_asymm_verify(operation->state, ua, paramCount, digest,
                digestLen, signature, signatureLen);

    if (res != TEE_SUCCESS && res != TEE_ERROR_SIGNATURE_INVALID)                                                                                                                            
        TEE_Panic(res);

    return res;
}

However, in fact, utee_asymm_verify may return other error codes, such as TEE_ERROR_BAD_PARAMETERS. If so, this will trigger TEE_Panic, leading to the TA instance being destroyed.

The basis for triggering TEE_Panic by an error code is what? Can we modify code like as follows:

TEE_AsymmetricVerifyDigest(...) {
...
if (res != TEE_SUCCESS && res != TEE_ERROR_SIGNATURE_INVALID && res != TEE_ERROR_BAD_PARAMETERS)                                                                                                            
        TEE_Panic(res);

    return res;
}

Thanks.

jenswi-linaro commented 1 week ago

GlobalPlatform TEE Internal Core API Specification (https://globalplatform.org/specs-library/tee-internal-core-api-specification/) defines the allowed return values for various functions, anything else will as a last resort result in a panic.

kyoWNC commented 1 week ago

Hi Jenswi,

Thanks for your quick reply.

Because our OP-TEE version does not support the TEE_MaskPanics API, and I saw in the description of the TEE_AsymmetricVerifyDigest API that an error code can be returned instead of triggering a panic.

Does this mean that I can modify the code to directly return an error code when the API receives a "TEE_ERROR_BAD_PARAMETERS" result, instead of triggering a panic, and still comply with the GPD rule?

Thanks

jenswi-linaro commented 1 week ago

The TEE_MaskPanics API isn't part of a released standard. From an upstream point of view, we'd rather wait until it's properly released.

Why do you get these "TEE_ERROR_BAD_PARAMETERS"? Is it a programming error in the TA or a limitation in OP-TEE? If it's the latter, we could fix the problem instead of working around it.

kyoWNC commented 1 week ago

We have a use case that needs to verify signature using ECDSA in the TA.

The call flow is

TEE_AsymmetricVerifyDigest -> utee_asymm_verify -> syscall_asymm_verify
-> crypto_acipher_ecc_verify(...) {
...
    /* check keysize vs sig_len */
    if ((key_size_bytes * 2) != sig_len) {
        res = TEE_ERROR_BAD_PARAMETERS;
        goto out;
    }
...
}

If the CA sends a signature length that is not twice the key size, TEE_ERROR_BAD_PARAMETERS will be received. This will cause the TEE_AsymmetricVerifyDigest API to trigger TEE_Panic, subsequently leading to the destruction of the TA instance.

I think the CA side is only interested in knowing if the signature is valid. Error codes like TEE_ERROR_BAD_PARAMETERS shouldn't lead to the destruction of the TA, as this would impact the development process on the CA side, such as requiring a session restart.

jenswi-linaro commented 1 week ago

Why not add a check in the TA that the signature has the correct size? It seems like a strange shortcut to have the GP API deviate from the standard to save a check in the TA.

kyoWNC commented 1 week ago

Adding checks is one solution before using the TEE_AsymmetricVerifyDigest API, but not the best one :( Here are my concerns: a. Many APIs encounter this issue; it's not practical to add various checks identical to those at the underlying level. b. There might be excessive redundant checks. c. The increase in code size affects the TA performance. (Multiple TA instances and sessions are in progress at the same time.)

Thanks for your time to answer me.

jenswi-linaro commented 1 week ago

a. Many APIs encounter this issue; it's not practical to add various checks identical to those at the underlying level.

Speaking of practical, expecting the world to adjust to your use cases may also prove to be impractical.

b. There might be excessive redundant checks. c. The increase in code size affects the TA performance. (Multiple TA instances and sessions are in progress at the same time.)

The time spent on this check is nothing compared to verifying a signature. You'll need numbers to back up that claim.

I don't see how these extra checks can be such an issue. Some of the panics may be inconvenient, but the idea with the panics is to catch programming errors (in the TA) early so they don't turn into security issues, for instance leaking secret information.

Another class of panics is when OP-TEE Core can't execute a request properly, the TA is terminated instead of leaving it in an unexpected state. Again, security has priority.

I think that the underlying issue is that you blindly forward data from an untrusted source while the APIs expect certain conditions to be met and if not are treated as an attempted attack.

kyoWNC commented 1 week ago

Thank you so much, I obtained a lot of important information from your reply