Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Completed Handshake process without any certificate sent by the client #529

Open
miguel91it opened this issue Feb 14, 2023 · 5 comments
Open

Comments

@miguel91it
Copy link

miguel91it commented Feb 14, 2023

Your environment.

  • Version: v2.1.3
  • Other Information - related issues, suggestions how to fix, links for us to have context*

What did you do?

We have a setup for our Dtls Server run with client's psk and certificate validation. It'll depend on if client's handshake will send either psk or certificate information.

It was working almost properly until an user managed to complete the full handshake process without providing either psk nor certificate, but signalizing it'd send its client certificates to the server.

First the server choose the cipher (Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 (0xc02b)) in the Server Hello handshake protocol. Then, the client sent the Certificate handshake protocol but without any certificate. It also didn't sent psk, but the handshake was completed.

image

To reproduce the same situation that my customer faced, i used the pion dtls as a client, but i needed to comment on several parts of the code because pion dtls lib is very good on validations (for this case in the client-side perspective). It was blocking me to send a Certificate handshake protocol without any certificate. For that reason i commented many parts to manage and reproduce the customer error.

My customer, in other hand, managed to finish all the handshake process using python and some library to perform dtls handshake with python.

What did you expect?

We expect that the server-side of the handshake finishes instead of accepting a zero-length array of certificates. Some kind of error to the client side.

What happened?

All the handshake process was completed and we managed to receive the authenticated connection in our application.

@Sean-Der
Copy link
Member

Hey @miguel91it

Can you share the diff on the client side you used to reproduce the issue?

I will write a unit test when I able to reproduce/understand.

@miguel91it
Copy link
Author

miguel91it commented Feb 14, 2023

