New function psa_copy_key #20

gilles-peskine-arm · 2019-01-19T12:59:49Z

Copy a key from one slot to another.
Implemented and tested.

Copy a key from one slot to another. Implemented and smoke-tested.

Split the testing into tests that exercise policies in test_suite_psa_crypto and tests that exercise slot content (slot states, key material) in test_suite_psa_crypto_slot_management. Test various cases of source and target policies with and without wildcards. Missing: testing of the policy constraint on psa_copy_key itself. Test several key types (raw data, AES, RSA). Test with the source or target being persistent. Add failure tests (incompatible policies, source slot empty, target slot occupied).

Test a few cases. The logic to combine the constraint is similar to the logic to combine the source and target, so it's ok to have less parameter domain coverage for constraints.

Some parts of the library don't support it, such as RSA PKCS#1v1.5 signature.

gilles-peskine-arm · 2019-01-28T14:45:42Z

Rebased again to pass depends-pkalgs.pl and depends-hashes.pl.

Patater · 2019-01-28T16:43:22Z

include/psa/crypto.h

+ * Copy key material from one location to another.
+ *
+ * This function is primarily useful to copy a key from one lifetime
+ * to another. The target key retains its lifetime and location.


This sentence is a bit confusing to me, perhaps because I'm not fully aware of what "lifetime" means in the context of key material in our library.

The function brief description says "Copy key material from one location to another," so this means that this function affects the location of the key material (i.e. location is the free variable). Then this description says the function is useful to copy between lifetimes (i.e. lifetime is the free variable). Then this says the target key keeps the same lifetime and location (i.e. neither lifetime nor location are changed).

The source key would also keep the same lifetime and location, wouldn't it?

Does the target key retain its policy if one was set previously? Is it effectively set to the bitwise-and of the previous target policy?

“Lifetime” is one of those words for which I'm looking for a better word. It roughly means “storage area”. I've toyed with calling it “storage area”, but there are cases where the two concepts don't correspond. Semantically, keys have the same lifetime if they're automatically erased by the same event: volatile keys are erased by a device shutdown, persistent keys by an application uninstallation or device reset, keys in a secure element when the secure element is reset, …

What do you mean by “free variable” here?

The source key is not modified in any way, so it keeps the same lifetime and location, no matter what you understand by “lifetime” and “location”.

The resulting policy for the target key is explained a couple of paragraphs further down.

What do you mean by “free variable” here?

The variable that changes upon use of this function.

I tweaked the wording, is it better?

Patater · 2019-01-28T16:43:38Z

include/psa/crypto.h

+ * to another. The target key retains its lifetime and location.
+ *
+ * In an implementation where slots have different ownerships,
+ * this functin may be used to share a key with a different party,


s/functin/function

Patater · 2019-01-28T16:45:56Z

include/psa/crypto.h

+ * The resulting key may only be used in a way that conforms to all
+ * three of: the policy of the source key, the policy previously set
+ * on the target, and the \p constraint parameter passed when calling
+ * this function.


Do we have a test to ensure that passing a lax constraint parameter will not make the effective key policy more lax than the source/target policy?

Also, API design question: if we can set the policy of the target key before doing a copy, why do we need to additionally support an additional constraint here? Is this new constraint applied to and saves as the new target key policy the copy is complete?

Regarding tests, see copy_key_policy and copy_fail in test_suite_psa_crypto.data.

The reason for the seemingly redundant constraint parameter is that it allows the copier to atomically constraint the policy. This is necessary for the use case described above: the owner of a key shares it with a recipient, and can't trust the recipient to have set a policy that the owner of the original key agrees to.

Patater · 2019-02-05T14:22:42Z

library/psa_crypto.c

+ * \param[in] constraint    The policy constraint to apply.
+ *
+ * \retval #PSA_SUCCESS
+ *         \c *policy contains the intersection of the original value of


Prefer \p over \c here, since we document a parameter here. This may require removing the *, which would be less surprising to readers, IMO.

I used \c because the argument isn't just a parameter name. The sentence is about the structure that policy points to, not about policy itself, so \p policy would be confusing (at least, it would confuse me, that's why I wrote *policy.

Patater · 2019-02-05T14:32:50Z

library/psa_crypto.c

+    size_t length;
+
+    buffer_size = PSA_KEY_EXPORT_MAX_SIZE( source->type,
+                                           psa_get_key_bits( source ) );


Is there not a way to get the actual exported size for a particular handle?

No. It would be convenient for applications but it would require extra code in the implementation.

It's a trade off between some additional code size in the implementation and potential RAM savings in some cases, I suppose.

Patater · 2019-02-05T14:38:59Z

library/psa_crypto.c

+    status = psa_restrict_key_policy( &new_policy, &source_slot->policy );
+    if( status != PSA_SUCCESS )
+        return( status );
+    if( constraint != NULL )


