crypto/tls: disable RSA-PSS in TLS 1.2

Most of the issues that led to the decision on golang#30055 were related to incompatibility with or faulty support for RSA-PSS (golang#29831, golang#29779, v1.5 signatures). RSA-PSS is required by TLS 1.3, but is also available to be negotiated in TLS 1.2. Altering TLS 1.2 behavior based on GODEBUG=tls13=1 feels surprising, so just disable RSA-PSS entirely in TLS 1.2 until TLS 1.3 is on by default, so breakage happens all at once. Updates golang#30055 Change-Id: Iee90454a20ded8895e5302e8bcbcd32e4e3031c2 Reviewed-on: https://go-review.googlesource.com/c/160998 Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Adam Langley <agl@golang.org>
nebulabox · Feb 20, 2019 · 19c58d6 · 19c58d6
1 parent 5a03ad1
commit 19c58d6
Show file tree

Hide file tree

Showing 16 changed files with 1,047 additions and 51 deletions.
diff --git a/doc/go1.12.html b/doc/go1.12.html
@@ -406,7 +406,8 @@ <h3 id="tls_1_3">TLS 1.3</h3>
   and renegotiation are available in TLS 1.3 and provide equivalent or
   better security and performance. Note that even though TLS 1.3 is backwards
   compatible with previous versions, certain legacy systems might not work
-  correctly when attempting to negotiate it.
+  correctly when attempting to negotiate it. RSA certificate keys too small
+  to be secure (including 512-bit keys) will not work with TLS 1.3.
 </p>
 
 <p>
@@ -500,13 +501,6 @@ <h3 id="minor_library_changes">Minor changes to the library</h3>
 
 <dl id="crypto/tls"><dt><a href="/pkg/crypto/tls/">crypto/tls</a></dt>
   <dd>
-    <p><!-- CL 146258 -->
-      TLS 1.2 clients and servers will now advertise and accept RSA-PSS
-      signature algorithms for use with regular RSA public keys. Certain
-      insecure certificate keys (including 512-bit RSA keys) will
-      now cause a handshake failure if RSA-PSS is selected.
-    </p>
-
     <p><!-- CL 143177 -->
       If a client sends an initial message that does not look like TLS, the server
       will no longer reply with an alert, and it will expose the underlying

diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
@@ -161,7 +161,7 @@ const (
 )
 
 // supportedSignatureAlgorithms contains the signature and hash algorithms that
