From 8519a24ba604f10edd0ac970040a2f8ec4891df7 Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Sun, 21 Oct 2018 14:17:50 +0900 Subject: [PATCH] cmd/*, lib/tlsutil: Refactor TLS stuff (fixes #5256) (#5276) This changes the TLS and certificate handling in a few ways: - We always use TLS 1.2, both for sync connections (as previously) and the GUI/REST/discovery stuff. This is a tightening of the requirements on the GUI. AS far as I can tell from caniusethis.com every browser from 2013 and forward supports TLS 1.2, so I think we should be fine. - We always greate ECDSA certificates. Previously we'd create ECDSA-with-RSA certificates for sync connections and pure RSA certificates for the web stuff. The new default is more modern and the same everywhere. These certificates are OK in TLS 1.2. - We use the Go CPU detection stuff to choose the cipher suites to use, indirectly. The TLS package uses CPU capabilities probing to select either AES-GCM (fast if we have AES-NI) or ChaCha20 (faster if we don't). These CPU detection things aren't exported though, so the tlsutil package now does a quick TLS handshake with itself as part of init(). If the chosen cipher suite was AES-GCM we prioritize that, otherwise we prefer ChaCha20. Some might call this ugly. I think it's awesome. --- cmd/stdiscosrv/main.go | 2 +- cmd/strelaypoolsrv/main.go | 2 +- cmd/strelaysrv/main.go | 2 +- cmd/syncthing/gui.go | 21 +---- cmd/syncthing/main.go | 40 +++------ lib/discover/global_test.go | 10 +-- lib/tlsutil/tlsutil.go | 161 ++++++++++++++++++++++++++++++++---- lib/tlsutil/tlsutil_test.go | 48 +++++++++++ 8 files changed, 217 insertions(+), 69 deletions(-) diff --git a/cmd/stdiscosrv/main.go b/cmd/stdiscosrv/main.go index 9c6199bf3..eafa1dda9 100644 --- a/cmd/stdiscosrv/main.go +++ b/cmd/stdiscosrv/main.go @@ -121,7 +121,7 @@ func main() { cert, err := tls.LoadX509KeyPair(certFile, keyFile) if err != nil { log.Println("Failed to load keypair. Generating one, this might take a while...") - cert, err = tlsutil.NewCertificate(certFile, keyFile, "stdiscosrv", 0) + cert, err = tlsutil.NewCertificate(certFile, keyFile, "stdiscosrv") if err != nil { log.Fatalln("Failed to generate X509 key pair:", err) } diff --git a/cmd/strelaypoolsrv/main.go b/cmd/strelaypoolsrv/main.go index 7f72a88d6..363c2bcb4 100644 --- a/cmd/strelaypoolsrv/main.go +++ b/cmd/strelaypoolsrv/main.go @@ -636,7 +636,7 @@ func createTestCertificate() tls.Certificate { } certFile, keyFile := filepath.Join(tmpDir, "cert.pem"), filepath.Join(tmpDir, "key.pem") - cert, err := tlsutil.NewCertificate(certFile, keyFile, "relaypoolsrv", 3072) + cert, err := tlsutil.NewCertificate(certFile, keyFile, "relaypoolsrv") if err != nil { log.Fatalln("Failed to create test X509 key pair:", err) } diff --git a/cmd/strelaysrv/main.go b/cmd/strelaysrv/main.go index 3a666349c..87d4f776b 100644 --- a/cmd/strelaysrv/main.go +++ b/cmd/strelaysrv/main.go @@ -166,7 +166,7 @@ func main() { cert, err := tls.LoadX509KeyPair(certFile, keyFile) if err != nil { log.Println("Failed to load keypair. Generating one, this might take a while...") - cert, err = tlsutil.NewCertificate(certFile, keyFile, "strelaysrv", 3072) + cert, err = tlsutil.NewCertificate(certFile, keyFile, "strelaysrv") if err != nil { log.Fatalln("Failed to generate X509 key pair:", err) } diff --git a/cmd/syncthing/gui.go b/cmd/syncthing/gui.go index 3f7be004c..13fc8bd6a 100644 --- a/cmd/syncthing/gui.go +++ b/cmd/syncthing/gui.go @@ -185,28 +185,13 @@ func (s *apiService) getListener(guiCfg config.GUIConfiguration) (net.Listener, name = tlsDefaultCommonName } - cert, err = tlsutil.NewCertificate(s.httpsCertFile, s.httpsKeyFile, name, httpsRSABits) + cert, err = tlsutil.NewCertificate(s.httpsCertFile, s.httpsKeyFile, name) } if err != nil { return nil, err } - tlsCfg := &tls.Config{ - Certificates: []tls.Certificate{cert}, - MinVersion: tls.VersionTLS10, // No SSLv3 - CipherSuites: []uint16{ - // No RC4 - tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, - tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, - tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, - tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, - tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, - tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, - tls.TLS_RSA_WITH_AES_128_CBC_SHA, - tls.TLS_RSA_WITH_AES_256_CBC_SHA, - tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA, - tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA, - }, - } + tlsCfg := tlsutil.SecureDefault() + tlsCfg.Certificates = []tls.Certificate{cert} if guiCfg.Network() == "unix" { // When listening on a UNIX socket we should unlink before bind, diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go index e54ac7637..ca33da4f6 100644 --- a/cmd/syncthing/main.go +++ b/cmd/syncthing/main.go @@ -78,8 +78,6 @@ const ( const ( bepProtocolName = "bep/1.0" tlsDefaultCommonName = "syncthing" - httpsRSABits = 2048 - bepRSABits = 0 // 384 bit ECDSA used instead defaultEventTimeout = time.Minute maxSystemErrors = 5 initialSystemLog = 10 @@ -471,7 +469,7 @@ func generate(generateDir string) { l.Warnln("Key exists; will not overwrite.") l.Infoln("Device ID:", protocol.NewDeviceID(cert.Certificate[0])) } else { - cert, err = tlsutil.NewCertificate(certFile, keyFile, tlsDefaultCommonName, bepRSABits) + cert, err = tlsutil.NewCertificate(certFile, keyFile, tlsDefaultCommonName) if err != nil { l.Fatalln("Create certificate:", err) } @@ -639,7 +637,7 @@ func syncthingMain(runtimeOptions RuntimeOptions) { cert, err := tls.LoadX509KeyPair(locations[locCertFile], locations[locKeyFile]) if err != nil { l.Infof("Generating ECDSA key and certificate for %s...", tlsDefaultCommonName) - cert, err = tlsutil.NewCertificate(locations[locCertFile], locations[locKeyFile], tlsDefaultCommonName, bepRSABits) + cert, err = tlsutil.NewCertificate(locations[locCertFile], locations[locKeyFile], tlsDefaultCommonName) if err != nil { l.Fatalln(err) } @@ -680,30 +678,6 @@ func syncthingMain(runtimeOptions RuntimeOptions) { }() } - // The TLS configuration is used for both the listening socket and outgoing - // connections. - - tlsCfg := &tls.Config{ - Certificates: []tls.Certificate{cert}, - NextProtos: []string{bepProtocolName}, - ClientAuth: tls.RequestClientCert, - SessionTicketsDisabled: true, - InsecureSkipVerify: true, - MinVersion: tls.VersionTLS12, - CipherSuites: []uint16{ - tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, - tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, - tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, - tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, - tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, - tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, - tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, - tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, - tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, - tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, - }, - } - perf := cpuBench(3, 150*time.Millisecond, true) l.Infof("Hashing performance is %.02f MB/s", perf) @@ -794,6 +768,16 @@ func syncthingMain(runtimeOptions RuntimeOptions) { cachedDiscovery := discover.NewCachingMux() mainService.Add(cachedDiscovery) + // The TLS configuration is used for both the listening socket and outgoing + // connections. + + tlsCfg := tlsutil.SecureDefault() + tlsCfg.Certificates = []tls.Certificate{cert} + tlsCfg.NextProtos = []string{bepProtocolName} + tlsCfg.ClientAuth = tls.RequestClientCert + tlsCfg.SessionTicketsDisabled = true + tlsCfg.InsecureSkipVerify = true + // Start connection management connectionsService := connections.NewService(cfg, myID, m, tlsCfg, cachedDiscovery, bepProtocolName, tlsDefaultCommonName) diff --git a/lib/discover/global_test.go b/lib/discover/global_test.go index 755aa8b7e..0e089a8b7 100644 --- a/lib/discover/global_test.go +++ b/lib/discover/global_test.go @@ -110,9 +110,8 @@ func TestGlobalOverHTTPS(t *testing.T) { t.Fatal(err) } - // Generate a server certificate, using fewer bits than usual to hurry the - // process along a bit. - cert, err := tlsutil.NewCertificate(dir+"/cert.pem", dir+"/key.pem", "syncthing", 1024) + // Generate a server certificate. + cert, err := tlsutil.NewCertificate(dir+"/cert.pem", dir+"/key.pem", "syncthing") if err != nil { t.Fatal(err) } @@ -176,9 +175,8 @@ func TestGlobalAnnounce(t *testing.T) { t.Fatal(err) } - // Generate a server certificate, using fewer bits than usual to hurry the - // process along a bit. - cert, err := tlsutil.NewCertificate(dir+"/cert.pem", dir+"/key.pem", "syncthing", 1024) + // Generate a server certificate. + cert, err := tlsutil.NewCertificate(dir+"/cert.pem", dir+"/key.pem", "syncthing") if err != nil { t.Fatal(err) } diff --git a/lib/tlsutil/tlsutil.go b/lib/tlsutil/tlsutil.go index 3598b8773..8b8014f8a 100644 --- a/lib/tlsutil/tlsutil.go +++ b/lib/tlsutil/tlsutil.go @@ -27,17 +27,74 @@ var ( ErrIdentificationFailed = fmt.Errorf("failed to identify socket type") ) -// NewCertificate generates and returns a new TLS certificate. If tlsRSABits -// is greater than zero we generate an RSA certificate with the specified -// number of bits. Otherwise we create a 384 bit ECDSA certificate. -func NewCertificate(certFile, keyFile, tlsDefaultCommonName string, tlsRSABits int) (tls.Certificate, error) { - var priv interface{} - var err error - if tlsRSABits > 0 { - priv, err = rsa.GenerateKey(rand.Reader, tlsRSABits) - } else { - priv, err = ecdsa.GenerateKey(elliptic.P384(), rand.Reader) +var ( + // The list of cipher suites we will use / suggest for TLS connections. + // This is built based on the component slices below, depending on what + // the hardware prefers. + cipherSuites []uint16 + + // Suites that are good and fast on hardware with AES-NI. These are + // reordered from the Go default to put the 256 bit ciphers above the + // 128 bit ones - because that looks cooler, even though there is + // probably no relevant difference in strength yet. + gcmSuites = []uint16{ + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, } + + // Suites that are good and fast on hardware *without* AES-NI. + chaChaSuites = []uint16{ + tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, + } + + // The rest of the suites, minus DES stuff. + otherSuites = []uint16{ + tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, + tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, + tls.TLS_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_RSA_WITH_AES_128_CBC_SHA256, + tls.TLS_RSA_WITH_AES_128_CBC_SHA, + tls.TLS_RSA_WITH_AES_256_CBC_SHA, + } +) + +func init() { + // Creates the list of ciper suites that SecureDefault uses. + cipherSuites = buildCipherSuites() +} + +// SecureDefault returns a tls.Config with reasonable, secure defaults set. +func SecureDefault() *tls.Config { + // paranoia + cs := make([]uint16, len(cipherSuites)) + copy(cs, cipherSuites) + + return &tls.Config{ + // TLS 1.2 is the minimum we accept + MinVersion: tls.VersionTLS12, + // We want the longer curves at the front, because that's more + // secure (so the web tells me, don't ask me to explain the + // details). + CurvePreferences: []tls.CurveID{tls.CurveP521, tls.CurveP384, tls.CurveP256}, + // The cipher suite lists built above. + CipherSuites: cs, + // We've put some thought into this choice and would like it to + // matter. + PreferServerCipherSuites: true, + } +} + +// NewCertificate generates and returns a new TLS certificate. +func NewCertificate(certFile, keyFile, commonName string) (tls.Certificate, error) { + priv, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader) if err != nil { return tls.Certificate{}, fmt.Errorf("generate key: %s", err) } @@ -48,11 +105,11 @@ func NewCertificate(certFile, keyFile, tlsDefaultCommonName string, tlsRSABits i template := x509.Certificate{ SerialNumber: new(big.Int).SetInt64(rand.Int63()), Subject: pkix.Name{ - CommonName: tlsDefaultCommonName, + CommonName: commonName, }, - NotBefore: notBefore, - NotAfter: notAfter, - + NotBefore: notBefore, + NotAfter: notAfter, + SignatureAlgorithm: x509.ECDSAWithSHA256, KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature, ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}, BasicConstraintsValid: true, @@ -185,3 +242,79 @@ func pemBlockForKey(priv interface{}) (*pem.Block, error) { return nil, fmt.Errorf("unknown key type") } } + +// buildCipherSuites returns a list of cipher suites with either AES-GCM or +// ChaCha20 at the top. This takes advantage of the CPU detection that the +// TLS package does to create an optimal cipher suite list for the current +// hardware. +func buildCipherSuites() []uint16 { + pref := preferredCipherSuite() + for _, suite := range gcmSuites { + if suite == pref { + // Go preferred an AES-GCM suite. Use those first. + return append(gcmSuites, append(chaChaSuites, otherSuites...)...) + } + } + // Use ChaCha20 at the top, then AES-GCM etc. + return append(chaChaSuites, append(gcmSuites, otherSuites...)...) +} + +// preferredCipherSuite returns the cipher suite that is selected for a TLS +// connection made with the Go defaults to ourselves. This is (currently, +// probably) either a ChaCha20 suite or an AES-GCM suite, depending on what +// the CPU detection has decided is fastest on this hardware. +// +// The function will return zero if something odd happens, and there's no +// guarantee what cipher suite would be chosen anyway, so the return value +// should be taken with a grain of salt. +func preferredCipherSuite() uint16 { + // This is one of our certs from NewCertificate above, to avoid having + // to generate one at init time just for this function. + crtBs := []byte(`-----BEGIN CERTIFICATE----- +MIIBXDCCAQOgAwIBAgIIQUODl2/bE4owCgYIKoZIzj0EAwIwFDESMBAGA1UEAxMJ +c3luY3RoaW5nMB4XDTE4MTAxNDA2MjU0M1oXDTQ5MTIzMTIzNTk1OVowFDESMBAG +A1UEAxMJc3luY3RoaW5nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEMqP+1lL4 +0s/xtI3ygExzYc/GvLHr0qetpBrUVHaDwS/cR1yXDsYaJpJcUNtrf1XK49IlpWW1 +Ds8seQsSg7/9BaM/MD0wDgYDVR0PAQH/BAQDAgWgMB0GA1UdJQQWMBQGCCsGAQUF +BwMBBggrBgEFBQcDAjAMBgNVHRMBAf8EAjAAMAoGCCqGSM49BAMCA0cAMEQCIFxY +MDBA92FKqZYSZjmfdIbT1OI6S9CnAFvL/pJZJwNuAiAV7osre2NiCHtXABOvsGrH +vKWqDvXcHr6Tlo+LmTAdyg== +-----END CERTIFICATE----- + `) + keyBs := []byte(`-----BEGIN EC PRIVATE KEY----- +MHcCAQEEIHtPxVHlj6Bhi9RgSR2/lAtIQ7APM9wmpaJAcds6TD2CoAoGCCqGSM49 +AwEHoUQDQgAEMqP+1lL40s/xtI3ygExzYc/GvLHr0qetpBrUVHaDwS/cR1yXDsYa +JpJcUNtrf1XK49IlpWW1Ds8seQsSg7/9BQ== +-----END EC PRIVATE KEY----- + `) + + cert, err := tls.X509KeyPair(crtBs, keyBs) + if err != nil { + return 0 + } + + serverCfg := &tls.Config{ + MinVersion: tls.VersionTLS12, + PreferServerCipherSuites: true, + Certificates: []tls.Certificate{cert}, + } + + clientCfg := &tls.Config{ + MinVersion: tls.VersionTLS12, + InsecureSkipVerify: true, + } + + c0, c1 := net.Pipe() + + c := tls.Client(c0, clientCfg) + go func() { + c.Handshake() + }() + + s := tls.Server(c1, serverCfg) + if err := s.Handshake(); err != nil { + return 0 + } + + return c.ConnectionState().CipherSuite +} diff --git a/lib/tlsutil/tlsutil_test.go b/lib/tlsutil/tlsutil_test.go index e5630344f..af8bd5e00 100644 --- a/lib/tlsutil/tlsutil_test.go +++ b/lib/tlsutil/tlsutil_test.go @@ -11,6 +11,7 @@ package tlsutil import ( "bytes" + "crypto/tls" "io" "net" "testing" @@ -74,6 +75,53 @@ func TestUnionedConnection(t *testing.T) { } } +func TestCheckCipherSuites(t *testing.T) { + // This is the set of cipher suites we expect - only the order should + // differ. + allSuites := []uint16{ + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, + tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, + tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, + tls.TLS_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_RSA_WITH_AES_128_CBC_SHA256, + tls.TLS_RSA_WITH_AES_128_CBC_SHA, + tls.TLS_RSA_WITH_AES_256_CBC_SHA, + } + + suites := buildCipherSuites() + + if len(suites) != len(allSuites) { + t.Fatal("should get a list representing all suites") + } + + // Check that the returned list of suites doesn't contain anything + // unexpecteds and is free from duplicates. + seen := make(map[uint16]struct{}) +nextSuite: + for _, s0 := range suites { + if _, ok := seen[s0]; ok { + t.Fatal("duplicate suite", s0) + } + for _, s1 := range allSuites { + if s0 == s1 { + seen[s0] = struct{}{} + continue nextSuite + } + } + t.Fatal("got unknown suite", s0) + } +} + type fakeAccepter struct { data []byte }