Should we always require constraint to make misuse of the API more difficult? What's the risk of someone passing in NULL here when they actually need to provide a non-null constraint.

constraint is mostly redundant with setting the policy on the target handle. If your use case isn't one where you can set the policy on the target handle, it looks useless. That's why it's optional.

I don't see any risk of misuse here because there's already a policy on the target slot. Setting a policy constraint during the copy is only useful when this function is called by a key sharing API where the sharer (who owns the source slot) can't otherwise atomically set the policy on the target slot. The policy argument to the sharing function should not be optional.

I considered making constraint override the policy on the target handle, but it seemed more likely to be error-prone. The normal sequence is 1: set policy, 2: create key material. If creating the key material with psa_copy_key overrode the policy, that would be surprising, and yet would likely go undetected because the resulting key would be more permissive than intended.

Patater · 2019-02-05T14:59:17Z

tests/suites/test_suite_psa_crypto.data

+
+Copy fail: RSA, ANY_HASH is not meaningful with OAEP
+depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_PK_WRITE_C:MBEDTLS_RSA_C:MBEDTLS_SHA256_C
+copy_fail:PSA_KEY_USAGE_EXPORT:PSA_ALG_RSA_OAEP(PSA_ALG_ANY_HASH):PSA_KEY_TYPE_RSA_KEYPAIR:"3082025e02010002818100af057d396ee84fb75fdbb5c2b13c7fe5a654aa8aa2470b541ee1feb0b12d25c79711531249e1129628042dbbb6c120d1443524ef4c0e6e1d8956eeb2077af12349ddeee54483bc06c2c61948cd02b202e796aebd94d3a7cbf859c2c1819c324cb82b9cd34ede263a2abffe4733f077869e8660f7d6834da53d690ef7985f6bc3020301000102818100874bf0ffc2f2a71d14671ddd0171c954d7fdbf50281e4f6d99ea0e1ebcf82faa58e7b595ffb293d1abe17f110b37c48cc0f36c37e84d876621d327f64bbe08457d3ec4098ba2fa0a319fba411c2841ed7be83196a8cdf9daa5d00694bc335fc4c32217fe0488bce9cb7202e59468b1ead119000477db2ca797fac19eda3f58c1024100e2ab760841bb9d30a81d222de1eb7381d82214407f1b975cbbfe4e1a9467fd98adbd78f607836ca5be1928b9d160d97fd45c12d6b52e2c9871a174c66b488113024100c5ab27602159ae7d6f20c3c2ee851e46dc112e689e28d5fcbbf990a99ef8a90b8bb44fd36467e7fc1789ceb663abda338652c3c73f111774902e840565927091024100b6cdbd354f7df579a63b48b3643e353b84898777b48b15f94e0bfc0567a6ae5911d57ad6409cf7647bf96264e9bd87eb95e263b7110b9a1f9f94acced0fafa4d024071195eec37e8d257decfc672b07ae639f10cbb9b0c739d0c809968d644a94e3fd6ed9287077a14583f379058f76a8aecd43c62dc8c0f41766650d725275ac4a1024100bb32d133edc2e048d463388b7be9cb4be29f4b6250be603e70e3647501c97ddde20a4e71be95fd5e71784e25aca4baf25be5738aae59bbfe1c997781447a2b24":PSA_KEY_USAGE_EXPORT:PSA_ALG_RSA_OAEP(PSA_ALG_SHA_256):-1:-1:PSA_ERROR_INVALID_ARGUMENT


Is the key able to get created successfully with this source policy? Maybe making the target policy instead have the policy of PSA_ALG_RSA_OAEP(PSA_ALG_ANY_HASH) would be better to ensure this test isn't passing on accident due to the import with a silly policy failing.

Looks like it is possible to import with this policy, which is interesting, because otherwise the copy_fail() test would fail at PSA_ASSERT(psa_set_key_policy(...)) or PSA_ASSERT(psa_import_key(...))

I don't understand your comment. This test won't pass if importing the key fails.

Should importing a key to a slot with a policy PSA_ALG_RSA_OAEP(PSA_ALG_ANY_HASH) fail?

If this policy is invalid, why don't we fail at import?

If this is a bug and we fix it, then this test wouldn't actually be testing that copy fails with PSA_ALG_RSA_OAEP(PSA_ALG_ANY_HASH). Is it a bug or working as intended?

The specification doesn't define “invalid policy”. It only says when a policy allows an operation. You can define “invalid policy” as “policy that does not allow any operation”, but it's a bit tricky for at least two reasons:

It makes sense to have a key that does not allow any operation, but can be exported. For example, to store a certificate next to the corresponding private key.

A policy isn't necessarily invalid in isolation: its validity depends on the key type.

