Signed-off-by: Jan Just Keijser <[email protected]...>

PATCH In review Jan Just Keijser 3 participants 7 messages Feb 7, 2012 – May 8, 2012

lore

Share & tools

lore

Jan Just Keijser

author

2012-02-07 15:13 UTC #0 lore raw

PATCH 1/1 Signed-off-by: Jan Just Keijser <[email protected]...>

Added support for Elliptic curves (ECDSA) + SHA2 family signed certificates.

---
 init.c         |    7 ++++
 options.c      |   15 ++++++++++
 options.h      |    6 ++++
 ssl.c          |    3 ++
 ssl_backend.h  |   10 ++++++
 ssl_openssl.c  |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ssl_polarssl.c |    9 ++++++
 7 files changed, 134 insertions(+), 0 deletions(-)

init.c +7 -0

index 525f441..51b0d64 100644
@@ -895,6 +895,9 @@ print_openssl_info (const struct options *options)
   if (options->show_ciphers || options->show_digests || options->show_engines
 #ifdef USE_SSL
       || options->show_tls_ciphers
+#ifdef USE_SSL_EC
+|| options->show_curves
+#endif
 #endif
     )
     {

@@ -907,6 +910,10 @@ print_openssl_info (const struct options *options)
 #ifdef USE_SSL
       if (options->show_tls_ciphers)
 	show_available_tls_ciphers ();
+#ifdef USE_SSL_EC
+      if (options->show_curves)
+    show_available_curves ();
+#endif
 #endif
       return true;
     }

options.c +15 -0

index 6b8ae22..ce23dbc 100644
@@ -836,6 +836,9 @@ init_options (struct options *o, const bool init_gc)
 #ifdef ENABLE_X509ALTUSERNAME
   o->x509_username_field = X509_USERNAME_FIELD_DEFAULT;
 #endif
+#ifdef USE_SSL_EC
+  o->curve_name = NULL;
+#endif
 #endif /* USE_SSL */
 #endif /* USE_CRYPTO */
 #ifdef ENABLE_PKCS11

@@ -6368,6 +6371,18 @@ add_option (struct options *options,
       VERIFY_PERMISSION (OPT_P_GENERAL);
       options->show_tls_ciphers = true;
     }
