Skip to content

Allow ECDH curve selection (WIP)#1793

Open
Varbin wants to merge 5 commits intoh2o:masterfrom
Varbin:varbin/curves
Open

Allow ECDH curve selection (WIP)#1793
Varbin wants to merge 5 commits intoh2o:masterfrom
Varbin:varbin/curves

Conversation

@Varbin
Copy link
Contributor

@Varbin Varbin commented Jun 17, 2018

This PR is a fix for #713 (How can I use a "stronger" ECDH curve?).

Current state:

  • It works, but details could be improved.

Changes:

  • The config option ecdh-curves allows to set curves for the ECDH key exchange.
  • Possible options are:
    • auto: Calls SSL_CTX_set_ecdh_auto if required and possible. It falls back to prime256v1.
    • a list of curve names (e.g. brainpoolP512r1:brainpoolP384r1:brainpoolP256r1:prime256v1) which are passed to SSL_CTX_set1_curves_list if possible. This requires a fairly recent OpenSSL/LibreSSL version. In recent OpenSSL versions TLS 1.3 group names may work, too.
    • a single curve name (e.g. prime256v1) which is passed to SSL_CTX_set1_curves_list or the combination OBJ_sn2nid->EC_KEY_new_by_curve_name->SSL_CTX_set_tmp_ecdh. While it sets only a single curve, it will work with any OpenSSL supporting ECDH.
      Update: Not anymore. Usage of OpenSSL 1.0.1 is discouraged. OpenSSL 1.0.1 is not supported by the OpenSSL-team anymore.
  • h2o will crash on errors while setting up ECC-curves for ECDH.
  • Update: The option SSL_OP_SINGLE_ECDH_USE will be set if possible.

To-Do:

  • add tests (how?)
  • update documentation (how?)
  • solve questions below

Questions (@kazuho):

  • Should the option SSL_CTX_set_options(ssl_ctx, SSL_OP_SINGLE_ECDH_USE) be set? A similar one is set for normal Diffie-Hellman.
    Solved: Will be set.
  • Should a better default than auto be set? On LibreSSL auto is a good default. Good curves might not get detected by a simple detection script which may leave the default a better one. On the hand auto can be bad: Currently on (older) OpenSSL it allows old 160-bit curves by default.
    Solved: No (current behavior).
  • Shall a warning be issued if SSL_CTX_set_ecdh_auto is not available and auto falls back to prime256v1?
    Solved: No (current behavior).

