Add TEE_ALG_RSASSA_PKCS1_V1_5 #2524

EDVTAZ · 2018-09-06T12:27:43Z

I've been working with @etienne-lms, to add asymmetrical algorithms to the
SKS proposal. While testing with OpenSSH, I came across the usage of the
CKM_RSA_PKCS mechanism with the C_Sign function, which I couldn't implement
with the current cryptographic TEE API.

This extension allows signing and verifying raw/pre-hashed data,
without specifying a hash function. It is needed for implementing
the pkcs#11 CKM_RSA_PKCS mechanism for signing and verifying in SKS.
This patch adds the extension to the API and updates parts of
libtomcrypt to a newer version, to support this algorithm.

Signed-off-by: Gabor Szekely szvgabor@gmail.com

jenswi-linaro · 2018-09-06T12:42:58Z

This PR consists of two parts, one update of LTC and one update of OP-TEE.
Please split into two commits.
How about upstreaming to LTC, is that started?
Do you have a link to a PR?

EDVTAZ · 2018-09-06T12:49:19Z

Ok, I will split it into two commits.
Regarding libtomcrypt, I just took their latest version, and copied the necessary files into optee. The only modification I made had to do with some optimisation that was added some time back in optee to ltc. I modified the copied files, so they were in line with those.

jenswi-linaro · 2018-09-06T12:57:33Z

OK, good. :-)

jforissier · 2018-09-06T13:00:27Z

Yes good indeed, it means we should take as a separate (and prerequisite) task to update LTC to the latest version...

EDVTAZ · 2018-09-07T08:20:55Z