+#ifdef USE_SSL_EC
+  else if (streq (p[0], "show-curves"))
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      options->show_curves = true;
+    }
+  else if (streq (p[0], "ecdh") && p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      options->curve_name= p[1];
+    }
+#endif
   else if (streq (p[0], "tls-server"))
     {
       VERIFY_PERMISSION (OPT_P_GENERAL);

options.h +6 -0

index 831d4f6..81e0757 100644
@@ -200,6 +200,9 @@ struct options
   bool show_engines;
 #ifdef USE_SSL
   bool show_tls_ciphers;
+#ifdef USE_SSL_EC
+  bool show_curves;
+#endif
 #endif
   bool genkey;
 #endif

@@ -533,6 +536,9 @@ struct options
   const char *priv_key_file;
   const char *pkcs12_file;
   const char *cipher_list;
+#ifdef USE_SSL_EC
+  const char *curve_name;
+#endif
   const char *tls_verify;
   const char *tls_export_cert;
   const char *tls_remote;

ssl.c +3 -0

index c26756e..54efe2f 100644
@@ -308,6 +308,9 @@ init_ssl (const struct options *options, struct tls_root_ctx *new_ctx)
     {
       tls_ctx_server_new(new_ctx);
       tls_ctx_load_dh_params(new_ctx, options->dh_file, options->dh_file_inline);
+#ifdef USE_SSL_EC
+      tls_ctx_load_ecdh_params(new_ctx, options->curve_name);
+#endif
     }
   else				/* if client */
     {

ssl_backend.h +10 -0

index 243c9e3..ebf9f36 100644
@@ -145,6 +145,16 @@ void tls_ctx_load_dh_params(struct tls_root_ctx *ctx, const char *dh_file
     );
 
 /**
+ * Load Elliptic Curve Parameters, and load them into the library-specific
+ * TLS context.
+ *
+ * @param ctx			TLS context to use
+ * @param curve_name	The name of the elliptic curve to load.
+ */
+void tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
+    );
+
+/**
  * Load PKCS #12 file for key, cert and (optionally) CA certs, and add to
  * library-specific TLS context.
  *

ssl_openssl.c +84 -0

102 lines not on this page · raw message

ssl_polarssl.c +9 -0

index c50cf0a..a7a6d61 100644
@@ -218,6 +218,15 @@ else
       (counter_type) 8 * mpi_size(&ctx->dhm_ctx->P));
 }
 
+#ifdef USE_SSL_EC
+void
+tls_ctx_load_ecdh_params (struct tls_root_ctx *ctx, const char *curve_name
+    )
+{
+    msg(M_WARN, "Elliptic Curves not yet supported by PolarSSL");
+}
+#endif
+
 int
 tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char *pkcs12_file,
 #if ENABLE_INLINE_FILES

Signature · 1 line

1.7.4.4

Alon Bar-Lev

2012-02-07 16:09 UTC #1 lore raw

This is nice!

Some questions:

1. Why not enable this automatically if openssl is sufficient version? There is no point in not using this if available.

2. I would have liked to see this in negotiation as well, so server will use EC if supported by the client and fallback if not... This way migration path can take place.

Alon.

Quoted 282 lines · Jan Just Keijser

Added support for Elliptic curves (ECDSA) + SHA2 family signed certificates.

---
 init.c         |    7 ++++
 options.c      |   15 ++++++++++
 options.h      |    6 ++++
 ssl.c          |    3 ++
 ssl_backend.h  |   10 ++++++
 ssl_openssl.c  |   84
++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ssl_polarssl.c |    9 ++++++
 7 files changed, 134 insertions(+), 0 deletions(-)

init.c +0 -0

index 525f441..51b0d64 100644
@@ -895,6 +895,9 @@ print_openssl_info (const struct options *options)
  if (options->show_ciphers || options->show_digests ||

options->show_engines

 #ifdef USE_SSL
      || options->show_tls_ciphers

init.c +7 -0

+#ifdef USE_SSL_EC
+|| options->show_curves
+#endif
 #endif
    )
    {

@@ -907,6 +910,10 @@ print_openssl_info (const struct options *options)
 #ifdef USE_SSL
      if (options->show_tls_ciphers)
       show_available_tls_ciphers ();
+#ifdef USE_SSL_EC
+      if (options->show_curves)
+    show_available_curves ();
+#endif
 #endif
      return true;
    }

options.c +15 -0

index 6b8ae22..ce23dbc 100644
@@ -836,6 +836,9 @@ init_options (struct options *o, const bool init_gc)
 #ifdef ENABLE_X509ALTUSERNAME
  o->x509_username_field = X509_USERNAME_FIELD_DEFAULT;
 #endif
+#ifdef USE_SSL_EC
+  o->curve_name = NULL;
+#endif
 #endif /* USE_SSL */
 #endif /* USE_CRYPTO */
 #ifdef ENABLE_PKCS11

@@ -6368,6 +6371,18 @@ add_option (struct options *options,
      VERIFY_PERMISSION (OPT_P_GENERAL);
      options->show_tls_ciphers = true;
    }
+#ifdef USE_SSL_EC
+  else if (streq (p[0], "show-curves"))
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      options->show_curves = true;
+    }
+  else if (streq (p[0], "ecdh") && p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      options->curve_name= p[1];
+    }
+#endif
  else if (streq (p[0], "tls-server"))
    {
      VERIFY_PERMISSION (OPT_P_GENERAL);

options.h +6 -0

index 831d4f6..81e0757 100644
@@ -200,6 +200,9 @@ struct options
  bool show_engines;
 #ifdef USE_SSL
  bool show_tls_ciphers;
+#ifdef USE_SSL_EC
+  bool show_curves;
+#endif
 #endif
  bool genkey;
 #endif

@@ -533,6 +536,9 @@ struct options
  const char *priv_key_file;
  const char *pkcs12_file;
  const char *cipher_list;
+#ifdef USE_SSL_EC
+  const char *curve_name;
+#endif
  const char *tls_verify;
  const char *tls_export_cert;
  const char *tls_remote;

ssl.c +3 -0

index c26756e..54efe2f 100644
@@ -308,6 +308,9 @@ init_ssl (const struct options *options, struct
 tls_root_ctx *new_ctx)
    {
      tls_ctx_server_new(new_ctx);
      tls_ctx_load_dh_params(new_ctx, options->dh_file,
 options->dh_file_inline);
+#ifdef USE_SSL_EC
+      tls_ctx_load_ecdh_params(new_ctx, options->curve_name);
+#endif
    }

  else                         /* if client */
    {

ssl_backend.h +0 -0

index 243c9e3..ebf9f36 100644
@@ -145,6 +145,16 @@ void tls_ctx_load_dh_params(struct tls_root_ctx *ctx,

const char *dh_file

);

/**
+ * Load Elliptic Curve Parameters, and load them into the library-specific
+ * TLS context.
+ *
+ * @param ctx TLS context to use
+ * @param curve_name The name of the elliptic curve to load.
+ */
+void tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name

ssl_backend.h +3 -0

+    );
+
+/**
 * Load PKCS #12 file for key, cert and (optionally) CA certs, and add to
 * library-specific TLS context.
 *

ssl_openssl.c +3 -0

index b95944c..912dd8f 100644
@@ -50,6 +50,9 @@
 #include <openssl/pkcs12.h>
 #include <openssl/x509.h>
 #include <openssl/crypto.h>
+#ifdef USE_SSL_EC
+#include <openssl/ec.h>
+#endif
 
 /*
 * Allocate space in SSL objects in which to store a struct tls_session

@@ -238,6 +241,46 @@ tls_ctx_load_dh_params (struct tls_root_ctx *ctx,

const char *dh_file
DH_free (dh);
}

ssl_openssl.c +37 -0

+void
+tls_ctx_load_ecdh_params (struct tls_root_ctx *ctx, const char *curve_name
+    )
+{
+#ifdef USE_SSL_EC
+  if (curve_name != NULL)
+  {
+    int nid;
+    EC_KEY   *ecdh  = NULL;
+
+    nid = OBJ_sn2nid(curve_name);
+
+    if (nid == 0)
+      msg(M_SSLERR, "unknown curve name (%s)", curve_name);
+    else
+    {
+      ecdh = EC_KEY_new_by_curve_name(nid);
+      if (ecdh == NULL)
+        msg (M_SSLERR, "Unable to create curve (%s)", curve_name);
+      else
+      {
+        const char *sname;
+
+        if (!SSL_CTX_set_tmp_ecdh(ctx->ctx, ecdh))
+          msg (M_SSLERR, "SSL_CTX_set_tmp_ecdh: cannot add curve");
+
+        /* Translate NID back to name , just for kicks */
+        sname   = OBJ_nid2sn(nid);
+        if (sname == NULL) sname = "(Unknown)";
+        msg (D_TLS_DEBUG_LOW, "ECDH curve %s added", sname);
+
+        EC_KEY_free(ecdh);
+      }
+    }
+  }
+#else
+  msg(M_SSLERR, "Elliptic Curves not supported by this version of

OpenSSL");

ssl_openssl.c +44 -0

+#endif
+}
+
 int
 tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char *pkcs12_file,
 #if ENABLE_INLINE_FILES

@@ -1273,6 +1316,47 @@ show_available_tls_ciphers ()
  SSL_CTX_free (ctx);
 }
 
+/*
+ *  * Show the Elliptic curves that are available for us to use
+ *   * in the OpenSSL library.
+ *    */
+#ifdef USE_SSL_EC
+void
+show_available_curves()
+{
+  EC_builtin_curve *curves = NULL;
+  size_t crv_len = 0;
+  size_t n = 0;
+
+  crv_len = EC_get_builtin_curves(NULL, 0);
+
+  curves = OPENSSL_malloc((int)(sizeof(EC_builtin_curve) * crv_len));
+
+  if (curves == NULL)
+    msg (M_SSLERR, "Cannot create EC_builtin_curve object");
+  else
+  {
+    if (EC_get_builtin_curves(curves, crv_len))
+    {
+      printf ("Available Elliptic curves:\n");
+      for (n = 0; n < crv_len; n++)
+      {
+        const char *sname;
+        sname   = OBJ_nid2sn(curves[n].nid);
+        if (sname == NULL) sname = "";
+
+        printf("%s\n", sname);
+      }
+    }
+    else
+    {
+      msg (M_SSLERR, "Cannot get list of builtin curves");
+    }
+    OPENSSL_free(curves);
+  }
+}
+#endif
+
 void
 get_highest_preference_tls_cipher (char *buf, int size)
 {

ssl_polarssl.c +9 -1

index c50cf0a..a7a6d61 100644
@@ -218,6 +218,15 @@ else
      (counter_type) 8 * mpi_size(&ctx->dhm_ctx->P));
 }
 
+#ifdef USE_SSL_EC
+void
+tls_ctx_load_ecdh_params (struct tls_root_ctx *ctx, const char *curve_name
+    )
+{
+    msg(M_WARN, "Elliptic Curves not yet supported by PolarSSL");
+}
+#endif
+
 int
 tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char *pkcs12_file,
 #if ENABLE_INLINE_FILES