psa_copy_key needs to calculate the intersection of two (or three) policies. There is no way to represent the intersection of two policies when it's empty, so it returns an error if the resulting policy would be empty. (Actually there's a way: use 0 for the algorithm. But since it would be surprising to have the copy of a usable key succeed but produce an unusable key, the function returns an error. This doesn't cost any code size since it's just a choice between alg = 0; return SUCCESS and return ERROR.)

Our implementation doesn't validate policies so as not to pay the code size. It wouldn't even be possible in general because if a key is in a secure element, it may not be possible to figure out whether the secure element can perform a certain operation without trying it.

If we change our implementation to reject PSA_ALG_RSA_OAEP(PSA_ALG_ANY_HASH) at set_key_policy time or at import time then this test would fail and we'd need to remove it (because there would no longer be a way to test what it's trying to test, which is that a policy of OAEP(ANY_HASH) does not allow OAEP(SHA_256)).

If we don't think it's a bug that we can import keys with PSA_ALG_RSA_OAEP(PSA_ALG_ANY_HASH), OK, no action to take here.

Patater · 2019-02-05T15:09:46Z

tests/suites/test_suite_psa_crypto.function

+        ASSERT_COMPARE( material->x, material->len,
+                        export_buffer, length );
+    }
+    if( ! exercise_key( target_handle, expected_usage, expected_alg ) )


Should this be TEST_ASSERT(exercise_key(...))?

If we exit this function without failing an assert, it's a pass. Was the intention to pass the test whether or not exercise_key() works and just avoid calling psa_close_key() when the exercise fails?

No, exercise_key is meant to be used this way. It returns 1 on success and 0 on failure, and calls test_fail on failure. Hence the usage idiom: if( ! exercise_key(…) ) goto exit;.

I do see that there's one place where it's misused. I'll fix and add a comment.

Calling TEST_ASSERT here would cause test_fail to be called a second time and so it would report an error at the point where exercise_key is called instead of the point where the test failure occurred. It's a design limitation in our test helper code that it doesn't really support failing a test inside an auxiliary function. Improving this is well out of scope of this PR.

OK, I didn't catch that exercise_key called test_fail() in all failing paths. This is fine.

Patater · 2019-02-05T15:19:53Z

tests/suites/test_suite_psa_crypto.data

+
+Copy fail: RSA, incompatible constraint
+depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_PK_WRITE_C:MBEDTLS_RSA_C:MBEDTLS_SHA256_C
+copy_fail:PSA_KEY_USAGE_EXPORT:PSA_ALG_RSA_PKCS1V15_SIGN(PSA_ALG_ANY_HASH):PSA_KEY_TYPE_RSA_KEYPAIR:"3082025e02010002818100af057d396ee84fb75fdbb5c2b13c7fe5a654aa8aa2470b541ee1feb0b12d25c79711531249e1129628042dbbb6c120d1443524ef4c0e6e1d8956eeb2077af12349ddeee54483bc06c2c61948cd02b202e796aebd94d3a7cbf859c2c1819c324cb82b9cd34ede263a2abffe4733f077869e8660f7d6834da53d690ef7985f6bc3020301000102818100874bf0ffc2f2a71d14671ddd0171c954d7fdbf50281e4f6d99ea0e1ebcf82faa58e7b595ffb293d1abe17f110b37c48cc0f36c37e84d876621d327f64bbe08457d3ec4098ba2fa0a319fba411c2841ed7be83196a8cdf9daa5d00694bc335fc4c32217fe0488bce9cb7202e59468b1ead119000477db2ca797fac19eda3f58c1024100e2ab760841bb9d30a81d222de1eb7381d82214407f1b975cbbfe4e1a9467fd98adbd78f607836ca5be1928b9d160d97fd45c12d6b52e2c9871a174c66b488113024100c5ab27602159ae7d6f20c3c2ee851e46dc112e689e28d5fcbbf990a99ef8a90b8bb44fd36467e7fc1789ceb663abda338652c3c73f111774902e840565927091024100b6cdbd354f7df579a63b48b3643e353b84898777b48b15f94e0bfc0567a6ae5911d57ad6409cf7647bf96264e9bd87eb95e263b7110b9a1f9f94acced0fafa4d024071195eec37e8d257decfc672b07ae639f10cbb9b0c739d0c809968d644a94e3fd6ed9287077a14583f379058f76a8aecd43c62dc8c0f41766650d725275ac4a1024100bb32d133edc2e048d463388b7be9cb4be29f4b6250be603e70e3647501c97ddde20a4e71be95fd5e71784e25aca4baf25be5738aae59bbfe1c997781447a2b24":PSA_KEY_USAGE_EXPORT:PSA_ALG_RSA_PKCS1V15_SIGN(PSA_ALG_ANY_HASH):PSA_KEY_USAGE_EXPORT:PSA_ALG_RSA_PSS(PSA_ALG_ANY_HASH):PSA_ERROR_INVALID_ARGUMENT


