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 0ea6ebfdbfc3058f829a78375e136f2b858b732c Mon Sep 17 00:00:00 2001 From: SparrowLii Date: Wed, 5 Aug 2020 17:11:26 +0800 Subject: [PATCH 4/4] crypto/x509:check the Key Usage extension There are two functions in crypto which relate to KU extension checking: CheckSignature(x509.go 823) and verifyHandshakeSignature(auth.go 22). Unfortunately it's difficult to check the KU inside the func body because KU is not included in the params, not to say the function doesn't know why it was called actually.So we need to check KU before use the certificate PublicKey. On the other hand, maybe we should add commits to explain the meaning of KU bits as we was starting to check them. --- src/crypto/tls/handshake_client_tls13.go | 7 +++++- src/crypto/tls/handshake_server.go | 4 ++++ src/crypto/tls/handshake_server_tls13.go | 5 ++++ src/crypto/tls/key_agreement.go | 6 +++++ src/crypto/x509/root_windows.go | 3 +++ src/crypto/x509/x509.go | 30 +++++++++++++++++++++++- 6 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/crypto/tls/handshake_client_tls13.go b/src/crypto/tls/handshake_client_tls13.go index 9c61105cf73d02..4bb7927432fe41 100644 --- a/src/crypto/tls/handshake_client_tls13.go +++ b/src/crypto/tls/handshake_client_tls13.go @@ -9,6 +9,7 @@ import ( "crypto" "crypto/hmac" "crypto/rsa" + "crypto/x509" "errors" "hash" "sync/atomic" @@ -481,7 +482,11 @@ func (hs *clientHandshakeStateTLS13) readServerCertificate() error { return errors.New("tls: certificate used with invalid signature algorithm") } signed := signedMessage(sigHash, serverSignatureContext, hs.transcript) - if err := verifyHandshakeSignature(sigType, c.peerCertificates[0].PublicKey, + cert := c.peerCertificates[0] + if cert.KeyUsage != 0 && cert.KeyUsage&x509.KeyUsageDigitalSignature == 0 { + return errors.New("tls: invalid signature: the certificate cannot sign this kind of data") + } + if err := verifyHandshakeSignature(sigType, cert.PublicKey, sigHash, signed, certVerify.signature); err != nil { c.sendAlert(alertDecryptError) return errors.New("tls: invalid signature by the server certificate: " + err.Error()) diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go index a86dc6d4a55a5e..18eef64dec61a6 100644 --- a/src/crypto/tls/handshake_server.go +++ b/src/crypto/tls/handshake_server.go @@ -623,6 +623,10 @@ func (hs *serverHandshakeState) doFullHandshake() error { } signed := hs.finishedHash.hashForClientCertificate(sigType, sigHash, hs.masterSecret) + cert:=c.peerCertificates[0] + if cert.KeyUsage != 0 && cert.KeyUsage&x509.KeyUsageDigitalSignature == 0 { + return errors.New("tls: invalid signature: the certificate cannot sign this kind of data") + } if err := verifyHandshakeSignature(sigType, pub, sigHash, signed, certVerify.signature); err != nil { c.sendAlert(alertDecryptError) return errors.New("tls: invalid signature by the client certificate: " + err.Error()) diff --git a/src/crypto/tls/handshake_server_tls13.go b/src/crypto/tls/handshake_server_tls13.go index 92d55e0293a46e..b29db12a9d71f9 100644 --- a/src/crypto/tls/handshake_server_tls13.go +++ b/src/crypto/tls/handshake_server_tls13.go @@ -9,6 +9,7 @@ import ( "crypto" "crypto/hmac" "crypto/rsa" + "crypto/x509" "errors" "hash" "io" @@ -816,6 +817,10 @@ func (hs *serverHandshakeStateTLS13) readClientCertificate() error { return errors.New("tls: client certificate used with invalid signature algorithm") } signed := signedMessage(sigHash, clientSignatureContext, hs.transcript) + cert := c.peerCertificates[0] + if cert.KeyUsage != 0 && cert.KeyUsage&x509.KeyUsageDigitalSignature == 0 { + return errors.New("tls: invalid signature: the certificate cannot sign this kind of data") + } if err := verifyHandshakeSignature(sigType, c.peerCertificates[0].PublicKey, sigHash, signed, certVerify.signature); err != nil { c.sendAlert(alertDecryptError) diff --git a/src/crypto/tls/key_agreement.go b/src/crypto/tls/key_agreement.go index 7e6534bd465e30..5b0cb50d540988 100644 --- a/src/crypto/tls/key_agreement.go +++ b/src/crypto/tls/key_agreement.go @@ -67,6 +67,9 @@ func (ka rsaKeyAgreement) generateClientKeyExchange(config *Config, clientHello return nil, nil, err } + if cert.KeyUsage != 0 && cert.KeyUsage&x509.KeyUsageKeyEncipherment == 0 { + return nil,nil,errors.New("tls: invalid signature: the certificate cannot sign this kind of secret dkey") + } encrypted, err := rsa.EncryptPKCS1v15(config.rand(), cert.PublicKey.(*rsa.PublicKey), preMasterSecret) if err != nil { return nil, nil, err @@ -319,6 +322,9 @@ func (ka *ecdheKeyAgreement) processServerKeyExchange(config *Config, clientHell sig = sig[2:] signed := hashForServerKeyExchange(sigType, sigHash, ka.version, clientHello.random, serverHello.random, serverECDHEParams) + if cert.KeyUsage != 0 && cert.KeyUsage&x509.KeyUsageDigitalSignature == 0 { + return errors.New("tls: invalid signature: the certificate cannot sign this kind of data") + } if err := verifyHandshakeSignature(sigType, cert.PublicKey, sigHash, signed, sig); err != nil { return errors.New("tls: invalid signature by the server certificate: " + err.Error()) } diff --git a/src/crypto/x509/root_windows.go b/src/crypto/x509/root_windows.go index 1e0f3acb6700e6..33caf02914865e 100644 --- a/src/crypto/x509/root_windows.go +++ b/src/crypto/x509/root_windows.go @@ -252,6 +252,9 @@ func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate if parent.PublicKeyAlgorithm != ECDSA { continue } + if parent.KeyUsage != 0 && parent.KeyUsage&KeyUsageCertSign == 0 { + return nil,errors.New("x509: invalid signature: parent certificate cannot sign this kind of certificate") + } if err := parent.CheckSignature(chain[i].SignatureAlgorithm, chain[i].RawTBSCertificate, chain[i].Signature); err != nil { return nil, err diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go index 8ce57fb1ec3fb8..919605d9e1c742 100644 --- a/src/crypto/x509/x509.go +++ b/src/crypto/x509/x509.go @@ -555,15 +555,40 @@ func oidFromNamedCurve(curve elliptic.Curve) (asn1.ObjectIdentifier, bool) { // KeyUsage represents the set of actions that are valid for a given key. It's // a bitmap of the KeyUsage* constants. type KeyUsage int - +//Bits in the KeyUsage type are used as follows(refers to RFC 5280 4.2.1.3): const ( + //The digitalSignature bit is asserted when the subject public key + //is used for verifying digital signatures, other than signatures on + //certificates (bit 5) and CRLs (bit 6) KeyUsageDigitalSignature KeyUsage = 1 << iota + //The contentCommitmentn bit is asserted when the subject public key is + //used to verify digital signatures, used to provide a non- repudiation + //service that protects against the signing entity falsely denying some + //action. KeyUsageContentCommitment + //The keyEncipherment bit is asserted when the subject public key is + //used for enciphering private or secret keys, i.e., for key transport. KeyUsageKeyEncipherment + //The dataEncipherment bit is asserted when the subject public key is used + //for directly enciphering raw user data without the use of an intermediate + //symmetric cipher.(Uncommon) KeyUsageDataEncipherment + //The keyAgreement bit is asserted when the subject public key is used for + //key agreement. i.e., for Diffie-Hellman. KeyUsageKeyAgreement + //The keyCertSign bit is asserted when the subject public key is used for verifying + //signatures on public key certificates. If the keyCertSign bit is asserted, + //then the cA bit in the basic constraints extension (Section 4.2.1.9) MUST + //also be asserted. KeyUsageCertSign + //The cRLSign bit is asserted when the subject public key is used for + //verifying signatures on certificate revocation lists (e.g., CRLs, delta + //CRLs, or ARLs). KeyUsageCRLSign + //The meaning of the encipherOnly/KeyUsageDecipherOnly bit is undefined in the + //absence of the keyAgreement bit. When the encipherOnly bit is asserted and + //the keyAgreement bit is also set, the subject public key may be used only for + //enciphering/deciphering data while performing key agreement. KeyUsageEncipherOnly KeyUsageDecipherOnly ) @@ -923,6 +948,9 @@ func checkSignature(algo SignatureAlgorithm, signed, signature []byte, publicKey // CheckCRLSignature checks that the signature in crl is from c. func (c *Certificate) CheckCRLSignature(crl *pkix.CertificateList) error { algo := getSignatureAlgorithmFromAI(crl.SignatureAlgorithm) + if c.KeyUsage != 0 && c.KeyUsage&KeyUsageCRLSign == 0 { + return errors.New("x509: invalid signature: this certificate cannot sign CRLs") + } return c.CheckSignature(algo, crl.TBSCertList.Raw, crl.SignatureValue.RightAlign()) }