digest size not checked when doing a ECDSA sign operation

[HenrikRosenquistAndersson]

When executing a ECDSA sign operation (TEE_AsymmetricSignDiges) you are suppose to pass in the sha1 hash of the data to sign.
The length of the hash is not checked in syscall_asymm_operate() and you receive a SUCCESS back with a signature even if you pass in an invalid hash length.
If you then pass in the same hash and invalid hash length and the received signature in TEE_AsymmetricVerifyDigest you get an invalid argument error back. (syscall_asymm_verify() checks that the hash length is valid.)

By the way. The latest Global Platform APIs support ECDSA sign/verify operations with other digests than sha1. Any plans to add support for this?

[jforissier]

Hi @HenrikRosenquistAndersson,

Thanks for reporting this, and sorry for the late reply.

OP-TEE implements the GlobalPlatform TEE Internal Core API Specification v1.1. My understanding of the spec is that neither TEE_AsymmetricSignDigest(), nor TEE_AsymmetricVerifyDigest() should check the hash length when the main algorithm is ECDSA. If I'm not mistaken, the behavior of TEE_AsymmetricSignDigest() is correct, and TEE_AsymmetricVerifyDigest() should be changed as follows: jforissier@ecdsa-sign [passes xtest, but not further tested].

What do you think of this patch? Am I missing something? Should we still restrict the hash sizes to the standard ones or is it OK to accept any value?

Thanks.

[HenrikRosenquistAndersson]

Yes you are right I have been looking at the v1.1.1 specification where the hash algorithm is described in the ECDSA algorithm.

So yes in v1.1 TEE_AsymmetricSignDigest() and TEE_AsymmetricVerifyDigest() shouldn't check the hash length for ECDSA because the hash algorithm isn't described. But for all other algorithms RSASSA, DSA it should be checked because the hash algorithm is mentioned there.

The code change looks fine in that sense.

I found one side comment when looking at the changed code
I'm don't really know DSA but the comment doesn't seem to match the code.

	/*
	 * Depending on the DSA algorithm (NIST), the digital signature
	 * output size may be truncated to the size of a key pair
	 * (Q prime size). Q prime size must be less or equal than the
	 * hash output length of the hash algorithm involved.
	 */
	if (data_len > hash_size) {
		res = TEE_ERROR_BAD_PARAMETERS;
		goto out;
	}

From reading the comment the DSA signature length could be less than the hash_size.
But that's not what is checked if I understand the code correctly. Isn't data_len just the size of the hashed data which has nothing to do with DSA and that length should match the hash_size? The parameter sig_len on the other hand is the pre-calculated DSA signature that should be verified and that doesn't have to match the hash length?

I was just curious since I though it looked strange and wanted to note that even if I don't know DSA.
It's getting late so I might have misunderstood something.

[jforissier]

#1497

As for DSA... I also have a hard time understanding the link between the comment and the code, but removing the special case (and just testing for data_len != hash_size in all cases) seems to break xtest -l 15 4006:

o regression_4006.151 Asym Crypto case 150 algo 0x70004131 line 3225
ERROR:   USER-TA: Panic 0xffff0006
ERROR:   TEE-CORE: TA panicked with code 0xffff0006 usr_sp 0x40000ec0 usr_lr 0x40008f37
/home/jerome/work/hikey_optee/optee_test/host/xtest/regression_4000.c:779: ret_orig has an unexpected value: 0x3 = TEEC_ORIGIN_TEE, expected 0x4 = TEEC_ORIGIN_TRUSTED_APP
/home/jerome/work/hikey_optee/optee_test/host/xtest/regression_4000.c:4102: ta_crypt_cmd_asymmetric_verify(c, &session, op, algo_params, num_algo_params, ptx_hash, ptx_hash_size, tv->ctx, tv->ctx_len) has an unexpected value: 0xffff3024 = TEE_ERROR_TARGET_DEAD, expected 0x0 = TEEC_SUCCESS
  regression_4006.151 FAILED

I need to take a closer look.

[jbech-linaro]

@jforissier did you manage to follow up on the DSA?

[jforissier]

No, I didn't.

[github-actions]

This issue has been marked as a stale issue because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.