Yes, it would be nice to update LTC altogether (judging by the timestamps in some of the files it's as old as 2007, and there have been releases since then in LTC), but as far as I can tell, that would be a very lengthy process, given how many modifications were made to it over the time in this repository compared to the original one.

etienne-lms

rephrase the commit. something like :

libtomcrypt: port LTC_PKCS_1_V1_5_NA1 from ltc v1.18.2

This change ports LTC_PKCS_1_V1_5_NA1 from libtomcrypt v1.18.2. This scheme is used for...

Chnage in ltc_provider could go in the 2nd commit.

Remove core/lib/libtomcrypt/src/pk/rsa/rsa_sign_hash.c.bak from the change.

etienne-lms · 2018-09-07T08:21:28Z

core/lib/libtomcrypt/src/tee_ltc_provider.c

@@ -13,6 +13,9 @@
 #include <string_ext.h>
 #include <string.h>
 #include <tee_api_types.h>
+#if defined(CFG_SECURE_KEY_SERVICES)


can remove the condition

etienne-lms · 2018-09-07T08:21:42Z

core/lib/libtomcrypt/src/tee_ltc_provider.c

@@ -270,6 +273,9 @@ static TEE_Result tee_algo_to_ltc_hashindex(uint32_t algo, int *ltc_hashindex)
 	case TEE_ALG_HMAC_SHA512:
 		*ltc_hashindex = find_hash("sha512");
 		break;
+#endif
+#if defined(CFG_SECURE_KEY_SERVICES)


can remove the condition, here and below.

etienne-lms

Rephrase the commit comment. Something like:

utee: support  prehash RSA sign and verify

Add TEE Core Internal API extension TEE_ALG_RSASSA_PKCS1_V1_5 to sing/verify pre-hashed ... This rely on libtomcryp LTC_PKCS_1_V1_5_NA1 ...

etienne-lms · 2018-09-07T08:28:57Z

core/tee/tee_svc_cryp.c

@@ -19,7 +19,7 @@
 #include <utee_defines.h>
 #include <util.h>
 #if defined(CFG_CRYPTO_HKDF) || defined(CFG_CRYPTO_CONCAT_KDF) || \
-	defined(CFG_CRYPTO_PBKDF2)
+	defined(CFG_CRYPTO_PBKDF2) || defined(CFG_SECURE_KEY_STORAGE)


I would prefer another CFG_. Maybe introduce CFG_CRYPTO_RSASSA to default enable in the related make script.
I even wonder if we could not simply drop the condition and always include tee_api_defines_extensions.h.

etienne-lms · 2018-09-07T08:30:15Z

core/tee/tee_svc_cryp.c

@@ -3277,6 +3277,9 @@ TEE_Result syscall_asymm_operate(unsigned long state,
 		}
 		break;

+#if defined(CFG_SECURE_KEY_SERVICES)


I would prefer another CFG_. Maybe introduce CFG_CRYPTO_RSASSA to default enable in the related make script.

jenswi-linaro · 2018-09-07T08:34:02Z

core/lib/libtomcrypt/src/pk/rsa/rsa_sign_hash.c.bak

@@ -0,0 +1,162 @@
+// SPDX-License-Identifier: BSD-2-Clause


Please remove this file

etienne-lms · 2018-09-07T08:35:26Z

Hi @EDVTAZ and thank for this PR. I think it is clean. Please try to format the commit comments as those already merged in the optee_os git tree. It helps maintenance.

@jforissier I have had a quick look a ltc update. Most changes are ok but few descriptors which requires updates in the tee_ltc_provider.c. Maybe there are other things to take care of..
The proposed changes here only syncs op-tee with libtomcrypt for this very LTC_PKCS_1_V1_5_NA1 support. Would it be ok to merge it before a libtomcrypt full resync?

jforissier · 2018-09-07T08:38:11Z

judging by the timestamps in some of the files it's as old as 2007

No, the last global sync was against a50cb361 2 years ago, and we have picked a few upstream commits since then.

jforissier · 2018-09-07T09:02:21Z

The first commit should be split further. It should contain only the LTC changes (no change to any OP-TEE files). From what I can tell, it is exactly a cherry-pick of upstream commit aa4bae5ae9a2 ("add option to do PKCS#1 v1.5 EMSA without ASN.1 around hash"), correct? If so, please mention this commit in the commit comment.

The IBART error (4003.18) has to be investigated, too.

jforissier · 2018-09-07T09:05:28Z

@etienne-lms yes, we can merge this feature without a full LTC sync.

jforissier · 2018-09-07T09:15:14Z

...and then we also want:

a new test added to xtest
some documentation in documentation/extensions.

EDVTAZ · 2018-09-10T15:00:55Z

I removed the accidentally committed backup file, and some of the #ifs. Also renamed CFG_SECURE_KEY_SERVICES to CFG_CRYPTO_RSASSA_NA1. I split the first commit into two and tried to rewrite the commit messages to be better.

Tomorrow I will investigate the IBART error and start working on an xtest test case and some documentation.

EDVTAZ · 2018-09-11T14:59:38Z

IBART error was present, because I was working with an older version of OPTEE, that didn't have this commit: e770203 . I rebased my branch, so the tests should be good now.

This test checks the TEE_ALG_RSASSA_PKCS1_V1_5 extension, that allows to sign data without including the hash OID in the signature. The test vector data is copied from one of the already present vectors, with the hash OID manually removed in the signature. Link: <OP-TEE/optee_os#2524> Signed-off-by: Gabor Szekely <szvgabor@gmail.com>

EDVTAZ · 2018-09-14T14:45:29Z

I created the xtest test case, see: OP-TEE/optee_test#309

I also found some some errors in my code while testing, which I corrected in 06f2d21

EDVTAZ · 2018-09-16T20:41:57Z

I have created the documentation for the extension, please let me know if it's all right.

jenswi-linaro · 2018-09-18T22:46:14Z

lib/libutee/include/tee_api_defines_extensions.h

+ * PKCS#1 v1.5 RSASSA pre-hashed sign/verify
+ */
+
+#define TEE_ALG_RSASSA_PKCS1_V1_5	0x70007830


This defines a value in a range that as far as I can tell is reserved for future specifications.

@jenswi-linaro indeed this is a problem. The future Internal Core API specification v1.2 (currently numbered v1.1.2.51) gets rid of the structure of algorithm identifiers (table 6-12) and defines a whole range for implementation-defined identifiers (0xF0000000-0xF0FFFFFF). A value in that range would be an option I guess, but that certainly means more invasive changes to the OP-TEE code which currently assumes the structure defined in the current spec.

@jenswi-linaro @jforissier then would 0xF0000830 be all right for this define?

@EDVTAZ I think it should be OK.

@EDVTAZ ...but 0xF0000001 would be even better.

I've tried to make this work, but a lot of macros that are used all over the place rely on the identifier's value having the above mentioned structure. Most of the time the macros are called in switch statements, so I can't insert cases for an identifier that doesn't follow the structure. I've inserted if-else statements before switch statements to get around this, but it seems I'm missing something somewhere, because it fails to work. The resulting code is also ugly and hard to read.

Could we maybe find a value that works with the current macros, or just keep the current value until the changes @jforissier mentioned get implemented?

@jforissier @EDVTAZ , just a gentle reminder about this, since it seems like the discussion suddenly stopped (a since I also think it is a useful addition).

This test checks the TEE_ALG_RSASSA_PKCS1_V1_5 extension, that allows to sign data without including the hash OID in the signature. The test vector data is copied from one of the already present vectors, with the hash OID manually removed in the signature. Link: <OP-TEE/optee_os#2524> Signed-off-by: Gabor Szekely <szvgabor@gmail.com>

EDVTAZ · 2018-11-01T23:59:40Z

Sorry this took so long, I've had a lot to do in the past month. The above two commits change the TEE_ALG_RSASSA_PKCS1_V1_5 identifier from 0x70007830 to 0xF0000830, so it is in the range of implementation-defined identifiers. This is the cleanest way I could come up with, to solve this problem without changing how TEE_ALG_GET_CLASS works. Please let me know if it's ok like this and if yes I will adjust the documentation to hold the right value as well.

jenswi-linaro

This seems a good way to approach the algorithm class problem.

jenswi-linaro · 2018-11-05T08:46:25Z

core/lib/libtomcrypt/src/pk/rsa/rsa_verify_hash.c

    return CRYPT_PK_INVALID_PADDING;
  }

