Add interruptible Crypto API operations - #375
Conversation
gilles-peskine-arm
left a comment
There was a problem hiding this comment.
I've reviewed the document with the partial implementation in TF-PSA-Crypto in mind. We know that we'll need to update our implementation since it followed a now outdated beta.
Many of my comments apply to multiple operation types. I only noted the first place where I noticed something.
My biggest concern is the verify flow. For verify-message, I don't think providing the signature before the message works out in practice.
| Identifier of the key to use for the operation. It must be an asymmetric key pair or asymmetric public key. The key must either permit the usage `PSA_KEY_USAGE_VERIFY_HASH` or `PSA_KEY_USAGE_VERIFY_MESSAGE`. | ||
| .. param:: psa_algorithm_t alg | ||
| An asymmetric signature algorithm: a value of type `psa_algorithm_t` such that :code:`PSA_ALG_IS_SIGN(alg)` is true. | ||
| .. param:: const uint8_t * signature |
There was a problem hiding this comment.
Can you remind me why the signature is passed during setup, rather than after the message?
Streaming protocols often send the message before the signature to verify, so in practice, this is likely to be a burden if the algorithm doesn't follow the sign-the-hash paradigm.
There was a problem hiding this comment.
That limits the algorithm selection: PureEdDSA, SLH-DSA, LMS, and XMSS all require the randomisation element from the signature as part of the hash prefix before the message content.
There was a problem hiding this comment.
Deferred-signature verification support has been added
|
|
||
| .. _interruptible-operations: | ||
|
|
||
| Interruptible operations |
There was a problem hiding this comment.
This section could use a subsection that discusses op counts:
xxx_iop_get_num_ops()functions (still meaningful aftercomplete()returnsPSA_SUCCESS).psa_iop_set_max_ops()and a brief statement that ops do not have a fixed meaning.
There was a problem hiding this comment.
This is currently present in lines 252-256. Is there more that should be said here?
There was a problem hiding this comment.
The section introduces the concepts, but I think it should be a bit more concrete. At the very least, it should mention the get_num_ops naming convention.
Quick response on this:
A algorithm-agnostic flow requires the signature before the message parts. |
Signed-off-by: Andrew Thoelke <andrew.thoelke@arm.com>
Signed-off-by: Andrew Thoelke <andrew.thoelke@arm.com>
Signed-off-by: Andrew Thoelke <andrew.thoelke@arm.com>
…ions Signed-off-by: Andrew Thoelke <andrew.thoelke@arm.com>
|
Following discussion - we need to support use cases where the signature is only provided after the message content (required for some streaming protocols), as well as use cases where the signature is provided before the message (which is required for some algorithms). For Using the current For the new flow, three additional APIs are provided:
This design, providing a distinct setup function, enables the implementation to detect the application intent explicitly and immediately, and respond with an error if not supported. With a more generic, single setup function without a signature; the application intent can only be inferred when |
Signed-off-by: Andrew Thoelke <andrew.thoelke@arm.com>
|
I think I've managed to address all of the substanive issues raised in the feedback so far. I would appreciate a re-review of the resulting updated PR. |
gilles-peskine-arm
left a comment
There was a problem hiding this comment.
Thanks for the updates. I only have a couple of minor nits now, except maybe for one point. I want to think further about verification with and without deferred signature.
| .. retval:: PSA_ERROR_BAD_STATE | ||
| The following conditions can result in this error: | ||
|
|
||
| * The operation state is not valid: it must be active, and `psa_generate_key_iop_complete()` must not have been called. |
There was a problem hiding this comment.
psa_generate_key_iop_custom() can also fail with PSA_ERROR_BAD_STATE if psa_generate_key_iop_custom() has already been called.
There was a problem hiding this comment.
I'll review all the sequencing statements affected by adding this function.
| The modulus is a product of two probabilistic primes between :math:`2^{n-1}` and :math:`2^n` where :math:`n` is the bit size specified in the attributes. | ||
|
|
||
| After a successful call to `psa_generate_key_iop_start()`, the operation is active. | ||
| The operation can be configured with custom production parameters by calling `psa_generate_key_iop_custom()`, or completed by calling `psa_generate_key_iop_complete()` repeatedly, until it returns a status code that is not :code:`PSA_OPERATION_INCOMPLETE`. |
There was a problem hiding this comment.
It's technically true that psa_generate_key_iop_custom() and psa_generate_key_iop_complete() are the next two functions that may be used for a normal flow (non-aborted, eventually succeeding). However, the way I understand this sentence is that one may either call custom() or the complete() loop. It's not clear either here or in the documentation of custom() that custom() should be followed by a complete() loop.
| .. retval:: PSA_ERROR_BAD_STATE | ||
| The following conditions can result in this error: | ||
|
|
||
| * The operation state is not valid: setup must be complete, and no call to `psa_sign_iop_set_context()`, `psa_sign_iop_hash()`, `psa_sign_iop_update()`, or `psa_sign_iop_complete()` has been made. |
There was a problem hiding this comment.
There's still an ambiguity here in the second clause, where it's not clear whether the clause describes what caused the error or what must be done to avoid the error. I think it would be simpler to understand if error descriptions always described what causes the error.
| * The operation state is not valid: setup must be complete, and no call to `psa_sign_iop_set_context()`, `psa_sign_iop_hash()`, `psa_sign_iop_update()`, or `psa_sign_iop_complete()` has been made. | |
| * The operation state is not valid: setup must be complete, and no call to `psa_sign_iop_set_context()`, `psa_sign_iop_hash()`, `psa_sign_iop_update()`, or `psa_sign_iop_complete()` must have been made. |
or
| * The operation state is not valid: setup must be complete, and no call to `psa_sign_iop_set_context()`, `psa_sign_iop_hash()`, `psa_sign_iop_update()`, or `psa_sign_iop_complete()` has been made. | |
| * The operation state is not valid: the operation is inactive, setup is incomplete, or a call to `psa_sign_iop_set_context()`, `psa_sign_iop_hash()`, `psa_sign_iop_update()`, or `psa_sign_iop_complete()` has been made. |
(I haven't made a list of where this pattern occurs.)
There was a problem hiding this comment.
This could be widespread - I think in all the MP operations, the 'The operation state is not valid:...' conditions for BAD_STATE go on to define what 'valid' means for the function. The statement here mixes requirement tone - 'setup must be complete' - with present tense state tone - 'no call to ... has been made'.
The second alternative is tempting, with the description actually presents the states that are invalid. I would introduce it as 'The operation state is invalid: ...' - this removes the [existing] ambiguity as to whether the statement that follows is a definition of 'valid states' or of 'not valid states'?
For consistency, this might be a change we should make across all operation functions? - which goes well beyond the remit of this PR. But we could do that for these interruptibles, and follow with a spec-wide edit for the existing multi-parts?
| * A successful call to `psa_generate_key_iop_complete()`. | ||
| * A call to `psa_generate_key_iop_abort()`. | ||
|
|
||
| If `psa_generate_key_iop_start()` returns an error, the operation object remains inactive, but its number of *ops* can be reset to zero. |
There was a problem hiding this comment.
This still isn't fully accurate: if psa_xxx_iop_start() is called on an already active object, it's a BAD_STATE error, but the object doesn't “remain inactive”.
There was a problem hiding this comment.
Quite right, but 'ouch!':
-
This affects all the interruptibles in this PR. The statement is only true if the operation was inactive prior to the call.
-
All existing multi-part operations suffer from a differently word defect of the same category. They state:
If
psa_xxx_setup()returns an error, the operation object is unchanged.Which is also only true if the operation was inactive to start with.
We changed the wording for the interruptibles, because the ops count can be changed by the call. But we have missed the 'If the operation was not inactive, psa_xxx_setup() returns PSA_ERROR_BAD_STATE, otherwise ...` that should preceed the current statements for all of these APIs.
There was a problem hiding this comment.
Looks like this warrants an issue to address this omission across all multi-part operations.
| .. retval:: PSA_SUCCESS | ||
| Success. | ||
| The interruptible operation must now be completed by calling `psa_generate_key_iop_complete()`. | ||
| .. retval:: PSA_ERROR_ALREADY_EXISTS |
There was a problem hiding this comment.
I definitely don't want to implement a key id reservation in TF-PSA-Crypto. I wouldn't mind if reservation was forbidden. But there may be lower-level implementations where persistent key slots are tied more directly to physical addresses where reserving makes more sense, so I'm ok with allowing it in the specification.
|
|
||
| .. _interruptible-operations: | ||
|
|
||
| Interruptible operations |
There was a problem hiding this comment.
The section introduces the concepts, but I think it should be a bit more concrete. At the very least, it should mention the get_num_ops naming convention.
|
I had another thought about the verification and deferred signatures. For both the multi-part and interruptible verification operations, the operation object wil have to copy some or all of the signature data if it is passed during setup. For PQC signatures, this can be a very sizeable buffer and will be challenging for constrained implementations with no dynamic allocation. However, although it might work (from a memory management point of view) to have the application deal with the signature memory issue, and pass the signature to both the setup and the finishing phases for an algorithm that requires the signature before the message - is there a security/cryptographic risk in having the application pass what is meant to be the same signature data twice to the implementation? |
This is a rebased version of #107 and #199, targetting version 1.6 of the specification.
Apart from adding the previously drafted APIs, this incorporates other changes to the documentation since the original PRs. The introductory text for interruptible signatures has also been updated to reflect the presence of both multi-part signatures and interruptibel signature APIs.