diff --git a/api/transport/stun.cc b/api/transport/stun.cc index 5ed4900088..b083f15834 100644 --- a/api/transport/stun.cc +++ b/api/transport/stun.cc @@ -47,6 +47,7 @@ namespace cricket { const char STUN_ERROR_REASON_TRY_ALTERNATE_SERVER[] = "Try Alternate Server"; const char STUN_ERROR_REASON_BAD_REQUEST[] = "Bad Request"; const char STUN_ERROR_REASON_UNAUTHORIZED[] = "Unauthorized"; +const char STUN_ERROR_REASON_UNKNOWN_ATTRIBUTE[] = "Unknown Attribute"; const char STUN_ERROR_REASON_FORBIDDEN[] = "Forbidden"; const char STUN_ERROR_REASON_STALE_CREDENTIALS[] = "Stale Credentials"; const char STUN_ERROR_REASON_ALLOCATION_MISMATCH[] = "Allocation Mismatch"; @@ -140,6 +141,18 @@ void StunMessage::ClearAttributes() { length_ = 0; } +std::vector StunMessage::GetNonComprehendedAttributes() const { + std::vector unknown_attributes; + for (auto& attr : attrs_) { + // "comprehension-required" range is 0x0000-0x7FFF. + if (attr->type() >= 0x0000 && attr->type() <= 0x7FFF && + GetAttributeValueType(attr->type()) == STUN_VALUE_UNKNOWN) { + unknown_attributes.push_back(attr->type()); + } + } + return unknown_attributes; +} + const StunAddressAttribute* StunMessage::GetAddress(int type) const { switch (type) { case STUN_ATTR_MAPPED_ADDRESS: { diff --git a/api/transport/stun.h b/api/transport/stun.h index 41f76a1ba7..51ca30653c 100644 --- a/api/transport/stun.h +++ b/api/transport/stun.h @@ -163,6 +163,10 @@ class StunMessage { void SetType(int type) { type_ = static_cast(type); } bool SetTransactionID(const std::string& str); + // Get a list of all of the attribute types in the "comprehension required" + // range that were not recognized. + std::vector GetNonComprehendedAttributes() const; + // Gets the desired attribute value, or NULL if no such attribute type exists. const StunAddressAttribute* GetAddress(int type) const; const StunUInt32Attribute* GetUInt32(int type) const; diff --git a/p2p/base/port.cc b/p2p/base/port.cc index a6eb333923..0f2b2c668b 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc @@ -469,6 +469,12 @@ bool Port::GetStunMessage(const char* data, return false; } + // Get list of attributes in the "comprehension-required" range that were not + // comprehended. If one or more is found, the behavior differs based on the + // type of the incoming message; see below. + std::vector unknown_attributes = + stun_msg->GetNonComprehendedAttributes(); + if (stun_msg->type() == STUN_BINDING_REQUEST) { // Check for the presence of USERNAME and MESSAGE-INTEGRITY (if ICE) first. // If not present, fail with a 400 Bad Request. @@ -507,6 +513,15 @@ bool Port::GetStunMessage(const char* data, STUN_ERROR_REASON_UNAUTHORIZED); return true; } + + // If a request contains unknown comprehension-required attributes, reply + // with an error. See RFC5389 section 7.3.1. + if (!unknown_attributes.empty()) { + SendUnknownAttributesErrorResponse(stun_msg.get(), addr, + unknown_attributes); + return true; + } + out_username->assign(remote_ufrag); } else if ((stun_msg->type() == STUN_BINDING_RESPONSE) || (stun_msg->type() == STUN_BINDING_ERROR_RESPONSE)) { @@ -527,6 +542,15 @@ bool Port::GetStunMessage(const char* data, return true; } } + // If a response contains unknown comprehension-required attributes, it's + // simply discarded and the transaction is considered failed. See RFC5389 + // sections 7.3.3 and 7.3.4. + if (!unknown_attributes.empty()) { + RTC_LOG(LS_ERROR) << ToString() + << ": Discarding STUN response due to unknown " + "comprehension-required attribute"; + return true; + } // NOTE: Username should not be used in verifying response messages. out_username->clear(); } else if (stun_msg->type() == STUN_BINDING_INDICATION) { @@ -534,6 +558,15 @@ bool Port::GetStunMessage(const char* data, << StunMethodToString(stun_msg->type()) << ": from " << addr.ToSensitiveString(); out_username->clear(); + + // If an indication contains unknown comprehension-required attributes,[] + // it's simply discarded. See RFC5389 section 7.3.2. + if (!unknown_attributes.empty()) { + RTC_LOG(LS_ERROR) << ToString() + << ": Discarding STUN indication due to " + "unknown comprehension-required attribute"; + return true; + } // No stun attributes will be verified, if it's stun indication message. // Returning from end of the this method. } else if (stun_msg->type() == GOOG_PING_REQUEST) { @@ -749,6 +782,44 @@ void Port::SendBindingErrorResponse(StunMessage* request, << addr.ToSensitiveString(); } +void Port::SendUnknownAttributesErrorResponse( + StunMessage* request, + const rtc::SocketAddress& addr, + const std::vector& unknown_types) { + RTC_DCHECK(request->type() == STUN_BINDING_REQUEST); + + // Fill in the response message. + StunMessage response; + response.SetType(STUN_BINDING_ERROR_RESPONSE); + response.SetTransactionID(request->transaction_id()); + + auto error_attr = StunAttribute::CreateErrorCode(); + error_attr->SetCode(STUN_ERROR_UNKNOWN_ATTRIBUTE); + error_attr->SetReason(STUN_ERROR_REASON_UNKNOWN_ATTRIBUTE); + response.AddAttribute(std::move(error_attr)); + + std::unique_ptr unknown_attr = + StunAttribute::CreateUnknownAttributes(); + for (uint16_t type : unknown_types) { + unknown_attr->AddType(type); + } + response.AddAttribute(std::move(unknown_attr)); + + response.AddMessageIntegrity(password_); + response.AddFingerprint(); + + // Send the response message. + rtc::ByteBufferWriter buf; + response.Write(&buf); + rtc::PacketOptions options(StunDscpValue()); + options.info_signaled_after_sent.packet_type = + rtc::PacketType::kIceConnectivityCheckResponse; + SendTo(buf.Data(), buf.Length(), addr, options, false); + RTC_LOG(LS_ERROR) << ToString() << ": Sending STUN binding error: reason=" + << STUN_ERROR_UNKNOWN_ATTRIBUTE << " to " + << addr.ToSensitiveString(); +} + void Port::KeepAliveUntilPruned() { // If it is pruned, we won't bring it up again. if (state_ == State::INIT) { diff --git a/p2p/base/port.h b/p2p/base/port.h index 4200bed096..bf1c041423 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h @@ -295,6 +295,10 @@ class Port : public PortInterface, const rtc::SocketAddress& addr, int error_code, const std::string& reason) override; + void SendUnknownAttributesErrorResponse( + StunMessage* request, + const rtc::SocketAddress& addr, + const std::vector& unknown_types); void set_proxy(const std::string& user_agent, const rtc::ProxyInfo& proxy) { user_agent_ = user_agent; diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc index eaa2545ee9..a7ac1fafdb 100644 --- a/p2p/base/port_unittest.cc +++ b/p2p/base/port_unittest.cc @@ -2222,6 +2222,110 @@ TEST_F(PortTest, TestHandleStunMessageBadFingerprint) { EXPECT_EQ(0, port->last_stun_error_code()); } +// Test handling a STUN message with unknown attributes in the +// "comprehension-required" range. Should respond with an error with the +// unknown attributes' IDs. +TEST_F(PortTest, + TestHandleStunRequestWithUnknownComprehensionRequiredAttribute) { + // Our port will act as the "remote" port. + std::unique_ptr port(CreateTestPort(kLocalAddr2, "rfrag", "rpass")); + + std::unique_ptr in_msg, out_msg; + auto buf = std::make_unique(); + rtc::SocketAddress addr(kLocalAddr1); + std::string username; + + // Build ordinary message with valid ufrag/pass. + in_msg = CreateStunMessageWithUsername(STUN_BINDING_REQUEST, "rfrag:lfrag"); + in_msg->AddMessageIntegrity("rpass"); + // Add a couple attributes with ID in comprehension-required range. + in_msg->AddAttribute(StunAttribute::CreateUInt32(0x7777)); + in_msg->AddAttribute(StunAttribute::CreateUInt32(0x4567)); + // ... And one outside the range. + in_msg->AddAttribute(StunAttribute::CreateUInt32(0xdead)); + in_msg->AddFingerprint(); + WriteStunMessage(*in_msg, buf.get()); + ASSERT_TRUE(port->GetStunMessage(buf->Data(), buf->Length(), addr, &out_msg, + &username)); + IceMessage* error_response = port->last_stun_msg(); + ASSERT_NE(nullptr, error_response); + + // Verify that the "unknown attribute" error response has the right error + // code, and includes an attribute that lists out the unrecognized attribute + // types. + EXPECT_EQ(STUN_ERROR_UNKNOWN_ATTRIBUTE, error_response->GetErrorCodeValue()); + const StunUInt16ListAttribute* unknown_attributes = + error_response->GetUnknownAttributes(); + ASSERT_NE(nullptr, unknown_attributes); + ASSERT_EQ(2u, unknown_attributes->Size()); + EXPECT_EQ(0x7777, unknown_attributes->GetType(0)); + EXPECT_EQ(0x4567, unknown_attributes->GetType(1)); +} + +// Similar to the above, but with a response instead of a request. In this +// case the response should just be ignored and transaction treated is failed. +TEST_F(PortTest, + TestHandleStunResponseWithUnknownComprehensionRequiredAttribute) { + // Generic setup. + auto lport = CreateTestPort(kLocalAddr1, "lfrag", "lpass"); + lport->SetIceRole(cricket::ICEROLE_CONTROLLING); + auto rport = CreateTestPort(kLocalAddr2, "rfrag", "rpass"); + rport->SetIceRole(cricket::ICEROLE_CONTROLLED); + lport->PrepareAddress(); + rport->PrepareAddress(); + ASSERT_FALSE(lport->Candidates().empty()); + ASSERT_FALSE(rport->Candidates().empty()); + Connection* lconn = + lport->CreateConnection(rport->Candidates()[0], Port::ORIGIN_MESSAGE); + Connection* rconn = + rport->CreateConnection(lport->Candidates()[0], Port::ORIGIN_MESSAGE); + + // Send request. + lconn->Ping(0); + ASSERT_TRUE_WAIT(lport->last_stun_msg() != NULL, kDefaultTimeout); + rconn->OnReadPacket(lport->last_stun_buf()->data(), + lport->last_stun_buf()->size(), /* packet_time_us */ -1); + + // Intercept request and add comprehension required attribute. + ASSERT_TRUE_WAIT(rport->last_stun_msg() != NULL, kDefaultTimeout); + auto modified_response = rport->last_stun_msg()->Clone(); + modified_response->AddAttribute(StunAttribute::CreateUInt32(0x7777)); + modified_response->RemoveAttribute(STUN_ATTR_FINGERPRINT); + modified_response->AddFingerprint(); + ByteBufferWriter buf; + WriteStunMessage(*modified_response, &buf); + lconn->OnReadPacket(buf.Data(), buf.Length(), /* packet_time_us */ -1); + // Response should have been ignored, leaving us unwritable still. + EXPECT_FALSE(lconn->writable()); +} + +// Similar to the above, but with an indication. As with a response, it should +// just be ignored. +TEST_F(PortTest, + TestHandleStunIndicationWithUnknownComprehensionRequiredAttribute) { + // Generic set up. + auto lport = CreateTestPort(kLocalAddr2, "lfrag", "lpass"); + lport->SetIceRole(cricket::ICEROLE_CONTROLLING); + auto rport = CreateTestPort(kLocalAddr2, "rfrag", "rpass"); + rport->SetIceRole(cricket::ICEROLE_CONTROLLED); + lport->PrepareAddress(); + rport->PrepareAddress(); + ASSERT_FALSE(lport->Candidates().empty()); + ASSERT_FALSE(rport->Candidates().empty()); + Connection* lconn = + lport->CreateConnection(rport->Candidates()[0], Port::ORIGIN_MESSAGE); + + // Generate indication with comprehension required attribute and verify it + // doesn't update last_ping_received. + auto in_msg = CreateStunMessage(STUN_BINDING_INDICATION); + in_msg->AddAttribute(StunAttribute::CreateUInt32(0x7777)); + in_msg->AddFingerprint(); + ByteBufferWriter buf; + WriteStunMessage(*in_msg, &buf); + lconn->OnReadPacket(buf.Data(), buf.Length(), /* packet_time_us */ -1); + EXPECT_EQ(0u, lconn->last_ping_received()); +} + // Test handling of STUN binding indication messages . STUN binding // indications are allowed only to the connection which is in read mode. TEST_F(PortTest, TestHandleStunBindingIndication) { diff --git a/p2p/base/stun_request.cc b/p2p/base/stun_request.cc index b4dba7d3a0..d7c233617e 100644 --- a/p2p/base/stun_request.cc +++ b/p2p/base/stun_request.cc @@ -125,7 +125,15 @@ bool StunRequestManager::CheckResponse(StunMessage* msg) { } StunRequest* request = iter->second; - if (msg->type() == GetStunSuccessResponseType(request->type())) { + if (!msg->GetNonComprehendedAttributes().empty()) { + // If a response contains unknown comprehension-required attributes, it's + // simply discarded and the transaction is considered failed. See RFC5389 + // sections 7.3.3 and 7.3.4. + RTC_LOG(LS_ERROR) << ": Discarding response due to unknown " + "comprehension-required attribute."; + delete request; + return false; + } else if (msg->type() == GetStunSuccessResponseType(request->type())) { request->OnResponse(msg); } else if (msg->type() == GetStunErrorResponseType(request->type())) { request->OnErrorResponse(msg); diff --git a/p2p/base/stun_request_unittest.cc b/p2p/base/stun_request_unittest.cc index 1f48c19ad7..ce573f087d 100644 --- a/p2p/base/stun_request_unittest.cc +++ b/p2p/base/stun_request_unittest.cc @@ -198,4 +198,22 @@ TEST_F(StunRequestTest, TestNoEmptyRequest) { delete res; } +// If the response contains an attribute in the "comprehension required" range +// which is not recognized, the transaction should be considered a failure and +// the response should be ignored. +TEST_F(StunRequestTest, TestUnrecognizedComprehensionRequiredAttribute) { + StunMessage* req = CreateStunMessage(STUN_BINDING_REQUEST, NULL); + + manager_.Send(new StunRequestThunker(req, this)); + StunMessage* res = CreateStunMessage(STUN_BINDING_ERROR_RESPONSE, req); + res->AddAttribute(StunAttribute::CreateUInt32(0x7777)); + EXPECT_FALSE(manager_.CheckResponse(res)); + + EXPECT_EQ(nullptr, response_); + EXPECT_FALSE(success_); + EXPECT_FALSE(failure_); + EXPECT_FALSE(timeout_); + delete res; +} + } // namespace cricket