Include IANA reference in the definition of ECC curves and DH groups #279

athoelke · 2019-09-24T12:24:51Z

Fixes ARMmbed/psa-crypto#262

include/psa/crypto_types.h

Define a vendor-range within the the private use ranges in the IANA registry. Provide recommendations for how to support vendor-defined curves and groups.

Connect the types to the key type construction macros by x-refs.

include/psa/crypto_types.h

include/psa/crypto_values.h

gilles-peskine-arm · 2019-10-03T17:01:18Z

include/psa/crypto_values.h

+/** Minimum value for a vendor-defined Diffie Hellman group identifier
+ *
+ * The range for vendor-defined group identifiers is a subset of the IANA
+ * registry private use range, `0x01fc` - `0x01ff`.


Unfortunately, this is a ridiculously small range whereas an implementation may well wish to define many custom groups, since many groups have been common in the wild and specific systems or protocols may define their own groups.

The TLS registry puts EC and FF groups in the same 16-bit range, but we don't have to. I propose to use some values in the IANA EC range for vendor FF groups. We can in particular use the GREASE values, which TLS uses for the purpose of not having a corresponding cryptographic algorithm.

From a quick glance, the proposed GREASE values that can be used in this way are:

0x0A0A, 0x1A1A, 0x2A2A, 0x3A3A, 0x4A4A, 0x5A5A, 0x6A6A, 0x7A7A, 0x8A8A, 0x9A9A, 0xAAAA, 0xBABA, 0xCACA, 0xDADA, 0xEAEA, 0xFAFA

This is still draft and subject to change. This might not affect this API element, but should modify the documentation of psa_dh_group_t.

Is the suggestion to directly list the values, as being permitted for vendor-defined additional DH groups, or indirectly by referencing the specification draft?

Do we want to slice up the GREASE identifiers, and reserve a set for PSA Crypto future use?

Is the [arbitrary] 50/50 split of the TLS support group private use range appropriate (for this and ECC)?

My proposal was to define the GREASE values (stated by value, with the GREASE spec given as an informative rationale) as reserved for vendors in psa_dh_group_t.

Though given that there are only 16 of them, that's not much. We can punt PSA crypto future use until later, but I do think we need more vendor FFDH values. I guess the only solution (short of moving to a bigger type) is to take values that are already assigned to elliptic curves and declare them as vendor-specific FFDH values.

I've added a commit the extends the recommendations for vendor defined DH groups.

include/psa/crypto_values.h

Rely on general reference to IANA documentation

gilles-peskine-arm

Approved for the documentation changes. The xxx_VENDOR_{MIN,MAX} macros need to be excluded from scripts/generate_psa_constants.py and tests/scripts/test_psa_constant_names.py to fix the build.

athoelke · 2019-10-09T14:13:41Z

The xxx_VENDOR_{MIN,MAX} macros need to be excluded from scripts/generate_psa_constants.py and tests/scripts/test_psa_constant_names.py to fix the build.

Can someone help with making the associated change to those scripts. There is no obviously correct place to exclude these definitions from the testing that is done here.

Why are these values not suitable for this test?

gilles-peskine-arm · 2019-10-09T14:16:59Z

generate_psa_constants.py works out preprocessor_expression → numerical_value mappings, and generates C code that constructs a mapping in the other direction. If the mapping is not one-to-one, it errors out. There are rules to exclude certain preprocessor expressions which are considered to be aliases of other expressions.

I'll update the scripts.

gilles-peskine-arm · 2019-10-09T14:47:28Z

The problem was that the (for now) mbedcrypto-specific value PSA_DH_GROUP_CUSTOM had the same value as PSA_DH_GROUP_VENDOR_MIN. I've moved it to be somewhere else in the standard range, far from values that are currently in used. It's for a feature that isn't implemented yet, and we can see later whether this becomes a standard feature (perhaps with the same number, perhaps not) or stays implementation-specific (in which case we'll change the number to be in the vendor range).

gilles-peskine-arm · 2019-10-09T16:32:07Z

CI report: the TLS head job failed in test_suite_x509parse because mbedtls needs a bug fix in mbed-crypto that is more recent than this PR. The TLS merge job succeeded so that's good. The crypto merge job failed in the Windows tests due to what seems to have been platform instability (“error MSB4166: Child node "4" exited prematurely”). I'm rerunning the Windows crypto tests: https://jenkins-internal.mbed.com/view/mbed-crypto/job/mbedtls-psa-release-new/574/

gilles-peskine-arm · 2019-10-09T16:57:59Z

https://jenkins-internal.mbed.com/view/mbed-crypto/job/mbedtls-psa-release-new/574/ passed so we have full CI approval. Since I made some of the changes, this needs approval from a team member other than me.

I pushed a commit to fix the problem. As far as I'm concerned, this PR is approved.

athoelke · 2019-10-11T11:08:23Z

Is there any chance that this could be reviewed, approved and merged today (if there is no issue with the implementation, obviuosly? - please? :-)

Patater

Looks good to me, but for one question.

CI failures are a known issue with DTLS testing (not introduced by this PR) and some ATECC608A example failures (not introduced by this PR).

Patater · 2019-10-11T11:27:10Z

include/psa/crypto_values.h

+/** Minimum value for a vendor-defined Diffie Hellman group identifier
+ *
+ * The range for vendor-defined group identifiers is a subset of the IANA
+ * registry private use range, `0x01fc` - `0x01ff`.


@athoelke To confirm, this range is still accurate, but the GREASE values are also available so the practical use of this range is limited.

Yes, that is correct. An minor improvement might be to refer to the psa_dh_group_t definition and its description of other ways to allocate a vendor-defined group number.

However, I'd like this merged sooner so I can render the reST and separate the specification from the doxygen source code, which is a pre-requistite to completing the remaing API changes before publishing 1.0.0 of the spec. I can clarify/improve the text after the split if desirable.

Patater

LGTM

* ARMmbed#272: Insert doxygen comments on old algorithms so they appear in PSA documentation * ARMmbed#285: SE driver: make persistent data work * ARMmbed#279: Include IANA reference in the definition of ECC curves and DH groups * ARMmbed#287: DRBG documentation improvements * ARMmbed#297: Fix int overflow in mbedtls_asn1_get_int (Credit to OSS-Fuzz)

Include IANA reference in the definition of ECC curves and DH groups

c6f03ef

Fixes ARM-software/psa-crypto-api#262

gilles-peskine-arm reviewed Sep 24, 2019

View reviewed changes

include/psa/crypto_types.h Outdated Show resolved Hide resolved

athoelke added 3 commits September 25, 2019 22:11

Tighten up language regarding direct use of the IANA registry values

c625045

Support for vendor-defined ECC curves and DH groups

fd368e5

Define a vendor-range within the the private use ranges in the IANA registry. Provide recommendations for how to support vendor-defined curves and groups.

Xref documentation for ECC curves and DH groups.

214064e

Connect the types to the key type construction macros by x-refs.

gilles-peskine-arm requested changes Oct 3, 2019

View reviewed changes

gilles-peskine-arm reviewed Oct 3, 2019

View reviewed changes

include/psa/crypto_values.h Outdated Show resolved Hide resolved

athoelke force-pushed the at-fix262 branch from ad5aaf4 to 214064e Compare October 4, 2019 10:59

athoelke added 2 commits October 7, 2019 15:28

Remove over-specific RFC references

691ec52

Rely on general reference to IANA documentation

Recommend use of GREASE values for vendor defined DH groups

6e59505

gilles-peskine-arm previously requested changes Oct 9, 2019

View reviewed changes

gilles-peskine-arm self-assigned this Oct 9, 2019

Change PSA_DH_GROUP_CUSTOM to not be in the vendor-defined range

43326f0

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 Oct 10, 2019

Patater reviewed Oct 11, 2019

View reviewed changes

Patater approved these changes Oct 11, 2019

View reviewed changes

Patater merged commit cb5fa8b into ARMmbed:development Oct 11, 2019

gilles-peskine-arm mentioned this pull request Oct 11, 2019

Update crypto submodule Mbed-TLS/mbedtls#2887

Merged

Include IANA reference in the definition of ECC curves and DH groups #279

Include IANA reference in the definition of ECC curves and DH groups #279

Uh oh!

Conversation

athoelke commented Sep 24, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm Oct 3, 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!

athoelke Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

athoelke Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

athoelke commented Oct 9, 2019

Uh oh!

gilles-peskine-arm commented Oct 9, 2019

Uh oh!

gilles-peskine-arm commented Oct 9, 2019

Uh oh!

gilles-peskine-arm commented Oct 9, 2019

Uh oh!

gilles-peskine-arm commented Oct 9, 2019

Uh oh!

athoelke commented Oct 11, 2019

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Patater Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

athoelke Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gilles-peskine-arm Oct 3, 2019 •

edited

Loading