search

Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.54k stars 2.6k forks source link

Investigate initial non-compliance report from TF-M #4152

Closed daverodgman closed 3 years ago

daverodgman commented 3 years ago

TF-M report the following discrepancies - investigate & fix where a fix is needed

daverodgman commented 3 years ago

From a quick read of the spec, there may be some non-compliance items here:

psa_sign_hash returns PSA_ERROR_DOES_NOT_EXIST for an invalid key handle. Mbed TLS appears to be out-of-spec here. https://armmbed.github.io/mbed-crypto/html/api/ops/sign.html#c.psa_sign_hash

psa_destroy_key returns PSA_ERROR_DOES_NOT_EXIST for a copied key (?). That doesn’t seem to be a specified return value - Mbed TLS seems out of spec. https://armmbed.github.io/mbed-crypto/html/api/keys/management.html#c.psa_destroy_key

psa_hash_compare returns PSA_ERROR_INVALID_ARGUMENT for an algorithm which isn’t a hash. This looks out of spec, it looks to me that we should return PSA_ERROR_NOT_SUPPORTED. https://armmbed.github.io/mbed-crypto/html/api/ops/hashes.html#c.psa_hash_compare

psa_hash_compute returns PSA_ERROR_INVALID_ARGUMENT for an algorithm which isn’t a hash. Agree that PSA_ERROR_NOT_SUPPORTED looks preferrable here, but I’m not sure PSA_ERROR_INVALID_ARGUMENT is actually wrong. https://armmbed.github.io/mbed-crypto/html/api/ops/hashes.html#c.psa_hash_compute

Mbed TLS is compliant:

psa_key_derivation_setup returns PSA_ERROR_NOT_SUPPORTED for a non-key-derivation-algorithm. The spec says it can return PSA_ERROR_INVALID_ARGUMENT if alg is not a key derivation algorithm, or PSA_ERROR_NOT_SUPPORTED if alg is not supported or is not a key derivation algorithm. It’s not specified which is preferred, so I think Mbed TLS is compliant. https://armmbed.github.io/mbed-crypto/html/api/ops/kdf.html#c.psa_key_derivation_setup

psa_hash_setup returns PSA_ERROR_INVALID_ARGUMENT if alg is not a hash algorithm. Mbed TLS seems correct. https://armmbed.github.io/mbed-crypto/html/api/ops/hashes.html#c.psa_hash_setup

psa_cipher_finish returns PSA_ERROR_INVALID_ARGUMENT for too short input. The spec says this is correct if “The total input size passed to this operation is not valid for this particular algorithm. For example, the algorithm is a based on block cipher and requires a whole number of blocks, but the total input size is not a multiple of the block size.”. Mbed TLS seems correct. https://armmbed.github.io/mbed-crypto/html/api/ops/ciphers.html#c.psa_cipher_finish

psa_raw_key_agreement returns PSA_ERROR_NOT_SUPPORTED for an unknown KDF. Spec says this is for “alg is not a supported key agreement algorithm”. Mbed TLS seems correct. https://armmbed.github.io/mbed-crypto/html/api/ops/ka.html#c.psa_raw_key_agreement

gilles-peskine-arm commented 3 years ago

In the NOT_SUPPORTED vs INVALID_ARGUMENT cases, either value is correct. The implementation can return PSA_ERROR_NOT_SUPPORTED if the algorithm is “not recognized” or PSA_ERROR_INVALID_ARGUMENT if the algorithm is “recognized and identified as not valid”. What the implementation recognizes is implementation-defined.

For psa_cipher_finish with “decrypt CBC_NO_PADDING (short in)” (assuming this means the input is empty or not a whole number of blocks), PSA_ERROR_BAD_STATE is definitely incorrect: this can only happen with an improper sequence of function calls or with corrupted memory objects. PSA_ERROR_INVALID_ARGUMENT is correct.

For PSA_ERROR_INVALID_HANDLE vs PSA_ERROR_DOES_NOT_EXIST, the specification is unclear. There was a time when key identifiers and key handles were separate concepts, and INVALID_HANDLE only concerned handles whereas DOES_NOT_EXIST only concerned key identifiers. Then handles were replaced by key identifiers with temporary validity. The space of possible key identifiers is divided into two ranges: one where the application picks the value and one where the implementation picks the value. If a key identifier is within the application range and does not correspond to an existing key, DOES_NOT_EXIST is unambiguously correct. But within the implementation range, it's unclear when an implementation should return INVALID_HANDLE and when it should return DOES_NOT_EXIST. This could be implementation-dependent, other than always returning INVALID_HANDLE for 0 (because 0 is never valid).

gilles-peskine-arm commented 3 years ago

In summary, Mbed TLS is compliant, except maybe for returning PSA_ERROR_DOES_NOT_EXIST instead of PSA_ERROR_INVALID_HANDLE, but that would depend on what value the tests use for “Invalid key handle”.

jk-arm commented 3 years ago

about the psa_destroy_key() the spec return value does not have the PSA_ERROR_DOES_NOT_EXISTS

image

jk-arm commented 3 years ago

@gilles-peskine-arm is there a reason for this change, the test input based on this check https://github.com/ARMmbed/mbedtls/blob/v2.23.0/include/psa/crypto_values.h#L614

https://github.com/ARMmbed/mbedtls/blob/v2.25.0/include/psa/crypto_values.h#L612

gilles-peskine-arm commented 3 years ago

Looking back to prior discussions about psa_destroy_key and PSA_ERROR_DOES_NOT_EXIST, there was a decision to completely unify “invalid key id value” and “non-existent key id that could be created” under the umbrella of PSA_ERROR_INVALID_HANDLE. So Mbed TLS should not use PSA_ERROR_DOES_NOT_EXIST anymore except when an application uses the legacy psa_open_key interface (and in the ITS layer, and possibly in the driver interface). I agree that this case is a bug (I now see that it doesn't depend on the value) and I've filed it as https://github.com/ARMmbed/mbedtls/issues/4162.

gilles-peskine-arm commented 3 years ago