Repository: pion/webrtc
Pull request #1709 (2021-03-17 23:23:38)

Close DTLS when fingerprint verification fails
Before we would set the PeerConnection to failed, but we would leave the
DTLSTransport. This means that a user could still interact with the
other transports.

Relates to #1708


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/1709?src=pr&el=h1) Report
> Merging [#1709](https://codecov.io/gh/pion/webrtc/pull/1709?src=pr&el=desc) (465865e) into [master](https://codecov.io/gh/pion/webrtc/commit/c901d6f0055b266f22254c53604f45a11e8ffc44?el=desc) (c901d6f) will **decrease** coverage by `5.10%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pion/webrtc/pull/1709/graphs/tree.svg?width=650&height=150&src=pr&token=XOOuQo07I2)](https://codecov.io/gh/pion/webrtc/pull/1709?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1709      +/-   ##
==========================================
- Coverage   74.96%   69.86%   -5.11%     
==========================================
  Files          79       57      -22     
  Lines        5749     2144    -3605     
==========================================
- Hits         4310     1498    -2812     
+ Misses       1063      543     -520     
+ Partials      376      103     -273     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `69.86% <ø> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/webrtc/pull/1709?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [icetcp.go](https://codecov.io/gh/pion/webrtc/pull/1709/diff?src=pr&el=tree#diff-aWNldGNwLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [rtpcodec.go](https://codecov.io/gh/pion/webrtc/pull/1709/diff?src=pr&el=tree#diff-cnRwY29kZWMuZ28=) | `0.00% <0.00%> (-90.00%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/1709/diff?src=pr&el=tree#diff-aWNlY2FuZGlkYXRlcGFpci5nbw==) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [track\_local.go](https://codecov.io/gh/pion/webrtc/pull/1709/diff?src=pr&el=tree#diff-dHJhY2tfbG9jYWwuZ28=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/1709/diff?src=pr&el=tree#diff-c3RhdHMuZ28=) | `0.00% <0.00%> (-67.86%)` | :arrow_down: |
| [internal/mux/muxfunc.go](https://codecov.io/gh/pion/webrtc/pull/1709/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbXV4L211eGZ1bmMuZ28=) | `0.00% <0.00%> (-47.83%)` | :arrow_down: |
| [gathering\_complete\_promise.go](https://codecov.io/gh/pion/webrtc/pull/1709/diff?src=pr&el=tree#diff-Z2F0aGVyaW5nX2NvbXBsZXRlX3Byb21pc2UuZ28=) | `60.00% <0.00%> (-40.00%)` | :arrow_down: |
| [internal/mux/endpoint.go](https://codecov.io/gh/pion/webrtc/pull/1709/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbXV4L2VuZHBvaW50Lmdv) | `22.22% <0.00%> (-38.89%)` | :arrow_down: |
| [sdpsemantics.go](https://codecov.io/gh/pion/webrtc/pull/1709/diff?src=pr&el=tree#diff-c2Rwc2VtYW50aWNzLmdv) | `0.00% <0.00%> (-37.50%)` | :arrow_down: |
| [dtlsrole.go](https://codecov.io/gh/pion/webrtc/pull/1709/diff?src=pr&el=tree#diff-ZHRsc3JvbGUuZ28=) | `68.96% <0.00%> (-31.04%)` | :arrow_down: |
| ... and [30 more](https://codecov.io/gh/pion/webrtc/pull/1709/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/webrtc/pull/1709?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/webrtc/pull/1709?src=pr&el=footer). Last update [c901d6f...465865e](https://codecov.io/gh/pion/webrtc/pull/1709?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Sean-Der: @jech mind giving this a look over!

This fixes the issue. But also wanted to make you aware of the issue.
Sean-Der: I applied a CVE for this. Lots of companies watch that so gives people more notice.

When we tag + get CVE-ID I will announce on Slack/Twitter. Just want to make sure people update as soon as possible.

Patch:
diff --git a/dtlstransport.go b/dtlstransport.go
index d1100929..cc25889a 100644
--- a/dtlstransport.go
+++ b/dtlstransport.go
@@ -17,6 +17,7 @@ import (
 
 	"github.com/pion/dtls/v2"
 	"github.com/pion/dtls/v2/pkg/crypto/fingerprint"
+	"github.com/pion/logging"
 	"github.com/pion/srtp/v2"
 	"github.com/pion/webrtc/v3/internal/mux"
 	"github.com/pion/webrtc/v3/internal/util"
@@ -49,6 +50,7 @@ type DTLSTransport struct {
 	dtlsMatcher mux.MatchFunc
 
 	api *API
+	log logging.LeveledLogger
 }
 
 // NewDTLSTransport creates a new DTLSTransport.
@@ -61,6 +63,7 @@ func (api *API) NewDTLSTransport(transport *ICETransport, certificates []Certifi
 		state:        DTLSTransportStateNew,
 		dtlsMatcher:  mux.MatchDTLS,
 		srtpReady:    make(chan struct{}),
+		log:          api.settingEngine.LoggerFactory.NewLogger("DTLSTransport"),
 	}
 
 	if len(certificates) > 0 {
@@ -324,15 +327,12 @@ func (t *DTLSTransport) Start(remoteParameters DTLSParameters) error {
 		return ErrNoSRTPProtectionProfile
 	}
 
-	t.conn = dtlsConn
-	t.onStateChange(DTLSTransportStateConnected)
-
 	if t.api.settingEngine.disableCertificateFingerprintVerification {
 		return nil
 	}
 
 	// Check the fingerprint if a certificate was exchanged
-	remoteCerts := t.conn.ConnectionState().PeerCertificates
+	remoteCerts := dtlsConn.ConnectionState().PeerCertificates
 	if len(remoteCerts) == 0 {
 		t.onStateChange(DTLSTransportStateFailed)
 		return errNoRemoteCertificate
@@ -341,15 +341,26 @@ func (t *DTLSTransport) Start(remoteParameters DTLSParameters) error {
 
 	parsedRemoteCert, err := x509.ParseCertificate(t.remoteCertificate)
 	if err != nil {
+		if closeErr := dtlsConn.Close(); closeErr != nil {
+			t.log.Error(err.Error())
+		}
+
 		t.onStateChange(DTLSTransportStateFailed)
 		return err
 	}
 
 	if err = t.validateFingerPrint(parsedRemoteCert); err != nil {
+		if closeErr := dtlsConn.Close(); closeErr != nil {
+			t.log.Error(err.Error())
+		}
+
 		t.onStateChange(DTLSTransportStateFailed)
 		return err
 	}
 
+	t.conn = dtlsConn
+	t.onStateChange(DTLSTransportStateConnected)
+
 	return t.startSRTP()
 }
 


Test patch:
diff --git a/dtlstransport_test.go b/dtlstransport_test.go
index 14de9701..a4aabe53 100644
--- a/dtlstransport_test.go
+++ b/dtlstransport_test.go
@@ -29,6 +29,10 @@ func TestInvalidFingerprintCausesFailed(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	pcAnswer.OnDataChannel(func(_ *DataChannel) {
+		t.Fatal("A DataChannel must not be created when Fingerprint verification fails")
+	})
+
 	defer closePairNow(t, pcOffer, pcAnswer)
 
 	offerChan := make(chan SessionDescription)
@@ -83,6 +87,12 @@ func TestInvalidFingerprintCausesFailed(t *testing.T) {
 
 	offerConnectionHasFailed.Wait()
 	answerConnectionHasFailed.Wait()
+
+	assert.Equal(t, pcOffer.SCTP().Transport().State(), DTLSTransportStateFailed)
+	assert.Nil(t, pcOffer.SCTP().Transport().conn)
+
+	assert.Equal(t, pcAnswer.SCTP().Transport().State(), DTLSTransportStateFailed)
+	assert.Nil(t, pcAnswer.SCTP().Transport().conn)
 }
 
 func TestPeerConnection_DTLSRoleSettingEngine(t *testing.T) {