diff --git a/p2p/base/ice_transport_internal.cc b/p2p/base/ice_transport_internal.cc index 1d5b6e7403..104a95b5af 100644 --- a/p2p/base/ice_transport_internal.cc +++ b/p2p/base/ice_transport_internal.cc @@ -14,6 +14,50 @@ namespace cricket { +using webrtc::RTCError; +using webrtc::RTCErrorType; + +RTCError VerifyCandidate(const Candidate& cand) { + // No address zero. + if (cand.address().IsNil() || cand.address().IsAnyIP()) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "candidate has address of zero"); + } + + // Disallow all ports below 1024, except for 80 and 443 on public addresses. + int port = cand.address().port(); + if (cand.protocol() == cricket::TCP_PROTOCOL_NAME && + (cand.tcptype() == cricket::TCPTYPE_ACTIVE_STR || port == 0)) { + // Expected for active-only candidates per + // http://tools.ietf.org/html/rfc6544#section-4.5 so no error. + // Libjingle clients emit port 0, in "active" mode. + return RTCError::OK(); + } + if (port < 1024) { + if ((port != 80) && (port != 443)) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "candidate has port below 1024, but not 80 or 443"); + } + + if (cand.address().IsPrivateIP()) { + return RTCError( + RTCErrorType::INVALID_PARAMETER, + "candidate has port of 80 or 443 with private IP address"); + } + } + + return RTCError::OK(); +} + +RTCError VerifyCandidates(const Candidates& candidates) { + for (const Candidate& candidate : candidates) { + RTCError error = VerifyCandidate(candidate); + if (!error.ok()) + return error; + } + return RTCError::OK(); +} + IceConfig::IceConfig() = default; IceConfig::IceConfig(int receiving_timeout_ms, diff --git a/p2p/base/ice_transport_internal.h b/p2p/base/ice_transport_internal.h index b735a1a742..b3eb2dc9e2 100644 --- a/p2p/base/ice_transport_internal.h +++ b/p2p/base/ice_transport_internal.h @@ -18,6 +18,7 @@ #include "absl/types/optional.h" #include "api/candidate.h" +#include "api/rtc_error.h" #include "api/transport/enums.h" #include "p2p/base/connection.h" #include "p2p/base/packet_transport_internal.h" @@ -74,6 +75,17 @@ enum class NominationMode { // The details are described in P2PTransportChannel. }; +// Utility method that checks if various required Candidate fields are filled in +// and contain valid values. If conditions are not met, an RTCError with the +// appropriated error number and description is returned. If the configuration +// is valid RTCError::OK() is returned. +webrtc::RTCError VerifyCandidate(const Candidate& cand); + +// Runs through a list of cricket::Candidate instances and calls VerifyCandidate +// for each one, stopping on the first error encounted and returning that error +// value if so. On success returns RTCError::OK(). +webrtc::RTCError VerifyCandidates(const Candidates& candidates); + // Information about ICE configuration. // TODO(deadbeef): Use absl::optional to represent unset values, instead of // -1. diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 7303c9325a..33b6e3f946 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -34,53 +34,6 @@ using webrtc::SdpType; -namespace { - -webrtc::RTCError VerifyCandidate(const cricket::Candidate& cand) { - // No address zero. - if (cand.address().IsNil() || cand.address().IsAnyIP()) { - return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, - "candidate has address of zero"); - } - - // Disallow all ports below 1024, except for 80 and 443 on public addresses. - int port = cand.address().port(); - if (cand.protocol() == cricket::TCP_PROTOCOL_NAME && - (cand.tcptype() == cricket::TCPTYPE_ACTIVE_STR || port == 0)) { - // Expected for active-only candidates per - // http://tools.ietf.org/html/rfc6544#section-4.5 so no error. - // Libjingle clients emit port 0, in "active" mode. - return webrtc::RTCError::OK(); - } - if (port < 1024) { - if ((port != 80) && (port != 443)) { - return webrtc::RTCError( - webrtc::RTCErrorType::INVALID_PARAMETER, - "candidate has port below 1024, but not 80 or 443"); - } - - if (cand.address().IsPrivateIP()) { - return webrtc::RTCError( - webrtc::RTCErrorType::INVALID_PARAMETER, - "candidate has port of 80 or 443 with private IP address"); - } - } - - return webrtc::RTCError::OK(); -} - -webrtc::RTCError VerifyCandidates(const cricket::Candidates& candidates) { - for (const cricket::Candidate& candidate : candidates) { - webrtc::RTCError error = VerifyCandidate(candidate); - if (!error.ok()) { - return error; - } - } - return webrtc::RTCError::OK(); -} - -} // namespace - namespace webrtc { JsepTransportController::JsepTransportController( @@ -333,19 +286,8 @@ void JsepTransportController::MaybeStartGathering() { RTCError JsepTransportController::AddRemoteCandidates( const std::string& transport_name, const cricket::Candidates& candidates) { - if (!network_thread_->IsCurrent()) { - return network_thread_->Invoke(RTC_FROM_HERE, [&] { - return AddRemoteCandidates(transport_name, candidates); - }); - } - RTC_DCHECK_RUN_ON(network_thread_); - - // Verify each candidate before passing down to the transport layer. - RTCError error = VerifyCandidates(candidates); - if (!error.ok()) { - return error; - } + RTC_DCHECK(VerifyCandidates(candidates).ok()); auto jsep_transport = GetJsepTransportByName(transport_name); if (!jsep_transport) { RTC_LOG(LS_WARNING) << "Not adding candidate because the JsepTransport " diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 15bfefee81..0411ab2924 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -2521,10 +2521,68 @@ void PeerConnection::NoteUsageEvent(UsageEvent event) { usage_pattern_.NoteUsageEvent(event); } +// Asynchronously adds remote candidates on the network thread. +void PeerConnection::AddRemoteCandidate(const std::string& mid, + const cricket::Candidate& candidate) { + RTC_DCHECK_RUN_ON(signaling_thread()); + + network_thread()->PostTask(ToQueuedTask( + network_thread_safety_, [this, mid = mid, candidate = candidate] { + RTC_DCHECK_RUN_ON(network_thread()); + std::vector candidates = {candidate}; + RTCError error = + transport_controller_->AddRemoteCandidates(mid, candidates); + if (error.ok()) { + signaling_thread()->PostTask(ToQueuedTask( + signaling_thread_safety_.flag(), + [this, candidate = std::move(candidate)] { + ReportRemoteIceCandidateAdded(candidate); + // Candidates successfully submitted for checking. + if (ice_connection_state() == + PeerConnectionInterface::kIceConnectionNew || + ice_connection_state() == + PeerConnectionInterface::kIceConnectionDisconnected) { + // If state is New, then the session has just gotten its first + // remote ICE candidates, so go to Checking. If state is + // Disconnected, the session is re-using old candidates or + // receiving additional ones, so go to Checking. If state is + // Connected, stay Connected. + // TODO(bemasc): If state is Connected, and the new candidates + // are for a newly added transport, then the state actually + // _should_ move to checking. Add a way to distinguish that + // case. + SetIceConnectionState( + PeerConnectionInterface::kIceConnectionChecking); + } + // TODO(bemasc): If state is Completed, go back to Connected. + })); + } else { + RTC_LOG(LS_WARNING) << error.message(); + } + })); +} + void PeerConnection::ReportUsagePattern() const { usage_pattern_.ReportUsagePattern(observer_); } +void PeerConnection::ReportRemoteIceCandidateAdded( + const cricket::Candidate& candidate) { + RTC_DCHECK_RUN_ON(signaling_thread()); + + NoteUsageEvent(UsageEvent::REMOTE_CANDIDATE_ADDED); + + if (candidate.address().IsPrivateIP()) { + NoteUsageEvent(UsageEvent::REMOTE_PRIVATE_CANDIDATE_ADDED); + } + if (candidate.address().IsUnresolvedIP()) { + NoteUsageEvent(UsageEvent::REMOTE_MDNS_CANDIDATE_ADDED); + } + if (candidate.address().family() == AF_INET6) { + NoteUsageEvent(UsageEvent::REMOTE_IPV6_CANDIDATE_ADDED); + } +} + bool PeerConnection::SrtpRequired() const { return (dtls_enabled_ || sdp_handler_->webrtc_session_desc_factory()->SdesPolicy() == diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 6833c58c68..fc0832468a 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -380,6 +380,10 @@ class PeerConnection : public PeerConnectionInternal, void SetIceConnectionState(IceConnectionState new_state); void NoteUsageEvent(UsageEvent event); + // Asynchronously adds a remote candidate on the network thread. + void AddRemoteCandidate(const std::string& mid, + const cricket::Candidate& candidate); + // Report the UMA metric SdpFormatReceived for the given remote description. void ReportSdpFormatReceived( const SessionDescriptionInterface& remote_description); @@ -502,10 +506,8 @@ class PeerConnection : public PeerConnectionInternal, const cricket::CandidatePairChangeEvent& event) RTC_RUN_ON(signaling_thread()); - void OnNegotiationNeeded(); - // Returns the specified SCTP DataChannel in sctp_data_channels_, // or nullptr if not found. SctpDataChannel* FindDataChannelBySid(int sid) const @@ -590,6 +592,8 @@ class PeerConnection : public PeerConnectionInternal, void ReportUsagePattern() const RTC_RUN_ON(signaling_thread()); + void ReportRemoteIceCandidateAdded(const cricket::Candidate& candidate); + // JsepTransportController::Observer override. // // Called by |transport_controller_| when processing transport information diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc index 8c1a764398..f37943f8e4 100644 --- a/pc/peer_connection_ice_unittest.cc +++ b/pc/peer_connection_ice_unittest.cc @@ -497,6 +497,21 @@ TEST_P(PeerConnectionIceTest, DuplicateIceCandidateIgnoredWhenAdded) { EXPECT_EQ(1u, caller->GetIceCandidatesFromRemoteDescription().size()); } +TEST_P(PeerConnectionIceTest, ErrorOnInvalidRemoteIceCandidateAdded) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + // Add a candidate to the remote description with a candidate that has an + // invalid address (port number == 2). + auto answer = callee->CreateAnswerAndSetAsLocal(); + cricket::Candidate bad_candidate = + CreateLocalUdpCandidate(SocketAddress("2.2.2.2", 2)); + RTC_LOG(LS_INFO) << "Bad candidate: " << bad_candidate.ToString(); + AddCandidateToFirstTransport(&bad_candidate, answer.get()); + // Now the call to SetRemoteDescription should fail. + EXPECT_FALSE(caller->SetRemoteDescription(std::move(answer))); +} + TEST_P(PeerConnectionIceTest, CannotRemoveIceCandidatesWhenPeerConnectionClosed) { const SocketAddress kCalleeAddress("1.1.1.1", 1111); diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 8588ca8dbf..b0a594b66a 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -4468,40 +4468,21 @@ bool SdpOfferAnswerHandler::UseCandidatesInSessionDescription( bool SdpOfferAnswerHandler::UseCandidate( const IceCandidateInterface* candidate) { RTC_DCHECK_RUN_ON(signaling_thread()); + + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + RTCErrorOr result = FindContentInfo(remote_description(), candidate); - if (!result.ok()) { - RTC_LOG(LS_ERROR) << "UseCandidate: Invalid candidate. " - << result.error().message(); + if (!result.ok()) return false; - } - std::vector candidates; - candidates.push_back(candidate->candidate()); - // Invoking BaseSession method to handle remote candidates. - RTCError error = transport_controller()->AddRemoteCandidates( - result.value()->name, candidates); - if (error.ok()) { - ReportRemoteIceCandidateAdded(candidate->candidate()); - // Candidates successfully submitted for checking. - if (pc_->ice_connection_state() == - PeerConnectionInterface::kIceConnectionNew || - pc_->ice_connection_state() == - PeerConnectionInterface::kIceConnectionDisconnected) { - // If state is New, then the session has just gotten its first remote ICE - // candidates, so go to Checking. - // If state is Disconnected, the session is re-using old candidates or - // receiving additional ones, so go to Checking. - // If state is Connected, stay Connected. - // TODO(bemasc): If state is Connected, and the new candidates are for a - // newly added transport, then the state actually _should_ move to - // checking. Add a way to distinguish that case. - pc_->SetIceConnectionState( - PeerConnectionInterface::kIceConnectionChecking); - } - // TODO(bemasc): If state is Completed, go back to Connected. - } else { - RTC_LOG(LS_WARNING) << error.message(); - } + + const cricket::Candidate& c = candidate->candidate(); + RTCError error = cricket::VerifyCandidate(c); + if (!error.ok()) + return false; + + pc_->AddRemoteCandidate(result.value()->name, c); + return true; } @@ -4546,20 +4527,6 @@ bool SdpOfferAnswerHandler::ReadyToUseRemoteCandidate( return has_transport; } -void SdpOfferAnswerHandler::ReportRemoteIceCandidateAdded( - const cricket::Candidate& candidate) { - pc_->NoteUsageEvent(UsageEvent::REMOTE_CANDIDATE_ADDED); - if (candidate.address().IsPrivateIP()) { - pc_->NoteUsageEvent(UsageEvent::REMOTE_PRIVATE_CANDIDATE_ADDED); - } - if (candidate.address().IsUnresolvedIP()) { - pc_->NoteUsageEvent(UsageEvent::REMOTE_MDNS_CANDIDATE_ADDED); - } - if (candidate.address().family() == AF_INET6) { - pc_->NoteUsageEvent(UsageEvent::REMOTE_IPV6_CANDIDATE_ADDED); - } -} - RTCErrorOr SdpOfferAnswerHandler::FindContentInfo( const SessionDescriptionInterface* description, const IceCandidateInterface* candidate) { diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index 2d2a529175..4f9490f517 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -494,8 +494,6 @@ class SdpOfferAnswerHandler : public SdpStateProvider, bool ReadyToUseRemoteCandidate(const IceCandidateInterface* candidate, const SessionDescriptionInterface* remote_desc, bool* valid); - void ReportRemoteIceCandidateAdded(const cricket::Candidate& candidate) - RTC_RUN_ON(signaling_thread()); RTCErrorOr FindContentInfo( const SessionDescriptionInterface* description,