Most tests seem to be using exportable keys. Shan't we add more tests for various combinations of exportable and non-exportable keys? We should test that copying to a non-exportable slot works and that the result is non-exportable, and that copying from a non-exportable key to an exportable target slot doesn't make the target key exportable. It's possible we get this test coverage though some of the existing tests (but I couldn't see that.)

There has to be some tests with exportable keys because that's the only way to make sure that the copied key is identical. (We can also use the key in a deterministic way and make sure that we get the same result as the original key. which would give the same level of confidence if the use exercises the key sufficiently, but it's a lot more work to test.)

“copying to a non-exportable slot works and that the result is non-exportable”: Copy key: AES, fewer usage flags, Copy key: AES, source=target, constraint with fewer usage flags, Copy key: RSA key pair, fewer usage flags

“copying from a non-exportable key to an exportable target slot doesn't make the target key exportable”: there's no test with this particular combination, the “more usage flags” tests check a different flag. I'll add those since it's conceivable that the policy checks when exporting would behave slightly differently from the policy checks when performing an operation (even though they invoke exactly the same code in our current code base).

Patater · 2019-02-06T14:59:24Z

tests/suites/test_suite_psa_crypto.data

@@ -503,10 +503,14 @@ Copy key: AES, fewer usage flags
 depends_on:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CTR


The commit message for "psa_copy_key: Add test cases to specifically check non-exportability" says "Test that copying a non-exportable key doesn't make it non-exportable".

Should this be "Test that copying a non-exportable key doesn't make it exportable"?

Yes. Reworded.

Test that copying a non-exportable key doesn't make it exportable. This complements similar tests that exercise a different usage flag.

In one place, exercise_key was used in a such a way that if the test failed inside exercise_key, the test suite would correctly report the test as failed but would not report the exact location of the failure. Fix this. Add documentation for exercise_key that explains how to use it.

Patater · 2019-02-07T15:41:20Z

CI failure is "DTLS proxy: 3d, gnutls server, fragmentation", a known flaky test.

gilles-peskine-arm force-pushed the psa-copy_key branch 2 times, most recently from f535577 to cad49af Compare January 28, 2019 12:01

gilles-peskine-arm added 2 commits January 28, 2019 14:41

New function psa_copy_key

f603c71

Copy a key from one slot to another. Implemented and smoke-tested.

Fix memory leak in psa_copy_key

122d002

gilles-peskine-arm force-pushed the psa-copy_key branch from cad49af to 7c1a117 Compare January 28, 2019 13:43

gilles-peskine-arm removed needs: preceding PR Requires another PR to be merged first needs: work The pull request needs rework before it can be merged. labels Jan 28, 2019

gilles-peskine-arm added 3 commits January 28, 2019 14:55

Add psa_copy_key tests with policy constraints

8c1247f

Test a few cases. The logic to combine the constraint is similar to the logic to combine the source and target, so it's ok to have less parameter domain coverage for constraints.

Don't consider RIPEMD160 a PSA_ALG_ANY_HASH candidate

6b156df

Some parts of the library don't support it, such as RSA PKCS#1v1.5 signature.

gilles-peskine-arm force-pushed the psa-copy_key branch from 7c1a117 to 6b156df Compare January 28, 2019 14:44

Patater reviewed Jan 28, 2019

View reviewed changes

Patater suggested changes Feb 5, 2019

View reviewed changes

psa_copy_key: minor documentation clarification

aec5a7f

Patater reviewed Feb 6, 2019

View reviewed changes

gilles-peskine-arm added 2 commits February 6, 2019 16:42

psa_copy_key: Add test cases to specifically check non-exportability

e84d5d2

Test that copying a non-exportable key doesn't make it exportable. This complements similar tests that exercise a different usage flag.

gilles-peskine-arm force-pushed the psa-copy_key branch from 8d10046 to c9516fb Compare February 6, 2019 15:59

Patater approved these changes Feb 6, 2019

View reviewed changes

dgreen-arm approved these changes Feb 7, 2019

View reviewed changes

Patater removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Feb 7, 2019

Patater merged commit 6fd4ee2 into ARMmbed:development Feb 7, 2019

Patater mentioned this pull request Apr 10, 2019

Remove non-crypto code #71

Merged

		@@ -503,10 +503,14 @@ Copy key: AES, fewer usage flags
		depends_on:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CTR

New function psa_copy_key #20

New function psa_copy_key #20

Uh oh!

Conversation

gilles-peskine-arm commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilles-peskine-arm commented Jan 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Patater Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Patater commented Feb 7, 2019

Uh oh!

Uh oh!

gilles-peskine-arm commented Jan 19, 2019 •

edited

Loading

Patater Feb 6, 2019 •

edited

Loading