Hi guys,
back to this old discussion.
On Fri, May 12, 2017 at 04:10:20PM +0200, Willy Tarreau wrote:
> On Tue, May 09, 2017 at 12:12:42AM +0200, Lukas Tribus wrote:
> > Haproxy can verify the certificate of backend TLS servers since day 1.
> >
> > The only thing missing is client SNI based backend certificate
> > verification, which yes - since we can pass client SNI to the TLS server
> > - we need to consider for the certificate verification process as well.
>
> In fact the cert name is checked, it's just that it can only check against
> a constant in the configuration. I agree that it's a problem when using SNI.
> Furthermore it forces one to completely disable verifyhost in case SNI is
> used.
>
> I tend to think that the best approach would be to always enable it when
> SNI is involved in fact, because if SNI is used to the server, it really
> means we want to check what cert is provided. This could then possibly be
> explicitly turned off by the "verify none" directive.
>
> I have absolutely no idea how to do that however, I don't know if we can
> retrieve the previously configured SNI using openssl's API after the
> connection is established.
So after digging I found a way to implement this. The configured SNI value
is indeed stored in the SSL session and can be extracted, more or less
easily depending on the version. I came up with the attached patch that
works for me on all cases below. I'm willing to merge it into 1.8-dev and
later backport to older versions once we're sure it does the right thing
and doesn't break compatibility with openssl forks.
Please give it a try, and if some can test it on libressl/boringssl, it
would be nice.
---------------
tested with the following server configurations (cert on port 8443 is valid
for "localhost" only) :
# passes checks and traffic (no hostname check)
server ssl 127.0.0.1:8443 ssl verify required check inter 100 ca-file
rsa2048.pem
# passes checks and traffic (localhost is what the server presents)
server ssl 127.0.0.1:8443 ssl verify required check inter 100 ca-file
rsa2048.pem verifyhost localhost
# fails checks and traffic (foo not matched on the server)
server ssl 127.0.0.1:8443 ssl verify required check inter 100 ca-file
rsa2048.pem verifyhost foo
# passes checks and traffic (verify none ignores the host)
server ssl 127.0.0.1:8443 ssl verify none check inter 100 ca-file
rsa2048.pem verifyhost foo
# passes checks and traffic (localhost is fine)
server ssl 127.0.0.1:8443 ssl verify required check inter 100 ca-file
rsa2048.pem sni str(localhost) verifyhost localhost
# passes checks and traffic (verifyhost overrides sni)
server ssl 127.0.0.1:8443 ssl verify required check inter 100 ca-file
rsa2048.pem sni str(foo) verifyhost localhost
# passes checks and traffic (localhost always valid)
server ssl 127.0.0.1:8443 ssl verify required check inter 100 ca-file
rsa2048.pem sni str(localhost)
# passes checks, and traffic without host or with "host: localhost" and
fails other hosts.
server ssl 127.0.0.1:8443 ssl verify required check inter 100 ca-file
rsa2048.pem sni req.hdr(host)
---------------
Cheers,
Willy
>From 14eea17b6f43f391a2ac7436ce5aafc740e121aa Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w...@1wt.eu>
Date: Wed, 5 Jul 2017 18:23:03 +0200
Subject: MINOR: ssl: compare server certificate names to the SNI on outgoing
connections
When support for passing SNI to the server was added in 1.6-dev3, there
was no way to validate that the certificate presented by the server would
really match the name requested in the SNI, which is quite a problem as
it allows other (valid) certificates to be presented instead (when hitting
the wrong server or due to a man in the middle).
This patch adds the missing check against the value passed in the SNI.
The "verifyhost" value keeps precedence if set. If no SNI is used and
no verifyhost directive is specified, then the certificate name is not
checked (this is unchanged).
In order to extract the SNI value, it was necessary to make use of
SSL_SESSION_get0_hostname(), which appeared in openssl 1.1.0. This is
a trivial function which returns the value of s->tlsext_hostname, so
it was open coded on older versions. I don't know what it does for
libressl / boringssl nor any possible other fork.
After some careful observation period it may make sense to backport
this to 1.7 and 1.6 as some users rightfully consider this limitation
as a bug.
---
doc/configuration.txt | 23 +++++++++++++++--------
src/ssl_sock.c | 22 +++++++++++++++++++++-
2 files changed, 36 insertions(+), 9 deletions(-)
diff --git a/doc/configuration.txt b/doc/configuration.txt
index 7cac3ca..0f425f4 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -11474,7 +11474,9 @@ sni <expression>
string and uses the result as the host name sent in the SNI TLS extension to
the server. A typical use case is to send the SNI received from the client in
a bridged HTTPS scenario, using the "ssl_fc_sni" sample fetch for the
- expression, though alternatives such as req.hdr(host) can also make sense.
+ expression, though alternatives such as req.hdr(host) can also make sense. If
+ "verify required" is set (which is the recommended setting), the resulting
+ name will also be matched against the server certificate's names.
source <addr>[:<pl>[-<ph>]] [usesrc { <addr2>[:<port2>] | client | clientip } ]
source <addr>[:<port>] [usesrc { <addr2>[:<port2>] | hdr_ip(<hdr>[,<occ>]) } ]
@@ -11562,11 +11564,15 @@ verify [none|required]
This setting is only available when support for OpenSSL was built in. If set
to 'none', server certificate is not verified. In the other case, The
certificate provided by the server is verified using CAs from 'ca-file'
- and optional CRLs from 'crl-file'. If 'ssl_server_verify' is not specified
- in global section, this is the default. On verify failure the handshake
- is aborted. It is critically important to verify server certificates when
- using SSL to connect to servers, otherwise the communication is prone to
- trivial man-in-the-middle attacks rendering SSL totally useless.
+ and optional CRLs from 'crl-file' after having checked that the names
+ provided in the certificate match either the static host name passed using
+ the "verifyhost" directive, or if not provided, the name passed using the
+ "sni" directive. When no name is found, the certificate's names are ignored.
+ If 'ssl_server_verify' is not specified in global section, this is the
+ default. On verify failure the handshake is aborted. It is critically
+ important to verify server certificates when using SSL to connect to servers,
+ otherwise the communication is prone to trivial man-in-the-middle attacks
+ rendering SSL totally useless.
verifyhost <hostname>
This setting is only available when support for OpenSSL was built in, and
@@ -11574,8 +11580,9 @@ verifyhost <hostname>
hostnames in the subject and subjectAlternateNames of the certificate
provided by the server are checked. If none of the hostnames in the
certificate match the specified hostname, the handshake is aborted. The
- hostnames in the server-provided certificate may include wildcards.
- See also "no-verifyhost" option.
+ hostnames in the server-provided certificate may include wildcards. Note
+ that the name provided here overrides (for the checks) any possible name
+ passed using "sni". See also "no-verifyhost" option.
weight <weight>
The "weight" parameter is used to adjust the server's weight relative to
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index cd39c6e..9128196 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -127,6 +127,13 @@
#define HASH_FUNCT EVP_sha256
#endif /* OPENSSL_NO_SHA256 */
+/* Needed to retrieve the configured SNI for an outgoing connection, but only
+ * appeared in openssl 1.1.0. I don't know how it's done with forked versions.
+ */
+#if OPENSSL_VERSION_NUMBER < 0x1010000fL
+#define SSL_SESSION_get0_hostname(s) s->tlsext_hostname
+#endif
+
/* ssl_methods flags for ssl options */
#define MC_SSL_O_ALL 0x0000
#define MC_SSL_O_NO_SSLV3 0x0001 /* disable SSLv3 */
@@ -3941,7 +3948,20 @@ static int ssl_sock_srv_verifycbk(int ok, X509_STORE_CTX
*ctx)
ssl = X509_STORE_CTX_get_ex_data(ctx,
SSL_get_ex_data_X509_STORE_CTX_idx());
conn = SSL_get_app_data(ssl);
+ /* we're checking against the configured "verifyhost" directive if
+ * present, or against the SNI used on this connection if present.
+ * If neither is set, the verification is OK.
+ */
servername = objt_server(conn->target)->ssl_ctx.verify_host;
+ if (!servername) {
+ SSL_SESSION *ssl_sess = SSL_get_session(conn->xprt_ctx);
+ if (!ssl_sess)
+ return ok;
+
+ servername = SSL_SESSION_get0_hostname(ssl);
+ if (!servername)
+ return ok;
+ }
/* We only need to verify the CN on the actual server cert,
* not the indirect CAs */
@@ -4138,7 +4158,7 @@ int ssl_sock_prepare_srv_ctx(struct server *srv)
}
SSL_CTX_set_verify(srv->ssl_ctx.ctx,
verify,
- srv->ssl_ctx.verify_host ? ssl_sock_srv_verifycbk :
NULL);
+ (srv->ssl_ctx.verify_host || (verify &
SSL_VERIFY_PEER)) ? ssl_sock_srv_verifycbk : NULL);
if (verify & SSL_VERIFY_PEER) {
if (srv->ssl_ctx.ca_file) {
/* load CAfile to verify */
--
1.7.12.1
