On Wed, Jul 26, 2017 at 09:58:57AM -0700, Kevin McArthur wrote:
> This seems to stop the primary vector. I can still tie up a valid sni with a
> misconfigured backend, but I'm not sure that would be a client-controlled
> condition.
And more importantly, the client's SNI is not the only source of SNI,
there are other valid use cases.
> Perhaps strict-sni should be defaulted?
No, it would be a pain to a lot of people (ie making it impossible for
a hosting provider to present an error page by default). And as mentionned
above, it would only address the issue in your use case, not all of them.
However I have a good news. I found that it was possible to access the
connection from the verify callback! With a connection comes the ability
to place a specific error code which we can verify later. So I did this,
1) add a new error code for a wrong certificate, and 2) add the check for
this specific use case (ie: cert name verification failed against a non-
hardcoded value, so fail immediately). It now immediately reports the
503 and you don't have the retries anymore.
I'm attaching the two patches that I intend to merge if there's no
objection.
Willy
>From d0880fd22431b8f65654b77a0c4c681265fe1131 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w...@1wt.eu>
Date: Wed, 26 Jul 2017 20:09:56 +0200
Subject: MINOR: ssl: add a new error code for wrong server certificates
If a server presents an unexpected certificate to haproxy, that is, a
certificate that doesn't match the expected name as configured in
verifyhost or as requested using SNI, we want to store that precious
information. Fortunately we have access to the connection in the
verification callback so it's possible to store an error code there.
For this purpose we use CO_ER_SSL_WRONG_CRT.
---
include/proto/connection.h | 1 +
include/types/connection.h | 1 +
src/ssl_sock.c | 3 +++
3 files changed, 5 insertions(+)
diff --git a/include/proto/connection.h b/include/proto/connection.h
index 09467ba..2f00863 100644
--- a/include/proto/connection.h
+++ b/include/proto/connection.h
@@ -605,6 +605,7 @@ static inline const char *conn_err_code_str(struct
connection *c)
case CO_ER_SSL_RENEG: return "Rejected a client-initiated SSL
renegociation attempt";
case CO_ER_SSL_CA_FAIL: return "SSL client CA chain cannot be
verified";
case CO_ER_SSL_CRT_FAIL: return "SSL client certificate not trusted";
+ case CO_ER_SSL_WRONG_CRT: return "Server presented an SSL certificate
for a wrong name";
case CO_ER_SSL_HANDSHAKE: return "SSL handshake failure";
case CO_ER_SSL_HANDSHAKE_HB: return "SSL handshake failure after
heartbeat";
case CO_ER_SSL_KILLED_HB: return "Stopped a TLSv1 heartbeat attack
(CVE-2014-0160)";
diff --git a/include/types/connection.h b/include/types/connection.h
index 1e3fb73..4b5b5d3 100644
--- a/include/types/connection.h
+++ b/include/types/connection.h
@@ -179,6 +179,7 @@ enum {
CO_ER_SSL_RENEG, /* forbidden client renegociation */
CO_ER_SSL_CA_FAIL, /* client cert verification failed in the CA
chain */
CO_ER_SSL_CRT_FAIL, /* client cert verification failed on the
certificate */
+ CO_ER_SSL_WRONG_CRT, /* server presented a certificate for a wrong
server */
CO_ER_SSL_HANDSHAKE, /* SSL error during handshake */
CO_ER_SSL_HANDSHAKE_HB, /* SSL error during handshake with heartbeat
present */
CO_ER_SSL_KILLED_HB, /* Stopped a TLSv1 heartbeat attack
(CVE-2014-0160) */
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 42d27de..be75d5b 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -4001,6 +4001,9 @@ static int ssl_sock_srv_verifycbk(int ok, X509_STORE_CTX
*ctx)
}
}
+ /* report the mismatch */
+ if (!ok && !conn->err_code)
+ conn->err_code = CO_ER_SSL_WRONG_CRT;
return ok;
}
--
1.7.12.1
>From 88dc39b889b511304222f4eaf8498e5df8ed7610 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w...@1wt.eu>
Date: Wed, 26 Jul 2017 20:13:37 +0200
Subject: BUG/MEDIUM: stream: don't retry SSL connections which fail the
hostname check
Commits 2ab8867 ("MINOR: ssl: compare server certificate names to the
SNI on outgoing connections") and 96c7b8d ("BUG/MINOR: ssl: Fix check
against SNI during server certificate verification") made it possible
to check that the server's certificate matches the name presented in
the SNI field. While it solves a class of problems, it opens another
one which is that by failing such a connection, we'll retry it and put
more load on the server. It can be a real problem if a user can trigger
this issue, which is what will very often happen when the SNI is forwarded
from the client to the server.
This patch solves this by detecting that this very specific hostname
verification failed and that the hostname was provided using SNI, and
then it simply disables retries and the failure is immediate.
At the time of writing this patch, the previous patches were not backported
(yet), so no backport is needed for this one unless the aforementionned
patches are backported as well. This patch requires d0880fd ("MINOR: ssl:
add a new error code for wrong server certificates").
---
src/stream.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/src/stream.c b/src/stream.c
index 1aa5475..e758a63 100644
--- a/src/stream.c
+++ b/src/stream.c
@@ -595,6 +595,7 @@ static int sess_update_st_con_tcp(struct stream *s)
static int sess_update_st_cer(struct stream *s)
{
struct stream_interface *si = &s->si[1];
+ struct connection *conn = objt_conn(si->end);
/* we probably have to release last stream from the server */
if (objt_server(s->target)) {
@@ -604,6 +605,28 @@ static int sess_update_st_cer(struct stream *s)
s->flags &= ~SF_CURR_SESS;
objt_server(s->target)->cur_sess--;
}
+
+ if ((si->flags & SI_FL_ERR) &&
+ conn && conn->err_code == CO_ER_SSL_WRONG_CRT &&
+ !objt_server(s->target)->ssl_ctx.verify_host) {
+ /* We tried to connect to a server which is configured
+ * with "verify required" and which doesn't have the
+ * "verifyhost" directive. The server presented a wrong
+ * certificate (a certificate for an unexpected name),
+ * which implies that we have used SNI in the handshake,
+ * and that the server doesn't have the associated cert
+ * and presented a default one.
+ *
+ * This is a serious enough issue not to retry. It's
+ * especially important because this wrong name might
+ * either be the result of a configuration error, and
+ * retrying will only hammer the server, or is caused
+ * by the use of a wrong SNI value, most likely
+ * provided by the client and we don't want to let the
+ * client provoke retries.
+ */
+ si->conn_retries = 0;
+ }
}
/* ensure that we have enough retries left */
--
1.7.12.1
