Build with only montgomery curves (+ DJB configuration) #2013

gilles-peskine-arm · 2018-09-12T14:10:11Z

Fix compile-time checks to allow building with ECP enabled, but no short Weierstrass curve (i.e. with only Montgomery curves: Curve25519 or Curve448). Fix #941, #1412.

I wasn't sure how to frame a non-regression test. scripts/curves.pl didn't catch this because it tries building with all-curves-but-one. This is the right way to do what this script was designed to do, which is to catch test cases that are missing a dependency on a curve, including tests that depend on multiple curves. Maybe we should also try building with a single curve, for each curve?

What I've done in this PR is to add a new reference configuration with only “DJB” crypto and NIST hashes: SHA-256, SHA-512, HMAC_DRBG, Chacha/Poly, and ECDH/ECDSA with Curve25519. I'm not sure if that configuration makes sense for the time being though, since we don't fully support Curve25519 for ECDSA and TLS. Suggestions welcome.

Fix a spurious dependency requirement that prevented a build with no elliptic curve other than Curve25519 and Curve448.

Fix the build when the only elliptic curves are Montgomery curves.

We declare the function unconditionally, but it returns FEATURE_UNAVAILABLE for Montgomery curves.

Fix warning about unused functions when the only elliptic curves are Montgomery curves.

In the TLS test programs ssl_client2 and ssl_server2, use HMAC_DRBG if CTR_DRBG is not available.

Patater · 2018-09-12T16:56:50Z

library/ecp.c

+    (void) P;
+    (void) n;
+    (void) Q;
+    return( MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE );


Why do we prefer a run-time error in this case to a link-time error?

Because the function existed and I don't want to remove a function in a minor version.

I'm don't think compatibility considerations apply here, as they would only matter for a non-default configuration that was simply not possible before (and never was). Prior to that change, with only Mongomery curves, you couldn't build ecp.c, so you didn't have that function either. After that change, with only Montgomery curves, you would start having the rest of the module but no that function, which you didn't have anyway. So I think I would prefer compile-time exclusion here.

Note that removing this function at compile-time also means making ECDSA dependent on at least one ECDSA-capable curve (currently, only Short Weierstrass curves) to be defined, because ECDSA uses muladd().

Patater · 2018-09-12T17:03:18Z

tests/suites/test_suite_cipher.chacha20.data

@@ -1,7 +1,3 @@
-Decrypt empty buffer
-depends_on:MBEDTLS_CHACHA20_C
-dec_empty_buf:


I don't understand why we can't have a dec_empty_buf test anymore. Should we instead parameterize the test to so it uses a cipher parameter?

It seems like we are reducing the intended test coverage here by removing this test. (I think we intended to have an empty buffer test that used CHACHA20.)

The test uses AES-CBC internally. We were running it multiple times, once from the AES test suite and once again from other test suites. Now we're not running it from the cipher.chacha20 and cipher.chachapoly test suites anymore, only from cipher.aes.

If we want an empty buffer test that uses a different cipher, we need to pass the cipher as a parameter. That's well out of scope of this PR.

BTW I don't think DJB likes HMAC_DRBG either, but CTR_DRBG and HMAC_DRBG are all we have. HMAC_DRBG builds on SHA2 which is pretty much indispensable anyway.

I agree with Gilles that removing the test does not reduce the actual test coverage (which might indeed differ from the intended one, though I'm not sure our intentions here were very clear), so I think it would better be handled as a separate issue, in order to avoid feature creep in this one.

BTW I don't think DJB likes HMAC_DRBG either, but CTR_DRBG and HMAC_DRBG are all we have. HMAC_DRBG builds on SHA2 which is pretty much indispensable anyway.

I assume DJB would prefer a DRBG based on ChaCha20, but as you say we don't have it (and I'm not sure one is standardized, so that could be risky to implement), and I think SHA-2 is the part of NIST Crypto that is also approved by DJB (used in Ed25519 for example), so as you say, since HMAC_DRBG builds on it... Btw, from my perspective the main benefit of a DRBG based on Chacha20 rather than HMAC would be speed, as otherwise both should have similar security properties.