-// the code advertises as supported in a TLS 1.2 ClientHello and in a TLS 1.2
+// the code advertises as supported in a TLS 1.2+ ClientHello and in a TLS 1.2+
 // CertificateRequest. The two fields are merged to match with TLS 1.3.
 // Note that in TLS 1.2, the ECDSA algorithms are not constrained to P-256, etc.
 var supportedSignatureAlgorithms = []SignatureScheme{
@@ -178,6 +178,9 @@ var supportedSignatureAlgorithms = []SignatureScheme{
 	ECDSAWithSHA1,
 }
 
+// RSA-PSS is disabled in TLS 1.2 for Go 1.12. See Issue 30055.
+var supportedSignatureAlgorithmsTLS12 = supportedSignatureAlgorithms[3:]
+
 // helloRetryRequestRandom is set as the Random value of a ServerHello
 // to signal that the message is actually a HelloRetryRequest.
 var helloRetryRequestRandom = []byte{ // See RFC 8446, Section 4.1.3.

diff --git a/src/crypto/tls/conn_test.go b/src/crypto/tls/conn_test.go
@@ -142,6 +142,7 @@ func runDynamicRecordSizingTest(t *testing.T, config *Config) {
 
 	handshakeDone := make(chan struct{})
 	recordSizesChan := make(chan []int, 1)
+	defer func() { <-recordSizesChan }() // wait for the goroutine to exit
 	go func() {
 		// This goroutine performs a TLS handshake over clientConn and
 		// then reads TLS records until EOF. It writes a slice that

diff --git a/src/crypto/tls/handshake_client_test.go b/src/crypto/tls/handshake_client_test.go
@@ -855,6 +855,30 @@ func TestHandshakeClientCertRSAPKCS1v15(t *testing.T) {
 	runClientTestTLS12(t, test)
 }
 
+func TestHandshakeClientCertPSSDisabled(t *testing.T) {
+	config := testConfig.Clone()
+	cert, _ := X509KeyPair([]byte(clientCertificatePEM), []byte(clientKeyPEM))
+	config.Certificates = []Certificate{cert}
+
+	test := &clientTest{
+		name:   "ClientCert-RSA-PSS-Disabled",
+		args:   []string{"-cipher", "AES128", "-Verify", "1"},
+		config: config,
+	}
+
+	// Restore the default signature algorithms, disabling RSA-PSS in TLS 1.2,
+	// and check that handshakes still work.
+	testSupportedSignatureAlgorithmsTLS12 := supportedSignatureAlgorithmsTLS12
+	defer func() { supportedSignatureAlgorithmsTLS12 = testSupportedSignatureAlgorithmsTLS12 }()
+	supportedSignatureAlgorithmsTLS12 = savedSupportedSignatureAlgorithmsTLS12
+
+	// Use t.Run to ensure the defer runs after all parallel tests end.
+	t.Run("", func(t *testing.T) {
+		runClientTestTLS12(t, test)
+		runClientTestTLS13(t, test)
+	})
+}
+
 func TestClientKeyUpdate(t *testing.T) {
 	test := &clientTest{
 		name:          "KeyUpdate",

diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go
@@ -463,7 +463,7 @@ func (hs *serverHandshakeState) doFullHandshake() error {
 		}
 		if c.vers >= VersionTLS12 {
 			certReq.hasSignatureAlgorithm = true
-			certReq.supportedSignatureAlgorithms = supportedSignatureAlgorithms
+			certReq.supportedSignatureAlgorithms = supportedSignatureAlgorithmsTLS12
 		}
 
 		// An empty list of certificateAuthorities signals to
@@ -559,7 +559,7 @@ func (hs *serverHandshakeState) doFullHandshake() error {
 		}
 
 		// Determine the signature type.
-		_, sigType, hashFunc, err := pickSignatureAlgorithm(pub, []SignatureScheme{certVerify.signatureAlgorithm}, supportedSignatureAlgorithms, c.vers)
+		_, sigType, hashFunc, err := pickSignatureAlgorithm(pub, []SignatureScheme{certVerify.signatureAlgorithm}, supportedSignatureAlgorithmsTLS12, c.vers)
 		if err != nil {
 			c.sendAlert(alertIllegalParameter)
 			return err

diff --git a/src/crypto/tls/handshake_server_test.go b/src/crypto/tls/handshake_server_test.go
@@ -1211,6 +1211,33 @@ func TestHandshakeServerRSAPSS(t *testing.T) {
 	runServerTestTLS13(t, test)
 }
 
+func TestHandshakeServerPSSDisabled(t *testing.T) {
+	test := &serverTest{
+		name:    "RSA-PSS-Disabled",
+		command: []string{"openssl", "s_client", "-no_ticket"},
+		wait:    true,
+	}
+
+	// Restore the default signature algorithms, disabling RSA-PSS in TLS 1.2,
+	// and check that handshakes still work.
+	testSupportedSignatureAlgorithmsTLS12 := supportedSignatureAlgorithmsTLS12
+	defer func() { supportedSignatureAlgorithmsTLS12 = testSupportedSignatureAlgorithmsTLS12 }()
+	supportedSignatureAlgorithmsTLS12 = savedSupportedSignatureAlgorithmsTLS12
+
+	runServerTestTLS12(t, test)
+	runServerTestTLS13(t, test)
+
+	test = &serverTest{
+		name:    "RSA-PSS-Disabled-Required",
+		command: []string{"openssl", "s_client", "-no_ticket", "-sigalgs", "rsa_pss_rsae_sha256"},
+		wait:    true,
+
+		expectHandshakeErrorIncluding: "peer doesn't support any common signature algorithms",
+	}
+
+	runServerTestTLS12(t, test)
+}
+
 func benchmarkHandshakeServer(b *testing.B, version uint16, cipherSuite uint16, curve CurveID, cert []byte, key crypto.PrivateKey) {
 	config := testConfig.Clone()
 	config.CipherSuites = []uint16{cipherSuite}
@@ -1390,49 +1417,82 @@ func TestClientAuth(t *testing.T) {
 		defer os.Remove(ecdsaCertPath)
 		ecdsaKeyPath = tempFile(clientECDSAKeyPEM)
 		defer os.Remove(ecdsaKeyPath)
-	} else {
-		t.Parallel()
 	}
 
-	config := testConfig.Clone()
-	config.ClientAuth = RequestClientCert
+	t.Run("Normal", func(t *testing.T) {
+		config := testConfig.Clone()
+		config.ClientAuth = RequestClientCert
 
-	test := &serverTest{
-		name:    "ClientAuthRequestedNotGiven",
-		command: []string{"openssl", "s_client", "-no_ticket", "-cipher", "AES128-SHA"},
-		config:  config,
-	}
-	runServerTestTLS12(t, test)
-	runServerTestTLS13(t, test)
+		test := &serverTest{
+			name:    "ClientAuthRequestedNotGiven",
+			command: []string{"openssl", "s_client", "-no_ticket", "-cipher", "AES128-SHA"},
+			config:  config,
+		}
+		runServerTestTLS12(t, test)
+		runServerTestTLS13(t, test)
 
-	test = &serverTest{
-		name: "ClientAuthRequestedAndGiven",
-		command: []string{"openssl", "s_client", "-no_ticket", "-cipher", "AES128-SHA",
-			"-cert", certPath, "-key", keyPath, "-sigalgs", "rsa_pss_rsae_sha256"},
-		config:            config,
-		expectedPeerCerts: []string{clientCertificatePEM},
-	}
-	runServerTestTLS12(t, test)
-	runServerTestTLS13(t, test)
+		config.ClientAuth = RequireAnyClientCert
 
-	test = &serverTest{
-		name: "ClientAuthRequestedAndECDSAGiven",
-		command: []string{"openssl", "s_client", "-no_ticket", "-cipher", "AES128-SHA",
-			"-cert", ecdsaCertPath, "-key", ecdsaKeyPath},
-		config:            config,
-		expectedPeerCerts: []string{clientECDSACertificatePEM},
-	}
-	runServerTestTLS12(t, test)
-	runServerTestTLS13(t, test)
+		test = &serverTest{
+			name: "ClientAuthRequestedAndGiven",
+			command: []string{"openssl", "s_client", "-no_ticket", "-cipher", "AES128-SHA",
+				"-cert", certPath, "-key", keyPath, "-sigalgs", "rsa_pss_rsae_sha256"},
+			config:            config,
+			expectedPeerCerts: []string{clientCertificatePEM},
+		}
+		runServerTestTLS12(t, test)
+		runServerTestTLS13(t, test)
+
+		test = &serverTest{
+			name: "ClientAuthRequestedAndECDSAGiven",
+			command: []string{"openssl", "s_client", "-no_ticket", "-cipher", "AES128-SHA",
+				"-cert", ecdsaCertPath, "-key", ecdsaKeyPath},
+			config:            config,
+			expectedPeerCerts: []string{clientECDSACertificatePEM},
+		}
+		runServerTestTLS12(t, test)
+		runServerTestTLS13(t, test)
+
+		test = &serverTest{
+			name: "ClientAuthRequestedAndPKCS1v15Given",
+			command: []string{"openssl", "s_client", "-no_ticket", "-cipher", "AES128-SHA",
+				"-cert", certPath, "-key", keyPath, "-sigalgs", "rsa_pkcs1_sha256"},
+			config:            config,
+			expectedPeerCerts: []string{clientCertificatePEM},
+		}
+		runServerTestTLS12(t, test)
+	})
 
-	test = &serverTest{
-		name: "ClientAuthRequestedAndPKCS1v15Given",
-		command: []string{"openssl", "s_client", "-no_ticket", "-cipher", "AES128-SHA",
-			"-cert", certPath, "-key", keyPath, "-sigalgs", "rsa_pkcs1_sha256"},
-		config:            config,
-		expectedPeerCerts: []string{clientCertificatePEM},
-	}
-	runServerTestTLS12(t, test)
+	// Restore the default signature algorithms, disabling RSA-PSS in TLS 1.2,
+	// and check that handshakes still work.
+	testSupportedSignatureAlgorithmsTLS12 := supportedSignatureAlgorithmsTLS12
+	defer func() { supportedSignatureAlgorithmsTLS12 = testSupportedSignatureAlgorithmsTLS12 }()
+	supportedSignatureAlgorithmsTLS12 = savedSupportedSignatureAlgorithmsTLS12
+
+	t.Run("PSSDisabled", func(t *testing.T) {
+		config := testConfig.Clone()
+		config.ClientAuth = RequireAnyClientCert
+
+		test := &serverTest{
+			name: "ClientAuthRequestedAndGiven-PSS-Disabled",
+			command: []string{"openssl", "s_client", "-no_ticket", "-cipher", "AES128-SHA",
+				"-cert", certPath, "-key", keyPath},
+			config:            config,
+			expectedPeerCerts: []string{clientCertificatePEM},
+		}
+		runServerTestTLS12(t, test)
+		runServerTestTLS13(t, test)
+
+		test = &serverTest{
+			name: "ClientAuthRequestedAndGiven-PSS-Disabled-Required",
+			command: []string{"openssl", "s_client", "-no_ticket", "-cipher", "AES128-SHA",
+				"-cert", certPath, "-key", keyPath, "-client_sigalgs", "rsa_pss_rsae_sha256"},
+			config: config,
+
+			expectHandshakeErrorIncluding: "client didn't provide a certificate",
+		}
+		runServerTestTLS12(t, test)
+	})
 }
 
 func TestSNIGivenOnFailure(t *testing.T) {
@@ -1722,6 +1782,7 @@ T+E0J8wlH24pgwQHzy7Ko2qLwn1b5PW8ecrlvP1g
 	if err != nil {
 		t.Fatal(err)
 	}
+
 	done := make(chan struct{})
 	go func() {
 		config := testConfig.Clone()
@@ -1739,4 +1800,15 @@ T+E0J8wlH24pgwQHzy7Ko2qLwn1b5PW8ecrlvP1g
 		t.Errorf(`expected "handshake failure", got %q`, err)
 	}
 	<-done
+
+	// With RSA-PSS disabled and TLS 1.2, this should work.
+
+	testSupportedSignatureAlgorithmsTLS12 := supportedSignatureAlgorithmsTLS12
+	defer func() { supportedSignatureAlgorithmsTLS12 = testSupportedSignatureAlgorithmsTLS12 }()
+	supportedSignatureAlgorithmsTLS12 = savedSupportedSignatureAlgorithmsTLS12
+
+	serverConfig := testConfig.Clone()
+	serverConfig.Certificates = []Certificate{cert}
+	serverConfig.MaxVersion = VersionTLS12
+	testHandshake(t, testConfig, serverConfig)
 }
diff --git a/src/crypto/tls/key_agreement.go b/src/crypto/tls/key_agreement.go
@@ -177,7 +177,7 @@ NextCandidate:
 		return nil, errors.New("tls: certificate private key does not implement crypto.Signer")
 	}
 
-	signatureAlgorithm, sigType, hashFunc, err := pickSignatureAlgorithm(priv.Public(), clientHello.supportedSignatureAlgorithms, supportedSignatureAlgorithms, ka.version)
+	signatureAlgorithm, sigType, hashFunc, err := pickSignatureAlgorithm(priv.Public(), clientHello.supportedSignatureAlgorithms, supportedSignatureAlgorithmsTLS12, ka.version)
 	if err != nil {
 		return nil, err
 	}