Validate SDP offers and answers

3 files changed, 56 insertions, 13 deletions

diff --git a/broker/http.go b/broker/http.go
index 2f81f1d..32f6a71 100644
--- a/broker/http.go
+++ b/broker/http.go
@@ -1,7 +1,9 @@
 package main
 
 import (
+	"bytes"
 	"errors"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"log"
@@ -137,10 +139,17 @@ func clientOffers(i *IPC, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	err = validateSDP(body)
+	if err != nil {
+		log.Println("Error client SDP: ", err.Error())
+		w.WriteHeader(http.StatusBadRequest)
+		return
+	}
+
 	// Handle the legacy version
 	//
 	// We support two client message formats. The legacy format is for backwards
-	// combatability and relies heavily on HTTP headers and status codes to convey
+	// compatability and relies heavily on HTTP headers and status codes to convey
 	// information.
 	isLegacy := false
 	if len(body) > 0 && body[0] == '{' {
@@ -197,7 +206,7 @@ func clientOffers(i *IPC, w http.ResponseWriter, r *http.Request) {
 }
 
 /*
-Expects snowflake proxes which have previously successfully received
+Expects snowflake proxies which have previously successfully received
 an offer from proxyHandler to respond with an answer in an HTTP POST,
 which the broker will pass back to the original client.
 */
@@ -209,6 +218,13 @@ func proxyAnswers(i *IPC, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	err = validateSDP(body)
+	if err != nil {
+		log.Println("Error proxy SDP: ", err.Error())
+		w.WriteHeader(http.StatusBadRequest)
+		return
+	}
+
 	arg := messages.Arg{
 		Body:       body,
 		RemoteAddr: "",
@@ -233,3 +249,12 @@ func proxyAnswers(i *IPC, w http.ResponseWriter, r *http.Request) {
 		log.Printf("proxyAnswers unable to write answer response with error: %v", err)
 	}
 }
+
+func validateSDP(SDP []byte) error {
+	// TODO: more validation likely needed
+	if !bytes.Contains(SDP, []byte("a=candidate")) {
+		return fmt.Errorf("SDP contains no candidate")
+	}
+
+	return nil
+}
diff --git a/client/lib/webrtc.go b/client/lib/webrtc.go
index d12688e..9afe1a4 100644
--- a/client/lib/webrtc.go
+++ b/client/lib/webrtc.go
@@ -4,8 +4,10 @@ import (
 	"crypto/rand"
 	"encoding/hex"
 	"errors"
+	"fmt"
 	"io"
 	"log"
+	"strings"
 	"sync"
 	"time"
 
@@ -143,10 +145,10 @@ func (c *WebRTCPeer) checkForStaleness(timeout time.Duration) {
 	}
 }
 
+// connect does the bulk of the work: gather ICE candidates, send the SDP offer to broker,
+// receive an answer from broker, and wait for data channel to open
 func (c *WebRTCPeer) connect(config *webrtc.Configuration, broker *BrokerChannel) error {
 	log.Println(c.id, " connecting...")
-	// TODO: When go-webrtc is more stable, it's possible that a new
-	// PeerConnection won't need to be re-prepared each time.
 	err := c.preparePeerConnection(config)
 	localDescription := c.pc.LocalDescription()
 	c.eventsLogger.OnNewSnowflakeEvent(event.EventOnOfferCreated{
@@ -187,7 +189,7 @@ func (c *WebRTCPeer) connect(config *webrtc.Configuration, broker *BrokerChannel
 }
 
 // preparePeerConnection creates a new WebRTC PeerConnection and returns it
-// after ICE candidate gathering is complete..
+// after non-trickle ICE candidate gathering is complete.
 func (c *WebRTCPeer) preparePeerConnection(config *webrtc.Configuration) error {
 	var err error
 	s := webrtc.SettingEngine{}
@@ -240,10 +242,8 @@ func (c *WebRTCPeer) preparePeerConnection(config *webrtc.Configuration) error {
 	})
 	c.transport = dc
 	c.open = make(chan struct{})
-	log.Println("WebRTC: DataChannel created.")
+	log.Println("WebRTC: DataChannel created")
 
-	// Allow candidates to accumulate until ICEGatheringStateComplete.
-	done := webrtc.GatheringCompletePromise(c.pc)
 	offer, err := c.pc.CreateOffer(nil)
 	// TODO: Potentially timeout and retry if ICE isn't working.
 	if err != nil {
@@ -252,16 +252,23 @@ func (c *WebRTCPeer) preparePeerConnection(config *webrtc.Configuration) error {
 		return err
 	}
 	log.Println("WebRTC: Created offer")
+
+	// Allow candidates to accumulate until ICEGatheringStateComplete.
+	done := webrtc.GatheringCompletePromise(c.pc)
+	// Start gathering candidates
 	err = c.pc.SetLocalDescription(offer)
 	if err != nil {
-		log.Println("Failed to prepare offer", err)
+		log.Println("Failed to apply offer", err)
 		c.pc.Close()
 		return err
 	}
 	log.Println("WebRTC: Set local description")
 
 	<-done // Wait for ICE candidate gathering to complete.
-	log.Println("WebRTC: PeerConnection created.")
+
+	if !strings.Contains(c.pc.LocalDescription().SDP, "\na=candidate:") {
+		return fmt.Errorf("SDP offer contains no candidate")
+	}
 	return nil
 }
 
diff --git a/proxy/lib/snowflake.go b/proxy/lib/snowflake.go
index 92d02dd..1dceb42 100644
--- a/proxy/lib/snowflake.go
+++ b/proxy/lib/snowflake.go
@@ -250,6 +250,8 @@ func (s *SignalingServer) pollOffer(sid string, proxyType string, acceptedRelayP
 	return nil, ""
 }
 
+// sendAnswer encodes an SDP answer, sends it to the broker
+// and wait for its response
 func (s *SignalingServer) sendAnswer(sid string, pc *webrtc.PeerConnection) error {
 	ld := pc.LocalDescription()
 	if !s.keepLocalAddresses {
@@ -485,6 +487,10 @@ func (sf *SnowflakeProxy) makePeerConnectionFromOffer(sdp *webrtc.SessionDescrip
 
 	// Wait for ICE candidate gathering to complete
 	<-done
+
+	if !strings.Contains(pc.LocalDescription().SDP, "\na=candidate:") {
+		return nil, fmt.Errorf("SDP answer contains no candidate")
+	}
 	log.Printf("Answer: \n\t%s", strings.ReplaceAll(pc.LocalDescription().SDP, "\n", "\n\t"))
 
 	return pc, nil
@@ -524,15 +530,16 @@ func (sf *SnowflakeProxy) makeNewPeerConnection(config webrtc.Configuration,
 		pc.Close()
 		return nil, err
 	}
-	log.Println("Probetest: Creating offer")
+	log.Println("Probetest: Created Offer")
 
 	// As of v3.0.0, pion-webrtc uses trickle ICE by default.
 	// We have to wait for candidate gathering to complete
 	// before we send the offer
 	done := webrtc.GatheringCompletePromise(pc)
+	// start the gathering of ICE candidates
 	err = pc.SetLocalDescription(offer)
 	if err != nil {
-		log.Println("Failed to prepare offer", err)
+		log.Println("Failed to apply offer", err)
 		pc.Close()
 		return nil, err
 	}
@@ -540,6 +547,11 @@ func (sf *SnowflakeProxy) makeNewPeerConnection(config webrtc.Configuration,
 
 	// Wait for ICE candidate gathering to complete
 	<-done
+
+	if !strings.Contains(pc.LocalDescription().SDP, "\na=candidate:") {
+		return nil, fmt.Errorf("Probetest SDP offer contains no candidate")
+	}
+
 	return pc, nil
 }
 
@@ -701,7 +713,6 @@ func (sf *SnowflakeProxy) checkNATType(config webrtc.Configuration, probeURL str
 		log.Printf("Error parsing url: %s", err.Error())
 	}
 
-	// create offer used for probetest
 	dataChan := make(chan struct{})
 	pc, err := sf.makeNewPeerConnection(config, dataChan)
 	if err != nil {