--

1.7.4.4

------------------------------------------------------------------------------
Keep Your Developer Skills Current with LearnDevNow!
The most comprehensive online learning library for Microsoft developers is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3, Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-d2d
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Jan Just Keijser

author

2012-02-08 12:00 UTC #2 lore raw

Quoted 7 lines · Alon Bar-Lev

This is nice!
Some questions:
1. Why not enable this automatically if openssl is sufficient version? There is no point in not using this if available.

some distro's (notably RedHat) disable EC support by default; so it's not possible to check the OpenSSL version number. There is a '#define OPENSSL_NO_EC' that could be used, so the configure.ac script should check for that. As other SSL implementations might use a different #define to state that ECs are supported (or are NOT supported), I figure the decision whether to support ECs should be made by the configure.ac script, not by the openvpn code itself.

Quoted 4 lines

2. I would have liked to see this in negotiation as well, so server will use EC if supported by the client and fallback if not... This way migration path can take place.

The client won't even enter the negotiation phase without this patch: if the client and server are configured to use ECDSA+SHA512 certs and the 'ecdh' parameters are NOT set on the server then the initial TLS handshake fails.

cheers,

JJK

Quoted 281 lines

On Tue, Feb 7, 2012 at 5:13 PM, Jan Just Keijser <[email protected]... <mailto:[email protected]...>> wrote:

    Added support for Elliptic curves (ECDSA) + SHA2 family signed
    certificates.
    ---
     init.c         |    7 ++++
     options.c      |   15 ++++++++++
     options.h      |    6 ++++
     ssl.c          |    3 ++
     ssl_backend.h  |   10 ++++++
     ssl_openssl.c  |   84
    ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
     ssl_polarssl.c |    9 ++++++
     7 files changed, 134 insertions(+), 0 deletions(-)

    diff --git a/init.c b/init.c
    index 525f441..51b0d64 100644
    --- a/init.c
    +++ b/init.c
    @@ -895,6 +895,9 @@ print_openssl_info (const struct options *options)
      if (options->show_ciphers || options->show_digests ||
    options->show_engines
     #ifdef USE_SSL
          || options->show_tls_ciphers
    +#ifdef USE_SSL_EC
    +|| options->show_curves
    +#endif
     #endif
        )
        {
    @@ -907,6 +910,10 @@ print_openssl_info (const struct options
    *options)
     #ifdef USE_SSL
          if (options->show_tls_ciphers)
           show_available_tls_ciphers ();
    +#ifdef USE_SSL_EC
    +      if (options->show_curves)
    +    show_available_curves ();
    +#endif
     #endif
          return true;
        }
    diff --git a/options.c b/options.c
    index 6b8ae22..ce23dbc 100644
    --- a/options.c
    +++ b/options.c
    @@ -836,6 +836,9 @@ init_options (struct options *o, const bool
    init_gc)
     #ifdef ENABLE_X509ALTUSERNAME
      o->x509_username_field = X509_USERNAME_FIELD_DEFAULT;
     #endif
    +#ifdef USE_SSL_EC
    +  o->curve_name = NULL;
    +#endif
     #endif /* USE_SSL */
     #endif /* USE_CRYPTO */
     #ifdef ENABLE_PKCS11
    @@ -6368,6 +6371,18 @@ add_option (struct options *options,
          VERIFY_PERMISSION (OPT_P_GENERAL);
          options->show_tls_ciphers = true;
        }
    +#ifdef USE_SSL_EC
    +  else if (streq (p[0], "show-curves"))
    +    {
    +      VERIFY_PERMISSION (OPT_P_GENERAL);
    +      options->show_curves = true;
    +    }
    +  else if (streq (p[0], "ecdh") && p[1])
    +    {
    +      VERIFY_PERMISSION (OPT_P_CRYPTO);
    +      options->curve_name= p[1];
    +    }
    +#endif
      else if (streq (p[0], "tls-server"))
        {
          VERIFY_PERMISSION (OPT_P_GENERAL);
    diff --git a/options.h b/options.h
    index 831d4f6..81e0757 100644
    --- a/options.h
    +++ b/options.h
    @@ -200,6 +200,9 @@ struct options
      bool show_engines;
     #ifdef USE_SSL
      bool show_tls_ciphers;
    +#ifdef USE_SSL_EC
    +  bool show_curves;
    +#endif
     #endif
      bool genkey;
     #endif
    @@ -533,6 +536,9 @@ struct options
      const char *priv_key_file;
      const char *pkcs12_file;
      const char *cipher_list;
    +#ifdef USE_SSL_EC
    +  const char *curve_name;
    +#endif
      const char *tls_verify;
      const char *tls_export_cert;
      const char *tls_remote;
    diff --git a/ssl.c b/ssl.c
    index c26756e..54efe2f 100644
    --- a/ssl.c
    +++ b/ssl.c
    @@ -308,6 +308,9 @@ init_ssl (const struct options *options,
    struct tls_root_ctx *new_ctx)
        {
          tls_ctx_server_new(new_ctx);
          tls_ctx_load_dh_params(new_ctx, options->dh_file,
    options->dh_file_inline);
    +#ifdef USE_SSL_EC
    +      tls_ctx_load_ecdh_params(new_ctx, options->curve_name);
    +#endif
        }
      else                         /* if client */
        {
    diff --git a/ssl_backend.h b/ssl_backend.h
    index 243c9e3..ebf9f36 100644
    --- a/ssl_backend.h
    +++ b/ssl_backend.h
    @@ -145,6 +145,16 @@ void tls_ctx_load_dh_params(struct
    tls_root_ctx *ctx, const char *dh_file
        );

     /**
    + * Load Elliptic Curve Parameters, and load them into the
    library-specific
    + * TLS context.
    + *
    + * @param ctx                  TLS context to use
    + * @param curve_name   The name of the elliptic curve to load.
    + */
    +void tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const
    char *curve_name
    +    );
    +
    +/**
     * Load PKCS #12 file for key, cert and (optionally) CA certs, and
    add to
     * library-specific TLS context.
     *
    diff --git a/ssl_openssl.c b/ssl_openssl.c
    index b95944c..912dd8f 100644
    --- a/ssl_openssl.c
    +++ b/ssl_openssl.c
    @@ -50,6 +50,9 @@
     #include <openssl/pkcs12.h>
     #include <openssl/x509.h>
     #include <openssl/crypto.h>
    +#ifdef USE_SSL_EC
    +#include <openssl/ec.h>
    +#endif

     /*
     * Allocate space in SSL objects in which to store a struct
    tls_session
    @@ -238,6 +241,46 @@ tls_ctx_load_dh_params (struct tls_root_ctx
    *ctx, const char *dh_file
      DH_free (dh);
     }

    +void
    +tls_ctx_load_ecdh_params (struct tls_root_ctx *ctx, const char
    *curve_name
    +    )
    +{
    +#ifdef USE_SSL_EC
    +  if (curve_name != NULL)
    +  {
    +    int nid;
    +    EC_KEY   *ecdh  = NULL;
    +
    +    nid = OBJ_sn2nid(curve_name);
    +
    +    if (nid == 0)
    +      msg(M_SSLERR, "unknown curve name (%s)", curve_name);
    +    else
    +    {
    +      ecdh = EC_KEY_new_by_curve_name(nid);
    +      if (ecdh == NULL)
    +        msg (M_SSLERR, "Unable to create curve (%s)", curve_name);
    +      else
    +      {
    +        const char *sname;
    +
    +        if (!SSL_CTX_set_tmp_ecdh(ctx->ctx, ecdh))
    +          msg (M_SSLERR, "SSL_CTX_set_tmp_ecdh: cannot add curve");
    +
    +        /* Translate NID back to name , just for kicks */
    +        sname   = OBJ_nid2sn(nid);
    +        if (sname == NULL) sname = "(Unknown)";
    +        msg (D_TLS_DEBUG_LOW, "ECDH curve %s added", sname);
    +
    +        EC_KEY_free(ecdh);
    +      }
    +    }
    +  }
    +#else
    +  msg(M_SSLERR, "Elliptic Curves not supported by this version of
    OpenSSL");
    +#endif
    +}
    +
     int
     tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char
    *pkcs12_file,
     #if ENABLE_INLINE_FILES
    @@ -1273,6 +1316,47 @@ show_available_tls_ciphers ()
      SSL_CTX_free (ctx);
     }

    +/*
    + *  * Show the Elliptic curves that are available for us to use
    + *   * in the OpenSSL library.
    + *    */
    +#ifdef USE_SSL_EC
    +void
    +show_available_curves()
    +{
    +  EC_builtin_curve *curves = NULL;
    +  size_t crv_len = 0;
    +  size_t n = 0;
    +
    +  crv_len = EC_get_builtin_curves(NULL, 0);
    +
    +  curves = OPENSSL_malloc((int)(sizeof(EC_builtin_curve) * crv_len));
    +
    +  if (curves == NULL)
    +    msg (M_SSLERR, "Cannot create EC_builtin_curve object");
    +  else
    +  {
    +    if (EC_get_builtin_curves(curves, crv_len))
    +    {
    +      printf ("Available Elliptic curves:\n");
    +      for (n = 0; n < crv_len; n++)
    +      {
    +        const char *sname;
    +        sname   = OBJ_nid2sn(curves[n].nid);
    +        if (sname == NULL) sname = "";
    +
    +        printf("%s\n", sname);
    +      }
    +    }
    +    else
    +    {
    +      msg (M_SSLERR, "Cannot get list of builtin curves");
    +    }
    +    OPENSSL_free(curves);
    +  }
    +}
    +#endif
    +
     void
     get_highest_preference_tls_cipher (char *buf, int size)
     {
    diff --git a/ssl_polarssl.c b/ssl_polarssl.c
    index c50cf0a..a7a6d61 100644
    --- a/ssl_polarssl.c
    +++ b/ssl_polarssl.c
    @@ -218,6 +218,15 @@ else
          (counter_type) 8 * mpi_size(&ctx->dhm_ctx->P));
     }

    +#ifdef USE_SSL_EC
    +void
    +tls_ctx_load_ecdh_params (struct tls_root_ctx *ctx, const char
    *curve_name
    +    )
    +{
    +    msg(M_WARN, "Elliptic Curves not yet supported by PolarSSL");
    +}
    +#endif
    +
     int
     tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char
    *pkcs12_file,
     #if ENABLE_INLINE_FILES
    --
    1.7.4.4

