admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix buffer overflow in HMAC validation of STUN messages.

Browse Source 
Review-Url: https://codereview.webrtc.org/2071873002
Cr-Commit-Position: refs/heads/master@{#13215}
This commit is contained in:
katrielc 2016-06-20 05:13:16 -07:00 committed by Commit bot
parent c853597598
commit 1a20610764
2 changed files with 36 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
webrtc/p2p/base/stun.cc
View File

@ -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;

33
webrtc/p2p/base/stun_unittest.cc
View File

@ -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<const char*>(kStunMessageWithZeroLength),
kStunHeaderSize + rtc::GetBE16(&kStunMessageWithZeroLength[2]),
kRfc5769SampleMsgPassword));
EXPECT_FALSE(StunMessage::ValidateMessageIntegrity(
reinterpret_cast<const char*>(kStunMessageWithExcessLength),
kStunHeaderSize + rtc::GetBE16(&kStunMessageWithExcessLength[2]),
kRfc5769SampleMsgPassword));
EXPECT_FALSE(StunMessage::ValidateMessageIntegrity(
reinterpret_cast<const char*>(kStunMessageWithSmallLength),
kStunHeaderSize + rtc::GetBE16(&kStunMessageWithSmallLength[2]),
kRfc5769SampleMsgPassword));
// Check that a too-short HMAC doesn't cause buffer overflow.
EXPECT_FALSE(StunMessage::ValidateMessageIntegrity(
reinterpret_cast<const char*>(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)];