Support wildcard hash in signature policies #15

gilles-peskine-arm · 2019-01-14T15:41:30Z

In a key policy for a public-key signature algorithm, you can specify an algorithm like PSA_ALG_RSA_PKCS1_V15_SIGN(PSA_ALG_ANY_HASH), and then the key may be used for PKCS#1v1.5 signature with any hash (i.e. PSA_ALG_RSA_PKCS1_V15_SIGN(PSA_ALG_SHA_256), PSA_ALG_RSA_PKCS1_V15_SIGN(PSA_ALG_SHA_512), …, or PSA_ALG_RSA_PKCS1_V15_SIGN_RAW).

This is a compromise between cleanliness (a key may only be used with a single algorithm) and the real world (especially X509 and TLS, where a certificate does not constraint which hash algorithms a signature key will be used for).

Test for a subclass of public-key algorithm: those that perform full-domain hashing, i.e. algorithms that can be broken down as sign(key, hash(message)).

gilles-peskine-arm · 2019-01-14T15:43:50Z

@hanno-arm @AndrzejKurek Does this look good enough for Mbed TLS over PSA Crypto?

library/psa_crypto.c

hanno-becker · 2019-01-14T16:03:26Z

@gilles-peskine-arm The use of a PSK for both pure-PSK and ECDHE-PSK handshakes is very natural and still not allowed. Will this be addressed in a separate PR?

Patater · 2019-01-14T16:26:27Z

include/psa/crypto_values.h

+ *
+ * \return 1 if \p alg is a hash-and-sign algorithm, 0 otherwise.
+ *         This macro may return either 0 or 1 if \p alg is not a supported
+ *         algorithm identifier.


Could we decide if this is going to return a 0 or a 1 now, when alg is not a supported algorithm identifier, instead of leaving it up to implementations? How about always 0 in such cases? I see advantage for applications in making the behavior predictable, but does the advantage of flexibility for implementations outweigh this?

This is a macro which returns a constant expression so that it can be used to calculate static buffer sizes in applications that don't need runtime algorithm agility. So it must be implemented based on some simple bit patterns, not based on what some cryptoprocessor has to say about the algorithm. In particular, this macro cannot determine whether an algorithm is supported, so it can't return 0 for all unsupported algorithms. There is no clear semantic rule to choose between 0 and 1 for an unsupported algorithm, hence the specification leaves the result undefined in that case.

This consideration applies to all the PSA_ALG_IS_xxx macros.

Patater · 2019-01-14T16:37:54Z

include/psa/crypto_values.h

+ *   ```
+ *   psa_key_policy_set_usage(&policy,
+ *                            PSA_KEY_USAGE_SIGN, //or PSA_KEY_USAGE_VERIFY
+ *                            PSA_xxx_SIGN(PSA_ALG_ANY_HASH));


PSA_xxx_SIGN should be used even when PSA_KEY_USAGE_VERIFY is specified, right? SIGN is the algorithm type, whether you are signing or verifying. My gut-reading of this was to think we need PSA_xxx_VERIFY, but that's not the case. An alternative like PSA_xxx_SIGNATURE might be too verbose, though.

I agree that PSA_xxx_SIGNATURE is better. Amended.

Patater · 2019-01-14T16:39:09Z

include/psa/crypto_values.h

+ *   ```
+ *
+ * This value may not be used to build other algorithms that are
+ * parametrized over a hash.


Other algorithms as in ones that don't satisfy PSA_ALG_IS_HASH_AND_SIGN? Should we call that out explicitly for clarity?

I added a sentence to this effect.

Patater · 2019-01-14T16:43:16Z

tests/suites/test_suite_psa_crypto.data

@@ -1162,10 +1192,14 @@ PSA sign: deterministic ECDSA SECP256R1 SHA-256, output buffer too small
 depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA256_C:MBEDTLS_ECDSA_C
 sign_fail:PSA_KEY_TYPE_ECC_KEYPAIR(PSA_ECC_CURVE_SECP256R1):"ab45435712649cb30bbddac49197eebf2740ffc7f874d9244c3460f54f322d3a":PSA_ALG_DETERMINISTIC_ECDSA( PSA_ALG_SHA_256 ):"9ac4335b469bbd791439248504dd0d49c71349a295fee5a1c68507f45a9e1c7b":63:PSA_ERROR_BUFFER_TOO_SMALL

-PSA sign: deterministic ECDSA SECP256R1, invalid hash
-depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_ECDSA_DETERMINISTIC:MBEDTLS_SHA256_C
+PSA sign: deterministic ECDSA SECP256R1, invalid hash (0)


What's the (0) mean?

It means 0 is used where a hash algorithm is expected.

OK, this was clarifying the test that was already written to use 0 as hash. It has nothing to do with the update to the dependencies or to support wildcards, really, except to differentiate it from the invalid hash (wildcard) test.

I changed the description of the text to make it unique, now that there is a second test case with an invalid hash. There's still a bit of room so I changed “invalid hash” to “invalid hash algorithm”.

You can use PSA_ALG_ANY_HASH to build the algorithm value for a hash-and-sign algorithm in a policy. Then the policy allows usage with this hash-and-sign family with any hash. Test that PSA_ALG_ANY_HASH-based policies allow a specific hash, but not a different hash-and-sign family. Test that PSA_ALG_ANY_HASH is not valid for operations, only in policies.

gilles-peskine-arm · 2019-01-14T18:41:23Z

I amended the last commit to improve the documentation as suggested. The previous version is in https://github.com/gilles-peskine-arm/mbed-crypto/tree/psa-signature_policy_wildcard-1

Patater

Other than what @hanno-arm requested, LGTM

gilles-peskine-arm · 2019-01-24T08:02:36Z

This PR isn't complete yet. I think the implementation is ok but might need a few more comments. And I want to add a few more tests.

PSA_ALG_HASH_ANY is specified as meaningful only for signature.

gilles-peskine-arm · 2019-01-24T12:07:17Z

Ok, I'm happy with the level of testing now (consistent with our general level).

yanesca

Looks good to me.

(I have a minor suggestion for improving the documentation.)

include/psa/crypto_values.h

library/psa_crypto.c

hanno-becker · 2019-01-25T17:38:49Z

tests/suites/test_suite_psa_crypto.data

 sign_fail:PSA_KEY_TYPE_ECC_KEYPAIR(PSA_ECC_CURVE_SECP256R1):"ab45435712649cb30bbddac49197eebf2740ffc7f874d9244c3460f54f322d3a":PSA_ALG_DETERMINISTIC_ECDSA( 0 ):"9ac4335b469bbd791439248504dd0d49c71349a295fee5a1c68507f45a9e1c7b":72:PSA_ERROR_INVALID_ARGUMENT

+PSA sign: deterministic ECDSA SECP256R1, invalid hash algorithm (wildcard)
+depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_ECDSA_DETERMINISTIC
+sign_fail:PSA_KEY_TYPE_ECC_KEYPAIR(PSA_ECC_CURVE_SECP256R1):"ab45435712649cb30bbddac49197eebf2740ffc7f874d9244c3460f54f322d3a":PSA_ALG_DETERMINISTIC_ECDSA( PSA_ALG_ANY_HASH ):"9ac4335b469bbd791439248504dd0d49c71349a295fee5a1c68507f45a9e1c7b":72:PSA_ERROR_INVALID_ARGUMENT


Minor: Remove spaces around PSA_ALG_ANY_HASH.

This seems to be non-uniform across the file.

Make it clearer what PSA_ALG_ANY_HASH can and cannot be used for.

Test vector ARMmbed#15 was encrypted twice. Decrypt it the second time.

New macro PSA_ALG_IS_HASH_AND_SIGN

d35b489

Test for a subclass of public-key algorithm: those that perform full-domain hashing, i.e. algorithms that can be broken down as sign(key, hash(message)).

gilles-peskine-arm added enhancement New feature or request needs: review The pull request is ready for review. This generally means that it has no known issues. labels Jan 14, 2019

hanno-becker reviewed Jan 14, 2019

View reviewed changes

library/psa_crypto.c Show resolved Hide resolved

hanno-becker reviewed Jan 14, 2019

View reviewed changes

library/psa_crypto.c Show resolved Hide resolved

Patater reviewed Jan 14, 2019

View reviewed changes

gilles-peskine-arm force-pushed the psa-signature_policy_wildcard branch from acec0cc to 30f77cd Compare January 14, 2019 18:39

gilles-peskine-arm mentioned this pull request Jan 19, 2019

New function psa_copy_key #20

Merged

Patater reviewed Jan 22, 2019

View reviewed changes

Add missing test dependency

870f5dc

Patater previously approved these changes Jan 23, 2019

View reviewed changes

Test that HASH_ANY is not meaningful for OAEP

eb2d4b9

PSA_ALG_HASH_ANY is specified as meaningful only for signature.

gilles-peskine-arm dismissed Patater’s stale review via eb2d4b9 January 24, 2019 12:06

Patater previously approved these changes Jan 24, 2019

View reviewed changes

yanesca previously approved these changes Jan 25, 2019

View reviewed changes

include/psa/crypto_values.h Show resolved Hide resolved

include/psa/crypto_values.h Show resolved Hide resolved

library/psa_crypto.c Show resolved Hide resolved

hanno-becker reviewed Jan 25, 2019

View reviewed changes

hanno-becker previously approved these changes Jan 25, 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 Jan 28, 2019

Improve the description of PSA_ALG_ANY_HASH

763fb9a

Make it clearer what PSA_ALG_ANY_HASH can and cannot be used for.

gilles-peskine-arm dismissed stale reviews from hanno-becker, yanesca, and Patater via 763fb9a January 28, 2019 12:29

yanesca approved these changes Jan 28, 2019

View reviewed changes

Patater approved these changes Jan 28, 2019

View reviewed changes

Patater merged commit 43bafcc into ARMmbed:development Jan 28, 2019

gilles-peskine-arm added a commit to gilles-peskine-arm/mbed-crypto that referenced this pull request Sep 19, 2019

Fix encrypt test case that should have been a decrypt test case

8b66955

Test vector ARMmbed#15 was encrypted twice. Decrypt it the second time.

gilles-peskine-arm added a commit to gilles-peskine-arm/mbed-crypto that referenced this pull request Sep 20, 2019

blowfish: Fix encrypt test case that should have been decrypt

bfcb69c

Test vector ARMmbed#15 was encrypted twice. Decrypt it the second time.

Support wildcard hash in signature policies #15

Support wildcard hash in signature policies #15

Uh oh!

Conversation

gilles-peskine-arm commented Jan 14, 2019

Uh oh!

gilles-peskine-arm commented Jan 14, 2019

Uh oh!

Uh oh!

Uh oh!

hanno-becker commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Jan 14, 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!

gilles-peskine-arm commented Jan 14, 2019

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Jan 24, 2019

Uh oh!

gilles-peskine-arm commented Jan 24, 2019

Uh oh!

yanesca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hanno-becker commented Jan 14, 2019 •

edited

Loading

Patater Jan 14, 2019 •

edited

Loading