ALPN in ClientHello for protocols other than HTTPS #789

jay · 2016-05-09T19:38:49Z

Reported by @jay in this mailing list post, filed here to make sure we don't forget:

I've been monitoring some FTPS in Wireshark and one thing I've noticed is the ClientHello contains the ALPN extension for http/1.1 and h2. Is it acceptable to be sending that for protocols other than HTTP?

And no, it is not acceptable. We should make sure to only used ALPN (and h2) when walking HTTPS.

bagder · 2016-05-09T14:56:48Z

I wish github allowed me to attach a patch more easily. But anyway, I have a patch that moves the ALPN and NPN bitflags to the connectdata struct and have them only be set TRUE if the protocol actually has support for them.

From 5fbb50ad787e72e6e68a997d85e61a45c12fbcb7 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Mon, 9 May 2016 16:50:11 +0200
Subject: [PATCH] TLS: move the ALPN/NPN enable bits to the connection

Only protocols that actually have a protocol registered for ALPN and NPN
should try to get that negotiated in the TLS handshake. That is only
HTTPS (well, http/1.1 and http/2) right now. Previously ALPN and NPN
would wrongly be used in all handshakes if libcurl was built with it
enabled.

Reported-by: Jay Satiro

Fixes #789
---
 lib/http.c          |  2 +-
 lib/url.c           |  9 +++++++++
 lib/urldata.h       |  8 +++++---
 lib/vtls/cyassl.c   |  4 ++--
 lib/vtls/gtls.c     |  4 ++--
 lib/vtls/mbedtls.c  |  4 ++--
 lib/vtls/nss.c      | 14 +++++++-------
 lib/vtls/openssl.c  |  6 +++---
 lib/vtls/polarssl.c |  6 +++---
 lib/vtls/schannel.c |  4 ++--
 10 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/lib/http.c b/lib/http.c