OK, noted as separate issue in #2014

Patater · 2018-09-12T17:05:10Z

programs/ssl/ssl_client2.c

@@ -39,12 +39,22 @@

 #if !defined(MBEDTLS_ENTROPY_C) || \
    !defined(MBEDTLS_SSL_TLS_C) || !defined(MBEDTLS_SSL_CLI_C) || \
-    !defined(MBEDTLS_NET_C) || !defined(MBEDTLS_CTR_DRBG_C)
+    !defined(MBEDTLS_NET_C)


What does Support HMAC_DRBG in the TLS test programs have to do with Build with only montgomery curves? Shouldn't this commit be in a different PR?

I wanted to enable the SSL test programs to at least build in the “DJB-only” configuration. DJB-only means no AES and therefore no CTR_DRBG.

Patater · 2018-09-12T17:07:22Z

I like the approach to testing this by adding a new configuration with "only 'DJB' crypto and NIST hashes".

mpg

I'm happy with the general approach. Other that a typo, my only real request for change is that I'd rather muladd() be removed at compile-time if no curve usable with it is defined.

mpg · 2018-09-13T07:54:25Z

programs/ssl/ssl_client2.c

+{
+    mbedtls_printf("MBEDTLS_CTR_DRBG_C and MBEDTLS_HMAC_DRBG_C not defined, "
+                   "or MBEDTLS_HMAC_DRBG_C defined without "
+                   "MBEDTLS_SHA256_C or MBEDTLS_512_C.\n");


Typo: SHA512

mpg · 2018-09-13T07:57:27Z

configs/config-montgomery.h

+ * Distinguishing features:
+ * - no RSA or classic DH, fully based on ECC
+ * - no NIST curves
+ *


also:

no AES (I think that's worth pointing out)

currently not usable, while waiting for some level of support of RFC 8422 (using X25519 and X448 in ECDH in TLS).

mpg · 2018-09-13T07:59:31Z

configs/config-montgomery.h

+#define MBEDTLS_PEM_PARSE_C
+
+/* Save RAM at the expense of ROM */
+#define MBEDTLS_AES_ROM_TABLES


Doesn't really make sense in an AES-less config, does it? While it technically doesn't hurt, I think it's confusing, if we assume these config files are not just for testing but also examples for users.

mpg · 2018-09-13T08:00:24Z

configs/config-montgomery.h

+
+/* Save RAM at the expense of speed, see ecp.h */
+#define MBEDTLS_ECP_WINDOW_SIZE        2
+#define MBEDTLS_ECP_FIXED_POINT_OPTIM  0


Only applicable to short-Weierstrass curves. Again, not technically relevant, but could be confusing for humans.

mpg · 2018-09-13T08:03:47Z

configs/config-montgomery.h

+
+/* Save ROM and a few bytes of RAM by specifying our own ciphersuite list */
+#define MBEDTLS_SSL_CIPHERSUITES                        \
+    MBEDTLS_TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, \


Note: this one will need more complete support of RFC 8422 (EdDSA), and also support of https://tools.ietf.org/html/draft-ietf-curdle-pkix-10 for the certificate chain if I'm not mistaken (unless of course one only uses explicitly-trusted self-signed certificates, in which case one would be better off using raw public keys, but we don't support that yet).

gilles-peskine-arm · 2018-09-14T16:22:44Z

With only Montgomery curves in the build, the ECP self-test fails, because mbedtls_ecp_curve_list only lists short Weierstrass curves. We're tracking this as #464.

gilles-peskine-arm · 2018-09-14T16:27:04Z

For several reasons, I've changed my mind as to how to test this.

Defining an ad hoc configuration lets us test one specific configuration that is of some interest. However, the configuration in question is of limited interest, because you can't use Montgomery curves for anything directly useful yet: the ECDH and ECDSA module don't support them, and they can't be used in TLS at all.
We normally keep supporting the sample configurations for a long time. The configuration in config-montgomery.h is fairly ad hoc and is likely to evolve as our support for Montgomery curves improves.
The limited configuration in config-montgomery.h didn't catch that the ECP self-test was failing since it didn't include MBEDTLS_SELF_TEST. This may happen with other options as well.

I think I'll switch to my other idea: modify curves.pl to test each single-curve configuration in addition to testing each all-curves-but-one configuration.

gilles-peskine-arm · 2018-09-17T19:37:27Z

Superseded by #2019.

#2019 doesn't have "Support HMAC_DRBG in the TLS test programs". I'll make a separate PR for that.

gilles-peskine-arm added 8 commits September 12, 2018 16:00

Add Curve25519 and Curve448 to check_config.h

b1b2d90

Fix a spurious dependency requirement that prevented a build with no elliptic curve other than Curve25519 and Curve448.

Add missing #ifdef around Short Weierstrass-specific code

ddc8032

Fix the build when the only elliptic curves are Montgomery curves.

Note that mbedtls_ecp_mul_add is only defined for Short Weierstrass

edff4df

We declare the function unconditionally, but it returns FEATURE_UNAVAILABLE for Montgomery curves.

Add missing #ifdef around Short Weierstrass-specific code

168209c

Fix warning about unused functions when the only elliptic curves are Montgomery curves.

Add ChangeLog entry for no-short-Weierstrass build fix

6bbec3b

Remove AES-only test from the cipher.chacha* test suites

d82a19e

Support HMAC_DRBG in the TLS test programs

32e6ad8

In the TLS test programs ssl_client2 and ssl_server2, use HMAC_DRBG if CTR_DRBG is not available.

New reference configuration with ChaCha/Poly and Curve25519

29efeea

gilles-peskine-arm added bug mbed TLS team needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces component-platform Portability layer and build scripts labels Sep 12, 2018

gilles-peskine-arm requested review from mpg and Patater September 12, 2018 14:10

Patater suggested changes Sep 12, 2018

View reviewed changes

mpg requested changes Sep 13, 2018

View reviewed changes

Patater mentioned this pull request Sep 13, 2018

dec_empty_buf() is not parameterized based on cipher #2014

Closed

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 14, 2018

gilles-peskine-arm changed the title ~~Build with only montgomery curves~~ Build with only montgomery curves (+ DJB configuration) Sep 17, 2018

gilles-peskine-arm mentioned this pull request Sep 17, 2018

Build with only Montgomery curves (conditional mul_add) #2019

Merged

gilles-peskine-arm closed this Sep 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build with only montgomery curves (+ DJB configuration) #2013

Build with only montgomery curves (+ DJB configuration) #2013

gilles-peskine-arm commented Sep 12, 2018

Patater Sep 12, 2018

gilles-peskine-arm Sep 12, 2018

mpg Sep 13, 2018

Patater Sep 12, 2018

gilles-peskine-arm Sep 12, 2018

gilles-peskine-arm Sep 12, 2018

mpg Sep 13, 2018

Patater Sep 13, 2018

Patater Sep 12, 2018

gilles-peskine-arm Sep 12, 2018

Patater Sep 13, 2018

Patater commented Sep 12, 2018

mpg left a comment

mpg Sep 13, 2018

mpg Sep 13, 2018

mpg Sep 13, 2018

mpg Sep 13, 2018

mpg Sep 13, 2018

gilles-peskine-arm commented Sep 14, 2018

gilles-peskine-arm commented Sep 14, 2018

gilles-peskine-arm commented Sep 17, 2018

Build with only montgomery curves (+ DJB configuration) #2013

Build with only montgomery curves (+ DJB configuration) #2013

Conversation

gilles-peskine-arm commented Sep 12, 2018

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

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

Patater commented Sep 12, 2018

mpg 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

gilles-peskine-arm commented Sep 14, 2018

gilles-peskine-arm commented Sep 14, 2018

gilles-peskine-arm commented Sep 17, 2018