restrict TLS cipher suites of the server (#5245)

This change restircts the supported cipher suites of the minio server. The server only supports AEAD ciphers (Chacha20Poly1305 and AES-GCM) The supported cipher suites are: - 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_ECDSA_WITH_AES_128_GCM_SHA256 - tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 - tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 Fixes #5244 and #5291
7 years ago · dd202a1a5f
parent ede504400f
commit dd202a1a5f
2 changed files with 113 additions and 4 deletions
--- a/pkg/http/server.go
+++ b/pkg/http/server.go
@ -25,6 +25,7 @@ import (
 	"time"

 	humanize "github.com/dustin/go-humanize"
+	"github.com/minio/minio-go/pkg/set"
 )

 const (
@ -64,16 +65,19 @@ type Server struct {
 // Start - start HTTP server
 func (srv *Server) Start() (err error) {
 	// Take a copy of server fields.
-	tlsConfig := srv.TLSConfig
+	var tlsConfig *tls.Config
+	if srv.TLSConfig != nil {
+		tlsConfig = srv.TLSConfig.Clone()
+	}
 	readTimeout := srv.ReadTimeout
 	writeTimeout := srv.WriteTimeout
-	handler := srv.Handler
+	handler := srv.Handler // if srv.Handler holds non-synced state -> possible data race

-	addrs := srv.Addrs
+	addrs := set.CreateStringSet(srv.Addrs...).ToSlice() // copy and remove duplicates
 	tcpKeepAliveTimeout := srv.TCPKeepAliveTimeout
 	updateBytesReadFunc := srv.UpdateBytesReadFunc
 	updateBytesWrittenFunc := srv.UpdateBytesWrittenFunc
-	errorLogFunc := srv.ErrorLogFunc
+	errorLogFunc := srv.ErrorLogFunc // if srv.ErrorLogFunc holds non-synced state -> possible data race

 	// Create new HTTP listener.
 	var listener *httpListener
@ -118,9 +122,12 @@ func (srv *Server) Start() (err error) {

 // Shutdown - shuts down HTTP server.
 func (srv *Server) Shutdown() error {
+	srv.listenerMutex.Lock()
 	if srv.listener == nil {
+		srv.listenerMutex.Unlock()
 		return errors.New("server not initialized")
 	}
+	srv.listenerMutex.Unlock()

 	if atomic.AddUint32(&srv.inShutdown, 1) > 1 {
 		// shutdown in progress
@ -149,12 +156,32 @@ func (srv *Server) Shutdown() error {
 	}
 }

+// Secure Go implementations of modern TLS ciphers
+// The following ciphers are excluded because:
+//  - RC4 ciphers:              RC4 is broken
+//  - 3DES ciphers:             Because of the 64 bit blocksize of DES (Sweet32)
+//  - CBC-SHA256 ciphers:       No countermeasures against Lucky13 timing attack
+//  - CBC-SHA ciphers:          Legacy ciphers (SHA-1) and non-constant time
+//                              implementation of CBC.
+//                              (CBC-SHA ciphers can be enabled again if required)
+//  - RSA key exchange ciphers: Disabled because of dangerous PKCS1-v1.5 RSA
+//                              padding scheme. See Bleichenbacher attacks.
+var defaultCipherSuites = []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_ECDSA_WITH_AES_128_GCM_SHA256,
+	tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
+	tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
+}
+
 // NewServer - creates new HTTP server using given arguments.
 func NewServer(addrs []string, handler http.Handler, certificate *tls.Certificate) *Server {
 	var tlsConfig *tls.Config
 	if certificate != nil {
 		tlsConfig = &tls.Config{
 			PreferServerCipherSuites: true,
+			CipherSuites:             defaultCipherSuites,
 			MinVersion:               tls.VersionTLS12,
 			NextProtos:               []string{"http/1.1", "h2"},
 		}
--- a/pkg/http/server_test.go
+++ b/pkg/http/server_test.go
@ -22,6 +22,7 @@ import (
 	"net/http"
 	"reflect"
 	"testing"
+	"time"
 )

 func TestNewServer(t *testing.T) {
@ -97,3 +98,84 @@ func TestNewServer(t *testing.T) {
 		}
 	}
 }
+
+func TestServerTLSCiphers(t *testing.T) {
+	var unsupportedCipherSuites = []uint16{
+		tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,    // Go stack contains (some) countermeasures against timing attacks (Lucky13)
+		tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, // No countermeasures against timing attacks
+		tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,    // Go stack contains (some) countermeasures against timing attacks (Lucky13)
+		tls.TLS_ECDHE_ECDSA_WITH_RC4_128_SHA,        // Broken cipher
+		tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,     // Sweet32
+		tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,      // Go stack contains (some) countermeasures against timing attacks (Lucky13)
+		tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256,   // No countermeasures against timing attacks
+		tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,      // Go stack contains (some) countermeasures against timing attacks (Lucky13)
+		tls.TLS_ECDHE_RSA_WITH_RC4_128_SHA,          // Broken cipher
+
+		// all RSA-PKCS1-v1.5 ciphers are disabled - danger of Bleichenbacher attack variants
+		tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA,   // Sweet32
+		tls.TLS_RSA_WITH_AES_128_CBC_SHA,    // Go stack contains (some) countermeasures against timing attacks (Lucky13)
+		tls.TLS_RSA_WITH_AES_128_CBC_SHA256, // No countermeasures against timing attacks
+		tls.TLS_RSA_WITH_AES_256_CBC_SHA,    // Go stack contains (some) countermeasures against timing attacks (Lucky13)
+		tls.TLS_RSA_WITH_RC4_128_SHA,        // Broken cipher
+
+		tls.TLS_RSA_WITH_AES_128_GCM_SHA256, // Disabled because of RSA-PKCS1-v1.5 - AES-GCM is considered secure.
+		tls.TLS_RSA_WITH_AES_256_GCM_SHA384, // Disabled because of RSA-PKCS1-v1.5 - AES-GCM is considered secure.
+	}
+
+	certificate, err := getTLSCert()
+	if err != nil {
+		t.Fatalf("Unable to parse private/certificate data. %v\n", err)
+	}
+
+	testCases := []struct {
+		ciphers            []uint16
+		resetServerCiphers bool
+		expectErr          bool
+	}{
+		{nil, false, false},
+		{defaultCipherSuites, false, false},
+		{unsupportedCipherSuites, false, true},
+		{nil, true, false},
+	}
+
+	for i, testCase := range testCases {
+		func() {
+			addr := "127.0.0.1:" + getNextPort()
+
+			server := NewServer([]string{addr},
+				http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+					fmt.Fprintf(w, "Hello, world")
+				}),
+				&certificate)
+			if testCase.resetServerCiphers {
+				// Use Go default ciphers.
+				server.TLSConfig.CipherSuites = nil
+			}
+
+			go func() {
+				server.Start()
+			}()
+			defer server.Shutdown()
+
+			client := http.Client{
+				Transport: &http.Transport{
+					TLSClientConfig: &tls.Config{
+						InsecureSkipVerify: true,
+						CipherSuites:       testCase.ciphers,
+					},
+				},
+			}
+
+			// There is no guaranteed way to know whether the HTTP server is started successfully.
+			// The only option is to connect and check.  Hence below sleep is used as workaround.
+			time.Sleep(1 * time.Second)
+
+			_, err := client.Get("https://" + addr)
+			expectErr := (err != nil)
+
+			if expectErr != testCase.expectErr {
+				t.Fatalf("test %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr)
+			}
+		}()
+	}
+}