Adriaan de Jong

2012-05-04 09:57 UTC #3 lore raw

In reply to Jan Just Keijser

Hi Janjust,

I've finally had the time to take a look at this patch with a colleague who is more familiar with the subject at hand :).

Hope this helps. Please see my comments inline.

Adriaan

Quoted 163 lines · Jan Just Keijser

Added support for Elliptic curves (ECDSA) + SHA2 family signed certificates.

---
 init.c         |    7 ++++
 options.c      |   15 ++++++++++
 options.h      |    6 ++++
 ssl.c          |    3 ++
 ssl_backend.h  |   10 ++++++
 ssl_openssl.c  |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ssl_polarssl.c |    9 ++++++
 7 files changed, 134 insertions(+), 0 deletions(-)

init.c +7 -0

index 525f441..51b0d64 100644
@@ -895,6 +895,9 @@ print_openssl_info (const struct options *options)
   if (options->show_ciphers || options->show_digests || options->show_engines
 #ifdef USE_SSL
       || options->show_tls_ciphers
+#ifdef USE_SSL_EC
+|| options->show_curves
+#endif
 #endif
     )
     {

@@ -907,6 +910,10 @@ print_openssl_info (const struct options *options)
 #ifdef USE_SSL
       if (options->show_tls_ciphers)
 	show_available_tls_ciphers ();
+#ifdef USE_SSL_EC
+      if (options->show_curves)
+    show_available_curves ();
+#endif
 #endif
       return true;
     }

options.c +15 -0

index 6b8ae22..ce23dbc 100644
@@ -836,6 +836,9 @@ init_options (struct options *o, const bool init_gc)
 #ifdef ENABLE_X509ALTUSERNAME
   o->x509_username_field = X509_USERNAME_FIELD_DEFAULT;
 #endif
+#ifdef USE_SSL_EC
+  o->curve_name = NULL;
+#endif
 #endif /* USE_SSL */
 #endif /* USE_CRYPTO */
 #ifdef ENABLE_PKCS11

@@ -6368,6 +6371,18 @@ add_option (struct options *options,
       VERIFY_PERMISSION (OPT_P_GENERAL);
       options->show_tls_ciphers = true;
     }
