Fix potential UAF in StunDictionaryWriter::CreateDelta + heap exhaustion in StunDictionaryView::ApplyDelta 1) The UAF is possible if Write used Set + Delete in same Delta, when later cleaning the delta (in ApplyDeltaAck) the state was left inconsistently so that subsequent CreateDelta could experience UAF. It is theoretically possible that an actual WebRTC program could do this, but I'm quite certain we currently *dont* use Delete. 2) The heap exhaustion in StunDictionaryView::ApplyDelta after crafting a malicious delta. This is not possible to do with existing code, but obviously an attacker could it learning about the ice ufra/passwd. BUG=b/513584807,b/513584726 Change-Id: Ic286ad4a7cc6c127fdcd3359505898a36eabdfee Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/474600 Commit-Queue: Jonas Oreland <jonaso@webrtc.org> Reviewed-by: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47777}
diff --git a/p2p/base/stun_dictionary.cc b/p2p/base/stun_dictionary.cc index cc28d8b..6974d1d 100644 --- a/p2p/base/stun_dictionary.cc +++ b/p2p/base/stun_dictionary.cc
@@ -134,12 +134,22 @@ // Now read all the attributes std::deque<std::unique_ptr<StunAttribute>> attrs; + // Track seen keys to detect duplicates. We use std::vector instead of + // std::set because the number of attributes in a delta is typically very + // small, making linear search on a vector faster in practice due to cache + // locality and avoiding heap allocations. + std::vector<uint16_t> seen_keys; while (buf.Length()) { uint16_t key, length, value_type; if (!buf.ReadUInt16(&key)) { return RTCError(RTCErrorType::INVALID_PARAMETER, "Failed to read attribute key"); } + if (std::find(seen_keys.begin(), seen_keys.end(), key) != seen_keys.end()) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "Duplicate key in delta"); + } + seen_keys.push_back(key); if (!buf.ReadUInt16(&length)) { return RTCError(RTCErrorType::INVALID_PARAMETER, "Failed to read attribute length"); @@ -353,15 +363,14 @@ // Apply a delta ack, i.e prune list of pending changes. void StunDictionaryWriter::ApplyDeltaAck(const StunUInt64Attribute& ack) { uint64_t acked_version = ack.value(); - auto entries_to_remove = std::remove_if( - pending_.begin(), pending_.end(), - [acked_version](const auto& p) { return p.first <= acked_version; }); - - // remove tombstones. - for (auto it = entries_to_remove; it != pending_.end(); ++it) { - tombstones_.erase((*it).second->type()); + for (const auto& p : pending_) { + if (p.first <= acked_version) { + tombstones_.erase(p.second->type()); + } } - pending_.erase(entries_to_remove, pending_.end()); + std::erase_if(pending_, [acked_version](const auto& p) { + return p.first <= acked_version; + }); } // Check if a key has a pending change (i.e a change
diff --git a/p2p/base/stun_dictionary_unittest.cc b/p2p/base/stun_dictionary_unittest.cc index 506443b..341ed16 100644 --- a/p2p/base/stun_dictionary_unittest.cc +++ b/p2p/base/stun_dictionary_unittest.cc
@@ -18,6 +18,7 @@ #include <vector> #include "api/transport/stun.h" +#include "rtc_base/byte_buffer.h" #include "rtc_base/logging.h" #include "rtc_base/socket_address.h" #include "test/gtest.h" @@ -338,4 +339,91 @@ } } +TEST(StunDictionary, DuplicateKeysDesyncBytesStored) { + StunDictionaryView dictionary; + StunDictionaryWriter writer; + + // 1. Store a value initially. + writer.SetUInt32(kKey1)->SetValue(10); + Sync(dictionary, writer); + EXPECT_EQ(dictionary.GetUInt32(kKey1)->value(), 10u); + int initial_bytes = dictionary.bytes_stored(); + EXPECT_EQ(initial_bytes, 12); + + // 2. Manually construct a delta with duplicate keys. + // We want version 3. + // It should contain: + // - Version attribute (version 3) + // - kKey1 (UINT32, value 27) + // - kKey1 (BYTE_STRING, length 0) [delete] + + ByteBufferWriter buf; + buf.WriteUInt16(StunDictionaryView::kDeltaMagic); + buf.WriteUInt16(StunDictionaryView::kDeltaVersion); + + // Version attribute + buf.WriteUInt16(StunDictionaryView::kVersionKey); + buf.WriteUInt16(8); + buf.WriteUInt16(STUN_VALUE_UINT64); + buf.WriteUInt64(3); // Version 3 + + // First attribute: kKey1 (UINT32, value 27) + buf.WriteUInt16(kKey1); + buf.WriteUInt16(4); // length + buf.WriteUInt16(STUN_VALUE_UINT32); + buf.WriteUInt32(27); + + // Second attribute: kKey1 (BYTE_STRING, length 0) [delete] + buf.WriteUInt16(kKey1); + buf.WriteUInt16(0); // length + buf.WriteUInt16(STUN_VALUE_BYTE_STRING); + + StunByteStringAttribute delta(STUN_ATTR_GOOG_DELTA, buf.DataView()); + + // 3. Apply the delta + auto delta_ack = dictionary.ApplyDelta(delta); + // We expect this to fail once fixed. + EXPECT_FALSE(delta_ack.ok()); + + if (delta_ack.ok()) { + // 4. Verify desync (only if we didn't fail as expected) + ASSERT_NE(dictionary.GetUInt32(kKey1), nullptr); + EXPECT_EQ(dictionary.GetUInt32(kKey1)->value(), 27u); + + // In the bugged version, this will be 8 instead of 12. + EXPECT_EQ(dictionary.bytes_stored(), 8); + } +} + +TEST(StunDictionary, TombstoneLifeline) { + StunDictionaryWriter writer; + + // 1. Set kKey1 (will be version 2) + writer.SetUInt32(kKey1)->SetValue(10); + + // 2. Set kKey2 (will be version 3) + writer.SetUInt32(kKey1 + 1)->SetValue(20); + + // 3. Delete kKey2 (will remove version 3, add version 4 tombstone) + writer.Delete(kKey1 + 1); + + // Now pending has: + // (2, kKey1_attr) + // (4, kKey2_tombstone) + + // We construct an ACK for version 2. + StunUInt64Attribute ack(STUN_ATTR_GOOG_DELTA_ACK, 2); + + // This should remove kKey1 from pending, but KEEP kKey2 tombstone. + writer.ApplyDeltaAck(ack); + + // If the bug is present, the tombstone for kKey2 has been deleted from + // writer.tombstones_, but it is still in pending_. Calling CreateDelta will + // try to access it and crash/UAF. + auto delta = writer.CreateDelta(); + + // If we survived, let's make sure the delta contains the tombstone. + ASSERT_NE(delta, nullptr); +} + } // namespace webrtc