Investigate initial non-compliance report from TF-M

[daverodgman]

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

• psa_hash_setup
  • alg = 0x01FFFFFF
  • expect PSA_ERROR_NOT_SUPPORTED
  • returns PSA_ERROR_INVALID_ARGUMENT
• psa_cipher_finish
  • decrypt CBC_NO_PADDING (short in)
  • expect PSA_ERROR_BAD_STATE
  • returns PSA_ERROR_INVALID_ARGUMENT
• psa_key_derivation_setup
  • alg = 0x9ffffff
  • expect PSA_ERROR_INVALID_ARGUMENT
  • returns PSA_ERROR_NOT_SUPPORTED
• psa_sign_hash
  • Invalid key handle
  • expect PSA_ERROR_INVALID_HANDLE
  • returns PSA_ERROR_DOES_NOT_EXIST
• psa_raw_key_agreement
  • unknown KDF
  • expect PSA_ERROR_INVALID_ARGUMENT
  • returns PSA_ERROR_NOT_SUPPORTED
• psa_destroy_key
  • psa_copy_key - 16 byte AES
  • expect PSA_ERROR_INVALID_HANDLE
  • returns PSA_ERROR_DOES_NOT_EXIST
• psa_hash_compute
  • alg = 0x01FFFFFF
  • expect PSA_ERROR_NOT_SUPPORTED
  • returns PSA_ERROR_INVALID_ARGUMENT
• psa_hash_compare
  • alg = 0x01FFFFFF
  • expect PSA_ERROR_NOT_SUPPORTED
  • returns PSA_ERROR_INVALID_ARGUMENT

[daverodgman]

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]

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]

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]

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

[jk-arm]

@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]

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 #4162.

[gilles-peskine-arm]

• Filed the PSA_ERROR_DOES_NOT_EXIST issue as Do not return PSA_ERROR_DOES_NOT_EXIST for a non-existing key #4162.
• The rest are issues with the compliance tests being too strict.