Hi Grant, Willy,
On 05/27/2017 09:03 PM, Grant Zhang wrote:
>
>> On May 26, 2017, at 22:21, Willy Tarreau <[email protected]> wrote:
>>
>> Hi Emeric, Grant,
>>
>> patch set now merged! Thank you both for this great work!
>>
>> Willy
>
> Bravo! Really appreciate your and Emeric's help in this effort.
>
> Grant
>
Here 3 new fixes:
I noticed a segfault sometime at connection closure (first patch)
I noticed buffer overflows using the cipher offloading in async:
The moving or reuse of buffer addresses passed to SSL_read/write in haproxy
is not compliant with the ASYNC API. I had a discussion about that on the
openssl-dev mailing list with Matt Caswell.
So the second patch disables the async mode for the symmetric stuff.
The last one to not call directly the conn_fd_handler from the async_fd_handler.
R,
Emeric
>From 535ef040f7c6ee31f8c8943d1db5236f66cb6e43 Mon Sep 17 00:00:00 2001
From: root <root@ded5021>
Date: Fri, 2 Jun 2017 15:54:06 +0000
Subject: [PATCH 3/3] BUG/MINOR: ssl: do not call directly the conn_fd_handler
from async_fd_handler
This patch modifies the way to re-enable the connection from the async fd
handler calling conn_update_sock_polling instead of the conn_fd_handler.
It also ensures that the polling is really stopped on the async fd.
---
src/ssl_sock.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index a009803..04c4cbb 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -363,17 +363,19 @@ fail_get:
static void ssl_async_fd_handler(int fd)
{
struct connection *conn = fdtab[fd].owner;
- int conn_fd = conn->t.sock.fd;
/* fd is an async enfine fd, we must stop
* to poll this fd until it is requested
*/
+ fd_stop_recv(fd);
fd_cant_recv(fd);
/* crypto engine is available, let's notify the associated
* connection that it can pursue its processing.
*/
- conn_fd_handler(conn_fd);
+ __conn_sock_want_recv(conn);
+ __conn_sock_want_send(conn);
+ conn_update_sock_polling(conn);
}
/*
--
2.1.4
>From 79b70ede0baed17e52c17ae2f6c93437fa68b824 Mon Sep 17 00:00:00 2001
From: root <root@ded5021>
Date: Tue, 6 Jun 2017 12:35:14 +0000
Subject: [PATCH 2/3] BUG/MAJOR: ssl: buffer overflow using offloaded ciphering
on async engine
The Openssl's ASYNC API does'nt support moving buffers on SSL_read/write
This patch disables the ASYNC mode dynamically when the handshake
is left and re-enables it on reneg.
---
doc/configuration.txt | 6 +++++-
src/ssl_sock.c | 39 ++++++++++++++++++++++++++++-----------
2 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 75f9961..836c8b9 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -1285,7 +1285,11 @@ ssl-engine <name> [algo <comma-seperated list of algorithms>]
ssl-mode-async
Adds SSL_MODE_ASYNC mode to the SSL context. This enables asynchronous TLS
I/O operations if asynchronous capable SSL engines are used. The current
- implementation supports a maximum of 32 engines.
+ implementation supports a maximum of 32 engines. The Openssl ASYNC API
+ doesn't support moving read/write buffers and is not compliant with
+ haproxy's buffer management. So the asynchronous mode is disabled on
+ read/write operations (it is only enabled during initial and reneg
+ handshakes).
tune.buffers.limit <number>
Sets a hard limit on the number of buffers which may be allocated per process.
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index a04deb6..a009803 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -4632,6 +4632,15 @@ int ssl_sock_handshake(struct connection *conn, unsigned int flag)
}
reneg_ok:
+
+#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+ /* ASYNC engine API doesn't support moving read/write
+ * buffers. So we disable ASYNC mode right after
+ * the handshake to avoid buffer oveflows.
+ */
+ if (global_ssl.async)
+ SSL_clear_mode(conn->xprt_ctx, SSL_MODE_ASYNC);
+#endif
/* Handshake succeeded */
if (!SSL_session_reused(conn->xprt_ctx)) {
if (objt_server(conn->target)) {
@@ -4750,6 +4759,11 @@ static int ssl_sock_to_buf(struct connection *conn, struct buffer *buf, int coun
/* handshake is running, and it needs to enable write */
conn->flags |= CO_FL_SSL_WAIT_HS;
__conn_sock_want_send(conn);
+#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+ /* Async mode can be re-enabled, because we're leaving data state.*/
+ if (global_ssl.async)
+ SSL_set_mode(conn->xprt_ctx, SSL_MODE_ASYNC);
+#endif
break;
}
else if (ret == SSL_ERROR_WANT_READ) {
@@ -4757,18 +4771,17 @@ static int ssl_sock_to_buf(struct connection *conn, struct buffer *buf, int coun
/* handshake is running, and it may need to re-enable read */
conn->flags |= CO_FL_SSL_WAIT_HS;
__conn_sock_want_recv(conn);
+#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+ /* Async mode can be re-enabled, because we're leaving data state.*/
+ if (global_ssl.async)
+ SSL_set_mode(conn->xprt_ctx, SSL_MODE_ASYNC);
+#endif
break;
}
/* we need to poll for retry a read later */
fd_cant_recv(conn->t.sock.fd);
break;
}
-#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
- else if (ret == SSL_ERROR_WANT_ASYNC) {
- ssl_async_process_fds(conn, conn->xprt_ctx);
- break;
- }
-#endif
/* otherwise it's a real error */
goto out_error;
}
@@ -4859,6 +4872,11 @@ static int ssl_sock_from_buf(struct connection *conn, struct buffer *buf, int fl
/* handshake is running, and it may need to re-enable write */
conn->flags |= CO_FL_SSL_WAIT_HS;
__conn_sock_want_send(conn);
+#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
+ /* Async mode can be re-enabled, because we're leaving data state.*/
+ if (global_ssl.async)
+ SSL_set_mode(conn->xprt_ctx, SSL_MODE_ASYNC);
+#endif
break;
}
/* we need to poll to retry a write later */
@@ -4869,14 +4887,13 @@ static int ssl_sock_from_buf(struct connection *conn, struct buffer *buf, int fl
/* handshake is running, and it needs to enable read */
conn->flags |= CO_FL_SSL_WAIT_HS;
__conn_sock_want_recv(conn);
- break;
- }
#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
- else if (ret == SSL_ERROR_WANT_ASYNC) {
- ssl_async_process_fds(conn, conn->xprt_ctx);
+ /* Async mode can be re-enabled, because we're leaving data state.*/
+ if (global_ssl.async)
+ SSL_set_mode(conn->xprt_ctx, SSL_MODE_ASYNC);
+#endif
break;
}
-#endif
goto out_error;
}
}
--
2.1.4
>From 8607971183105b82c2183c85b2a2ae9314119c99 Mon Sep 17 00:00:00 2001
From: root <root@ded5021>
Date: Wed, 31 May 2017 10:02:53 +0000
Subject: [PATCH 1/3] BUG/MAJOR: ssl: fix segfault on connection close using
async engines.
This patch ensure that the ASYNC fd handlers won't be wake up
too early, disabling the event cache for this fd on connection close
and when a WANT_ASYNC is rised by Openssl.
The calls to SSL_read/SSL_write/SSL_do_handshake before rising a real read
event from the ASYNC fd, generated an EAGAIN followed by a context switch
for some engines, or a blocked read for the others.
On connection close it resulted in a too early call to SSL_free followed
by a segmentation fault.
---
src/ssl_sock.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index dd63c19..a04deb6 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -448,8 +448,16 @@ static void inline ssl_async_process_fds(struct connection *conn, SSL *ssl)
/* We activate the polling for all known async fds */
SSL_get_all_async_fds(ssl, add_fd, &num_add_fds);
- for (i=0 ; i < num_add_fds ; i++)
+ for (i=0 ; i < num_add_fds ; i++) {
fd_want_recv(add_fd[i]);
+ /* To ensure that the fd cache won't be used
+ * We'll prefer to catch a real RD event
+ * because handling an EAGAIN on this fd will
+ * result in a context switch and also
+ * some engines uses a fd in blocking mode.
+ */
+ fd_cant_recv(add_fd[i]);
+ }
/* We must also prevent the conn_handler
* to be called until a read event was
@@ -4912,6 +4920,10 @@ static void ssl_sock_close(struct connection *conn) {
fdtab[afd].iocb = ssl_async_fd_free;
fdtab[afd].owner = conn->xprt_ctx;
fd_want_recv(afd);
+ /* To ensure that the fd cache won't be used
+ * and we'll catch a real RD event.
+ */
+ fd_cant_recv(afd);
}
conn->xprt_ctx = NULL;
jobs++;
--
2.1.4
