diff --git a/agent.go b/agent.go index 2944c91..4bca466 100644 --- a/agent.go +++ b/agent.go @@ -47,6 +47,11 @@ func (bp byPairPriority) Less(i, j int) bool { return bp.candidatePairs[i].Priority() > bp.candidatePairs[j].Priority() } +type bindingRequest struct { + transactionID []byte + destination net.Addr +} + // Agent represents the ICE agent type Agent struct { onConnectionStateChangeHdlr func(ConnectionState) @@ -99,7 +104,7 @@ type Agent struct { buffer *packetio.Buffer // LRU of outbound Binding request Transaction IDs - pendingBindingRequests [][]byte + pendingBindingRequests []bindingRequest // State for closing done chan struct{} @@ -172,7 +177,7 @@ func NewAgent(config *AgentConfig) (*Agent, error) { connectionState: ConnectionStateNew, localCandidates: make(map[NetworkType][]*Candidate), remoteCandidates: make(map[NetworkType][]*Candidate), - pendingBindingRequests: make([][]byte, 0, maxPendingBindingRequests), + pendingBindingRequests: make([]bindingRequest, 0, maxPendingBindingRequests), localUfrag: randSeq(16), localPwd: randSeq(32), @@ -313,7 +318,10 @@ func (a *Agent) pingCandidate(local, remote *Candidate) { a.pendingBindingRequests = a.pendingBindingRequests[overflow:] } - a.pendingBindingRequests = append(a.pendingBindingRequests, transactionID) + a.pendingBindingRequests = append(a.pendingBindingRequests, bindingRequest{ + transactionID: transactionID, + destination: remote.addr(), + }) a.sendSTUN(msg, local, remote) } @@ -622,58 +630,82 @@ func (a *Agent) handleInboundControlling(m *stun.Message, localCandidate, remote } } -// Assert that the passed TransactionID is in our pendingBindingRequests and remove if it is -func (a *Agent) handleInboundBindingSuccess(id []byte) bool { +// Assert that the passed TransactionID is in our pendingBindingRequests and returns the destination +// If the bindingRequest was valid remove it from our pending cache +func (a *Agent) handleInboundBindingSuccess(id []byte) (bool, net.Addr) { for i := range a.pendingBindingRequests { - if bytes.Equal(a.pendingBindingRequests[i], id) { - a.pendingBindingRequests = append(a.pendingBindingRequests[:i], a.pendingBindingRequests[i+1:]...) - return true + if bytes.Equal(a.pendingBindingRequests[i].transactionID, id) { + defer func() { + a.pendingBindingRequests = append(a.pendingBindingRequests[:i], a.pendingBindingRequests[i+1:]...) + }() + return true, a.pendingBindingRequests[i].destination } } - return false + return false, nil } // handleInbound processes STUN traffic from a remote candidate func (a *Agent) handleInbound(m *stun.Message, local *Candidate, remote net.Addr) { + var err error if m == nil || local == nil { return - } - a.log.Tracef("inbound STUN from %s to %s", remote.String(), local.String()) - - switch { - case m.Method == stun.MethodBinding && m.Class == stun.ClassSuccessResponse: - 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 { - a.log.Warnf("discard message from (%s), %v", remote, err) - return - } else if err := assertInboundMessageIntegrity(m, []byte(a.localPwd)); err != nil { - a.log.Warnf("discard message from (%s), %v", remote, err) - return - } - default: + } else if m.Method != stun.MethodBinding || !(m.Class == stun.ClassSuccessResponse || m.Class == stun.ClassRequest) { + a.log.Tracef("unhandled STUN from %s to %s class(%s) method(%s)", remote.String(), local.String(), m.Method.String(), m.Class.String()) return } remoteCandidate := a.findRemoteCandidate(local.NetworkType, remote) - if remoteCandidate == nil { - a.log.Debugf("detected a new peer-reflexive candiate: %s ", remote) - pflxCandidate, err := handleNewPeerReflexiveCandidate(local, remote) - if err != nil { - a.log.Warn(err.Error()) + if m.Class == stun.ClassSuccessResponse { + if err = assertInboundMessageIntegrity(m, []byte(a.remotePwd)); err != nil { + a.log.Warnf("discard message from (%s), %v", remote, err) + return } - a.addRemoteCandidate(pflxCandidate) - return - } - remoteCandidate.seen(false) + ok, transactionAddr := a.handleInboundBindingSuccess(m.TransactionID) + if !ok { + a.log.Errorf("discard message from (%s), invalid TransactionID %s", remote, m.TransactionID) + return + } + // Assert that NAT is not symmetric + // https://tools.ietf.org/html/rfc8445#section-7.2.5.2.1 + if !addrEqual(transactionAddr, remote) { + a.log.Debugf("discard message: transaction source and destination does not match expected(%s), actual(%s)", transactionAddr, remote) + return + } else if remoteCandidate == nil { // Should fail previous check, better to be safe though + a.log.Warnf("discard success message from (%s), no such remote", remote) + return + } + } else { + if err = assertInboundUsername(m, a.localUfrag+":"+a.remoteUfrag); err != nil { + a.log.Warnf("discard message from (%s), %v", remote, err) + return + } else if err = assertInboundMessageIntegrity(m, []byte(a.localPwd)); err != nil { + a.log.Warnf("discard message from (%s), %v", remote, err) + return + } + + if remoteCandidate == nil { + ip, port, networkType, ok := parseAddr(remote) + if !ok { + a.log.Errorf("Failed to create parse remote net.Addr when creating remote prflx candidate") + return + } + + prflxCandidate, err := NewCandidatePeerReflexive(networkType.String(), ip, port, local.Component, "", 0) + if err != nil { + a.log.Errorf("Failed to create new remote prflx candidate (%s)", err) + return + } + remoteCandidate = prflxCandidate + + a.log.Debugf("adding a new peer-reflexive candiate: %s ", remote) + a.addRemoteCandidate(remoteCandidate) + } + } + + a.log.Tracef("inbound STUN from %s to %s", remote.String(), local.String()) + remoteCandidate.seen(false) if a.isControlling { a.handleInboundControlling(m, local, remoteCandidate) } else { diff --git a/agent_test.go b/agent_test.go index 74770ed..0dd945c 100644 --- a/agent_test.go +++ b/agent_test.go @@ -10,6 +10,17 @@ import ( "github.com/pion/transport/test" ) +type mockPacketConn struct { +} + +func (m *mockPacketConn) ReadFrom(p []byte) (n int, addr net.Addr, err error) { return 0, nil, nil } +func (m *mockPacketConn) WriteTo(p []byte, addr net.Addr) (n int, err error) { return 0, nil } +func (m *mockPacketConn) Close() error { return nil } +func (m *mockPacketConn) LocalAddr() net.Addr { return nil } +func (m *mockPacketConn) SetDeadline(t time.Time) error { return nil } +func (m *mockPacketConn) SetReadDeadline(t time.Time) error { return nil } +func (m *mockPacketConn) SetWriteDeadline(t time.Time) error { return nil } + func TestPairSearch(t *testing.T) { // Limit runtime in case of deadlocks lim := test.TimeOut(time.Second * 10) @@ -185,7 +196,7 @@ func TestHandlePeerReflexive(t *testing.T) { lim := test.TimeOut(time.Second * 2) defer lim.Stop() - t.Run("UDP pflx candidate from handleInboud()", func(t *testing.T) { + t.Run("UDP pflx candidate from handleInbound()", func(t *testing.T) { var config AgentConfig a, err := NewAgent(&config) @@ -195,6 +206,7 @@ func TestHandlePeerReflexive(t *testing.T) { ip := net.ParseIP("192.168.0.2") local, err := NewCandidateHost("udp", ip, 777, 1) + local.conn = &mockPacketConn{} if err != nil { t.Fatalf("failed to create a new candidate: %v", err) } @@ -276,56 +288,36 @@ func TestHandlePeerReflexive(t *testing.T) { } }) - t.Run("TCP prflx with handleNewPeerReflexiveCandidate()", func(t *testing.T) { - var config AgentConfig - a, err := NewAgent(&config) - + t.Run("Success from unknown remote, prflx candidate MUST only be created via Binding Request", func(t *testing.T) { + a, err := NewAgent(&AgentConfig{}) if err != nil { - t.Fatal("Error constructing ice.Agent") + t.Fatalf("Error constructing ice.Agent") } - ip := net.ParseIP("192.168.0.2") - local, err := NewCandidateHost("tcp", ip, 777, 1) + a.pendingBindingRequests = []bindingRequest{ + {[]byte("ABC"), &net.UDPAddr{}}, + } + + local, err := NewCandidateHost("udp", net.ParseIP("192.168.0.2"), 777, 1) + local.conn = &mockPacketConn{} if err != nil { t.Fatalf("failed to create a new candidate: %v", err) } - remote := &net.TCPAddr{IP: net.ParseIP("172.17.0.3"), Port: 999} - - candidate, err := handleNewPeerReflexiveCandidate(local, remote) + remote := &net.UDPAddr{IP: net.ParseIP("172.17.0.3"), Port: 999} + msg, err := stun.Build(stun.ClassSuccessResponse, stun.MethodBinding, []byte("ABC"), + &stun.MessageIntegrity{ + Key: []byte(a.remotePwd), + }, + &stun.Fingerprint{}, + ) if err != nil { - t.Fatalf("handleNewPeerReflexiveCandidate() should not fail: %v", err) - } - a.addRemoteCandidate(candidate) - - // length of remote candidate list must be one now - if len(a.remoteCandidates) != 1 { - t.Fatal("failed to add a network type to the remote candidate list") + t.Fatal(err) } - // length of remote candidate list for a network type must be 1 - set := a.remoteCandidates[local.NetworkType] - if len(set) != 1 { - t.Fatal("failed to add prflx candidate to remote candidate list") - } - - c := set[0] - - if c.Type != CandidateTypePeerReflexive { - t.Fatal("candidate type must be prflx") - } - - if !c.IP.Equal(net.ParseIP("172.17.0.3")) { - t.Fatal("IP address mismatch") - } - - if c.Port != 999 { - t.Fatal("Port number mismatch") - } - - err = a.Close() - if err != nil { - t.Fatalf("Close agent emits error %v", err) + a.handleInbound(msg, local, remote) + if len(a.remoteCandidates) != 0 { + t.Fatal("unknown remote was able to create a candidate") } }) } @@ -387,6 +379,7 @@ func TestInboundValidity(t *testing.T) { remote := &net.UDPAddr{IP: net.ParseIP("172.17.0.3"), Port: 999} local, err := NewCandidateHost("udp", net.ParseIP("192.168.0.2"), 777, 1) + local.conn = &mockPacketConn{} if err != nil { t.Fatalf("failed to create a new candidate: %v", err) } @@ -465,6 +458,36 @@ func TestInboundValidity(t *testing.T) { t.Fatal("Binding with valid values (but no fingerprint) was unable to create prflx candidate") } }) + + t.Run("Success with invalid TransactionID", func(t *testing.T) { + a, err := NewAgent(&AgentConfig{}) + if err != nil { + t.Fatalf("Error constructing ice.Agent") + } + + local, err := NewCandidateHost("udp", net.ParseIP("192.168.0.2"), 777, 1) + local.conn = &mockPacketConn{} + if err != nil { + t.Fatalf("failed to create a new candidate: %v", err) + } + + remote := &net.UDPAddr{IP: net.ParseIP("172.17.0.3"), Port: 999} + msg, err := stun.Build(stun.ClassSuccessResponse, stun.MethodBinding, []byte("ABC"), + &stun.MessageIntegrity{ + Key: []byte(a.remotePwd), + }, + &stun.Fingerprint{}, + ) + if err != nil { + t.Fatal(err) + } + + a.handleInbound(msg, local, remote) + if len(a.remoteCandidates) != 0 { + t.Fatal("unknown remote was able to create a candidate") + } + }) + } func TestInvalidAgentStarts(t *testing.T) { diff --git a/candidate.go b/candidate.go index 65d37f0..e665bfa 100644 --- a/candidate.go +++ b/candidate.go @@ -117,39 +117,6 @@ func NewCandidateRelay(network string, ip net.IP, port int, component uint16, re }, nil } -// handleNewPeerReflexiveCandidate creates a ReflexiveCandidate from a remote transport address -func handleNewPeerReflexiveCandidate(local *Candidate, remote net.Addr) (*Candidate, error) { - var ip net.IP - var port int - - switch addr := remote.(type) { - case *net.UDPAddr: - ip = addr.IP - port = addr.Port - case *net.TCPAddr: - ip = addr.IP - port = addr.Port - default: - return nil, fmt.Errorf("unsupported address type %T", addr) - } - - pflxCandidate, err := NewCandidatePeerReflexive( - local.NetworkType.String(), // assume, same as that of local - ip, - port, - local.Component, - "", // unknown at this moment. TODO: need a review - 0, // unknown at this moment. TODO: need a review - ) - - if err != nil { - return nil, flattenErrs([]error{fmt.Errorf("failed to create peer-reflexive candidate: %v", remote), err}) - } - - // Add pflxCandidate to the remote candidate list - return pflxCandidate, nil -} - // start runs the candidate using the provided connection func (c *Candidate) start(a *Agent, conn net.PacketConn) { c.agent = a diff --git a/util.go b/util.go index 59fbf99..554c500 100644 --- a/util.go +++ b/util.go @@ -68,3 +68,27 @@ func flattenErrs(errs []error) error { return fmt.Errorf(strings.Join(errstrings, "\n")) } + +func parseAddr(in net.Addr) (net.IP, int, NetworkType, bool) { + switch addr := in.(type) { + case *net.UDPAddr: + return addr.IP, addr.Port, NetworkTypeUDP4, true + case *net.TCPAddr: + return addr.IP, addr.Port, NetworkTypeTCP4, true + } + return nil, 0, 0, false +} + +func addrEqual(a, b net.Addr) bool { + aIP, aPort, aType, aOk := parseAddr(a) + if !aOk { + return false + } + + bIP, bPort, bType, bOk := parseAddr(b) + if !bOk { + return false + } + + return aType == bType && aIP.Equal(bIP) && aPort == bPort +}