diff --git a/api/transport/stun.cc b/api/transport/stun.cc index e1bf03be62..1b5bf0c409 100644 --- a/api/transport/stun.cc +++ b/api/transport/stun.cc @@ -246,6 +246,31 @@ const StunUInt16ListAttribute* StunMessage::GetUnknownAttributes() const { GetAttribute(STUN_ATTR_UNKNOWN_ATTRIBUTES)); } +StunMessage::IntegrityStatus StunMessage::ValidateMessageIntegrity( + const std::string& password) { + password_ = password; + if (GetByteString(STUN_ATTR_MESSAGE_INTEGRITY)) { + if (ValidateMessageIntegrityOfType( + STUN_ATTR_MESSAGE_INTEGRITY, kStunMessageIntegritySize, + buffer_.c_str(), buffer_.size(), password)) { + integrity_ = IntegrityStatus::kIntegrityOk; + } else { + integrity_ = IntegrityStatus::kIntegrityBad; + } + } else if (GetByteString(STUN_ATTR_GOOG_MESSAGE_INTEGRITY_32)) { + if (ValidateMessageIntegrityOfType( + STUN_ATTR_GOOG_MESSAGE_INTEGRITY_32, kStunMessageIntegrity32Size, + buffer_.c_str(), buffer_.size(), password)) { + integrity_ = IntegrityStatus::kIntegrityOk; + } else { + integrity_ = IntegrityStatus::kIntegrityBad; + } + } else { + integrity_ = IntegrityStatus::kNoIntegrity; + } + return integrity_; +} + bool StunMessage::ValidateMessageIntegrity(const char* data, size_t size, const std::string& password) { @@ -353,11 +378,6 @@ bool StunMessage::AddMessageIntegrity(const std::string& password) { password.size()); } -bool StunMessage::AddMessageIntegrity(const char* key, size_t keylen) { - return AddMessageIntegrityOfType(STUN_ATTR_MESSAGE_INTEGRITY, - kStunMessageIntegritySize, key, keylen); -} - bool StunMessage::AddMessageIntegrity32(absl::string_view password) { return AddMessageIntegrityOfType(STUN_ATTR_GOOG_MESSAGE_INTEGRITY_32, kStunMessageIntegrity32Size, password.data(), @@ -395,6 +415,8 @@ bool StunMessage::AddMessageIntegrityOfType(int attr_type, // Insert correct HMAC into the attribute. msg_integrity_attr->CopyBytes(hmac, attr_size); + password_.assign(key, keylen); + integrity_ = IntegrityStatus::kIntegrityOk; return true; } @@ -473,6 +495,9 @@ bool StunMessage::AddFingerprint() { } bool StunMessage::Read(ByteBufferReader* buf) { + // Keep a copy of the buffer data around for later verification. + buffer_.assign(buf->Data(), buf->Length()); + if (!buf->ReadUInt16(&type_)) { return false; } diff --git a/api/transport/stun.h b/api/transport/stun.h index 8893b2a1ff..682a17a945 100644 --- a/api/transport/stun.h +++ b/api/transport/stun.h @@ -16,6 +16,7 @@ #include #include + #include #include #include @@ -149,15 +150,24 @@ class StunMessage { StunMessage(); virtual ~StunMessage(); + // The verification status of the message. This is checked on parsing, + // or set by AddMessageIntegrity. + enum class IntegrityStatus { + kNotSet, + kNoIntegrity, // Message-integrity attribute missing + kIntegrityOk, // Message-integrity checked OK + kIntegrityBad, // Message-integrity verification failed + }; + int type() const { return type_; } size_t length() const { return length_; } const std::string& transaction_id() const { return transaction_id_; } uint32_t reduced_transaction_id() const { return reduced_transaction_id_; } // Returns true if the message confirms to RFC3489 rather than - // RFC5389. The main difference between two version of the STUN + // RFC5389. The main difference between the two versions of the STUN // protocol is the presence of the magic cookie and different length - // of transaction ID. For outgoing packets version of the protocol + // of transaction ID. For outgoing packets the version of the protocol // is determined by the lengths of the transaction ID. bool IsLegacy() const; @@ -191,19 +201,27 @@ class StunMessage { // Remote all attributes and releases them. void ClearAttributes(); - // Validates that a raw STUN message has a correct MESSAGE-INTEGRITY value. - // This can't currently be done on a StunMessage, since it is affected by - // padding data (which we discard when reading a StunMessage). - static bool ValidateMessageIntegrity(const char* data, - size_t size, - const std::string& password); - static bool ValidateMessageIntegrity32(const char* data, - size_t size, - const std::string& password); + // Validates that a STUN message has a correct MESSAGE-INTEGRITY value. + // This uses the buffered raw-format message stored by Read(). + IntegrityStatus ValidateMessageIntegrity(const std::string& password); + + // Returns the current integrity status of the message. + IntegrityStatus integrity() const { return integrity_; } + + // Shortcut for checking if integrity is verified. + bool IntegrityOk() const { + return integrity_ == IntegrityStatus::kIntegrityOk; + } + + // Returns the password attribute used to set or check the integrity. + // Can only be called after adding or checking the integrity. + std::string password() const { + RTC_DCHECK(integrity_ != IntegrityStatus::kNotSet); + return password_; + } // Adds a MESSAGE-INTEGRITY attribute that is valid for the current message. bool AddMessageIntegrity(const std::string& password); - bool AddMessageIntegrity(const char* key, size_t keylen); // Adds a STUN_ATTR_GOOG_MESSAGE_INTEGRITY_32 attribute that is valid for the // current message. @@ -244,6 +262,30 @@ class StunMessage { bool EqualAttributes(const StunMessage* other, std::function attribute_type_mask) const; + // Expose raw-buffer ValidateMessageIntegrity function for testing. + static bool ValidateMessageIntegrityForTesting(const char* data, + size_t size, + const std::string& password) { + return ValidateMessageIntegrity(data, size, password); + } + // Expose raw-buffer ValidateMessageIntegrity function for testing. + static bool ValidateMessageIntegrity32ForTesting( + const char* data, + size_t size, + const std::string& password) { + return ValidateMessageIntegrity32(data, size, password); + } + // Validates that a STUN message in byte buffer form + // has a correct MESSAGE-INTEGRITY value. + // These functions are not recommended and will be deprecated; use + // ValidateMessageIntegrity(password) on the parsed form instead. + static bool ValidateMessageIntegrity(const char* data, + size_t size, + const std::string& password); + static bool ValidateMessageIntegrity32(const char* data, + size_t size, + const std::string& password); + protected: // Verifies that the given attribute is allowed for this message. virtual StunAttributeValueType GetAttributeValueType(int type) const; @@ -269,6 +311,10 @@ class StunMessage { std::string transaction_id_; uint32_t reduced_transaction_id_; uint32_t stun_magic_cookie_; + // The original buffer for messages created by Read(). + std::string buffer_; + IntegrityStatus integrity_ = IntegrityStatus::kNotSet; + std::string password_; }; // Base class for all STUN/TURN attributes. diff --git a/api/transport/stun_unittest.cc b/api/transport/stun_unittest.cc index bf2717e007..bf791f257d 100644 --- a/api/transport/stun_unittest.cc +++ b/api/transport/stun_unittest.cc @@ -1196,24 +1196,24 @@ TEST_F(StunTest, FailToReadRtcpPacket) { // Check our STUN message validation code against the RFC5769 test messages. TEST_F(StunTest, ValidateMessageIntegrity) { // Try the messages from RFC 5769. - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity( + EXPECT_TRUE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kRfc5769SampleRequest), sizeof(kRfc5769SampleRequest), kRfc5769SampleMsgPassword)); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kRfc5769SampleRequest), sizeof(kRfc5769SampleRequest), "InvalidPassword")); - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity( + EXPECT_TRUE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kRfc5769SampleResponse), sizeof(kRfc5769SampleResponse), kRfc5769SampleMsgPassword)); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kRfc5769SampleResponse), sizeof(kRfc5769SampleResponse), "InvalidPassword")); - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity( + EXPECT_TRUE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kRfc5769SampleResponseIPv6), sizeof(kRfc5769SampleResponseIPv6), kRfc5769SampleMsgPassword)); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kRfc5769SampleResponseIPv6), sizeof(kRfc5769SampleResponseIPv6), "InvalidPassword")); @@ -1222,40 +1222,40 @@ TEST_F(StunTest, ValidateMessageIntegrity) { ComputeStunCredentialHash(kRfc5769SampleMsgWithAuthUsername, kRfc5769SampleMsgWithAuthRealm, kRfc5769SampleMsgWithAuthPassword, &key); - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity( + EXPECT_TRUE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kRfc5769SampleRequestLongTermAuth), sizeof(kRfc5769SampleRequestLongTermAuth), key)); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kRfc5769SampleRequestLongTermAuth), sizeof(kRfc5769SampleRequestLongTermAuth), "InvalidPassword")); // Try some edge cases. - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kStunMessageWithZeroLength), sizeof(kStunMessageWithZeroLength), kRfc5769SampleMsgPassword)); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kStunMessageWithExcessLength), sizeof(kStunMessageWithExcessLength), kRfc5769SampleMsgPassword)); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kStunMessageWithSmallLength), sizeof(kStunMessageWithSmallLength), kRfc5769SampleMsgPassword)); // Again, but with the lengths matching what is claimed in the headers. - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kStunMessageWithZeroLength), kStunHeaderSize + rtc::GetBE16(&kStunMessageWithZeroLength[2]), kRfc5769SampleMsgPassword)); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kStunMessageWithExcessLength), kStunHeaderSize + rtc::GetBE16(&kStunMessageWithExcessLength[2]), kRfc5769SampleMsgPassword)); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kStunMessageWithSmallLength), kStunHeaderSize + rtc::GetBE16(&kStunMessageWithSmallLength[2]), kRfc5769SampleMsgPassword)); // Check that a too-short HMAC doesn't cause buffer overflow. - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(kStunMessageWithBadHmacAtEnd), sizeof(kStunMessageWithBadHmacAtEnd), kRfc5769SampleMsgPassword)); @@ -1268,8 +1268,8 @@ TEST_F(StunTest, ValidateMessageIntegrity) { if (i > 0) buf[i - 1] ^= 0x01; EXPECT_EQ(i >= sizeof(buf) - 8, - StunMessage::ValidateMessageIntegrity(buf, sizeof(buf), - kRfc5769SampleMsgPassword)); + StunMessage::ValidateMessageIntegrityForTesting( + buf, sizeof(buf), kRfc5769SampleMsgPassword)); } } @@ -1291,7 +1291,7 @@ TEST_F(StunTest, AddMessageIntegrity) { rtc::ByteBufferWriter buf1; EXPECT_TRUE(msg.Write(&buf1)); - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity( + EXPECT_TRUE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(buf1.Data()), buf1.Length(), kRfc5769SampleMsgPassword)); @@ -1309,7 +1309,7 @@ TEST_F(StunTest, AddMessageIntegrity) { rtc::ByteBufferWriter buf3; EXPECT_TRUE(msg2.Write(&buf3)); - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity( + EXPECT_TRUE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(buf3.Data()), buf3.Length(), kRfc5769SampleMsgPassword)); } @@ -1317,40 +1317,40 @@ TEST_F(StunTest, AddMessageIntegrity) { // Check our STUN message validation code against the RFC5769 test messages. TEST_F(StunTest, ValidateMessageIntegrity32) { // Try the messages from RFC 5769. - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity32( + EXPECT_TRUE(StunMessage::ValidateMessageIntegrity32ForTesting( reinterpret_cast(kSampleRequestMI32), sizeof(kSampleRequestMI32), kRfc5769SampleMsgPassword)); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32ForTesting( reinterpret_cast(kSampleRequestMI32), sizeof(kSampleRequestMI32), "InvalidPassword")); // Try some edge cases. - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32ForTesting( reinterpret_cast(kStunMessageWithZeroLength), sizeof(kStunMessageWithZeroLength), kRfc5769SampleMsgPassword)); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32ForTesting( reinterpret_cast(kStunMessageWithExcessLength), sizeof(kStunMessageWithExcessLength), kRfc5769SampleMsgPassword)); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32ForTesting( reinterpret_cast(kStunMessageWithSmallLength), sizeof(kStunMessageWithSmallLength), kRfc5769SampleMsgPassword)); // Again, but with the lengths matching what is claimed in the headers. - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32ForTesting( reinterpret_cast(kStunMessageWithZeroLength), kStunHeaderSize + rtc::GetBE16(&kStunMessageWithZeroLength[2]), kRfc5769SampleMsgPassword)); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32ForTesting( reinterpret_cast(kStunMessageWithExcessLength), kStunHeaderSize + rtc::GetBE16(&kStunMessageWithExcessLength[2]), kRfc5769SampleMsgPassword)); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32ForTesting( reinterpret_cast(kStunMessageWithSmallLength), kStunHeaderSize + rtc::GetBE16(&kStunMessageWithSmallLength[2]), kRfc5769SampleMsgPassword)); // Check that a too-short HMAC doesn't cause buffer overflow. - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32ForTesting( reinterpret_cast(kStunMessageWithBadHmacAtEnd), sizeof(kStunMessageWithBadHmacAtEnd), kRfc5769SampleMsgPassword)); @@ -1363,7 +1363,7 @@ TEST_F(StunTest, ValidateMessageIntegrity32) { if (i > 0) buf[i - 1] ^= 0x01; EXPECT_EQ(i >= sizeof(buf) - 8, - StunMessage::ValidateMessageIntegrity32( + StunMessage::ValidateMessageIntegrity32ForTesting( buf, sizeof(buf), kRfc5769SampleMsgPassword)); } } @@ -1384,7 +1384,7 @@ TEST_F(StunTest, AddMessageIntegrity32) { rtc::ByteBufferWriter buf1; EXPECT_TRUE(msg.Write(&buf1)); - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity32( + EXPECT_TRUE(StunMessage::ValidateMessageIntegrity32ForTesting( reinterpret_cast(buf1.Data()), buf1.Length(), kRfc5769SampleMsgPassword)); @@ -1402,7 +1402,7 @@ TEST_F(StunTest, AddMessageIntegrity32) { rtc::ByteBufferWriter buf3; EXPECT_TRUE(msg2.Write(&buf3)); - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity32( + EXPECT_TRUE(StunMessage::ValidateMessageIntegrity32ForTesting( reinterpret_cast(buf3.Data()), buf3.Length(), kRfc5769SampleMsgPassword)); } @@ -1420,14 +1420,14 @@ TEST_F(StunTest, AddMessageIntegrity32AndMessageIntegrity) { rtc::ByteBufferWriter buf1; EXPECT_TRUE(msg.Write(&buf1)); - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity32( + EXPECT_TRUE(StunMessage::ValidateMessageIntegrity32ForTesting( reinterpret_cast(buf1.Data()), buf1.Length(), "password1")); - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity( + EXPECT_TRUE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(buf1.Data()), buf1.Length(), "password2")); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrity32ForTesting( reinterpret_cast(buf1.Data()), buf1.Length(), "password2")); - EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + EXPECT_FALSE(StunMessage::ValidateMessageIntegrityForTesting( reinterpret_cast(buf1.Data()), buf1.Length(), "password1")); } diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index 8adfeb418d..0aa2bcbeff 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc @@ -480,6 +480,7 @@ void Connection::OnReadPacket(const char* data, // If this is a STUN response, then update the writable bit. // Log at LS_INFO if we receive a ping on an unwritable connection. rtc::LoggingSeverity sev = (!writable() ? rtc::LS_INFO : rtc::LS_VERBOSE); + msg->ValidateMessageIntegrity(remote_candidate().password()); switch (msg->type()) { case STUN_BINDING_REQUEST: RTC_LOG_V(sev) << ToString() << ": Received " @@ -505,8 +506,7 @@ void Connection::OnReadPacket(const char* data, // id's match. case STUN_BINDING_RESPONSE: case STUN_BINDING_ERROR_RESPONSE: - if (msg->ValidateMessageIntegrity(data, size, - remote_candidate().password())) { + if (msg->IntegrityOk()) { requests_.CheckResponse(msg.get()); } // Otherwise silently discard the response message. @@ -523,8 +523,7 @@ void Connection::OnReadPacket(const char* data, break; case GOOG_PING_RESPONSE: case GOOG_PING_ERROR_RESPONSE: - if (msg->ValidateMessageIntegrity32(data, size, - remote_candidate().password())) { + if (msg->IntegrityOk()) { requests_.CheckResponse(msg.get()); } break; diff --git a/p2p/base/port.cc b/p2p/base/port.cc index 436da59f5b..79b83b7f2e 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc @@ -493,7 +493,8 @@ bool Port::GetStunMessage(const char* data, } // If ICE, and the MESSAGE-INTEGRITY is bad, fail with a 401 Unauthorized - if (!stun_msg->ValidateMessageIntegrity(data, size, password_)) { + if (stun_msg->ValidateMessageIntegrity(password_) != + StunMessage::IntegrityStatus::kIntegrityOk) { RTC_LOG(LS_ERROR) << ToString() << ": Received " << StunMethodToString(stun_msg->type()) << " with bad M-I from " << addr.ToSensitiveString() @@ -559,7 +560,8 @@ bool Port::GetStunMessage(const char* data, // 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) { - if (!stun_msg->ValidateMessageIntegrity32(data, size, password_)) { + if (stun_msg->ValidateMessageIntegrity(password_) != + StunMessage::IntegrityStatus::kIntegrityOk) { RTC_LOG(LS_ERROR) << ToString() << ": Received " << StunMethodToString(stun_msg->type()) << " with bad M-I from " << addr.ToSensitiveString() diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc index bb5bfbdcd3..293a8d1f8b 100644 --- a/p2p/base/port_unittest.cc +++ b/p2p/base/port_unittest.cc @@ -1726,9 +1726,8 @@ TEST_F(PortTest, TestSendStunMessage) { EXPECT_EQ(kDefaultPrflxPriority, priority_attr->value()); EXPECT_EQ("rfrag:lfrag", username_attr->GetString()); EXPECT_TRUE(msg->GetByteString(STUN_ATTR_MESSAGE_INTEGRITY) != NULL); - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity( - lport->last_stun_buf()->data(), lport->last_stun_buf()->size(), - "rpass")); + EXPECT_EQ(StunMessage::IntegrityStatus::kIntegrityOk, + msg->ValidateMessageIntegrity("rpass")); const StunUInt64Attribute* ice_controlling_attr = msg->GetUInt64(STUN_ATTR_ICE_CONTROLLING); ASSERT_TRUE(ice_controlling_attr != NULL); @@ -1767,9 +1766,8 @@ TEST_F(PortTest, TestSendStunMessage) { ASSERT_TRUE(addr_attr != NULL); EXPECT_EQ(lport->Candidates()[0].address(), addr_attr->GetAddress()); EXPECT_TRUE(msg->GetByteString(STUN_ATTR_MESSAGE_INTEGRITY) != NULL); - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity( - rport->last_stun_buf()->data(), rport->last_stun_buf()->size(), - "rpass")); + EXPECT_EQ(StunMessage::IntegrityStatus::kIntegrityOk, + msg->ValidateMessageIntegrity("rpass")); EXPECT_TRUE(msg->GetUInt32(STUN_ATTR_FINGERPRINT) != NULL); EXPECT_TRUE(StunMessage::ValidateFingerprint( lport->last_stun_buf()->data(), lport->last_stun_buf()->size())); @@ -1798,9 +1796,8 @@ TEST_F(PortTest, TestSendStunMessage) { EXPECT_EQ(STUN_ERROR_SERVER_ERROR, error_attr->code()); EXPECT_EQ(std::string(STUN_ERROR_REASON_SERVER_ERROR), error_attr->reason()); EXPECT_TRUE(msg->GetByteString(STUN_ATTR_MESSAGE_INTEGRITY) != NULL); - EXPECT_TRUE(StunMessage::ValidateMessageIntegrity( - rport->last_stun_buf()->data(), rport->last_stun_buf()->size(), - "rpass")); + EXPECT_EQ(StunMessage::IntegrityStatus::kIntegrityOk, + msg->ValidateMessageIntegrity("rpass")); EXPECT_TRUE(msg->GetUInt32(STUN_ATTR_FINGERPRINT) != NULL); EXPECT_TRUE(StunMessage::ValidateFingerprint( lport->last_stun_buf()->data(), lport->last_stun_buf()->size())); diff --git a/p2p/base/stun_request.cc b/p2p/base/stun_request.cc index 44376ced95..2870dcdfc5 100644 --- a/p2p/base/stun_request.cc +++ b/p2p/base/stun_request.cc @@ -120,6 +120,18 @@ bool StunRequestManager::CheckResponse(StunMessage* msg) { } StunRequest* request = iter->second; + + // Now that we know the request, we can see if the response is + // integrity-protected or not. + // For some tests, the message integrity is not set in the request. + // Complain, and then don't check. + bool skip_integrity_checking = false; + if (request->msg()->integrity() == StunMessage::IntegrityStatus::kNotSet) { + skip_integrity_checking = true; + } else { + msg->ValidateMessageIntegrity(request->msg()->password()); + } + if (!msg->GetNonComprehendedAttributes().empty()) { // If a response contains unknown comprehension-required attributes, it's // simply discarded and the transaction is considered failed. See RFC5389 @@ -129,6 +141,9 @@ bool StunRequestManager::CheckResponse(StunMessage* msg) { delete request; return false; } else if (msg->type() == GetStunSuccessResponseType(request->type())) { + if (!msg->IntegrityOk() && !skip_integrity_checking) { + return false; + } request->OnResponse(msg); } else if (msg->type() == GetStunErrorResponseType(request->type())) { request->OnErrorResponse(msg); diff --git a/p2p/base/turn_port.cc b/p2p/base/turn_port.cc index c78a94642d..2bb4c614a6 100644 --- a/p2p/base/turn_port.cc +++ b/p2p/base/turn_port.cc @@ -724,16 +724,6 @@ bool TurnPort::HandleIncomingPacket(rtc::AsyncPacketSocket* socket, return false; } - // This must be a response for one of our requests. - // Check success responses, but not errors, for MESSAGE-INTEGRITY. - if (IsStunSuccessResponseType(msg_type) && - !StunMessage::ValidateMessageIntegrity(data, size, hash())) { - RTC_LOG(LS_WARNING) << ToString() - << ": Received TURN message with invalid " - "message integrity, msg_type: " - << msg_type; - return true; - } request_manager_.CheckResponse(data, size); return true; diff --git a/p2p/base/turn_server.cc b/p2p/base/turn_server.cc index 25985be87a..60b3d94b72 100644 --- a/p2p/base/turn_server.cc +++ b/p2p/base/turn_server.cc @@ -306,7 +306,7 @@ bool TurnServer::GetKey(const StunMessage* msg, std::string* key) { } bool TurnServer::CheckAuthorization(TurnServerConnection* conn, - const StunMessage* msg, + StunMessage* msg, const char* data, size_t size, const std::string& key) { @@ -322,14 +322,14 @@ bool TurnServer::CheckAuthorization(TurnServerConnection* conn, const StunByteStringAttribute* nonce_attr = msg->GetByteString(STUN_ATTR_NONCE); - // Fail if no M-I. + // Fail if no MESSAGE_INTEGRITY. if (!mi_attr) { SendErrorResponseWithRealmAndNonce(conn, msg, STUN_ERROR_UNAUTHORIZED, STUN_ERROR_REASON_UNAUTHORIZED); return false; } - // Fail if there is M-I but no username, nonce, or realm. + // Fail if there is MESSAGE_INTEGRITY but no username, nonce, or realm. if (!username_attr || !realm_attr || !nonce_attr) { SendErrorResponse(conn, msg, STUN_ERROR_BAD_REQUEST, STUN_ERROR_REASON_BAD_REQUEST); @@ -343,9 +343,9 @@ bool TurnServer::CheckAuthorization(TurnServerConnection* conn, return false; } - // Fail if bad username or M-I. - // We need |data| and |size| for the call to ValidateMessageIntegrity. - if (key.empty() || !StunMessage::ValidateMessageIntegrity(data, size, key)) { + // Fail if bad MESSAGE_INTEGRITY. + if (key.empty() || msg->ValidateMessageIntegrity(key) != + StunMessage::IntegrityStatus::kIntegrityOk) { SendErrorResponseWithRealmAndNonce(conn, msg, STUN_ERROR_UNAUTHORIZED, STUN_ERROR_REASON_UNAUTHORIZED); return false; diff --git a/p2p/base/turn_server.h b/p2p/base/turn_server.h index 05916ea8bc..c63eeb8cdf 100644 --- a/p2p/base/turn_server.h +++ b/p2p/base/turn_server.h @@ -278,7 +278,7 @@ class TurnServer : public sigslot::has_slots<> { bool GetKey(const StunMessage* msg, std::string* key); bool CheckAuthorization(TurnServerConnection* conn, - const StunMessage* msg, + StunMessage* msg, const char* data, size_t size, const std::string& key); diff --git a/test/fuzzers/stun_parser_fuzzer.cc b/test/fuzzers/stun_parser_fuzzer.cc index 720a699662..6ca9eac8b2 100644 --- a/test/fuzzers/stun_parser_fuzzer.cc +++ b/test/fuzzers/stun_parser_fuzzer.cc @@ -24,5 +24,6 @@ void FuzzOneInput(const uint8_t* data, size_t size) { std::unique_ptr stun_msg(new cricket::IceMessage()); rtc::ByteBufferReader buf(message, size); stun_msg->Read(&buf); + stun_msg->ValidateMessageIntegrity(""); } } // namespace webrtc diff --git a/test/fuzzers/stun_validator_fuzzer.cc b/test/fuzzers/stun_validator_fuzzer.cc index 44252fafbc..421638db1b 100644 --- a/test/fuzzers/stun_validator_fuzzer.cc +++ b/test/fuzzers/stun_validator_fuzzer.cc @@ -18,6 +18,6 @@ void FuzzOneInput(const uint8_t* data, size_t size) { const char* message = reinterpret_cast(data); cricket::StunMessage::ValidateFingerprint(message, size); - cricket::StunMessage::ValidateMessageIntegrity(message, size, ""); + cricket::StunMessage::ValidateMessageIntegrityForTesting(message, size, ""); } } // namespace webrtc