+#ifdef USE_SSL_EC
+  else if (streq (p[0], "show-curves"))
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      options->show_curves = true;
+    }
+  else if (streq (p[0], "ecdh") && p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      options->curve_name= p[1];
+    }
+#endif
   else if (streq (p[0], "tls-server"))
     {
       VERIFY_PERMISSION (OPT_P_GENERAL);

options.h +6 -0

index 831d4f6..81e0757 100644
@@ -200,6 +200,9 @@ struct options
   bool show_engines;
 #ifdef USE_SSL
   bool show_tls_ciphers;
+#ifdef USE_SSL_EC
+  bool show_curves;
+#endif
 #endif
   bool genkey;
 #endif

@@ -533,6 +536,9 @@ struct options
   const char *priv_key_file;
   const char *pkcs12_file;
   const char *cipher_list;
+#ifdef USE_SSL_EC
+  const char *curve_name;
+#endif
   const char *tls_verify;
   const char *tls_export_cert;
   const char *tls_remote;

ssl.c +3 -0

index c26756e..54efe2f 100644
@@ -308,6 +308,9 @@ init_ssl (const struct options *options, struct tls_root_ctx *new_ctx)
     {
       tls_ctx_server_new(new_ctx);
       tls_ctx_load_dh_params(new_ctx, options->dh_file, options->dh_file_inline);
+#ifdef USE_SSL_EC
+      tls_ctx_load_ecdh_params(new_ctx, options->curve_name);
+#endif
     }
   else				/* if client */
     {

ssl_backend.h +10 -0

index 243c9e3..ebf9f36 100644
@@ -145,6 +145,16 @@ void tls_ctx_load_dh_params(struct tls_root_ctx *ctx, const char *dh_file
     );
 
 /**
+ * Load Elliptic Curve Parameters, and load them into the library-specific
+ * TLS context.
+ *
+ * @param ctx			TLS context to use
+ * @param curve_name	The name of the elliptic curve to load.
+ */
+void tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
+    );
+
+/**
  * Load PKCS #12 file for key, cert and (optionally) CA certs, and add to
  * library-specific TLS context.
  *

ssl_openssl.c +19 -0

index b95944c..912dd8f 100644
@@ -50,6 +50,9 @@
 #include <openssl/pkcs12.h>
 #include <openssl/x509.h>
 #include <openssl/crypto.h>
+#ifdef USE_SSL_EC
+#include <openssl/ec.h>
+#endif
 
 /*
  * Allocate space in SSL objects in which to store a struct tls_session

@@ -238,6 +241,46 @@ tls_ctx_load_dh_params (struct tls_root_ctx *ctx, const char *dh_file
   DH_free (dh);
 }
 
+void
+tls_ctx_load_ecdh_params (struct tls_root_ctx *ctx, const char *curve_name
+    )
+{
+#ifdef USE_SSL_EC
+  if (curve_name != NULL)
+  {
+    int nid;
+    EC_KEY   *ecdh  = NULL;
+  
+    nid = OBJ_sn2nid(curve_name);
+  
+    if (nid == 0)
+      msg(M_SSLERR, "unknown curve name (%s)", curve_name);
+    else
+    {

ssl_openssl.c tls_ctx_load_dh_params (struct tls_root_ctx *ctx, const char *dh_file · line 269

Quoted 10 lines

ssl_openssl.c +10 -0

+      ecdh = EC_KEY_new_by_curve_name(nid);
+      if (ecdh == NULL)
+        msg (M_SSLERR, "Unable to create curve (%s)", curve_name);
+      else
+      {
+        const char *sname;
+  
+        if (!SSL_CTX_set_tmp_ecdh(ctx->ctx, ecdh))
+          msg (M_SSLERR, "SSL_CTX_set_tmp_ecdh: cannot add curve");
+

What is happening here exactly? Is the same key being reused for every connection, or is it magically regenerated by OpenSSL on every connection?

Quoted 75 lines

ssl_openssl.c +55 -0

+        /* Translate NID back to name , just for kicks */
+        sname   = OBJ_nid2sn(nid);
+        if (sname == NULL) sname = "(Unknown)";
+        msg (D_TLS_DEBUG_LOW, "ECDH curve %s added", sname);
+  
+        EC_KEY_free(ecdh);
+      }
+    }
+  }
+#else
+  msg(M_SSLERR, "Elliptic Curves not supported by this version of OpenSSL");
+#endif
+}
+
 int
 tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char *pkcs12_file,
 #if ENABLE_INLINE_FILES

@@ -1273,6 +1316,47 @@ show_available_tls_ciphers ()
   SSL_CTX_free (ctx);
 }
 
+/*
+ *  * Show the Elliptic curves that are available for us to use
+ *   * in the OpenSSL library.
+ *    */
+#ifdef USE_SSL_EC
+void
+show_available_curves()
+{
+  EC_builtin_curve *curves = NULL;
+  size_t crv_len = 0;
+  size_t n = 0;
+
+  crv_len = EC_get_builtin_curves(NULL, 0);
+
+  curves = OPENSSL_malloc((int)(sizeof(EC_builtin_curve) * crv_len));
+
+  if (curves == NULL)
+    msg (M_SSLERR, "Cannot create EC_builtin_curve object");
+  else
+  {
+    if (EC_get_builtin_curves(curves, crv_len))
+    {
+      printf ("Available Elliptic curves:\n");
+      for (n = 0; n < crv_len; n++)
+      {
+        const char *sname;
+        sname   = OBJ_nid2sn(curves[n].nid);
+        if (sname == NULL) sname = "";
+
+        printf("%s\n", sname);
+      }
+    }
+    else
+    {
+      msg (M_SSLERR, "Cannot get list of builtin curves");
+    }
+    OPENSSL_free(curves);
+  }
+}
+#endif
+
 void
 get_highest_preference_tls_cipher (char *buf, int size)
 {

ssl_polarssl.c +2 -0

index c50cf0a..a7a6d61 100644
@@ -218,6 +218,15 @@ else
       (counter_type) 8 * mpi_size(&ctx->dhm_ctx->P));
 }
 
+#ifdef USE_SSL_EC
+void

ssl_polarssl.c else · line 232

Quoted 10 lines

ssl_polarssl.c +7 -0

+tls_ctx_load_ecdh_params (struct tls_root_ctx *ctx, const char *curve_name
+    )
+{
+    msg(M_WARN, "Elliptic Curves not yet supported by PolarSSL");
+}
+#endif
+
 int
 tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char *pkcs12_file,
 #if ENABLE_INLINE_FILES

Codewise I don't have many comments. I do have a few questions surrounding the use of OpenSSL:

- How are curves for ECDSA set?

- Is the default curve used, or is the same curve reused?

- I don't quite understand how setting an ECDH curve relates to the
ECDSA signature size. Is this an OpenSSL specific issue?

These are partially based on a lack of knowledge of the OpenSSL implementation, so forgive me for any newbie questions there.

Finally one more comment: to be accepted into the main branch, this needs manpage and command line documentation.

Jan Just Keijser

author

2012-05-04 23:03 UTC #4 lore raw

