diff --git a/webrtc/p2p/base/stun.cc b/webrtc/p2p/base/stun.cc index 180597ee77..78b188afef 100644 --- a/webrtc/p2p/base/stun.cc +++ b/webrtc/p2p/base/stun.cc @@ -145,7 +145,7 @@ bool StunMessage::ValidateMessageIntegrity(const char* data, size_t size, // Finding Message Integrity attribute in stun message. size_t current_pos = kStunHeaderSize; bool has_message_integrity_attr = false; - while (current_pos < size) { + while (current_pos + 4 <= size) { uint16_t attr_type, attr_length; // Getting attribute type and length. attr_type = rtc::GetBE16(&data[current_pos]); @@ -154,7 +154,8 @@ bool StunMessage::ValidateMessageIntegrity(const char* data, size_t size, // If M-I, sanity check it, and break out. if (attr_type == STUN_ATTR_MESSAGE_INTEGRITY) { if (attr_length != kStunMessageIntegritySize || - current_pos + attr_length > size) { + current_pos + sizeof(attr_type) + sizeof(attr_length) + attr_length > + size) { return false; } has_message_integrity_attr = true; diff --git a/webrtc/p2p/base/stun_unittest.cc b/webrtc/p2p/base/stun_unittest.cc index d7ca9991f8..033c5ddc6d 100644 --- a/webrtc/p2p/base/stun_unittest.cc +++ b/webrtc/p2p/base/stun_unittest.cc @@ -243,6 +243,19 @@ static const unsigned char kStunMessageWithSmallLength[] = { 0x21, 0x12, 0xA4, 0x53, }; +static const unsigned char kStunMessageWithBadHmacAtEnd[] = { + 0x00, 0x01, 0x00, 0x14, // message length exactly 20 + 0x21, 0x12, 0xA4, 0x42, // magic cookie + '0', '1', '2', '3', // transaction ID + '4', '5', '6', '7', + '8', '9', 'a', 'b', + 0x00, 0x08, 0x00, 0x14, // type=STUN_ATTR_MESSAGE_INTEGRITY, length=20 + '0', '0', '0', '0', // We lied, there are only 16 bytes of HMAC. + '0', '0', '0', '0', + '0', '0', '0', '0', + '0', '0', '0', '0', +}; + // RTCP packet, for testing we correctly ignore non stun packet types. // V=2, P=false, RC=0, Type=200, Len=6, Sender-SSRC=85, etc static const unsigned char kRtcpPacket[] = { @@ -1194,6 +1207,26 @@ TEST_F(StunTest, ValidateMessageIntegrity) { sizeof(kStunMessageWithSmallLength), kRfc5769SampleMsgPassword)); + // Again, but with the lengths matching what is claimed in the headers. + EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + reinterpret_cast(kStunMessageWithZeroLength), + kStunHeaderSize + rtc::GetBE16(&kStunMessageWithZeroLength[2]), + kRfc5769SampleMsgPassword)); + EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + reinterpret_cast(kStunMessageWithExcessLength), + kStunHeaderSize + rtc::GetBE16(&kStunMessageWithExcessLength[2]), + kRfc5769SampleMsgPassword)); + EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + reinterpret_cast(kStunMessageWithSmallLength), + kStunHeaderSize + rtc::GetBE16(&kStunMessageWithSmallLength[2]), + kRfc5769SampleMsgPassword)); + + // Check that a too-short HMAC doesn't cause buffer overflow. + EXPECT_FALSE(StunMessage::ValidateMessageIntegrity( + reinterpret_cast(kStunMessageWithBadHmacAtEnd), + sizeof(kStunMessageWithBadHmacAtEnd), + kRfc5769SampleMsgPassword)); + // Test that munging a single bit anywhere in the message causes the // message-integrity check to fail, unless it is after the M-I attribute. char buf[sizeof(kRfc5769SampleRequest)];