-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
ALPN in ClientHello for protocols other than HTTPS #789
Conversation
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
|
@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 |
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
I've turned this issue into a pull request using hub. issue to pr is a deprecated feature (for several years now).
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. |
thanks @jay ! |
Reported by @jay in this mailing list post, filed here to make sure we don't forget:
And no, it is not acceptable. We should make sure to only used ALPN (and h2) when walking HTTPS.