Hi Adriaan,

Quoted 180 lines · Adriaan de Jong

Hi Janjust,

I've finally had the time to take a look at this patch with a colleague who is more familiar with the subject at hand :).

Hope this helps. Please see my comments inline.

Adriaan

Quoted 169 lines · Jan Just Keijser

Added support for Elliptic curves (ECDSA) + SHA2 family signed certificates.

---
 init.c         |    7 ++++
 options.c      |   15 ++++++++++
 options.h      |    6 ++++
 ssl.c          |    3 ++
 ssl_backend.h  |   10 ++++++
 ssl_openssl.c  |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ssl_polarssl.c |    9 ++++++
 7 files changed, 134 insertions(+), 0 deletions(-)

init.c +7 -0

index 525f441..51b0d64 100644
@@ -895,6 +895,9 @@ print_openssl_info (const struct options *options)
   if (options->show_ciphers || options->show_digests || options->show_engines
 #ifdef USE_SSL
       || options->show_tls_ciphers
+#ifdef USE_SSL_EC
+|| options->show_curves
+#endif
 #endif
     )
     {

@@ -907,6 +910,10 @@ print_openssl_info (const struct options *options)
 #ifdef USE_SSL
       if (options->show_tls_ciphers)
 	show_available_tls_ciphers ();
+#ifdef USE_SSL_EC
+      if (options->show_curves)
+    show_available_curves ();
+#endif
 #endif
       return true;
     }

options.c +15 -0

index 6b8ae22..ce23dbc 100644
@@ -836,6 +836,9 @@ init_options (struct options *o, const bool init_gc)
 #ifdef ENABLE_X509ALTUSERNAME
   o->x509_username_field = X509_USERNAME_FIELD_DEFAULT;
 #endif
+#ifdef USE_SSL_EC
+  o->curve_name = NULL;
+#endif
 #endif /* USE_SSL */
 #endif /* USE_CRYPTO */
 #ifdef ENABLE_PKCS11

@@ -6368,6 +6371,18 @@ add_option (struct options *options,
       VERIFY_PERMISSION (OPT_P_GENERAL);
       options->show_tls_ciphers = true;
     }
+#ifdef USE_SSL_EC
+  else if (streq (p[0], "show-curves"))
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      options->show_curves = true;
+    }
+  else if (streq (p[0], "ecdh") && p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      options->curve_name= p[1];
+    }
+#endif
   else if (streq (p[0], "tls-server"))
     {
       VERIFY_PERMISSION (OPT_P_GENERAL);

options.h +6 -0

index 831d4f6..81e0757 100644
@@ -200,6 +200,9 @@ struct options
   bool show_engines;
 #ifdef USE_SSL
   bool show_tls_ciphers;
+#ifdef USE_SSL_EC
+  bool show_curves;
+#endif
 #endif
   bool genkey;
 #endif

@@ -533,6 +536,9 @@ struct options
   const char *priv_key_file;
   const char *pkcs12_file;
   const char *cipher_list;
+#ifdef USE_SSL_EC
+  const char *curve_name;
+#endif
   const char *tls_verify;
   const char *tls_export_cert;
   const char *tls_remote;

ssl.c +3 -0

index c26756e..54efe2f 100644
@@ -308,6 +308,9 @@ init_ssl (const struct options *options, struct tls_root_ctx *new_ctx)
     {
       tls_ctx_server_new(new_ctx);
       tls_ctx_load_dh_params(new_ctx, options->dh_file, options->dh_file_inline);
+#ifdef USE_SSL_EC
+      tls_ctx_load_ecdh_params(new_ctx, options->curve_name);
+#endif
     }
   else				/* if client */
     {

ssl_backend.h +10 -0

index 243c9e3..ebf9f36 100644
@@ -145,6 +145,16 @@ void tls_ctx_load_dh_params(struct tls_root_ctx *ctx, const char *dh_file
     );
 
 /**
+ * Load Elliptic Curve Parameters, and load them into the library-specific
+ * TLS context.
+ *
+ * @param ctx			TLS context to use
+ * @param curve_name	The name of the elliptic curve to load.
+ */
+void tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
+    );
+
+/**
  * Load PKCS #12 file for key, cert and (optionally) CA certs, and add to
  * library-specific TLS context.
  *

ssl_openssl.c +25 -0

index b95944c..912dd8f 100644
@@ -50,6 +50,9 @@
 #include <openssl/pkcs12.h>
 #include <openssl/x509.h>
 #include <openssl/crypto.h>
+#ifdef USE_SSL_EC
+#include <openssl/ec.h>
+#endif
 
 /*
  * Allocate space in SSL objects in which to store a struct tls_session

@@ -238,6 +241,46 @@ tls_ctx_load_dh_params (struct tls_root_ctx *ctx, const char *dh_file
   DH_free (dh);
 }
 
+void
+tls_ctx_load_ecdh_params (struct tls_root_ctx *ctx, const char *curve_name
+    )
+{
+#ifdef USE_SSL_EC
+  if (curve_name != NULL)
+  {
+    int nid;
+    EC_KEY   *ecdh  = NULL;
+  
+    nid = OBJ_sn2nid(curve_name);
+  
+    if (nid == 0)
+      msg(M_SSLERR, "unknown curve name (%s)", curve_name);
+    else
+    {
+      ecdh = EC_KEY_new_by_curve_name(nid);
+      if (ecdh == NULL)
+        msg (M_SSLERR, "Unable to create curve (%s)", curve_name);
+      else
+      {
+        const char *sname;

Quoted 10 lines

ssl_openssl.c tls_ctx_load_dh_params (struct tls_root_ctx *ctx, const char *dh_file
Quoted 5 lines
ssl_openssl.c +4 -0
+  
+        if (!SSL_CTX_set_tmp_ecdh(ctx->ctx, ecdh))
+          msg (M_SSLERR, "SSL_CTX_set_tmp_ecdh: cannot add curve");
+  
 
What is happening here exactly? Is the same key being reused for every connection, or is it magically regenerated by OpenSSL on every connection?

the function SSL_CTX_set_tmp_ecdh is very similar to SSL_CTX_set_tmp (for DH keys).
It adds the curve to the SSL CTX (ConTeXt); this CTX is used only when negotiating the TLS channel - NOT the data channel. It is updated by the OpenSSL TLS code automagically.

Quoted 92 lines

Quoted 76 lines

ssl_openssl.c +14 -0

+        /* Translate NID back to name , just for kicks */
+        sname   = OBJ_nid2sn(nid);
+        if (sname == NULL) sname = "(Unknown)";
+        msg (D_TLS_DEBUG_LOW, "ECDH curve %s added", sname);
+  
+        EC_KEY_free(ecdh);
+      }
+    }
+  }
+#else
+  msg(M_SSLERR, "Elliptic Curves not supported by this version of OpenSSL");
+#endif
+}
+
 int
 tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char *pkcs12_file,

