From 4d9ee000c8d2cbcdd8284007c1e0f2da7bc3c7c3 Mon Sep 17 00:00:00 2001
From: Matt Holt <mholt@users.noreply.github.com>
Date: Fri, 30 Mar 2018 14:40:04 -0600
Subject: [PATCH] httpserver: Prevent TLS client authentication bypass in 3
ways (#2099)
- Introduce StrictHostMatching mode for sites that require clientauth
- Error if QUIC is enabled whilst TLS clientauth is configured
(Our QUIC implementation does not yet support TLS clientauth, but
maybe it will in the future - fixes #2095)
- Error if one but not all TLS configs for the same hostname have a
different ClientAuth CA pool
---
caddyhttp/httpserver/plugin.go | 22 +++++++++++++++++++---
caddyhttp/httpserver/server.go | 12 ++++++++++++
caddyhttp/httpserver/siteconfig.go | 10 ++++++++++
caddytls/config.go | 8 ++++++++
4 files changed, 49 insertions(+), 3 deletions(-)
diff --git a/caddyhttp/httpserver/plugin.go b/caddyhttp/httpserver/plugin.go
index ad2d87388..1508b41ec 100644
--- a/caddyhttp/httpserver/plugin.go
+++ b/caddyhttp/httpserver/plugin.go
@@ -15,6 +15,7 @@
package httpserver
import (
+ "crypto/tls"
"flag"
"fmt"
"log"
@@ -207,8 +208,13 @@ func (h *httpContext) InspectServerBlocks(sourceFile string, serverBlocks []cadd
// MakeServers uses the newly-created siteConfigs to
// create and return a list of server instances.
func (h *httpContext) MakeServers() ([]caddy.Server, error) {
- // make sure TLS is disabled for explicitly-HTTP sites
- // (necessary when HTTP address shares a block containing tls)
+ // Iterate each site configuration and make sure that:
+ // 1) TLS is disabled for explicitly-HTTP sites (necessary
+ // when an HTTP address shares a block containing tls)
+ // 2) if QUIC is enabled, TLS ClientAuth is not, because
+ // currently, QUIC does not support ClientAuth (TODO:
+ // revisit this when our QUIC implementation supports it)
+ // 3) if TLS ClientAuth is used, StrictHostMatching is on
for _, cfg := range h.siteConfigs {
if !cfg.TLS.Enabled {
continue
@@ -230,6 +236,17 @@ func (h *httpContext) MakeServers() ([]caddy.Server, error) {
// instead of 443 because it doesn't know about TLS.
cfg.Addr.Port = HTTPSPort
}
+ if cfg.TLS.ClientAuth != tls.NoClientCert {
+ if QUIC {
+ return nil, fmt.Errorf("cannot enable TLS client authentication with QUIC, because QUIC does not yet support it")
+ }
+ // this must be enabled so that a client cannot connect
+ // using SNI for another site on this listener that
+ // does NOT require ClientAuth, and then send HTTP
+ // requests with the Host header of this site which DOES
+ // require client auth, thus bypassing it...
+ cfg.StrictHostMatching = true
+ }
}
// we must map (group) each config to a bind address
@@ -556,7 +573,6 @@ var directives = []string{
"minify", // github.com/hacdias/caddy-minify
"ipfilter", // github.com/pyed/ipfilter
"ratelimit", // github.com/xuqingfeng/caddy-rate-limit
- "search", // github.com/pedronasser/caddy-search
"expires", // github.com/epicagency/caddy-expires
"forwardproxy", // github.com/caddyserver/forwardproxy
"basicauth",
diff --git a/caddyhttp/httpserver/server.go b/caddyhttp/httpserver/server.go
index a98a8fdf3..8028fce85 100644
--- a/caddyhttp/httpserver/server.go
+++ b/caddyhttp/httpserver/server.go
@@ -416,6 +416,18 @@ func (s *Server) serveHTTP(w http.ResponseWriter, r *http.Request) (int, error)
r.URL = trimPathPrefix(r.URL, pathPrefix)
}
+ // enforce strict host matching, which ensures that the SNI
+ // value (if any), matches the Host header; essential for
+ // sites that rely on TLS ClientAuth sharing a port with
+ // sites that do not - if mismatched, close the connection
+ if vhost.StrictHostMatching && r.TLS != nil &&
+ strings.ToLower(r.TLS.ServerName) != strings.ToLower(hostname) {
+ r.Close = true
+ log.Printf("[ERROR] %s - strict host matching: SNI (%s) and HTTP Host (%s) values differ",
+ vhost.Addr, r.TLS.ServerName, hostname)
+ return http.StatusForbidden, nil
+ }
+
return vhost.middlewareChain.ServeHTTP(w, r)
}
diff --git a/caddyhttp/httpserver/siteconfig.go b/caddyhttp/httpserver/siteconfig.go
index 1cd9a0504..3b29f911a 100644
--- a/caddyhttp/httpserver/siteconfig.go
+++ b/caddyhttp/httpserver/siteconfig.go
@@ -36,6 +36,16 @@ type SiteConfig struct {
// TLS configuration
TLS *caddytls.Config
+ // If true, the Host header in the HTTP request must
+ // match the SNI value in the TLS handshake (if any).
+ // This should be enabled whenever a site relies on
+ // TLS client authentication, for example; or any time
+ // you want to enforce that THIS site's TLS config
+ // is used and not the TLS config of any other site
+ // on the same listener. TODO: Check how relevant this
+ // is with TLS 1.3.
+ StrictHostMatching bool
+
// Uncompiled middleware stack
middleware []Middleware
diff --git a/caddytls/config.go b/caddytls/config.go
index 808ed5271..80f1633f0 100644
--- a/caddytls/config.go
+++ b/caddytls/config.go
@@ -513,6 +513,14 @@ func assertConfigsCompatible(cfg1, cfg2 *Config) error {
if c1.ClientAuth != c2.ClientAuth {
return fmt.Errorf("client authentication policy mismatch")
}
+ if c1.ClientAuth != tls.NoClientCert && c2.ClientAuth != tls.NoClientCert && c1.ClientCAs != c2.ClientCAs {
+ // Two hosts defined on the same listener are not compatible if they
+ // have ClientAuth enabled, because there's no guarantee beyond the
+ // hostname which config will be used (because SNI only has server name).
+ // To prevent clients from bypassing authentication, require that
+ // ClientAuth be configured in an unambiguous manner.
+ return fmt.Errorf("multiple hosts requiring client authentication ambiguously configured")
+ }
return nil
}