From f5e8cc510d580984bcca6b200be9868a2ddfeabe Mon Sep 17 00:00:00 2001 From: Sean DuBois Date: Mon, 15 Apr 2019 22:29:11 -0700 Subject: [PATCH] Validate inbound success messages Validate all inbound binding success messages, before peers could create valid prflx candidates without doing a full request response Resolves #21 --- agent.go | 67 ++++++++++++++++++++++++++++++++--------------- gather_test.go | 41 +++++++++++++++++++++++++++++ transport_test.go | 2 +- 3 files changed, 88 insertions(+), 22 deletions(-) create mode 100644 gather_test.go diff --git a/agent.go b/agent.go index 82d6cbb..cfcdc16 100644 --- a/agent.go +++ b/agent.go @@ -3,6 +3,7 @@ package ice import ( + "bytes" "fmt" "math/rand" "net" @@ -28,6 +29,9 @@ const ( // the number of bytes that can be buffered before we start to error maxBufferSize = 1000 * 1000 // 1MB + // the number of outbound binding requests we cache + maxPendingBindingRequests = 50 + stunAttrHeaderLength = 4 ) @@ -94,6 +98,9 @@ type Agent struct { buffer *packetio.Buffer + // LRU of outbound Binding request Transaction IDs + pendingBindingRequests [][]byte + // State for closing done chan struct{} err atomicError @@ -160,11 +167,12 @@ func NewAgent(config *AgentConfig) (*Agent, error) { } a := &Agent{ - tieBreaker: rand.New(rand.NewSource(time.Now().UnixNano())).Uint64(), - gatheringState: GatheringStateComplete, // TODO trickle-ice - connectionState: ConnectionStateNew, - localCandidates: make(map[NetworkType][]*Candidate), - remoteCandidates: make(map[NetworkType][]*Candidate), + tieBreaker: rand.New(rand.NewSource(time.Now().UnixNano())).Uint64(), + gatheringState: GatheringStateComplete, // TODO trickle-ice + connectionState: ConnectionStateNew, + localCandidates: make(map[NetworkType][]*Candidate), + remoteCandidates: make(map[NetworkType][]*Candidate), + pendingBindingRequests: make([][]byte, 0, maxPendingBindingRequests), localUfrag: randSeq(16), localPwd: randSeq(32), @@ -268,8 +276,9 @@ func (a *Agent) pingCandidate(local, remote *Candidate) { // agent MUST NOT include the USE-CANDIDATE attribute in a Binding // request. + transactionID := stun.GenerateTransactionID() if a.isControlling { - msg, err = stun.Build(stun.ClassRequest, stun.MethodBinding, stun.GenerateTransactionID(), + msg, err = stun.Build(stun.ClassRequest, stun.MethodBinding, transactionID, &stun.Username{Username: a.remoteUfrag + ":" + a.localUfrag}, &stun.UseCandidate{}, &stun.IceControlling{TieBreaker: a.tieBreaker}, @@ -280,7 +289,7 @@ func (a *Agent) pingCandidate(local, remote *Candidate) { &stun.Fingerprint{}, ) } else { - msg, err = stun.Build(stun.ClassRequest, stun.MethodBinding, stun.GenerateTransactionID(), + msg, err = stun.Build(stun.ClassRequest, stun.MethodBinding, transactionID, &stun.Username{Username: a.remoteUfrag + ":" + a.localUfrag}, &stun.IceControlled{TieBreaker: a.tieBreaker}, &stun.Priority{Priority: local.Priority()}, @@ -297,6 +306,12 @@ func (a *Agent) pingCandidate(local, remote *Candidate) { } a.log.Tracef("ping STUN from %s to %s\n", local.String(), remote.String()) + if overflow := len(a.pendingBindingRequests) - maxPendingBindingRequests; overflow > 1 { + a.log.Debugf("Discarded %d pending binding requests, pendingBindingRequests is full", overflow) + a.pendingBindingRequests = a.pendingBindingRequests[overflow:] + } + + a.pendingBindingRequests = append(a.pendingBindingRequests, transactionID) a.sendSTUN(msg, local, remote) } @@ -575,12 +590,11 @@ func (a *Agent) handleInboundControlled(m *stun.Message, localCandidate, remoteC } successResponse := m.Method == stun.MethodBinding && m.Class == stun.ClassSuccessResponse - _, usepair := m.GetOneAttribute(stun.AttrUseCandidate) - a.log.Tracef("got controlled message (success? %t, usepair? %t)", successResponse, usepair) - // Remember the working pair and select it when marked with usepair - a.setValidPair(localCandidate, remoteCandidate, usepair, false) - - if !successResponse { + a.log.Tracef("got controlled message (success? %t)", successResponse) + if successResponse { + // Remember the working pair and select it when marked with usepair + a.setValidPair(localCandidate, remoteCandidate, true, false) + } else { // Send success response a.sendBindingSuccess(m, localCandidate, remoteCandidate) } @@ -594,21 +608,29 @@ func (a *Agent) handleInboundControlling(m *stun.Message, localCandidate, remote a.log.Debug("useCandidate && a.isControlling == true") return } - a.log.Tracef("got controlling message: %#v", m) successResponse := m.Method == stun.MethodBinding && m.Class == stun.ClassSuccessResponse - // Remember the working pair and select it when receiving a success response - a.setValidPair(localCandidate, remoteCandidate, successResponse, true) - - if !successResponse { + a.log.Tracef("got controlled message (success? %t)", successResponse) + if successResponse { + // Remember the working pair and select it when receiving a success response + a.setValidPair(localCandidate, remoteCandidate, true, true) + } else { // Send success response a.sendBindingSuccess(m, localCandidate, remoteCandidate) - - // We received a ping from the controlled agent. We know the pair works so now we ping with use-candidate set: - a.pingCandidate(localCandidate, remoteCandidate) } } +// Assert that the passed TransactionID is in our pendingBindingRequests and remove if it is +func (a *Agent) handleInboundBindingSuccess(id []byte) bool { + for i := range a.pendingBindingRequests { + if bytes.Equal(a.pendingBindingRequests[i], id) { + a.pendingBindingRequests = append(a.pendingBindingRequests[:i], a.pendingBindingRequests[:i+1]...) + return true + } + } + return false +} + // handleInbound processes STUN traffic from a remote candidate func (a *Agent) handleInbound(m *stun.Message, local *Candidate, remote net.Addr) { if m == nil || local == nil { @@ -621,6 +643,9 @@ func (a *Agent) handleInbound(m *stun.Message, local *Candidate, remote net.Addr if err := assertInboundMessageIntegrity(m, []byte(a.remotePwd)); err != nil { a.log.Warnf("discard message from (%s), %v", remote, err) return + } else if !a.handleInboundBindingSuccess(m.TransactionID) { + a.log.Warnf("discard message from (%s), invalid TransactionID %s", remote, m.TransactionID) + return } case m.Method == stun.MethodBinding && m.Class == stun.ClassRequest: if err := assertInboundUsername(m, a.localUfrag+":"+a.remoteUfrag); err != nil { diff --git a/gather_test.go b/gather_test.go new file mode 100644 index 0000000..f932d1b --- /dev/null +++ b/gather_test.go @@ -0,0 +1,41 @@ +package ice + +import ( + "net" + "testing" +) + +func TestListenUDP(t *testing.T) { + localIPs := localInterfaces([]NetworkType{NetworkTypeUDP4}) + if len(localIPs) == 0 { + t.Fatal("localInterfaces found no interfaces, unable to test") + } + + ip := localIPs[0] + + conn, err := listenUDP(0, 0, udp, &net.UDPAddr{IP: ip, Port: 0}) + if err != nil { + t.Fatalf("listenUDP error with no port restriction %v", err) + } else if conn == nil { + t.Fatalf("listenUDP error with no port restriction return a nil conn") + } + + _, err = listenUDP(500, 499, udp, &net.UDPAddr{IP: ip, Port: 0}) + if err != ErrPort { + t.Fatal("listenUDP with invalid port range did not return ErrPort") + } + + conn, err = listenUDP(5000, 5000, udp, &net.UDPAddr{IP: ip, Port: 0}) + if err != nil { + t.Fatalf("listenUDP error with no port restriction %v", err) + } else if conn == nil { + t.Fatalf("listenUDP error with no port restriction return a nil conn") + } + + _, port, err := net.SplitHostPort(conn.LocalAddr().String()) + if err != nil { + t.Fatal(err) + } else if port != "5000" { + t.Fatalf("listenUDP with port restriction of 5000 listened on incorrect port (%s)", port) + } +} diff --git a/transport_test.go b/transport_test.go index 2daf338..c263634 100644 --- a/transport_test.go +++ b/transport_test.go @@ -63,7 +63,7 @@ func TestTimeout(t *testing.T) { panic(err) } - testTimeout(t, ca, 30*time.Second) + testTimeout(t, ca, defaultConnectionTimeout) ca, cb = pipeWithTimeout(5*time.Second, 3*time.Second) err = cb.Close()