#if ENABLE_INLINE_FILES

ssl_openssl.c +41 -0

@@ -1273,6 +1316,47 @@ show_available_tls_ciphers ()
   SSL_CTX_free (ctx);
 }
 
+/*
+ *  * Show the Elliptic curves that are available for us to use
+ *   * in the OpenSSL library.
+ *    */
+#ifdef USE_SSL_EC
+void
+show_available_curves()
+{
+  EC_builtin_curve *curves = NULL;
+  size_t crv_len = 0;
+  size_t n = 0;
+
+  crv_len = EC_get_builtin_curves(NULL, 0);
+
+  curves = OPENSSL_malloc((int)(sizeof(EC_builtin_curve) * crv_len));
+
+  if (curves == NULL)
+    msg (M_SSLERR, "Cannot create EC_builtin_curve object");
+  else
+  {
+    if (EC_get_builtin_curves(curves, crv_len))
+    {
+      printf ("Available Elliptic curves:\n");
+      for (n = 0; n < crv_len; n++)
+      {
+        const char *sname;
+        sname   = OBJ_nid2sn(curves[n].nid);
+        if (sname == NULL) sname = "";
+
+        printf("%s\n", sname);
+      }
+    }
+    else
+    {
+      msg (M_SSLERR, "Cannot get list of builtin curves");
+    }
+    OPENSSL_free(curves);
+  }
+}
+#endif
+
 void
 get_highest_preference_tls_cipher (char *buf, int size)
 {

ssl_polarssl.c +2 -0

index c50cf0a..a7a6d61 100644
@@ -218,6 +218,15 @@ else
       (counter_type) 8 * mpi_size(&ctx->dhm_ctx->P));
 }
 
+#ifdef USE_SSL_EC
+void

ssl_polarssl.c else · line 232

Quoted 10 lines

ssl_polarssl.c +7 -0

+tls_ctx_load_ecdh_params (struct tls_root_ctx *ctx, const char *curve_name
+    )
+{
+    msg(M_WARN, "Elliptic Curves not yet supported by PolarSSL");
+}
+#endif
+
 int
 tls_ctx_load_pkcs12(struct tls_root_ctx *ctx, const char *pkcs12_file,
 #if ENABLE_INLINE_FILES

Codewise I don't have many comments. I do have a few questions surrounding the use of OpenSSL:

- How are curves for ECDSA set?

Quoted 9 lines

- Is the default curve used, or is the same curve reused?
- I don't quite understand how setting an ECDH curve relates to the
ECDSA signature size. Is this an OpenSSL specific issue?
These are partially based on a lack of knowledge of the OpenSSL implementation, so forgive me for any newbie questions there.

as I said, the SSL_CTX_set_tmp_ecdh is used only for the TLS control channel connection. It does *NOT* add elliptic curve support on the data channel, as this requires quite a few more changes, some of which are not supported by the OpenSSL libs right now (e.g. the fact that openvpn calls the signing functions directly).

The main reason for adding this patch is to allow the use of certificates which are generated with ECDH keys and which are signed with a SHA2 family hash; without this patch , this is not allowed.

Quoted 4 lines

Finally one more comment: to be accepted into the main branch, this needs manpage and command line documentation.

very true... but that applies to more patches ;)

HTH,

JJK

Adriaan de Jong

2012-05-07 07:14 UTC #5 lore raw

Hi Jan-Just,

Quoted 6 lines

-----Original Message-----
From: Jan Just Keijser [mailto:[email protected]...]
Quoted 3 lines · Adriaan de Jong
Quoted 1 line · Jan Just Keijser

<Snip>

