New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specification of ECC named curve used in ECDH key exchange #14

Open
BenBE opened this Issue Nov 9, 2015 · 2 comments

Comments

Projects
None yet
3 participants
@BenBE

BenBE commented Nov 9, 2015

In recent versions ejabberd allows for the DH parameters to be set in its configuration. But a similar setting for the ECDH parameters is missing. In a quick shot, of which the results are included below, I tried to add this feature in p1_tls, so it can be used in ejabberd.

The intention is to allow the used named curve for ECC to be specified in the ejabberd config by providing its name in a setting like "ecdh_curvename" or "s2s_ecdh_curvename" (for s2s).

diff --git a/c_src/p1_tls_drv.c b/c_src/p1_tls_drv.c
index 30c5e62..010e830 100644
--- a/c_src/p1_tls_drv.c
+++ b/c_src/p1_tls_drv.c
@@ -55,6 +55,7 @@ typedef unsigned __int32 uint32_t;
 #endif

 #define CIPHERS "DEFAULT:!EXPORT:!LOW:!RC4:!SSLv2"
+#define NID_ECDH_DEFAULTCURVE NID_X9_62_prime256v1

 /**
  * Prepare the SSL options flag.
@@ -326,15 +327,23 @@ static int verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
  * for details.
  */
 #ifndef OPENSSL_NO_ECDH
-static void setup_ecdh(SSL_CTX *ctx)
+static void setup_ecdh(SSL_CTX *ctx, char* ecdh_curvename)
 {
+   int nidCurve;
    EC_KEY *ecdh;

    if (SSLeay() < 0x1000005fL) {
       return;
    }

-   ecdh = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
+   if (ecdh_curvename && (( nidCurve = OBJ_sn2nid(ecdh_curvename) ))) {
+      ecdh = EC_KEY_new_by_curve_name(nidCurve);
+   }
+
+   if (ecdh == NULL) {
+      ecdh = EC_KEY_new_by_curve_name(NID_ECDH_DEFAULTCURVE);
+   }
+
    SSL_CTX_set_options(ctx, SSL_OP_SINGLE_ECDH_USE);
    SSL_CTX_set_tmp_ecdh(ctx, ecdh);

@@ -537,10 +546,13 @@ static ErlDrvSSizeT tls_drv_control(ErlDrvData handle,
     size_t protocol_options_len = strlen(protocol_options);
     char *dh_file = protocol_options + protocol_options_len + 1;
     size_t dh_file_len = strlen(dh_file);
+    char *ecdh_curvename = dh_file + dh_file_len + 1;
+    size_t ecdh_curvename_len = strlen(ecdh_curvename);
     char *hash_key = (char *)driver_alloc(key_file_len +
                           ciphers_len +
                           protocol_options_len +
-                          dh_file_len + 1);
+                          dh_file_len +
+                          ecdh_curvename + 1);
     long options = 0L;

     if (protocol_options_len != 0) {
@@ -556,13 +568,16 @@ static ErlDrvSSizeT tls_drv_control(ErlDrvData handle,
        free(popts);
     }

-    sprintf(hash_key, "%s%s%s%s", key_file, ciphers, protocol_options,
-        dh_file);
+    sprintf(hash_key, "%s%s%s%s%s", key_file, ciphers, protocol_options,
+        dh_file, ecdh_curvename);
     SSL_CTX *ssl_ctx = hash_table_lookup(hash_key, &key_mtime, &dh_mtime);

     if (dh_file_len == 0)
        dh_file = NULL;

+    if (ecdh_curvename_len == 0)
+       ecdh_curvename = NULL;
+
     if (is_modified(key_file, &key_mtime) ||
         is_modified(dh_file, &dh_mtime) ||
         ssl_ctx == NULL)
@@ -588,7 +603,7 @@ static ErlDrvSSizeT tls_drv_control(ErlDrvData handle,
        SSL_CTX_set_cipher_list(ctx, ciphers);

 #ifndef OPENSSL_NO_ECDH
-       setup_ecdh(ctx);
+       setup_ecdh(ctx, ecdh_curvename);
 #endif
 #ifndef OPENSSL_NO_DH
        res = setup_dh(ctx, dh_file);
diff --git a/src/p1_tls.erl b/src/p1_tls.erl
index 74158cc..73eb1aa 100644
--- a/src/p1_tls.erl
+++ b/src/p1_tls.erl
@@ -145,11 +145,19 @@ tcp_to_tls(TCPSocket, Options) ->
                        false ->
                            <<>>
                    end,
+          ECCurveName = case lists:keysearch(eccurvename, 1, Options) of
+                       {value, {eccurvename, E}} ->
+                           iolist_to_binary(E);
+                       false ->
+                           <<>>
+                   end,
           CertFile1 = iolist_to_binary(CertFile),
      case catch port_control(Port, Command bor Flags,
-                 <<CertFile1/binary, 0, Ciphers/binary,
-                   0, ProtocolOpts/binary, 0, DHFile/binary,
-                   0>>)
+                 <<CertFile1/binary, 0,
+                   Ciphers/binary, 0,
+                   ProtocolOpts/binary, 0,
+                   DHFile/binary, 0,
+                   ECCurveName/binary, 0>>)
          of
        {'EXIT', {badarg, _}} -> {error, einval};
        <<0>> ->

I know this is by far not perfect, but should provide a good starting point for further developments.

Thanks to emias (IRC) for reviewing of and commenting on this initial PoC.

@stef

This comment has been minimized.

Show comment
Hide comment
@stef

stef Jan 2, 2016

this would be nice to be able to configure on a more granular level, so that i can specify multiple curves at least. different servers unfortunately use different curves, like prime256v1 or secp521r1. having different curves prohibits s2s interoperability.

stef commented Jan 2, 2016

this would be nice to be able to configure on a more granular level, so that i can specify multiple curves at least. different servers unfortunately use different curves, like prime256v1 or secp521r1. having different curves prohibits s2s interoperability.

@fancycode

This comment has been minimized.

Show comment
Hide comment
@fancycode

fancycode Feb 9, 2017

With OpenSSL 1.1.0 you could even configure a list of curves to support:
https://www.openssl.org/docs/man1.1.0/ssl/SSL_CTX_set1_curves_list.html

fancycode commented Feb 9, 2017

With OpenSSL 1.1.0 you could even configure a list of curves to support:
https://www.openssl.org/docs/man1.1.0/ssl/SSL_CTX_set1_curves_list.html

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