crypto: don't check hash size when the main algorithm is ECDSA #1497
Conversation
core/tee/tee_svc_cryp.c
Outdated
| goto out; | ||
|
|
||
| if (TEE_ALG_GET_MAIN_ALG(cs->algo) == TEE_MAIN_ALGO_DSA) { | ||
| if (TEE_ALG_GET_MAIN_ALG(cs->algo) != TEE_MAIN_ALGO_ECDSA) { |
There was a problem hiding this comment.
I think this function would be easier to read if what's inside this block was copied/split into corresponding places for case TEE_MAIN_ALGO_RSA: and case TEE_MAIN_ALGO_DSA:. I think we can live with a duplicated call to tee_hash_get_digest_size().
The reason it looks like it does today is because TEE_MAIN_ALGO_ECDSA was added afterwards.
|
BTW: |
|
Looks ok to me (with or without Jens' comment) @HenrikRosenquistAndersson, if you wish, you can provided your mail to @jforissier so he can add a tag |
|
I don't know how to directly send my email to @jforissier but it is Henrik.Andersson@se.bosch.com |
|
@HenrikRosenquistAndersson thanks! |
core/tee/tee_svc_cryp.c
Outdated
| hash_algo = TEE_DIGEST_HASH_TO_ALGO(cs->algo); | ||
| res = tee_hash_get_digest_size(hash_algo, &hash_size); | ||
| if (res != TEE_SUCCESS) | ||
| goto out; |
There was a problem hiding this comment.
Since we're in a switch() we can break on error instead, this is done in some cases but not all here.
I'd recommend breaking on error instead of goto out.
|
With my comment addressed: |
|
Travis/checkpatch is a bit unhappy about the length of some comments. Other than that: |
|
The checkpatch warnings (>80 characters) are for the first patch only, the comment block is moved after that so the overall diff is OK actually. |
syscall_asymm_verify() (and therefore TEE_AsymmetricVerifyDigest()) incorrectly assumes that the hash algorithm is SHA1 when the main algorithm is ECDSA, and will panic the TA if the hash size is not set accordingly. This behavior does not comply with the TEE Internal Core API v1.1, which states: "Where a hash algorithm is specified in the algorithm, digestLen SHALL be equal to the digest length of this hash algorithm". For TEE_ALG_ECDSA_P192, TEE_ALG_ECDSA_P224, TEE_ALG_ECDSA_P256, TEE_ALG_ECDSA_P384 and TEE_ALG_ECDSA_P521, no hash algorithm is specified, and so we must not restrict the hash size to any specific value. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reported-by: Henrik Andersson <Henrik.Andersson@se.bosch.com> Acked-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Joakim Bech <joakim.bech@linaro.org>
syscall_asymm_verify() (and therefore TEE_AsymmetricVerifyDigest())
incorrectly assumes that the hash algorithm is SHA1 when the main
algorithm is ECDSA, and will panic the TA if the hash size is not set
accordingly. This behavior does not comply with the TEE Internal Core
API v1.1, which states:
"Where a hash algorithm is specified in the algorithm, digestLen SHALL
be equal to the digest length of this hash algorithm".
For TEE_ALG_ECDSA_P192, TEE_ALG_ECDSA_P224, TEE_ALG_ECDSA_P256,
TEE_ALG_ECDSA_P384 and TEE_ALG_ECDSA_P521, no hash algorithm is
specified, and so we must not restrict the hash size to any specific
value.
Signed-off-by: Jerome Forissier jerome.forissier@linaro.org