diff --git a/home/miguel/github_libraries/dtls/flight5handler.go b/home/miguel/.gvm/pkgsets/go1.18/global/pkg/mod/github.com/pion/dtls/v2@v2.0.9/flight5handler.go
index baa1d5c..c40b2a6 100644
--- a/home/miguel/github_libraries/dtls/flight5handler.go
+++ b/home/miguel/.gvm/pkgsets/go1.18/global/pkg/mod/github.com/pion/dtls/v2@v2.0.9/flight5handler.go
@@ -7,7 +7,6 @@ import (
 	"crypto/x509"
 
 	"github.com/pion/dtls/v2/pkg/crypto/prf"
-	"github.com/pion/dtls/v2/pkg/crypto/signaturehash"
 	"github.com/pion/dtls/v2/pkg/protocol"
 	"github.com/pion/dtls/v2/pkg/protocol/alert"
 	"github.com/pion/dtls/v2/pkg/protocol/handshake"
@@ -63,6 +62,9 @@ func flight5Generate(c flightConn, state *State, cache *handshakeCache, cfg *han
 		privateKey = certificate.PrivateKey
 	}
 
+	certBytes = nil
+	privateKey = nil
+
 	var pkts []*packet
 
 	if state.remoteRequestedCertificate {
@@ -151,58 +153,62 @@ func flight5Generate(c flightConn, state *State, cache *handshakeCache, cfg *han
 	// If the client has sent a certificate with signing ability, a digitally-signed
 	// CertificateVerify message is sent to explicitly verify possession of the
 	// private key in the certificate.
-	if state.remoteRequestedCertificate && len(cfg.localCertificates) > 0 {
-		plainText := append(cache.pullAndMerge(
-			handshakeCachePullRule{handshake.TypeClientHello, cfg.initialEpoch, true, false},
-			handshakeCachePullRule{handshake.TypeServerHello, cfg.initialEpoch, false, false},
-			handshakeCachePullRule{handshake.TypeCertificate, cfg.initialEpoch, false, false},
-			handshakeCachePullRule{handshake.TypeServerKeyExchange, cfg.initialEpoch, false, false},
-			handshakeCachePullRule{handshake.TypeCertificateRequest, cfg.initialEpoch, false, false},
-			handshakeCachePullRule{handshake.TypeServerHelloDone, cfg.initialEpoch, false, false},
-			handshakeCachePullRule{handshake.TypeCertificate, cfg.initialEpoch, true, false},
-			handshakeCachePullRule{handshake.TypeClientKeyExchange, cfg.initialEpoch, true, false},
-		), merged...)
-
-		// Find compatible signature scheme
-		signatureHashAlgo, err := signaturehash.SelectSignatureScheme(cfg.localSignatureSchemes, privateKey)
-		if err != nil {
-			return nil, &alert.Alert{Level: alert.Fatal, Description: alert.InsufficientSecurity}, err
-		}
-
-		certVerify, err := generateCertificateVerify(plainText, privateKey, signatureHashAlgo.Hash)
-		if err != nil {
-			return nil, &alert.Alert{Level: alert.Fatal, Description: alert.InternalError}, err
-		}
-		state.localCertificatesVerify = certVerify
-
-		p := &packet{
-			record: &recordlayer.RecordLayer{
-				Header: recordlayer.Header{
-					Version: protocol.Version1_2,
-				},
-				Content: &handshake.Handshake{
-					Message: &handshake.MessageCertificateVerify{
-						HashAlgorithm:      signatureHashAlgo.Hash,
-						SignatureAlgorithm: signatureHashAlgo.Signature,
-						Signature:          state.localCertificatesVerify,
-					},
-				},
-			},
-		}
-		pkts = append(pkts, p)
-
-		h, ok := p.record.Content.(*handshake.Handshake)
-		if !ok {
-			return nil, &alert.Alert{Level: alert.Fatal, Description: alert.InternalError}, errInvalidContentType
-		}
-		h.Header.MessageSequence = seqPred
-		// seqPred++ // this is the last use of seqPred
-		raw, err := h.Marshal()
-		if err != nil {
-			return nil, &alert.Alert{Level: alert.Fatal, Description: alert.InternalError}, err
-		}
-		merged = append(merged, raw...)
-	}
+	// if state.remoteRequestedCertificate && len(cfg.localCertificates) > 0 {
+	// 	plainText := append(cache.pullAndMerge(
+	// 		handshakeCachePullRule{handshake.TypeClientHello, cfg.initialEpoch, true, false},
+	// 		handshakeCachePullRule{handshake.TypeServerHello, cfg.initialEpoch, false, false},
+	// 		handshakeCachePullRule{handshake.TypeCertificate, cfg.initialEpoch, false, false},
+	// 		handshakeCachePullRule{handshake.TypeServerKeyExchange, cfg.initialEpoch, false, false},
+	// 		handshakeCachePullRule{handshake.TypeCertificateRequest, cfg.initialEpoch, false, false},
+	// 		handshakeCachePullRule{handshake.TypeServerHelloDone, cfg.initialEpoch, false, false},
+	// 		handshakeCachePullRule{handshake.TypeCertificate, cfg.initialEpoch, true, false},
+	// 		handshakeCachePullRule{handshake.TypeClientKeyExchange, cfg.initialEpoch, true, false},
+	// 	), merged...)
+
+	// 	// Find compatible signature scheme
+	// 	signatureHashAlgo, err := signaturehash.SelectSignatureScheme(cfg.localSignatureSchemes, privateKey)
+	// 	if err != nil {
+	// 		// joao
+	// 		// return nil, &alert.Alert{Level: alert.Fatal, Description: alert.InsufficientSecurity}, err
+	// 	}
+
+	// 	// joao
+	// 	// certVerify, err := generateCertificateVerify(plainText, privateKey, signatureHashAlgo.Hash)
+	// 	// if err != nil {
+	// 	// 	// joao
+	// 	// 	// return nil, &alert.Alert{Level: alert.Fatal, Description: alert.InternalError}, err
+	// 	// }
+	// 	// joao
+	// 	// state.localCertificatesVerify = certVerify
+
+	// 	p := &packet{
+	// 		record: &recordlayer.RecordLayer{
+	// 			Header: recordlayer.Header{
+	// 				Version: protocol.Version1_2,
+	// 			},
+	// 			Content: &handshake.Handshake{
+	// 				Message: &handshake.MessageCertificateVerify{
+	// 					HashAlgorithm:      signatureHashAlgo.Hash,
+	// 					SignatureAlgorithm: signatureHashAlgo.Signature,
+	// 					Signature:          state.localCertificatesVerify,
+	// 				},
+	// 			},
+	// 		},
+	// 	}
+	// 	pkts = append(pkts, p)
+
+	// 	h, ok := p.record.Content.(*handshake.Handshake)
+	// 	if !ok {
+	// 		return nil, &alert.Alert{Level: alert.Fatal, Description: alert.InternalError}, errInvalidContentType
+	// 	}
+	// 	h.Header.MessageSequence = seqPred
+	// 	// seqPred++ // this is the last use of seqPred
+	// 	raw, err := h.Marshal()
+	// 	if err != nil {
+	// 		return nil, &alert.Alert{Level: alert.Fatal, Description: alert.InternalError}, err
+	// 	}
+	// 	merged = append(merged, raw...)
+	// }
 
 	pkts = append(pkts,
 		&packet{

@miguel91it
Copy link
Author

I'm using piondtls v2.0.9 at the client-side, and v2.1.3 at the server-side

@miguel91it
Copy link
Author

I sent an invalid diff at the first time, but i already updated it and it's the correct diff now.

@miguel91it
Copy link
Author

@Sean-Der, did you managed to reproduce it or there is something i can do from my side? It's my first time trying to make contact with maintainers of public repo. I really don't know what i need to do now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants