Add slot number attribute #201

gilles-peskine-arm · 2019-08-02T18:40:11Z

Add a slot number attribute for the sake of keys in a secure element. Report the slot number when relevant.

The main point of having a slot number attribute is to allow applications to choose the slot number when creating a key, which is done in the successor PR #202.

~~Successor of #197. The first new commit is "Add infrastructure for key attribute flags".~~

k-stachowiak

My brain may be short circuiting, but one of the bitwise operations looks off to me. Otherwise, looks good.

library/psa_crypto.c

AndrzejKurek · 2019-08-07T14:04:08Z

library/psa_crypto_core.h

@@ -68,7 +68,7 @@ typedef struct

 /* A mask of key attribute flags used only internally.
 * Currently there aren't any. */
-#define MBEDTLS_PSA_KA_MASK_INTERNAL_ONLY (     \
+#define PSA_KA_MASK_INTERNAL_ONLY (     \


Is there any policy for PSA macros similar to this one with MbedTLS ones?

No. Mbed TLS had no internal headers (library/*.h), but Mbed Crypto does in its PSA parts (library/psa_*.h).

AndrzejKurek · 2019-08-07T14:05:53Z

include/psa/crypto_extra.h

+static inline void psa_clear_key_slot_number(
+    psa_key_attributes_t *attributes )
+{
+    attributes->core.flags &= ~MBEDTLS_PSA_KA_FLAG_HAS_SLOT_NUMBER;


Wouldn't it be good to reset the slot number to an invalid number here?

Yes, but there's no invalid slot number at the moment. I'm considering introducing some though, if only to be future-proof. A slot number is 64 bits, maybe we should say that a slot number is 62 bits, reserve values 0xc000000000000000..0xfffffffffffffffe and declare 0xffffffffffffffff to be permanently invalid? This would not remove the need for the HAS_SLOT_NUMBER flag, because 0 is valid.

AndrzejKurek · 2019-08-07T14:16:38Z

include/psa/crypto_struct.h


 /* A mask of key attribute flags used externally only.
 * Only meant for internal checks inside the library. */
 #define MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY (      \
+        MBEDTLS_PSA_KA_FLAG_HAS_SLOT_NUMBER |    \


Why or it with zero?

So we can add/remove flags just by editing their line and not have to modify the surrounding punctuation.

AndrzejKurek · 2019-08-07T14:25:11Z

library/psa_crypto.c

+static_assert( ( MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE ) == 0,
+               "One or more key attribute flag is listed as both external-only and dual-use" );
+static_assert( ( MBEDTLS_PSA_KA_MASK_INTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE ) == 0,
+               "One or more key attribute flag is listed as both external-only and dual-use" );


external-only -> internal-only

AndrzejKurek · 2019-08-07T14:28:20Z

library/psa_crypto.c

@@ -714,20 +714,24 @@ static inline size_t psa_get_key_slot_bits( const psa_key_slot_t *slot )
 *
 * \return The key size in bits, calculated from the key data.
 */
-static size_t psa_calculate_key_bits( const psa_key_slot_t *slot )
+static psa_key_bits_t psa_calculate_key_bits( const psa_key_slot_t *slot )


Is it possible for a key to have ( bits % 8 ) != 0?

Yes, for some key types. Currently in Mbed Crypto this is only supported with two elliptic curves: Curve25519 (255) and SECP521R1 (521).

AndrzejKurek · 2019-08-07T14:39:36Z

library/psa_crypto_core.h

-#define PSA_KEY_SLOT_FLAG_ALLOCATED ( (uint16_t) 0x0001 )
+/** Test whether a key slot is occupied.
+ *
+ * A key slot is occupied iff the key type is nonzero. This works because


iff -> if

No. Iff = “if and only if”

Oh, right - Chris has reminded me of this acronym - if and only if. I really don't have it memorized.

AndrzejKurek · 2019-08-07T14:44:30Z

library/psa_crypto_slot_management.c

-        {
-            psa_key_slot_set_bits_in_flags( *p_slot,
-                                            PSA_KEY_SLOT_FLAG_ALLOCATED );
+        if( ! psa_is_key_slot_occupied( *p_slot ) )


This approach has one flaw: psa_internal_allocate_key_slot can be called twice on the same key slot and succeed.

Yes, but does it matter? If the caller hasn't done anything with the slot yet, the slot is still free. Callers don't “release” the slot if they decide not to write anything after all. I do see how this could be error-prone though. How about renaming the function to psa_get_empty_key_slot (which used to exist)? I think that would make the new behavior of the function intuitive.

Yes, this would make sense. Although this function is used internally, after seeing just its name, imagine the following situation:
I want to allocate a batch (let's say, 3) keys and then set the same attributes / perform operations with all of them - I would receive the same key, three times.

Add infrastructure for internal, external and dual-use flags, with a compile-time check (if static_assert is available) to ensure that the same numerical value doesn't get declared for two different purposes in crypto_struct.h (external or dual-use) and psa_crypto_core.h (internal).

Add a slot_number field to psa_key_attributes_t and getter/setter functions. Since slot numbers can have the value 0, indicate the presence of the field via a separate flag. In psa_get_key_attributes(), report the slot number if the key is in a secure element. When creating a key, for now, applications cannot choose a slot number. A subsequent commit will add this capability in the secure element HAL.

Test the behavior of the getter/setter functions. Test that psa_get_key_slot_number() reports a slot number for a key in a secure element, and doesn't report a slot number for a key that is not in a secure element. Test that psa_get_key_slot_number() reports the correct slot number for a key in a secure element.

check-names.sh rejects MBEDTLS_XXX identifiers that are not defined in a public header.

This didn't break anything now, but would have broken things once we start to add internal flags.

This function no longer modifies anything, so it doesn't actually allocate the slot. Now, it just returns the empty key slot, and it's up to the caller to cause the slot to be in use (or not).

gilles-peskine-arm · 2019-08-08T09:00:16Z

I've rebased on top of the target branch now that #197 is merged. The rebase shows a large diff because other things were also merged, including development There were no conflicts. The previous version is in https://github.com/gilles-peskine-arm/mbed-crypto/tree/psa-se_driver-set_key_slot_number-1

k-stachowiak

Thanks for addressing my comments.

gilles-peskine-arm · 2019-08-08T17:19:14Z

Crypto CI ok except for API/ABI check.

gilles-peskine-arm added needs: review The pull request is ready for review. This generally means that it has no known issues. api-spec Issue or PR about the PSA specifications labels Aug 2, 2019

gilles-peskine-arm mentioned this pull request Aug 2, 2019

Secure element key registration #183

Merged

gilles-peskine-arm added needs: preceding PR Requires another PR to be merged first enhancement New feature or request labels Aug 2, 2019

gilles-peskine-arm mentioned this pull request Aug 5, 2019

Let applications create a key in a specific secure element slot #202

Merged

k-stachowiak self-requested a review August 7, 2019 10:44

k-stachowiak reviewed Aug 7, 2019

View reviewed changes

library/psa_crypto.c Outdated Show resolved Hide resolved

AndrzejKurek reviewed Aug 7, 2019

View reviewed changes

gilles-peskine-arm requested review from k-stachowiak and AndrzejKurek August 7, 2019 16:23

gilles-peskine-arm added 8 commits August 8, 2019 10:58

Add missing guard around a union field

74f3352

Rename internal macro to pass check-names.sh

5a68056

check-names.sh rejects MBEDTLS_XXX identifiers that are not defined in a public header.

Fix erasure of external flags

013f547

This didn't break anything now, but would have broken things once we start to add internal flags.

Fix copypasta

094dac1

Rename psa_internal_allocate_key_slot to psa_get_empty_key_slot

edbed56

This function no longer modifies anything, so it doesn't actually allocate the slot. Now, it just returns the empty key slot, and it's up to the caller to cause the slot to be in use (or not).

gilles-peskine-arm force-pushed the psa-se_driver-set_key_slot_number branch from da73da0 to edbed56 Compare August 8, 2019 08:58

gilles-peskine-arm removed the needs: preceding PR Requires another PR to be merged first label Aug 8, 2019

k-stachowiak approved these changes Aug 8, 2019

View reviewed changes

AndrzejKurek approved these changes Aug 8, 2019

View reviewed changes

gilles-peskine-arm removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Aug 8, 2019

gilles-peskine-arm merged commit 311809a into ARMmbed:psa-api-1.0-beta Aug 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add slot number attribute #201

Add slot number attribute #201

gilles-peskine-arm commented Aug 2, 2019 •

edited

Loading

k-stachowiak left a comment

AndrzejKurek Aug 7, 2019

gilles-peskine-arm Aug 7, 2019

AndrzejKurek Aug 7, 2019

gilles-peskine-arm Aug 7, 2019

AndrzejKurek Aug 7, 2019

gilles-peskine-arm Aug 7, 2019

AndrzejKurek Aug 7, 2019 •

edited

Loading

AndrzejKurek Aug 7, 2019

gilles-peskine-arm Aug 7, 2019

AndrzejKurek Aug 7, 2019

gilles-peskine-arm Aug 7, 2019

AndrzejKurek Aug 7, 2019

AndrzejKurek Aug 7, 2019 •

edited

Loading

gilles-peskine-arm Aug 7, 2019

AndrzejKurek Aug 7, 2019

gilles-peskine-arm commented Aug 8, 2019

k-stachowiak left a comment

gilles-peskine-arm commented Aug 8, 2019

Add slot number attribute #201

Add slot number attribute #201

Conversation

gilles-peskine-arm commented Aug 2, 2019 • edited Loading

k-stachowiak 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

AndrzejKurek Aug 7, 2019 • edited Loading

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

AndrzejKurek Aug 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Aug 8, 2019

k-stachowiak left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Aug 8, 2019

gilles-peskine-arm commented Aug 2, 2019 •

edited

Loading

AndrzejKurek Aug 7, 2019 •

edited

Loading

AndrzejKurek Aug 7, 2019 •

edited

Loading