From e10e39db4d2901d236d5e16f8daf4dca966f2d20 Mon Sep 17 00:00:00 2001 From: SparrowLii Date: Tue, 28 Jul 2020 11:27:46 +0800 Subject: [PATCH 1/4] crypto/tls: make checkForResumption side-effect free Fixes #39406 When use checkForResumption() function it could be side-effect sometime. 1. hs.sessionState will be changed and retained although the function return false. 2. hs.suite will be changed and retained when the statements below return false. So we should use a local variable, cilentSessionState, to replace hs.sessionState in the function. And move the set-suite statements down to avoid being changed too early. --- src/crypto/tls/handshake_server.go | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go index 16d3e643f0b28e..a86dc6d4a55a5e 100644 --- a/src/crypto/tls/handshake_server.go +++ b/src/crypto/tls/handshake_server.go @@ -357,26 +357,26 @@ func (hs *serverHandshakeState) checkForResumption() bool { if plaintext == nil { return false } - hs.sessionState = &sessionState{usedOldKey: usedOldKey} - ok := hs.sessionState.unmarshal(plaintext) + clientSessionState := &sessionState{usedOldKey: usedOldKey} + ok := clientSessionState.unmarshal(plaintext) if !ok { return false } - createdAt := time.Unix(int64(hs.sessionState.createdAt), 0) + createdAt := time.Unix(int64(clientSessionState.createdAt), 0) if c.config.time().Sub(createdAt) > maxSessionTicketLifetime { return false } // Never resume a session for a different TLS version. - if c.vers != hs.sessionState.vers { + if c.vers != clientSessionState.vers { return false } cipherSuiteOk := false // Check that the client is still offering the ciphersuite in the session. for _, id := range hs.clientHello.cipherSuites { - if id == hs.sessionState.cipherSuite { + if id == clientSessionState.cipherSuite { cipherSuiteOk = true break } @@ -385,14 +385,7 @@ func (hs *serverHandshakeState) checkForResumption() bool { return false } - // Check that we also support the ciphersuite from the session. - hs.suite = selectCipherSuite([]uint16{hs.sessionState.cipherSuite}, - c.config.cipherSuites(), hs.cipherSuiteOk) - if hs.suite == nil { - return false - } - - sessionHasClientCerts := len(hs.sessionState.certificates) != 0 + sessionHasClientCerts := len(clientSessionState.certificates) != 0 needClientCerts := requiresClientCert(c.config.ClientAuth) if needClientCerts && !sessionHasClientCerts { return false @@ -401,6 +394,15 @@ func (hs *serverHandshakeState) checkForResumption() bool { return false } + // Check that we also support the ciphersuite from the session. + hs.suite = selectCipherSuite([]uint16{clientSessionState.cipherSuite}, + c.config.cipherSuites(), hs.cipherSuiteOk) + if hs.suite == nil { + return false + } + + hs.sessionState = clientSessionState + return true } From b99029685d9e99c8336f175a79f6c3b8e48f8b0d Mon Sep 17 00:00:00 2001 From: SparrowLii Date: Wed, 29 Jul 2020 14:44:32 +0800 Subject: [PATCH 2/4] just a tortolseGit test --- .idea/.gitignore | 8 ++++++++ .idea/go.iml | 8 ++++++++ .idea/misc.xml | 6 ++++++ .idea/modules.xml | 8 ++++++++ .idea/vcs.xml | 6 ++++++ 5 files changed, 36 insertions(+) create mode 100644 .idea/.gitignore create mode 100644 .idea/go.iml create mode 100644 .idea/misc.xml create mode 100644 .idea/modules.xml create mode 100644 .idea/vcs.xml diff --git a/.idea/.gitignore b/.idea/.gitignore new file mode 100644 index 00000000000000..4aa91ea5ab95b0 --- /dev/null +++ b/.idea/.gitignore @@ -0,0 +1,8 @@ +# Default ignored files +/shelf/ +/workspace.xml +# Datasource local storage ignored files +/dataSources/ +/dataSources.local.xml +# Editor-based HTTP Client requests +/httpRequests/ diff --git a/.idea/go.iml b/.idea/go.iml new file mode 100644 index 00000000000000..bf4c9d396a05d7 --- /dev/null +++ b/.idea/go.iml @@ -0,0 +1,8 @@ + + + + + + + + \ No newline at end of file diff --git a/.idea/misc.xml b/.idea/misc.xml new file mode 100644 index 00000000000000..ef004d16cf58ee --- /dev/null +++ b/.idea/misc.xml @@ -0,0 +1,6 @@ + + + + + \ No newline at end of file diff --git a/.idea/modules.xml b/.idea/modules.xml new file mode 100644 index 00000000000000..f3071541f3cff7 --- /dev/null +++ b/.idea/modules.xml @@ -0,0 +1,8 @@ + + + + + + + + \ No newline at end of file diff --git a/.idea/vcs.xml b/.idea/vcs.xml new file mode 100644 index 00000000000000..9661ac713428ef --- /dev/null +++ b/.idea/vcs.xml @@ -0,0 +1,6 @@ + + + + + + \ No newline at end of file From 50e68f6dc800755400f2e9cc036d0895dab41134 Mon Sep 17 00:00:00 2001 From: SparrowLii Date: Wed, 29 Jul 2020 14:49:59 +0800 Subject: [PATCH 3/4] delete the useless files --- .idea/.gitignore | 8 -------- .idea/go.iml | 8 -------- .idea/misc.xml | 6 ------ .idea/modules.xml | 8 -------- .idea/vcs.xml | 6 ------ 5 files changed, 36 deletions(-) delete mode 100644 .idea/.gitignore delete mode 100644 .idea/go.iml delete mode 100644 .idea/misc.xml delete mode 100644 .idea/modules.xml delete mode 100644 .idea/vcs.xml diff --git a/.idea/.gitignore b/.idea/.gitignore deleted file mode 100644 index 4aa91ea5ab95b0..00000000000000 --- a/.idea/.gitignore +++ /dev/null @@ -1,8 +0,0 @@ -# Default ignored files -/shelf/ -/workspace.xml -# Datasource local storage ignored files -/dataSources/ -/dataSources.local.xml -# Editor-based HTTP Client requests -/httpRequests/ diff --git a/.idea/go.iml b/.idea/go.iml deleted file mode 100644 index bf4c9d396a05d7..00000000000000 --- a/.idea/go.iml +++ /dev/null @@ -1,8 +0,0 @@ - - - - - - - - \ No newline at end of file diff --git a/.idea/misc.xml b/.idea/misc.xml deleted file mode 100644 index ef004d16cf58ee..00000000000000 --- a/.idea/misc.xml +++ /dev/null @@ -1,6 +0,0 @@ - - - - - \ No newline at end of file diff --git a/.idea/modules.xml b/.idea/modules.xml deleted file mode 100644 index f3071541f3cff7..00000000000000 --- a/.idea/modules.xml +++ /dev/null @@ -1,8 +0,0 @@ - - - - - - - - \ No newline at end of file diff --git a/.idea/vcs.xml b/.idea/vcs.xml deleted file mode 100644 index 9661ac713428ef..00000000000000 --- a/.idea/vcs.xml +++ /dev/null @@ -1,6 +0,0 @@ - - - - - - \ No newline at end of file From 497da02dab59cbccf85d5d69bd5e02276d7e2fa3 Mon Sep 17 00:00:00 2001 From: SparrowLii Date: Fri, 31 Jul 2020 11:17:19 +0800 Subject: [PATCH 4/4] crypto/tls: simplify the process of cipher suites checking. Updates #39406 The process of checking server's cipher suites now is based on the logical relationship following: If the cipherSuite supports ECDHE,judge hs.ecdheOK ,then judge ecSignOK and rsaSignOk to check signer; if not supports, judge rsaOK. Then check if the suite needs tls1.2 version. This relationship is complicated, Fragile and hard to modify.Besides, it need 4 bool parameters in hs and a lot of "if...else" statements in cipherSuiteOk() function. So we can use one parameter, cipherSuite, to replace those 4 in hs.Then we just need two bit operations in cipherSuiteOK() instead of many "if...else"s.What we need to do is completing the const and cipherSuites list in cipher_suites.go(They are partially omitted based on logical relationship, which is not plain at all.) --- src/crypto/tls/cipher_suites.go | 39 ++++++++++------ src/crypto/tls/handshake_server.go | 73 ++++++++++-------------------- 2 files changed, 48 insertions(+), 64 deletions(-) diff --git a/src/crypto/tls/cipher_suites.go b/src/crypto/tls/cipher_suites.go index ea16ef95bf05c7..32fc5a01bfcd2f 100644 --- a/src/crypto/tls/cipher_suites.go +++ b/src/crypto/tls/cipher_suites.go @@ -138,6 +138,15 @@ const ( // certificate is ECDSA or EdDSA. If this is not set then the cipher suite // is RSA based. suiteECSign + // suiteRSASign indicates that the cipher suite involves an RSA + // signature and therefore may only be selected when the server's + // certificate is RSA. If this is not set then the cipher suite + // is ECDSA or EdDSA based. + suiteRSASign + // suiteRSASDecrypt indicates that the cipher suite involves RSA + //Decryption. This means that it should only be selected when the + // client indicates that it support RSA. + suiteRSASDecrypt // suiteTLS12 indicates that the cipher suite should only be advertised // and accepted when using TLS 1.2. suiteTLS12 @@ -167,29 +176,29 @@ type cipherSuite struct { var cipherSuites = []*cipherSuite{ // Ciphersuite order is chosen so that ECDHE comes before plain RSA and // AEADs are the top preference. - {TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, 32, 0, 12, ecdheRSAKA, suiteECDHE | suiteTLS12, nil, nil, aeadChaCha20Poly1305}, + {TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, 32, 0, 12, ecdheRSAKA, suiteECDHE |suiteRSASign| suiteTLS12, nil, nil, aeadChaCha20Poly1305}, {TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, 32, 0, 12, ecdheECDSAKA, suiteECDHE | suiteECSign | suiteTLS12, nil, nil, aeadChaCha20Poly1305}, - {TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 16, 0, 4, ecdheRSAKA, suiteECDHE | suiteTLS12, nil, nil, aeadAESGCM}, + {TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 16, 0, 4, ecdheRSAKA, suiteECDHE |suiteRSASign | suiteTLS12, nil, nil, aeadAESGCM}, {TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, 16, 0, 4, ecdheECDSAKA, suiteECDHE | suiteECSign | suiteTLS12, nil, nil, aeadAESGCM}, - {TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, 32, 0, 4, ecdheRSAKA, suiteECDHE | suiteTLS12 | suiteSHA384, nil, nil, aeadAESGCM}, + {TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, 32, 0, 4, ecdheRSAKA, suiteECDHE |suiteRSASign| suiteTLS12 | suiteSHA384, nil, nil, aeadAESGCM}, {TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, 32, 0, 4, ecdheECDSAKA, suiteECDHE | suiteECSign | suiteTLS12 | suiteSHA384, nil, nil, aeadAESGCM}, - {TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, 16, 32, 16, ecdheRSAKA, suiteECDHE | suiteTLS12 | suiteDefaultOff, cipherAES, macSHA256, nil}, - {TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, 16, 20, 16, ecdheRSAKA, suiteECDHE, cipherAES, macSHA1, nil}, + {TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, 16, 32, 16, ecdheRSAKA, suiteECDHE |suiteRSASign| suiteTLS12 | suiteDefaultOff, cipherAES, macSHA256, nil}, + {TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, 16, 20, 16, ecdheRSAKA, suiteECDHE|suiteRSASign, cipherAES, macSHA1, nil}, {TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, 16, 32, 16, ecdheECDSAKA, suiteECDHE | suiteECSign | suiteTLS12 | suiteDefaultOff, cipherAES, macSHA256, nil}, {TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, 16, 20, 16, ecdheECDSAKA, suiteECDHE | suiteECSign, cipherAES, macSHA1, nil}, - {TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, 32, 20, 16, ecdheRSAKA, suiteECDHE, cipherAES, macSHA1, nil}, + {TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, 32, 20, 16, ecdheRSAKA, suiteECDHE|suiteRSASign, cipherAES, macSHA1, nil}, {TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, 32, 20, 16, ecdheECDSAKA, suiteECDHE | suiteECSign, cipherAES, macSHA1, nil}, - {TLS_RSA_WITH_AES_128_GCM_SHA256, 16, 0, 4, rsaKA, suiteTLS12, nil, nil, aeadAESGCM}, - {TLS_RSA_WITH_AES_256_GCM_SHA384, 32, 0, 4, rsaKA, suiteTLS12 | suiteSHA384, nil, nil, aeadAESGCM}, - {TLS_RSA_WITH_AES_128_CBC_SHA256, 16, 32, 16, rsaKA, suiteTLS12 | suiteDefaultOff, cipherAES, macSHA256, nil}, - {TLS_RSA_WITH_AES_128_CBC_SHA, 16, 20, 16, rsaKA, 0, cipherAES, macSHA1, nil}, - {TLS_RSA_WITH_AES_256_CBC_SHA, 32, 20, 16, rsaKA, 0, cipherAES, macSHA1, nil}, - {TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA, 24, 20, 8, ecdheRSAKA, suiteECDHE, cipher3DES, macSHA1, nil}, - {TLS_RSA_WITH_3DES_EDE_CBC_SHA, 24, 20, 8, rsaKA, 0, cipher3DES, macSHA1, nil}, + {TLS_RSA_WITH_AES_128_GCM_SHA256, 16, 0, 4, rsaKA, suiteRSASDecrypt|suiteTLS12, nil, nil, aeadAESGCM}, + {TLS_RSA_WITH_AES_256_GCM_SHA384, 32, 0, 4, rsaKA, suiteRSASDecrypt|suiteTLS12 | suiteSHA384, nil, nil, aeadAESGCM}, + {TLS_RSA_WITH_AES_128_CBC_SHA256, 16, 32, 16, rsaKA, suiteRSASDecrypt|suiteTLS12 | suiteDefaultOff, cipherAES, macSHA256, nil}, + {TLS_RSA_WITH_AES_128_CBC_SHA, 16, 20, 16, rsaKA, suiteRSASDecrypt, cipherAES, macSHA1, nil}, + {TLS_RSA_WITH_AES_256_CBC_SHA, 32, 20, 16, rsaKA, suiteRSASDecrypt, cipherAES, macSHA1, nil}, + {TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA, 24, 20, 8, ecdheRSAKA, suiteECDHE|suiteRSASign, cipher3DES, macSHA1, nil}, + {TLS_RSA_WITH_3DES_EDE_CBC_SHA, 24, 20, 8, rsaKA, suiteRSASDecrypt, cipher3DES, macSHA1, nil}, // RC4-based cipher suites are disabled by default. - {TLS_RSA_WITH_RC4_128_SHA, 16, 20, 0, rsaKA, suiteDefaultOff, cipherRC4, macSHA1, nil}, - {TLS_ECDHE_RSA_WITH_RC4_128_SHA, 16, 20, 0, ecdheRSAKA, suiteECDHE | suiteDefaultOff, cipherRC4, macSHA1, nil}, + {TLS_RSA_WITH_RC4_128_SHA, 16, 20, 0, rsaKA, suiteRSASDecrypt|suiteDefaultOff, cipherRC4, macSHA1, nil}, + {TLS_ECDHE_RSA_WITH_RC4_128_SHA, 16, 20, 0, ecdheRSAKA, suiteECDHE |suiteRSASign| suiteDefaultOff, cipherRC4, macSHA1, nil}, {TLS_ECDHE_ECDSA_WITH_RC4_128_SHA, 16, 20, 0, ecdheECDSAKA, suiteECDHE | suiteECSign | suiteDefaultOff, cipherRC4, macSHA1, nil}, } diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go index a86dc6d4a55a5e..626cc671cbc6dc 100644 --- a/src/crypto/tls/handshake_server.go +++ b/src/crypto/tls/handshake_server.go @@ -15,7 +15,6 @@ import ( "fmt" "io" "sync/atomic" - "time" ) // serverHandshakeState contains details of a server handshake in progress. @@ -25,14 +24,11 @@ type serverHandshakeState struct { clientHello *clientHelloMsg hello *serverHelloMsg suite *cipherSuite - ecdheOk bool - ecSignOk bool - rsaDecryptOk bool - rsaSignOk bool sessionState *sessionState finishedHash finishedHash masterSecret []byte cert *Certificate + cipherSuites int } // serverHandshake performs a TLS handshake as a server. @@ -232,9 +228,8 @@ func (hs *serverHandshakeState) processClientHello() error { hs.hello.scts = hs.cert.SignedCertificateTimestamps } - hs.ecdheOk = supportsECDHE(c.config, hs.clientHello.supportedCurves, hs.clientHello.supportedPoints) - - if hs.ecdheOk { + if supportsECDHE(c.config, hs.clientHello.supportedCurves, hs.clientHello.supportedPoints) { + hs.cipherSuites |= suiteECDHE // Although omitting the ec_point_formats extension is permitted, some // old OpenSSL version will refuse to handshake if not present. // @@ -246,11 +241,11 @@ func (hs *serverHandshakeState) processClientHello() error { if priv, ok := hs.cert.PrivateKey.(crypto.Signer); ok { switch priv.Public().(type) { case *ecdsa.PublicKey: - hs.ecSignOk = true + hs.cipherSuites |= suiteECSign case ed25519.PublicKey: - hs.ecSignOk = true + hs.cipherSuites |= suiteECSign case *rsa.PublicKey: - hs.rsaSignOk = true + hs.cipherSuites |= suiteRSASign default: c.sendAlert(alertInternalError) return fmt.Errorf("tls: unsupported signing key type (%T)", priv.Public()) @@ -259,12 +254,15 @@ func (hs *serverHandshakeState) processClientHello() error { if priv, ok := hs.cert.PrivateKey.(crypto.Decrypter); ok { switch priv.Public().(type) { case *rsa.PublicKey: - hs.rsaDecryptOk = true + hs.cipherSuites |= suiteRSASDecrypt default: c.sendAlert(alertInternalError) return fmt.Errorf("tls: unsupported decryption key type (%T)", priv.Public()) } } + if hs.c.vers >= VersionTLS12 { + hs.cipherSuites |= suiteTLS12 + } return nil } @@ -325,24 +323,8 @@ func (hs *serverHandshakeState) pickCipherSuite() error { } func (hs *serverHandshakeState) cipherSuiteOk(c *cipherSuite) bool { - if c.flags&suiteECDHE != 0 { - if !hs.ecdheOk { - return false - } - if c.flags&suiteECSign != 0 { - if !hs.ecSignOk { - return false - } - } else if !hs.rsaSignOk { - return false - } - } else if !hs.rsaDecryptOk { - return false - } - if hs.c.vers < VersionTLS12 && c.flags&suiteTLS12 != 0 { - return false - } - return true + flag := c.flags & (suiteECDHE | suiteECSign | suiteRSASign | suiteRSASDecrypt | suiteTLS12) + return hs.cipherSuites&flag == flag } // checkForResumption reports whether we should perform resumption on this connection. @@ -357,26 +339,21 @@ func (hs *serverHandshakeState) checkForResumption() bool { if plaintext == nil { return false } - clientSessionState := &sessionState{usedOldKey: usedOldKey} - ok := clientSessionState.unmarshal(plaintext) + hs.sessionState = &sessionState{usedOldKey: usedOldKey} + ok := hs.sessionState.unmarshal(plaintext) if !ok { return false } - createdAt := time.Unix(int64(clientSessionState.createdAt), 0) - if c.config.time().Sub(createdAt) > maxSessionTicketLifetime { - return false - } - // Never resume a session for a different TLS version. - if c.vers != clientSessionState.vers { + if c.vers != hs.sessionState.vers { return false } cipherSuiteOk := false // Check that the client is still offering the ciphersuite in the session. for _, id := range hs.clientHello.cipherSuites { - if id == clientSessionState.cipherSuite { + if id == hs.sessionState.cipherSuite { cipherSuiteOk = true break } @@ -385,7 +362,14 @@ func (hs *serverHandshakeState) checkForResumption() bool { return false } - sessionHasClientCerts := len(clientSessionState.certificates) != 0 + // Check that we also support the ciphersuite from the session. + hs.suite = selectCipherSuite([]uint16{hs.sessionState.cipherSuite}, + c.config.cipherSuites(), hs.cipherSuiteOk) + if hs.suite == nil { + return false + } + + sessionHasClientCerts := len(hs.sessionState.certificates) != 0 needClientCerts := requiresClientCert(c.config.ClientAuth) if needClientCerts && !sessionHasClientCerts { return false @@ -394,15 +378,6 @@ func (hs *serverHandshakeState) checkForResumption() bool { return false } - // Check that we also support the ciphersuite from the session. - hs.suite = selectCipherSuite([]uint16{clientSessionState.cipherSuite}, - c.config.cipherSuites(), hs.cipherSuiteOk) - if hs.suite == nil { - return false - } - - hs.sessionState = clientSessionState - return true }