Varbin added 3 commits June 17, 2018 23:29
The new signature is setup_ecc_key(SSL_CTX, const char *). The additional argument can have one of the following values:

 - "auto": Follow the old behaviour to enable the default list of ECC curves if possible, which may be not-so-secure (see h2o#713).
 - A list of curve names or NIDs, seperated by a colon (e.g.: "P-256:brainpoolP512r"). A fairly recent version of Open/LibreSSL is required. With OpenSSL 1.1.0 group names like "ffdhe4096" may work for TLS 1.3, too.
 - A short name of a curve (e.g. "brainpoolP512r"). This will work with any OpenSSL version supporting ECDH.

If the linked Open/LibreSSL does not support ECDHE, the silently does nothing. This may change in the future. If anything fails, the function returns but h2o won't crash.

The behaviour is more or less identic to nginx'. In the current state the curve settings are compile time only.
The config key is a subkey of the ssl tree. Its value is passed to setup_ecc_key(SSL_CTX, const char *).

Minor changes:
 - A proper error, referencing the line in the config file, is shown if something fails.
 - h2o now crashes when there is an error with the ECDH setup.
Documentation now has information about the new config option in the SSL section.
@kazuho
Copy link
Member

kazuho commented Jun 18, 2018

This is something that we definitely need. Thank you very much for working on the PR.

Should the option SSL_CTX_set_options(ssl_ctx, SSL_OP_SINGLE_ECDH_USE) be set?

Yes. Please call the function whenever SSL_OP_SINGLE_ECDH_USE is defined.

Should a better default than auto be set? On LibreSSL auto is a good default. Good curves might not get detected by a simple detection script which may leave the default a better one. On the hand auto can be bad: Currently on (older) OpenSSL it allows old 160-bit curves by default.
Shall a warning be issued if SSL_CTX_set_ecdh_auto is not available and auto falls back to prime256v1?

IMO the principle should be that unless there is a bug, we should preserve the current behavior as-is, at the same time providing the users to change the curve being used. Considering that, how about an approach like below:

  • always call SSL_CTX_set_ecdh_auto if it is available, or set prime256v1 if not
  • then, if curves are specified by the user, call SSL_CTX_set1_curves_list if it is available, or generate an error if it is unavailable

Note that you need to call SSL_CTX_set_ecdh_auto even if you are calling SSL_CTX_set1_curves_list, see https://bugs.ruby-lang.org/issues/12324.

@Varbin
Copy link
Contributor Author

Varbin commented Jun 18, 2018

then, if curves are specified by the user, call SSL_CTX_set1_curves_list if it is available, or generate an error if it is unavailable

Shouldn't it be possible to set a single curve if SSL_CTX_set1_curves_list isn't provided? It might be required by certain standards to use h2o (e.g. PCI DSS which disallows 160 bit curves) and unfortunately some server software can be really old (e.g. Debian Jessie with OpenSSL 1.0.1).

On the other hand it might create some confusion why a complete list cannot be passed.

@kazuho
Copy link
Member

kazuho commented Jun 18, 2018

@Varbin

Shouldn't it be possible to set a single curve if SSL_CTX_set1_curves_list isn't provided? It might be required by certain standards to use h2o (e.g. PCI DSS which disallows 160 bit curves) and unfortunately some server software can be really old (e.g. Debian Jessie with OpenSSL 1.0.1).

My understanding is that both SSL_CTX_set_ecdh_auto and SSL_CTX_set1_curves_list appeared in OpenSSL 1.0.2, and the call to the former is required if we want to use ECDH in any form (that's what I have read from https://bugs.ruby-lang.org/issues/12324, maybe I am wrong).

Assuming that is correct, I'd anticipate that our users willing to prohibit 160-bit curves (which are enabled by SSL_CTX_set_ecdh_auto) can do so by explicitly specifying the curves they want.

(And FWIW, I am fine with disabling 160-bit curve by default, possibly by having our own list of default curves).

I do not think that we would want to support OpenSSL 1.0.1 and earlier, considering the fact that they do not support ALPN (which is a prerequisite for HTTP/2). Note that Chromium has dropped support for NPN (the predecessor of ALPN) in 2016 (see https://blog.chromium.org/2016/02/transitioning-from-spdy-to-http2.html).

@Varbin
Copy link
Contributor Author

Varbin commented Jun 18, 2018

The problem with a self created default list is, that compared to a cipher list, curves cannot be excluded. So it will be difficult to keep it up to date so auto seems to be the right choice here (curve detection is possible with OBJ_sn2nid but takes a lot of work and might make it too complicated. This might be topic for a future PR.

SSL_CTX_set_ecdh_auto will already always be called whenever possible (OpenSSL 1.1.0+ does not require it anymore, as it will be set by default).

The current code in this PR supports OpenSSL 1.0.1 and earlier.

OpenSSL 1.0.1 is outdated. As suggested by @kazuho setting a custom curve (besides "auto") is not supported with OpenSSL <= 1.0.1 anymore.

Theis requirement is documented.
@Varbin
Copy link
Contributor Author

Varbin commented Jun 27, 2018

@kazuho As you suggested, custom curves cannot be set with OpenSSL <= 1.0.1 anymore.

About tests: Is there some trivial way to test curves from picotest + Test:More? I could write one with Bash + testssl.sh and as it supports JSON output is supported. But I guess this won't integrate well into the testing framework.

@Varbin
Copy link
Contributor Author

Varbin commented Jul 23, 2018

Idea for tests: Try connections with different parameters against different server configurations.

server curvesclient curvesconnection
default (auto)default
P-384:P-521P-256
P-384:P-521P-384
P-384:P-521P-521

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants