Pull request: all: imp tls cipher selection

Closes #2993. Squashed commit of the following: commit 6c521e5 Author: Ainar Garipov <[email protected]> Date: Tue Jan 25 21:39:48 2022 +0300 all: imp tls cipher selection
AdguardTeam · Jan 26, 2022 · 504c54a · 504c54a
1 parent 90c17c7
commit 504c54a
Show file tree

Hide file tree

Showing 7 changed files with 52 additions and 62 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -60,7 +60,13 @@ In this release, the schema version has changed from 12 to 13.
 
 - Go 1.16 support.
 
+### Security
+
+- Weaker cipher suites that use the CBC (cipher block chaining) mode of
+  operation have been disabled (#2993).
+
 [#1730]: https://github.com/AdguardTeam/AdGuardHome/issues/1730
+[#2993]: https://github.com/AdguardTeam/AdGuardHome/issues/2993
 [#3057]: https://github.com/AdguardTeam/AdGuardHome/issues/3057
 [#3367]: https://github.com/AdguardTeam/AdGuardHome/issues/3367
 

diff --git a/internal/aghtls/aghtls.go b/internal/aghtls/aghtls.go
@@ -0,0 +1,31 @@
+// Package aghtls contains utilities for work with TLS.
+package aghtls
+
+import "crypto/tls"
+
+// SaferCipherSuites returns a set of default cipher suites with vulnerable and
+// weak cipher suites removed.
+func SaferCipherSuites() (safe []uint16) {
+	suites := tls.CipherSuites()
+	for _, s := range suites {
+		switch s.ID {
+		case
+			tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA,
+			tls.TLS_RSA_WITH_AES_128_CBC_SHA,
+			tls.TLS_RSA_WITH_AES_256_CBC_SHA,
+			tls.TLS_RSA_WITH_AES_128_CBC_SHA256,
+			tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
+			tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
+			tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,
+			tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
+			tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
+			tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,
+			tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256:
+			// Less safe 3DES and CBC suites, go on.
+		default:
+			safe = append(safe, s.ID)
+		}
+	}
+
+	return safe
+}
diff --git a/internal/dnsforward/config.go b/internal/dnsforward/config.go
@@ -11,6 +11,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/AdguardTeam/AdGuardHome/internal/aghtls"
 	"github.com/AdguardTeam/AdGuardHome/internal/filtering"
 	"github.com/AdguardTeam/dnsproxy/proxy"
 	"github.com/AdguardTeam/dnsproxy/upstream"
@@ -426,6 +427,7 @@ func (s *Server) prepareTLS(proxyConfig *proxy.Config) error {
 
 	proxyConfig.TLSConfig = &tls.Config{
 		GetCertificate: s.onGetCertificate,
+		CipherSuites:   aghtls.SaferCipherSuites(),
 		MinVersion:     tls.VersionTLS12,
 	}
 

diff --git a/internal/home/dns.go b/internal/home/dns.go
@@ -211,7 +211,6 @@ func generateServerConfig() (newConf dnsforward.ServerConfig, err error) {
 	}
 
 	newConf.TLSv12Roots = Context.tlsRoots
-	newConf.TLSCiphers = Context.tlsCiphers
 	newConf.TLSAllowUnencryptedDoH = tlsConf.AllowUnencryptedDoH
 
 	newConf.FilterHandler = applyAdditionalFiltering

diff --git a/internal/home/home.go b/internal/home/home.go
@@ -22,6 +22,7 @@ import (
 	"github.com/AdguardTeam/AdGuardHome/internal/aghalg"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghos"
+	"github.com/AdguardTeam/AdGuardHome/internal/aghtls"
 	"github.com/AdguardTeam/AdGuardHome/internal/dhcpd"
 	"github.com/AdguardTeam/AdGuardHome/internal/dnsforward"
 	"github.com/AdguardTeam/AdGuardHome/internal/filtering"
@@ -80,7 +81,6 @@ type homeContext struct {
 	disableUpdate    bool   // If set, don't check for updates
 	controlLock      sync.Mutex
 	tlsRoots         *x509.CertPool // list of root CAs for TLSv1.2
-	tlsCiphers       []uint16       // list of TLS ciphers to use
 	transport        *http.Transport
 	client           *http.Client
 	appSignalChannel chan os.Signal // Channel for receiving OS signals by the console app
@@ -145,13 +145,13 @@ func setupContext(args options) {
 	initConfig()
 
 	Context.tlsRoots = LoadSystemRootCAs()
-	Context.tlsCiphers = InitTLSCiphers()
 	Context.transport = &http.Transport{
 		DialContext: customDialContext,
 		Proxy:       getHTTPProxy,
 		TLSClientConfig: &tls.Config{
-			RootCAs:    Context.tlsRoots,
-			MinVersion: tls.VersionTLS12,
+			RootCAs:      Context.tlsRoots,
+			CipherSuites: aghtls.SaferCipherSuites(),
+			MinVersion:   tls.VersionTLS12,
 		},
 	}
 	Context.client = &http.Client{
@@ -182,7 +182,7 @@ func setupContext(args options) {
 
 // logIfUnsupported logs a formatted warning if the error is one of the
 // unsupported errors and returns nil.  If err is nil, logIfUnsupported returns
-// nil.  Otherise, it returns err.
+// nil.  Otherwise, it returns err.
 func logIfUnsupported(msg string, err error) (outErr error) {
 	if errors.As(err, new(*aghos.UnsupportedError)) {
 		log.Debug(msg, err)

diff --git a/internal/home/tls.go b/internal/home/tls.go
@@ -26,7 +26,6 @@ import (
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/google/go-cmp/cmp"
-	"golang.org/x/sys/cpu"
 )
 
 var tlsWebHandlersRegistered = false
@@ -731,52 +730,3 @@ func LoadSystemRootCAs() (roots *x509.CertPool) {
 
 	return nil
 }
-
-// InitTLSCiphers performs the same work as initDefaultCipherSuites() from
-// crypto/tls/common.go but don't uses lots of other default ciphers.
-func InitTLSCiphers() (ciphers []uint16) {
-	// Check the cpu flags for each platform that has optimized GCM
-	// implementations.  The worst case is when all these variables are
-	// false.
-	var (
-		hasGCMAsmAMD64 = cpu.X86.HasAES && cpu.X86.HasPCLMULQDQ
-		hasGCMAsmARM64 = cpu.ARM64.HasAES && cpu.ARM64.HasPMULL
-		// Keep in sync with crypto/aes/cipher_s390x.go.
-		hasGCMAsmS390X = cpu.S390X.HasAES &&
-			cpu.S390X.HasAESCBC &&
-			cpu.S390X.HasAESCTR &&
-			(cpu.S390X.HasGHASH || cpu.S390X.HasAESGCM)
-
-		hasGCMAsm = hasGCMAsmAMD64 || hasGCMAsmARM64 || hasGCMAsmS390X
-	)
-
-	if hasGCMAsm {
-		// If AES-GCM hardware is provided then prioritize AES-GCM
-		// cipher suites.
-		ciphers = []uint16{
-			tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
-			tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
-			tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
-			tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
-			tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
-			tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
-		}
-	} else {
-		// Without AES-GCM hardware, we put the ChaCha20-Poly1305 cipher
-		// suites first.
-		ciphers = []uint16{
-			tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
-			tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
-			tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
-			tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
-			tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
-			tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
-		}
-	}
-
-	return append(
-		ciphers,
-		tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,
-		tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,
-	)
-}
diff --git a/internal/home/web.go b/internal/home/web.go
@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
+	"github.com/AdguardTeam/AdGuardHome/internal/aghtls"
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
 	"github.com/AdguardTeam/golibs/netutil"
@@ -34,14 +35,13 @@ const (
 )
 
 type webConfig struct {
+	clientFS     fs.FS
+	clientBetaFS fs.FS
+
 	BindHost     net.IP
 	BindPort     int
 	BetaBindPort int
 	PortHTTPS    int
-	firstRun     bool
-
-	clientFS     fs.FS
-	clientBetaFS fs.FS
 
 	// ReadTimeout is an option to pass to http.Server for setting an
 	// appropriate field.
@@ -54,6 +54,8 @@ type webConfig struct {
 	// WriteTimeout is an option to pass to http.Server for setting an
 	// appropriate field.
 	WriteTimeout time.Duration
+
+	firstRun bool
 }
 
 // HTTPSServer - HTTPS Server
@@ -263,9 +265,9 @@ func (web *Web) tlsServerLoop() {
 			Addr:     address,
 			TLSConfig: &tls.Config{
 				Certificates: []tls.Certificate{web.httpsServer.cert},
-				MinVersion:   tls.VersionTLS12,
 				RootCAs:      Context.tlsRoots,
-				CipherSuites: Context.tlsCiphers,
+				CipherSuites: aghtls.SaferCipherSuites(),
+				MinVersion:   tls.VersionTLS12,
 			},
 			Handler:           withMiddlewares(Context.mux, limitRequestBody),
 			ReadTimeout:       web.conf.ReadTimeout,