-  if (padding == LTC_PKCS_1_PSS) {
+  if (padding != LTC_PKCS_1_V1_5_NA1) {


So now we're testing the hash for LTC_PKCS_1_V1_5 too. A bugfix?

These changes were taken from the libtomcrypt repo: libtom/libtomcrypt@aa4bae5#diff-f8abc0f8fcc24d9959cf4792e56ccce5L58 I think it is intentional, although it is not mentioned in the commit message.

jenswi-linaro · 2018-11-05T08:52:04Z

core/lib/libtomcrypt/src/pk/rsa/rsa_verify_hash.c

+      /* test OID */
+      if ((reallen == outlen) &&
+          (digestinfo[0].size == hash_descriptor[hash_idx]->OIDlen) &&
+        (XMEMCMP(digestinfo[0].data, hash_descriptor[hash_idx]->OID, sizeof(unsigned long) * hash_descriptor[hash_idx]->OIDlen) == 0) &&


Why change from XMEM_NEQ() to XMEMCMP() (same for the other lines below)?

This change was introduced here: libtom/libtomcrypt@e9c90e7#diff-f8abc0f8fcc24d9959cf4792e56ccce5

jenswi-linaro · 2018-11-05T08:56:44Z

core/tee/tee_svc_cryp.c

@@ -2062,6 +2069,9 @@ TEE_Result syscall_cryp_state_alloc(unsigned long algo, unsigned long mode,
 		break;
 	case TEE_OPERATION_ASYMMETRIC_CIPHER:
 	case TEE_OPERATION_ASYMMETRIC_SIGNATURE:
+#ifdef CFG_CRYPTO_RSASSA_NA1
+RSASSA_NA1:


Labels should use lower case letters.
I think it's possible to the the __maybe_unused attribute on labels too, if that works please remove the ifdef here.

jenswi-linaro · 2018-11-05T09:03:51Z

core/tee/tee_svc_cryp.c

-		if (data_len != hash_size) {
-			res = TEE_ERROR_BAD_PARAMETERS;
-			break;
+#ifdef CFG_CRYPTO_RSASSA_NA1


This ifdef and the one below clutters the code. It seems it would be harmless to remove them. If you agree please remove them, else we'll need to find some other way to avoid having them here.

If I'm not mistaken, TEE_ALG_RSASSA_PKCS1_V1_5 is included here in this file, so to remove the ifdef in question, we would need to remove all the ifdefs that guard the include that I linked.

This ifdeffing isn't OK so it will need to be fixed. I'm OK with unconditionally including tee_api_defines_extensions.h, another option is to put in a helper function cs->algo != TEE_ALG_RSASSA_PKCS1_V1_5 that would return true if CFG_CRYPTO_RSASSA_NA1 isn't defined. However, I'd prefer the less complicated solution.

I removed the guards

etienne-lms

hello @EDVTAZ,
Could you rebase your commits on latest HEAD of the master branch?
Also, since your current patch serie is a collection of changes and late fixup, could you squash the fixup commits into their related change commit? to make a well formed patch serie.

EDVTAZ · 2018-11-11T00:00:45Z

I've rebased and squashed the commits that belong together, I hope it's all right like this.
I can't tell why the travis-ci fails, xtest passes locally.

jbech-linaro · 2018-11-11T09:45:09Z

@EDVTAZ , Travis failed because it hit the time limit. I don't think we have to bother about that. IBART passes which indicates that things are fine.

jenswi-linaro · 2018-11-12T07:31:38Z

For "libtomcrypt: port LTC_PKCS_1_V1_5_NA1 from ltc v1.18.2" please apply:
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
For the others:
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

This change ports LTC_PKCS_1_V1_5_NA1 from libtomcrypt v1.18.2. This scheme allows to do PKCS#1 v1.5 EMSA without ASN.1 around the hash. It is used for implementing the pkcs#11 CKM_RSA_PKCS mechanism for signing and verifying in SKS. This commit is a cherry pick of aa4bae5ae9a2 from the libtomcrypt repository. Link: <libtom/libtomcrypt@aa4bae5ae9a2> Acked-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Gabor Szekely <szvgabor@gmail.com>

This change integrates the LTC_PKCS_1_V1_5_NA1 into OPTEE as an extension as TEE_ALG_RSASSA_PKCS1_V1_5. This scheme allows to do PKCS#1 v1.5 EMSA without ASN.1 around the hash. It is used for implementing the pkcs#11 CKM_RSA_PKCS mechanism for signing and verifying in SKS. Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Gabor Szekely <szvgabor@gmail.com>

Add TEE Core Internal API extension TEE_ALG_RSASSA_PKCS1_V1_5 to sign/verify pre-hashed PKCS#1 v1.5 EMSA without ASN.1 around the hash. This relies on libtomcrypt LTC_PKCS_1_V1_5_NA1. The extension can be turned on with CFG_CRYPTO_RSASSA_NA1. Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Gabor Szekely <szvgabor@gmail.com>

Add documentation for the the TEE_ALG_RSASSA_V1_5 extension. Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Gabor Szekely <szvgabor@gmail.com>

etienne-lms · 2018-11-12T10:58:12Z

Look ok to me, just a late comment:
CFG_CRYPTO_RSASSA_NA1 is introduced in 2nd change ("core: crypto: add TEE_ALG_RSASSA_PKCS1_V1_5") but described in the comment of the 3rd change ("utee: support prehashed RSA sign/ver without ASN.1"). This is not a big issue.
Yet, a comment related to CFG_CRYPTO_RSASSA_NA1: can we add a change to default enable the feature (i.e CFG_CRYPTO_RSASSA_NA1=y in mk/config.mk). I see no reason to no support this extension in the default configuration.

This test checks the TEE_ALG_RSASSA_PKCS1_V1_5 extension, that allows to sign data without including the hash OID in the signature. The test vector data is copied from one of the already present vectors, with the hash OID manually removed in the signature. Link: <OP-TEE/optee_os#2524> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Gabor Szekely <szvgabor@gmail.com>

EDVTAZ · 2018-11-13T11:05:01Z

I have added CFG_CRYPTO_RSASSA_NA1 to the default config.

jforissier · 2018-11-14T09:11:08Z

For "mk/config.mk: default enable CFG_CRYPTO_RSASSA_NA1":
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

Enable the TEE_ALG_RSASSA_PKCS1_V1_5 extension by default. Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Gabor Szekely <szvgabor@gmail.com>

This test checks the TEE_ALG_RSASSA_PKCS1_V1_5 extension, that allows to sign data without including the hash OID in the signature. The test vector data is copied from one of the already present vectors, with the hash OID manually removed in the signature. Link: <OP-TEE/optee_os#2524> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Signed-off-by: Gabor Szekely <szvgabor@gmail.com>

EDVTAZ force-pushed the RSA_extension branch from 74d71e6 to 1bd6b44 Compare September 6, 2018 12:37

EDVTAZ force-pushed the RSA_extension branch 2 times, most recently from ab2c007 to e9cbd3c Compare September 7, 2018 08:15

etienne-lms reviewed Sep 7, 2018

View reviewed changes

jenswi-linaro reviewed Sep 7, 2018

View reviewed changes

EDVTAZ force-pushed the RSA_extension branch from e9cbd3c to e3e7e10 Compare September 10, 2018 15:00

EDVTAZ force-pushed the RSA_extension branch from e3e7e10 to 6ecc5d0 Compare September 11, 2018 14:48

EDVTAZ force-pushed the RSA_extension branch from 6ecc5d0 to ab54dfc Compare September 14, 2018 14:39

EDVTAZ mentioned this pull request Sep 14, 2018

regression: 4006: add test for RSASSA no ASN.1 OP-TEE/optee_test#309

Merged

EDVTAZ force-pushed the RSA_extension branch from ab54dfc to d63afdc Compare September 16, 2018 20:38

EDVTAZ force-pushed the RSA_extension branch from d63afdc to 66ea0db Compare September 16, 2018 22:32

jenswi-linaro reviewed Sep 18, 2018

View reviewed changes

jenswi-linaro reviewed Nov 5, 2018

View reviewed changes

EDVTAZ force-pushed the RSA_extension branch from 595c869 to 78d2cf2 Compare November 9, 2018 15:26

etienne-lms reviewed Nov 10, 2018

View reviewed changes

EDVTAZ force-pushed the RSA_extension branch from b77e96b to 52f6493 Compare November 10, 2018 22:48

EDVTAZ added 4 commits November 12, 2018 09:29

documentation: extensions: add TEE_ALG_RSASSA_V1_5

82711c5

Add documentation for the the TEE_ALG_RSASSA_V1_5 extension. Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Signed-off-by: Gabor Szekely <szvgabor@gmail.com>

EDVTAZ force-pushed the RSA_extension branch from 52f6493 to 82711c5 Compare November 12, 2018 08:32

EDVTAZ force-pushed the RSA_extension branch from 3f41ee4 to c72d18a Compare November 13, 2018 09:05

mk/config.mk: default enable CFG_CRYPTO_RSASSA_NA1

7b4c58b

Enable the TEE_ALG_RSASSA_PKCS1_V1_5 extension by default. Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Signed-off-by: Gabor Szekely <szvgabor@gmail.com>

EDVTAZ force-pushed the RSA_extension branch from c72d18a to 7b4c58b Compare November 14, 2018 12:07

jforissier merged commit 017dfaf into OP-TEE:master Nov 15, 2018

Add TEE_ALG_RSASSA_PKCS1_V1_5 #2524

Add TEE_ALG_RSASSA_PKCS1_V1_5 #2524

Conversation

EDVTAZ commented Sep 6, 2018

jenswi-linaro commented Sep 6, 2018

EDVTAZ commented Sep 6, 2018

jenswi-linaro commented Sep 6, 2018

jforissier commented Sep 6, 2018

EDVTAZ commented Sep 7, 2018 • edited

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms commented Sep 7, 2018

jforissier commented Sep 7, 2018

jforissier commented Sep 7, 2018 • edited

jforissier commented Sep 7, 2018

jforissier commented Sep 7, 2018

EDVTAZ commented Sep 10, 2018

EDVTAZ commented Sep 11, 2018

EDVTAZ commented Sep 14, 2018

EDVTAZ commented Sep 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EDVTAZ commented Nov 1, 2018

jenswi-linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

EDVTAZ commented Nov 11, 2018

jbech-linaro commented Nov 11, 2018

jenswi-linaro commented Nov 12, 2018

etienne-lms commented Nov 12, 2018

EDVTAZ commented Nov 13, 2018

jforissier commented Nov 14, 2018

EDVTAZ commented Sep 7, 2018 •

edited

jforissier commented Sep 7, 2018 •

edited