Relax input restrictions for key derivation #276

gilles-peskine-arm · 2019-09-23T16:30:10Z

Key derivation requires a PSA_KEY_TYPE_DERIVE key for a PSA_KEY_DERIVATION_INPUT_SECRET step. This is the common case, but sometimes applications have a buffer in memory which they'll only use for a single key derivation. Sometimes this is even an empty buffer, as when deriving an IV in EAP-TLS. So allow the SECRET to be passed with psa_key_derivation_input_bytes.

Conversely, allow other inputs to be passed as RAW_DATA key objects with psa_key_derivation_input_key.

Fix ARMmbed/psa-crypto#257. Needed to repair the ssl-opt.sh test case export keys functionality which fails when using the latest version of Mbed Crypto (since #257 was merged).

AndrzejKurek · 2019-09-24T10:27:55Z

library/psa_crypto.c

+    switch( step )
+    {
+        case PSA_KEY_DERIVATION_INPUT_SECRET:
+            if( key_type == PSA_KEY_TYPE_DERIVE || key_type == 0 )


What does a key type of zero mean? Why not use a define for anything it is?

0 is not a valid key type. Here it's used to mean that the input is a direct string rather than a key object.

A separate define signalling that (to be used in test data, since it's ambiguous there too) would be useful. If not - at least a comment here and in tests.

Done: I used PSA_KEY_TYPE_NONE explicitly, both in psa_crypto.c and in the tests.

AndrzejKurek

Please clarify the usage of zero as key type.

yanesca · 2019-09-24T10:57:05Z

This solves the problem, but I think it relaxes the API too much and makes it easier to misuse. (Of course whatever we do, a dedicated user can misuse it anyways, but I don't think we should make it any easier than it is necessary.)

This way the user can input any data as secret in a single call, potentially making all the derived keys insecure. (Before they had to import it into a key slot to do so, that should raise an alarm in most people and hopefully make them think twice.)

Performing an operation on a key that was originally intended for non-secret data can potentially leak information about the key. (In this particular case I don't think it does, but this interface should be able to serve legacy and future algorithms too, we shouldn't lay traps like this for them if we don't have to.)

We could avoid this if instead of relaxing the constraints, we added a third variant of TLS PRF for IV generation that does not take a secret input at all and throws an error if psa_key_derivation_output_key() is called on the operation. What do you think?

…t-616-fix-ecdsa-rng Fix deterministic ECDSA RNG misuse

gilles-peskine-arm · 2019-09-24T14:08:58Z

We could avoid this if instead of relaxing the constraints, we added a third variant of TLS PRF for IV generation that does not take a secret input at all and throws an error if psa_key_derivation_output_key() is called on the operation.

I don't like adding another algorithm. What do we do when the next protocol comes along and does the same with some other KDF?

On the other hand, I do like the idea of forbidding psa_key_derivation_output_key() if the SECRET input isn't a key. Do you think that's enough?

athoelke · 2019-09-24T14:28:44Z

On the other hand, I do like the idea of forbidding psa_key_derivation_output_key() if the SECRET input isn't a key. Do you think that's enough?

It's definitely cute, and makes some conceptual sense: "don't permit secret outputs if there were no secret inputs".

If the caller really wants to us the output as a key they then have to import the output bytes as a key - which probably alerts a reviewer to check what is happening in that code.

If the caller wants to deterministically generate a key from a non-secret input, then (as currently) they have to import the non-secret to a key-derivation key first. Which also triggers a reviewer to inspect the application behaviour.

yanesca · 2019-09-24T14:30:24Z

On the other hand, I do like the idea of forbidding psa_key_derivation_output_key() if the SECRET input isn't a key. Do you think that's enough?

Yes, I think it should be enough to make allowing non-key SECRET inputs safer.

On the other hand I still feel uncomfortable about allowing keys as a non-SECRET input. This relaxation is not necessary for the current fix, can't we just keep the restriction and hope that eventually we won't need to lift it?

gilles-peskine-arm · 2019-09-24T14:42:55Z

Label-type inputs have to be RAW_DATA, not DERIVE. It makes sense to me to allow RAW_DATA for a label or info (a personalization string kept in persistent storage), or for a salt or seed (a device-specific value that may or may not be secret and is kept in persistent storage). @yanesca What makes you uncomfortable? The risk that an application will pass the wrong key (which would have to be of the wrong type)?

yanesca · 2019-09-24T14:59:54Z

I am imagining here a RAW_DATA that is supposed to be secret and a key derivation algorithm that leaks information about for example the label. (Yes, such a key derivation algorithm is not safe in the first place.)

I admit that what I am afraid of is highly hypothetical, I just thought if we can avoid it then why not?

gilles-peskine-arm · 2019-09-24T15:03:22Z

It wouldn't necessarily be bad for a KDF to leak the “label”, although I can't think of a common construction that comes anywhere near it.

RAW_DATA is often not supposed to be kept secret anyway. A common use case is to store a certificate next to the corresponding private key.

A RAW_DATA does need to have the DERIVE usage flag to be used as a label (etc) in a key derivation.

yanesca · 2019-09-24T15:30:51Z

In that case I think it is enough to constrain the secret outputs as discussed above.

gilles-peskine-arm · 2019-09-24T16:25:20Z

I implemented the constraint on output_key.

And now I need to resolve a merge conflict. I'm afraid I'm going to need to rebase this. The current state is in https://github.com/gilles-peskine-arm/mbed-crypto/tree/psa-key_derivation-relax_inputs-2.

Systematically use "PSA key derivation setup" for derive_setup. This resolves the ambiguity between derive_setup and derive_input calls.

This commit only makes derive_input more flexible so that the key derivation API can be tested with different key types and raw data for each input step. The behavior of the test cases remains the same.

Allow a direct input as the SECRET input step in a key derivation, in addition to allowing DERIVE keys. This makes it easier for applications to run a key derivation where the "secret" input is obtained from somewhere else. This makes it possible for the "secret" input to be empty (keys cannot be empty), which some protocols do (for example the IV derivation in EAP-TLS). Conversely, allow a RAW_DATA key as the INFO/LABEL/SALT/SEED input to a key derivation, in addition to allowing direct inputs. This doesn't improve security, but removes a step when a personalization parameter is stored in the key store, and allows this personalization parameter to remain opaque. Add test cases that explore step/key-type-and-keyhood combinations.

This is valid C99 (since the variable in question is not a VLA and is not used) but not accepted by IAR 8.20.

No behavior change, just a readability improvement.

Update the documentation of psa_key_derivation_input_key() and psa_key_derivation_input_bytes() now that the key/buffer distinction is not mandatory.

No behavior change.

After passing some inputs, try getting one byte of output, just to check that this succeeds (for a valid sequence of inputs) or fails with BAD_STATE (for an invalid sequence of inputs). Either output a 1-byte key or a 1-byte buffer depending on the test data. The test data was expanded as follows: * Output key type (or not a key): same as the SECRET input if success is expected, otherwise NONE. * Expected status: PSA_SUCCESS after valid inputs, BAD_STATE after any invalid input.

If none of the inputs to a key derivation is a PSA_KEY_DERIVATION_INPUT_SECRET passed with psa_key_derivation_input_key(), forbid psa_key_derivation_output_key(). It usually doesn't make sense to derive a key object if the secret isn't itself a proper key.

gilles-peskine-arm · 2019-09-24T16:45:45Z

I rebased on top of development plus one new commit 6ddb4d8 "Improve descriptions of derive test cases". Thanks to that commit, there were no conflicts.

gilles-peskine-arm · 2019-09-24T17:14:38Z

include/psa/crypto_struct.h

@@ -255,6 +255,7 @@ typedef struct psa_tls12_prf_key_derivation_s
 struct psa_key_derivation_s
 {
    psa_algorithm_t alg;
+    unsigned int can_output_key : 1;


This leaves 31 bits unused. #281

yanesca · 2019-09-26T12:52:14Z

The CI fails only on the ABI check and the known export key issue.

yanesca

Looks good to me.

athoelke · 2019-09-26T12:54:54Z

I'm happy with the documentation improvements. 👍

gilles-peskine-arm added bug Something isn't working needs: review The pull request is ready for review. This generally means that it has no known issues. labels Sep 23, 2019

gilles-peskine-arm mentioned this pull request Sep 23, 2019

Fix "export keys functionality" with the latest mbed-crypto Mbed-TLS/mbedtls#2858

Merged

Patater requested review from AndrzejKurek and yanesca September 24, 2019 08:40

AndrzejKurek reviewed Sep 24, 2019

View reviewed changes

AndrzejKurek suggested changes Sep 24, 2019

View reviewed changes

athoelke pushed a commit to athoelke/mbed-crypto that referenced this pull request Sep 24, 2019

Merge pull request ARMmbed#276 from dgreen-arm/dev/dgreen-arm/iotcryp…

c7cde03

…t-616-fix-ecdsa-rng Fix deterministic ECDSA RNG misuse

gilles-peskine-arm added needs: work The pull request needs rework before it can be merged. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Sep 24, 2019

gilles-peskine-arm requested a review from AndrzejKurek September 24, 2019 16:23

gilles-peskine-arm added 6 commits September 24, 2019 18:37

Improve descriptions of derive test cases

6ddb4d8

Systematically use "PSA key derivation setup" for derive_setup. This resolves the ambiguity between derive_setup and derive_input calls.

PSA crypto KDF: test bytes/key input independently of the step type

6842ba4

This commit only makes derive_input more flexible so that the key derivation API can be tested with different key types and raw data for each input step. The behavior of the test cases remains the same.

Consistently abort key derivation operations on input error

593773d

Don't jump past a variable declaration

46d7faf

This is valid C99 (since the variable in question is not a VLA and is not used) but not accepted by IAR 8.20.

Use the constant PSA_KEY_TYPE_NONE rather than 0

b896519

No behavior change, just a readability improvement.

gilles-peskine-arm added 4 commits September 24, 2019 18:39

Key derivation: allow both keys and direct inputs (function doc)

7ebd4dc

Update the documentation of psa_key_derivation_input_key() and psa_key_derivation_input_bytes() now that the key/buffer distinction is not mandatory.

derive_input test function: More logical parameter order

2058c07

No behavior change.

gilles-peskine-arm force-pushed the psa-key_derivation-relax_inputs branch from 2cda070 to 178c9aa Compare September 24, 2019 16:44

gilles-peskine-arm added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: work The pull request needs rework before it can be merged. labels Sep 24, 2019

gilles-peskine-arm mentioned this pull request Sep 24, 2019

Occupy bits in struct psa_key_derivation_s #281

Closed

gilles-peskine-arm commented Sep 24, 2019

View reviewed changes

yanesca approved these changes Sep 26, 2019

View reviewed changes

AndrzejKurek approved these changes Sep 26, 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 Sep 26, 2019

gilles-peskine-arm merged commit 37b5c83 into ARMmbed:development Sep 26, 2019

Relax input restrictions for key derivation #276

Relax input restrictions for key derivation #276

Uh oh!

Conversation

gilles-peskine-arm commented Sep 23, 2019

Uh oh!

AndrzejKurek Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

AndrzejKurek Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Sep 24, 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!

AndrzejKurek left a comment

Choose a reason for hiding this comment

Uh oh!

yanesca commented Sep 24, 2019

Uh oh!

gilles-peskine-arm commented Sep 24, 2019

Uh oh!

athoelke commented Sep 24, 2019

Uh oh!

yanesca commented Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gilles-peskine-arm commented Sep 24, 2019

Uh oh!

yanesca commented Sep 24, 2019

Uh oh!

gilles-peskine-arm commented Sep 24, 2019

Uh oh!

yanesca commented Sep 24, 2019

Uh oh!

gilles-peskine-arm commented Sep 24, 2019

Uh oh!

gilles-peskine-arm commented Sep 24, 2019

Uh oh!

gilles-peskine-arm Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

yanesca commented Sep 26, 2019

Uh oh!

yanesca left a comment

Choose a reason for hiding this comment

Uh oh!

athoelke commented Sep 26, 2019

Uh oh!

Uh oh!

gilles-peskine-arm Sep 24, 2019 •

edited

Loading

yanesca commented Sep 24, 2019 •

edited

Loading