index 2a7280d..6a76b88 100644
--- a/lib/http.c
+++ b/lib/http.c
@@ -143,11 +143,11 @@ const struct Curl_handler Curl_handler_https = {
   ZERO_NULL,                            /* perform_getsock */
   ZERO_NULL,                            /* disconnect */
   ZERO_NULL,                            /* readwrite */
   PORT_HTTPS,                           /* defport */
   CURLPROTO_HTTPS,                      /* protocol */
-  PROTOPT_SSL | PROTOPT_CREDSPERREQUEST /* flags */
+  PROTOPT_SSL | PROTOPT_CREDSPERREQUEST | PROTOPT_ALPN_NPN /* flags */
 };
 #endif

 CURLcode Curl_http_setup_conn(struct connectdata *conn)
 {
diff --git a/lib/url.c b/lib/url.c
index 70ccd0f..605212e 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -6165,10 +6165,19 @@ static CURLcode create_conn(struct SessionHandle *data,
     /* We have decided that we want a new connection. However, we may not
        be able to do that if we have reached the limit of how many
        connections we are allowed to open. */
     struct connectbundle *bundle = NULL;

+    if(conn->handler->flags & PROTOPT_ALPN_NPN) {
+      /* The protocol wants it, so set the bits if enabled in the easy handle
+         (default) */
+      if(data->set.ssl_enable_alpn)
+        conn->bits.tls_enable_alpn = TRUE;
+      if(data->set.ssl_enable_npn)
+        conn->bits.tls_enable_npn = TRUE;
+    }
+
     if(waitpipe)
       /* There is a connection that *might* become usable for pipelining
          "soon", and we wait for that */
       connections_available = FALSE;
     else
diff --git a/lib/urldata.h b/lib/urldata.h
index c0b2e2f..25594d3 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -542,10 +542,12 @@ struct ConnectBits {
                  connection */
   bool type_set;  /* type= was used in the URL */
   bool multiplex; /* connection is multiplexed */

   bool tcp_fastopen; /* use TCP Fast Open */
+  bool tls_enable_npn;  /* TLS NPN extension? */
+  bool tls_enable_alpn; /* TLS ALPN extension? */
 };

 struct hostname {
   char *rawalloc; /* allocated "raw" version of the name */
   char *encalloc; /* allocated IDN-encoded version of the name */
@@ -813,11 +815,11 @@ struct Curl_handler {
                                       gets a default */
 #define PROTOPT_NOURLQUERY (1<<6)   /* protocol can't handle
                                         url query strings (?foo=bar) ! */
 #define PROTOPT_CREDSPERREQUEST (1<<7) /* requires login credentials per
                                           request instead of per connection */
-
+#define PROTOPT_ALPN_NPN (1<<8) /* set ALPN and/or NPN for this */

 /* return the count of bytes sent, or -1 on error */
 typedef ssize_t (Curl_send)(struct connectdata *conn, /* connection data */
                             int sockindex,            /* socketindex */
                             const void *buf,          /* data to write */
@@ -1669,12 +1671,12 @@ struct UserDefined {
   long tcp_keepintvl;    /* seconds between TCP keepalive probes */
   bool tcp_fastopen;     /* use TCP Fast Open */

   size_t maxconnects;  /* Max idle connections in the connection cache */

-  bool ssl_enable_npn;  /* TLS NPN extension? */
-  bool ssl_enable_alpn; /* TLS ALPN extension? */
+  bool ssl_enable_npn;      /* TLS NPN extension? */
+  bool ssl_enable_alpn;     /* TLS ALPN extension? */
   bool path_as_is;      /* allow dotdots? */
   bool pipewait;        /* wait for pipe/multiplex status before starting a
                            new connection */
   long expect_100_timeout; /* in milliseconds */

diff --git a/lib/vtls/cyassl.c b/lib/vtls/cyassl.c
index 1109a1a..da737c7 100644
--- a/lib/vtls/cyassl.c
+++ b/lib/vtls/cyassl.c
@@ -349,11 +349,11 @@ cyassl_connect_step1(struct connectdata *conn,
     failf(data, "SSL: couldn't create a context (handle)!");
     return CURLE_OUT_OF_MEMORY;
   }

 #ifdef HAVE_ALPN
-  if(data->set.ssl_enable_alpn) {
+  if(conn->bits.tls_enable_alpn) {
     char protocols[128];
     *protocols = '\0';

     /* wolfSSL's ALPN protocol name list format is a comma separated string of
        protocols in descending order of preference, eg: "h2,http/1.1" */
@@ -523,11 +523,11 @@ cyassl_connect_step2(struct connectdata *conn,
     return CURLE_NOT_BUILT_IN;
 #endif
   }

 #ifdef HAVE_ALPN
-  if(data->set.ssl_enable_alpn) {
+  if(conn->bits.tls_enable_alpn) {
     int rc;
     char *protocol = NULL;
     unsigned short protocol_len = 0;

     rc = wolfSSL_ALPN_GetProtocol(conssl->handle, &protocol, &protocol_len);
diff --git a/lib/vtls/gtls.c b/lib/vtls/gtls.c
index 9d1cd50..1b5a6a4 100644
--- a/lib/vtls/gtls.c
+++ b/lib/vtls/gtls.c
@@ -639,11 +639,11 @@ gtls_connect_step1(struct connectdata *conn,
     return CURLE_SSL_CONNECT_ERROR;
   }
 #endif

 #ifdef HAS_ALPN
-  if(data->set.ssl_enable_alpn) {
+  if(conn->bits.tls_enable_alpn) {
     int cur = 0;
     gnutls_datum_t protocols[2];

 #ifdef USE_NGHTTP2
     if(data->set.httpversion >= CURL_HTTP_VERSION_2) {
@@ -1238,11 +1238,11 @@ gtls_connect_step3(struct connectdata *conn,
   ptr = gnutls_compression_get_name(gnutls_compression_get(session));
   /* the *_get_name() says "NULL" if GNUTLS_COMP_NULL is returned */
   infof(data, "\t compression: %s\n", ptr);

 #ifdef HAS_ALPN
-  if(data->set.ssl_enable_alpn) {
+  if(conn->bits.tls_enable_alpn) {
     rc = gnutls_alpn_get_selected_protocol(session, &proto);
     if(rc == 0) {
       infof(data, "ALPN, server accepted to use %.*s\n", proto.size,
           proto.data);

diff --git a/lib/vtls/mbedtls.c b/lib/vtls/mbedtls.c
index 6b26a97..f0048ef 100644
--- a/lib/vtls/mbedtls.c
+++ b/lib/vtls/mbedtls.c
@@ -399,11 +399,11 @@ mbed_connect_step1(struct connectdata *conn,
     infof(data, "WARNING: failed to configure "
           "server name indication (SNI) TLS extension\n");
   }

 #ifdef HAS_ALPN
-  if(data->set.ssl_enable_alpn) {
+  if(conn->bits.tls_enable_alpn) {
     const char **p = &connssl->protocols[0];
 #ifdef USE_NGHTTP2
     if(data->set.httpversion >= CURL_HTTP_VERSION_2)
       *p++ = NGHTTP2_PROTO_VERSION_ID;
 #endif
@@ -559,11 +559,11 @@ mbed_connect_step2(struct connectdata *conn,
     mbedtls_x509_crt_free(p);
     free(p);
   }

 #ifdef HAS_ALPN
-  if(data->set.ssl_enable_alpn) {
+  if(conn->bits.tls_enable_alpn) {
     next_protocol = mbedtls_ssl_get_alpn_protocol(&connssl->ssl);

     if(next_protocol) {
       infof(data, "ALPN, server accepted to use %s\n", next_protocol);
 #ifdef USE_NGHTTP2
diff --git a/lib/vtls/nss.c b/lib/vtls/nss.c
index 3922d9c..02c8727 100644
--- a/lib/vtls/nss.c
+++ b/lib/vtls/nss.c
@@ -3,11 +3,11 @@
  *  Project                     ___| | | |  _ \| |
  *                             / __| | | | |_) | |
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
- * Copyright (C) 1998 - 2015, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 1998 - 2016, Daniel Stenberg, <daniel@haxx.se>, et al.
  *
  * This software is licensed as described in the file COPYING, which
  * you should have received as part of this distribution. The terms
  * are also available at https://curl.haxx.se/docs/copyright.html.
  *
@@ -694,11 +694,11 @@ static void HandshakeCallback(PRFileDesc *sock, void *arg)
   unsigned int buflenmax = 50;
   unsigned char buf[50];
   unsigned int buflen;
   SSLNextProtoState state;

-  if(!conn->data->set.ssl_enable_npn && !conn->data->set.ssl_enable_alpn) {
+  if(!conn->bits.tls_enable_npn && !conn->bits.tls_enable_alpn) {
     return;
   }

   if(SSL_GetNextProto(sock, &state, buf, &buflen, buflenmax) == SECSuccess) {

@@ -1742,18 +1742,18 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
       goto error;
   }
 #endif

 #ifdef SSL_ENABLE_NPN
-  if(SSL_OptionSet(connssl->handle, SSL_ENABLE_NPN, data->set.ssl_enable_npn
-        ? PR_TRUE : PR_FALSE) != SECSuccess)
+  if(SSL_OptionSet(connssl->handle, SSL_ENABLE_NPN, conn->bits.tls_enable_npn
+                   ? PR_TRUE : PR_FALSE) != SECSuccess)
     goto error;
 #endif

 #ifdef SSL_ENABLE_ALPN
-  if(SSL_OptionSet(connssl->handle, SSL_ENABLE_ALPN, data->set.ssl_enable_alpn
-        ? PR_TRUE : PR_FALSE) != SECSuccess)
+  if(SSL_OptionSet(connssl->handle, SSL_ENABLE_ALPN, conn->bits.tls_enable_alpn
+                   ? PR_TRUE : PR_FALSE) != SECSuccess)
     goto error;
 #endif

 #if NSSVERNUM >= 0x030f04 /* 3.15.4 */
   if(data->set.ssl.falsestart) {
@@ -1766,11 +1766,11 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
       goto error;
   }
 #endif

 #if defined(SSL_ENABLE_NPN) || defined(SSL_ENABLE_ALPN)
-  if(data->set.ssl_enable_npn || data->set.ssl_enable_alpn) {
+  if(conn->bits.tls_enable_npn || conn->bits.tls_enable_alpn) {
     int cur = 0;
     unsigned char protocols[128];

 #ifdef USE_NGHTTP2
     if(data->set.httpversion >= CURL_HTTP_VERSION_2) {
diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index 2d18b1b..823dceb 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -1837,16 +1837,16 @@ static CURLcode ossl_connect_step1(struct connectdata *conn, int sockindex)
   }

   SSL_CTX_set_options(connssl->ctx, ctx_options);

 #ifdef HAS_NPN
-  if(data->set.ssl_enable_npn)
+  if(conn->bits.tls_enable_npn)
     SSL_CTX_set_next_proto_select_cb(connssl->ctx, select_next_proto_cb, conn);
 #endif

 #ifdef HAS_ALPN
-  if(data->set.ssl_enable_alpn) {
+  if(conn->bits.tls_enable_alpn) {
     int cur = 0;
     unsigned char protocols[128];

 #ifdef USE_NGHTTP2
     if(data->set.httpversion >= CURL_HTTP_VERSION_2) {
@@ -2163,11 +2163,11 @@ static CURLcode ossl_connect_step2(struct connectdata *conn, int sockindex)

 #ifdef HAS_ALPN
     /* Sets data and len to negotiated protocol, len is 0 if no protocol was
      * negotiated
      */
-    if(data->set.ssl_enable_alpn) {
+    if(conn->bits.tls_enable_alpn) {
       const unsigned char* neg_protocol;
       unsigned int len;
       SSL_get0_alpn_selected(connssl->handle, &neg_protocol, &len);
       if(len != 0) {
         infof(data, "ALPN, server accepted to use %.*s\n", len, neg_protocol);
diff --git a/lib/vtls/polarssl.c b/lib/vtls/polarssl.c
index 6c7a786..aa4da3f 100644
--- a/lib/vtls/polarssl.c
+++ b/lib/vtls/polarssl.c
@@ -3,11 +3,11 @@
  *  Project                     ___| | | |  _ \| |
  *                             / __| | | | |_) | |
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
- * Copyright (C) 2012 - 2015, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 2012 - 2016, Daniel Stenberg, <daniel@haxx.se>, et al.
  * Copyright (C) 2010 - 2011, Hoi-Ho Chan, <hoiho.chan@gmail.com>
  *
  * This software is licensed as described in the file COPYING, which
  * you should have received as part of this distribution. The terms
  * are also available at https://curl.haxx.se/docs/copyright.html.
@@ -362,11 +362,11 @@ polarssl_connect_step1(struct connectdata *conn,
      infof(data, "WARNING: failed to configure "
                  "server name indication (SNI) TLS extension\n");
   }

 #ifdef HAS_ALPN
-  if(data->set.ssl_enable_alpn) {
+  if(conn->bits.tls_enable_alpn) {
     static const char* protocols[3];
     int cur = 0;

 #ifdef USE_NGHTTP2
     if(data->set.httpversion >= CURL_HTTP_VERSION_2) {
@@ -517,11 +517,11 @@ polarssl_connect_step2(struct connectdata *conn,
     x509_crt_free(p);
     free(p);
   }

 #ifdef HAS_ALPN
-  if(data->set.ssl_enable_alpn) {
+  if(conn->bits.tls_enable_alpn) {
     const char *next_protocol = ssl_get_alpn_protocol(&connssl->ssl);

     if(next_protocol != NULL) {
       infof(data, "ALPN, server accepted to use %s\n", next_protocol);

diff --git a/lib/vtls/schannel.c b/lib/vtls/schannel.c
index 4790735..a2fba73 100644
--- a/lib/vtls/schannel.c
+++ b/lib/vtls/schannel.c
@@ -229,11 +229,11 @@ schannel_connect_step1(struct connectdata *conn, int sockindex)
     ) {
     infof(data, "schannel: using IP address, SNI is not supported by OS.\n");
   }

 #ifdef HAS_ALPN
-  if(data->set.ssl_enable_alpn) {
+  if(conn->bits.tls_enable_alpn) {
     int cur = 0;
     int list_start_index = 0;
     unsigned int* extension_len = NULL;
     unsigned short* list_len = NULL;

@@ -628,11 +628,11 @@ schannel_connect_step3(struct connectdata *conn, int sockindex)
       failf(data, "schannel: failed to setup stream orientation");
     return CURLE_SSL_CONNECT_ERROR;
   }

 #ifdef HAS_ALPN
-  if(data->set.ssl_enable_alpn) {
+  if(conn->bits.tls_enable_alpn) {
     sspi_status = s_pSecFn->QueryContextAttributes(&connssl->ctxt->ctxt_handle,
       SECPKG_ATTR_APPLICATION_PROTOCOL, &alpn_result);

     if(sspi_status != SEC_E_OK) {
       failf(data, "schannel: failed to retrieve ALPN result");
-- 
2.8.1

JCMais · 2016-05-09T15:15:59Z

@bagder while I don't know any other way to attach a patch, there is a specific syntax highlight for diff text:

```diff
content
```

Example:

diff --git a/lib/http.c b/lib/http.c
index 2a7280d..6a76b88 100644
--- a/lib/http.c
+++ b/lib/http.c
@@ -143,11 +143,11 @@ const struct Curl_handler Curl_handler_https = {
   ZERO_NULL,                            /* perform_getsock */
   ZERO_NULL,                            /* disconnect */
   ZERO_NULL,                            /* readwrite */
   PORT_HTTPS,                           /* defport */
   CURLPROTO_HTTPS,                      /* protocol */
-  PROTOPT_SSL | PROTOPT_CREDSPERREQUEST /* flags */
+  PROTOPT_SSL | PROTOPT_CREDSPERREQUEST | PROTOPT_ALPN_NPN /* flags */
 };
 #endif

jay · 2016-05-09T19:23:47Z

I'm fine with this, I would have done it similarly. 👍

Only protocols that actually have a protocol registered for ALPN and NPN should try to get that negotiated in the TLS handshake. That is only HTTPS (well, http/1.1 and http/2) right now. Previously ALPN and NPN would wrongly be used in all handshakes if libcurl was built with it enabled. Reported-by: Jay Satiro Fixes curl#789

jay · 2016-05-09T19:52:24Z

I've turned this issue into a pull request using hub. issue to pr is a deprecated feature (for several years now).

hub pull-request -i 789 -b curl/curl:master -h jay/curl:alpn_https_only https://github.com/curl/curl/issues/789

Note I made the head on jay/curl instead of bagder/curl or curl/curl. Typically I would not do it this way unless it was my work. Instead I'd ask the reporter to create a branch (and convert) that way they can update it if necessary. This is just an example.

bagder · 2016-05-09T20:59:28Z

thanks @jay !

bagder added the TLS label May 2, 2016

bagder changed the title ~~ALPN in ClientHello for protocols other than HTTP~~ ALPN in ClientHello for protocols other than HTTPS May 2, 2016

jay merged commit f6767f5 into curl:master May 9, 2016

jay deleted the alpn_https_only branch May 9, 2016 19:57

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ALPN in ClientHello for protocols other than HTTPS #789

ALPN in ClientHello for protocols other than HTTPS #789

jay commented May 9, 2016

bagder commented May 9, 2016

JCMais commented May 9, 2016 •

edited

jay commented May 9, 2016

jay commented May 9, 2016 •

edited

bagder commented May 9, 2016

ALPN in ClientHello for protocols other than HTTPS #789

ALPN in ClientHello for protocols other than HTTPS #789

Conversation

jay commented May 9, 2016

bagder commented May 9, 2016

JCMais commented May 9, 2016 • edited

jay commented May 9, 2016

jay commented May 9, 2016 • edited

bagder commented May 9, 2016

JCMais commented May 9, 2016 •

edited

jay commented May 9, 2016 •

edited