Enforce "comprehension-required" STUN rules.
If a STUN attribute is in the "comprehension-required" range
(0x0000-0x7FFF), and the implementation does not recognize it, this
should be treated as an error (as per RFC5389), with different behavior
depending on the type of the message received.
Bug: webrtc:9063
Change-Id: Ic31b0cdd3c26772c21d770b44fe4ee4a1b47030a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/64500
Commit-Queue: Taylor <deadbeef@webrtc.org>
Reviewed-by: Jonas Oreland <jonaso@webrtc.org>
Reviewed-by: Qingsi Wang <qingsi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30925}
diff --git a/api/transport/stun.cc b/api/transport/stun.cc
index 5ed4900..b083f15 100644
--- a/api/transport/stun.cc
+++ b/api/transport/stun.cc
@@ -47,6 +47,7 @@
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 @@
length_ = 0;
}
+std::vector<uint16_t> StunMessage::GetNonComprehendedAttributes() const {
+ std::vector<uint16_t> 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 41f76a1..51ca306 100644
--- a/api/transport/stun.h
+++ b/api/transport/stun.h
@@ -163,6 +163,10 @@
void SetType(int type) { type_ = static_cast<uint16_t>(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<uint16_t> 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 a6eb333..0f2b2c6 100644
--- a/p2p/base/port.cc
+++ b/p2p/base/port.cc
@@ -469,6 +469,12 @@
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<uint16_t> 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 @@
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 @@
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 @@
<< 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 @@
<< addr.ToSensitiveString();
}
+void Port::SendUnknownAttributesErrorResponse(
+ StunMessage* request,
+ const rtc::SocketAddress& addr,
+ const std::vector<uint16_t>& 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<StunUInt16ListAttribute> 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 4200bed..bf1c041 100644
--- a/p2p/base/port.h
+++ b/p2p/base/port.h
@@ -295,6 +295,10 @@
const rtc::SocketAddress& addr,
int error_code,
const std::string& reason) override;
+ void SendUnknownAttributesErrorResponse(
+ StunMessage* request,
+ const rtc::SocketAddress& addr,
+ const std::vector<uint16_t>& 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 eaa2545..a7ac1fa 100644
--- a/p2p/base/port_unittest.cc
+++ b/p2p/base/port_unittest.cc
@@ -2222,6 +2222,110 @@
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<TestPort> port(CreateTestPort(kLocalAddr2, "rfrag", "rpass"));
+
+ std::unique_ptr<IceMessage> in_msg, out_msg;
+ auto buf = std::make_unique<ByteBufferWriter>();
+ 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<char>(),
+ 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 b4dba7d..d7c2336 100644
--- a/p2p/base/stun_request.cc
+++ b/p2p/base/stun_request.cc
@@ -125,7 +125,15 @@
}
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 1f48c19..ce573f0 100644
--- a/p2p/base/stun_request_unittest.cc
+++ b/p2p/base/stun_request_unittest.cc
@@ -198,4 +198,22 @@
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