Quoted 32 lines

Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+void
+tls_ctx_load_ecdh_params (struct tls_root_ctx *ctx, const char
*curve_name
Quoted 19 lines
Quoted 19 lines
ssl_openssl.c +19 -0
+    )
+{
+#ifdef USE_SSL_EC
+  if (curve_name != NULL)
+  {
+    int nid;
+    EC_KEY   *ecdh  = NULL;
+
+    nid = OBJ_sn2nid(curve_name);
+
+    if (nid == 0)
+      msg(M_SSLERR, "unknown curve name (%s)", curve_name);
+    else
+    {
+      ecdh = EC_KEY_new_by_curve_name(nid);
+      if (ecdh == NULL)
+        msg (M_SSLERR, "Unable to create curve (%s)", curve_name);
+      else
+      {
Quoted 8 lines
ssl_openssl.c tls_ctx_load_dh_params (struct tls_root_ctx *ctx, const char *dh_file · line 269
Quoted 6 lines
ssl_openssl.c +5 -0
+        const char *sname;
+
+        if (!SSL_CTX_set_tmp_ecdh(ctx->ctx, ecdh))
+          msg (M_SSLERR, "SSL_CTX_set_tmp_ecdh: cannot add curve");
+
What is happening here exactly? Is the same key being reused for
every
Quoted 1 line
connection, or is it magically regenerated by OpenSSL on every

Quoted 8 lines

connection?
Quoted 2 lines
the function SSL_CTX_set_tmp_ecdh is very similar to SSL_CTX_set_tmp (for DH keys).
It adds the curve to the SSL CTX (ConTeXt); this CTX is used only when negotiating the TLS channel - NOT the data channel. It is updated by the OpenSSL TLS code automagically.

Thanks, I realise this is just about the control channel. Still: for a server setup, multiple control channels are set up (one for each connection).

What I'm reading here, but that could be a mistake is the following:

1. Determine Curve NID
2. Generate a new DH key based on the curve NID (ecdh = EC_KEY_new_by_curve_name(nid);)
3. For the global context, set that key as the one be used.

Now, if that is correct, the same key is reused across multiple connections for a server setup.

Again, note that my assumptions here might be incorrect, especially in step 2. Unfortunately, I can't find decent documentation for the OpenSSL functions.

Quoted 13 lines

ssl_openssl.c +13 -0

+        /* Translate NID back to name , just for kicks */
+        sname   = OBJ_nid2sn(nid);
+        if (sname == NULL) sname = "(Unknown)";
+        msg (D_TLS_DEBUG_LOW, "ECDH curve %s added", sname);
+
+        EC_KEY_free(ecdh);
+      }
+    }
+  }
+#else
+  msg(M_SSLERR, "Elliptic Curves not supported by this version of
+OpenSSL"); #endif }
+

<Snip>

Quoted 15 lines

Quoted 6 lines
Codewise I don't have many comments. I do have a few questions surrounding the use of OpenSSL:
- How are curves for ECDSA set?
Quoted 9 lines
- Is the default curve used, or is the same curve reused?
- I don't quite understand how setting an ECDH curve relates to the
ECDSA signature size. Is this an OpenSSL specific issue?
These are partially based on a lack of knowledge of the OpenSSL implementation, so forgive me for any newbie questions there.

Quoted 6 lines

as I said, the SSL_CTX_set_tmp_ecdh is used only for the TLS control channel connection. It does *NOT* add elliptic curve support on the data channel, as this requires quite a few more changes, some of which are not supported by the OpenSSL libs right now (e.g. the fact that openvpn calls the signing functions directly).

What I'm mostly concerned with is the control channel setup. As I understand it, there's two steps involved that can use elliptic curve crypto:

1. Authentication using ECDSA/RSA/DSA/whatever
2. Anonymous DH/ECDH negotiation to get a session key

What I'm wondering is, if you use ECDSA, how is the curve for ECDSA set? As I understand it, this patch only sets the ECDH curve and key, not the

Quoted 4 lines

The main reason for adding this patch is to allow the use of certificates which are generated with ECDH keys and which are signed with a SHA2 family hash; without this patch , this is not allowed.

That's what my question boils down to: isn't ECDSA, not ECDH the certificate-based part of the algorithm? ECDH is anonymous, isn't it?

Quoted 6 lines

Quoted 4 lines
Finally one more comment: to be accepted into the main branch, this needs manpage and command line documentation.
very true... but that applies to more patches ;)

Unfortunately true...

I hope that clarifies my concerns a little. Unfortunately, I haven't found a good authoritative source for the functionality of SSL_CTX_set_tmp_ecdh. If it works in a similar manner to SSL_CTX_set_tmp_rsa, then you would need to use SSL_CTX_set_tmp_ecdh_callback instead to generate a new key for every control channel (see the ssl_openssl.c code for an example of that).

Kind regards,

Adriaan

Jan Just Keijser

author

2012-05-08 01:48 UTC #6 lore raw

Hi Adriaan,

Quoted 86 lines · Adriaan de Jong

<snip>
Quoted 37 lines
Quoted 3 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+void
+tls_ctx_load_ecdh_params (struct tls_root_ctx *ctx, const char
*curve_name
Quoted 19 lines
Quoted 19 lines
ssl_openssl.c +19 -0
+    )
+{
+#ifdef USE_SSL_EC
+  if (curve_name != NULL)
+  {
+    int nid;
+    EC_KEY   *ecdh  = NULL;
+
+    nid = OBJ_sn2nid(curve_name);
+
+    if (nid == 0)
+      msg(M_SSLERR, "unknown curve name (%s)", curve_name);
+    else
+    {
+      ecdh = EC_KEY_new_by_curve_name(nid);
+      if (ecdh == NULL)
+        msg (M_SSLERR, "Unable to create curve (%s)", curve_name);
+      else
+      {
Quoted 9 lines
ssl_openssl.c tls_ctx_load_dh_params (struct tls_root_ctx *ctx, const char *dh_file · line 269
Quoted 7 lines
ssl_openssl.c +5 -0
+        const char *sname;
+
+        if (!SSL_CTX_set_tmp_ecdh(ctx->ctx, ecdh))
+          msg (M_SSLERR, "SSL_CTX_set_tmp_ecdh: cannot add curve");
+
What is happening here exactly? Is the same key being reused for
every
Quoted 2 lines
connection, or is it magically regenerated by OpenSSL on every
Quoted 9 lines
connection?
Quoted 1 line
the function SSL_CTX_set_tmp_ecdh is very similar to SSL_CTX_set_tmp (for DH keys).
It adds the curve to the SSL CTX (ConTeXt); this CTX is used only when negotiating the TLS channel - NOT the data channel. It is updated by the OpenSSL TLS code automagically.
Thanks, I realise this is just about the control channel. Still: for a server setup, multiple control channels are set up (one for each connection).
What I'm reading here, but that could be a mistake is the following:
1. Determine Curve NID
2. Generate a new DH key based on the curve NID (ecdh = EC_KEY_new_by_curve_name(nid);)
3. For the global context, set that key as the one be used.
Now, if that is correct, the same key is reused across multiple connections for a server setup.
Again, note that my assumptions here might be incorrect, especially in step 2. Unfortunately, I can't find decent documentation for the OpenSSL functions.
<snip>
What I'm mostly concerned with is the control channel setup. As I understand it, there's two steps involved that can use elliptic curve crypto:
1. Authentication using ECDSA/RSA/DSA/whatever
2. Anonymous DH/ECDH negotiation to get a session key
What I'm wondering is, if you use ECDSA, how is the curve for ECDSA set? As I understand it, this patch only sets the ECDH curve and key, not the
Quoted 5 lines
The main reason for adding this patch is to allow the use of certificates which are generated with ECDH keys and which are signed with a SHA2 family hash; without this patch , this is not allowed.
That's what my question boils down to: isn't ECDSA, not ECDH the certificate-based part of the algorithm? ECDH is anonymous, isn't it?
Quoted 5 lines
Quoted 5 lines
Finally one more comment: to be accepted into the main branch, this needs manpage and command line documentation.

Quoted 8 lines

Quoted 3 lines
very true... but that applies to more patches ;)
Unfortunately true...
I hope that clarifies my concerns a little. Unfortunately, I haven't found a good authoritative source for the functionality of SSL_CTX_set_tmp_ecdh. If it works in a similar manner to SSL_CTX_set_tmp_rsa, then you would need to use SSL_CTX_set_tmp_ecdh_callback instead to generate a new key for every control channel (see the ssl_openssl.c code for an example of that).

Your questions are all very valid.
The documentation on these functions is indeed atrocious ... from what I understood the SSL_CTX_set_tmp_ecdh function is very similar to the SSL_CTX_set_tmp_dh function; there's also a 'set_tmp_dh_callback' function which is not used very often, as the DH params are static for a single server. If you read the openssl code , file ssl/s3_lib.c and look for SSL_CTRL_SET_TMP_ECDH then you'll find that the s3_lib.c code does treat the calls pretty much the same - this leads me to believe that the SSL_CTX_set_tmp_ecdh_callback function does NOT need to be used. To make sure I will post a question to the openssl developers on this.

The main reason for adding this function to the openvpn server instance is to ensure that enough 'ECDH entropy' , for lack of a better word, is available so that SHA2 message signing is possible. Again, this is my understanding of things, but I'm about as blind as you are when it comes to understanding this part of the OpenSSL code base.

HTH,

JJK