diff --git a/pc/BUILD.gn b/pc/BUILD.gn index c79a2adee1..334d1af01d 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -74,6 +74,7 @@ rtc_static_library("rtc_pc_base") { deps = [ "..:webrtc_common", + "../api:array_view", "../api:call_api", "../api:libjingle_peerconnection_api", "../api:optional", diff --git a/pc/dtlssrtptransport.cc b/pc/dtlssrtptransport.cc index ff1bc0bfb1..0b98a96293 100644 --- a/pc/dtlssrtptransport.cc +++ b/pc/dtlssrtptransport.cc @@ -165,8 +165,8 @@ void DtlsSrtpTransport::SetupRtpDtlsSrtp() { } int selected_crypto_suite; - std::vector send_key; - std::vector recv_key; + rtc::ZeroOnFreeBuffer send_key; + rtc::ZeroOnFreeBuffer recv_key; if (!ExtractParams(rtp_dtls_transport_, &selected_crypto_suite, &send_key, &recv_key) || @@ -198,8 +198,8 @@ void DtlsSrtpTransport::SetupRtcpDtlsSrtp() { } int selected_crypto_suite; - std::vector rtcp_send_key; - std::vector rtcp_recv_key; + rtc::ZeroOnFreeBuffer rtcp_send_key; + rtc::ZeroOnFreeBuffer rtcp_recv_key; if (!ExtractParams(rtcp_dtls_transport_, &selected_crypto_suite, &rtcp_send_key, &rtcp_recv_key) || !srtp_transport_->SetRtcpParams( @@ -215,8 +215,8 @@ void DtlsSrtpTransport::SetupRtcpDtlsSrtp() { bool DtlsSrtpTransport::ExtractParams( cricket::DtlsTransportInternal* dtls_transport, int* selected_crypto_suite, - std::vector* send_key, - std::vector* recv_key) { + rtc::ZeroOnFreeBuffer* send_key, + rtc::ZeroOnFreeBuffer* recv_key) { if (!dtls_transport || !dtls_transport->IsDtlsActive()) { return false; } @@ -239,7 +239,7 @@ bool DtlsSrtpTransport::ExtractParams( } // OK, we're now doing DTLS (RFC 5764) - std::vector dtls_buffer(key_len * 2 + salt_len * 2); + rtc::ZeroOnFreeBuffer dtls_buffer(key_len * 2 + salt_len * 2); // RFC 5705 exporter using the RFC 5764 parameters if (!dtls_transport->ExportKeyingMaterial(kDtlsSrtpExporterLabel, NULL, 0, @@ -251,8 +251,8 @@ bool DtlsSrtpTransport::ExtractParams( } // Sync up the keys with the DTLS-SRTP interface - std::vector client_write_key(key_len + salt_len); - std::vector server_write_key(key_len + salt_len); + rtc::ZeroOnFreeBuffer client_write_key(key_len + salt_len); + rtc::ZeroOnFreeBuffer server_write_key(key_len + salt_len); size_t offset = 0; memcpy(&client_write_key[0], &dtls_buffer[offset], key_len); offset += key_len; @@ -269,13 +269,12 @@ bool DtlsSrtpTransport::ExtractParams( } if (role == rtc::SSL_SERVER) { - *send_key = server_write_key; - *recv_key = client_write_key; + *send_key = std::move(server_write_key); + *recv_key = std::move(client_write_key); } else { - *send_key = client_write_key; - *recv_key = server_write_key; + *send_key = std::move(client_write_key); + *recv_key = std::move(server_write_key); } - return true; } diff --git a/pc/dtlssrtptransport.h b/pc/dtlssrtptransport.h index 4f6e697555..02002b052a 100644 --- a/pc/dtlssrtptransport.h +++ b/pc/dtlssrtptransport.h @@ -18,6 +18,7 @@ #include "p2p/base/dtlstransportinternal.h" #include "pc/rtptransportinternaladapter.h" #include "pc/srtptransport.h" +#include "rtc_base/buffer.h" namespace webrtc { @@ -68,8 +69,8 @@ class DtlsSrtpTransport : public RtpTransportInternalAdapter { void SetupRtcpDtlsSrtp(); bool ExtractParams(cricket::DtlsTransportInternal* dtls_transport, int* selected_crypto_suite, - std::vector* send_key, - std::vector* recv_key); + rtc::ZeroOnFreeBuffer* send_key, + rtc::ZeroOnFreeBuffer* recv_key); void SetDtlsTransport(cricket::DtlsTransportInternal* new_dtls_transport, cricket::DtlsTransportInternal** old_dtls_transport); void SetRtpDtlsTransport(cricket::DtlsTransportInternal* rtp_dtls_transport); diff --git a/pc/externalhmac.cc b/pc/externalhmac.cc index 899c33822c..8e730e9f4c 100644 --- a/pc/externalhmac.cc +++ b/pc/externalhmac.cc @@ -13,6 +13,7 @@ #include // For malloc/free. #include "rtc_base/logging.h" +#include "rtc_base/zero_memory.h" #include "third_party/libsrtp/crypto/include/crypto_kernel.h" #include "third_party/libsrtp/include/srtp.h" @@ -94,9 +95,7 @@ srtp_err_status_t external_hmac_alloc(srtp_auth_t** a, } srtp_err_status_t external_hmac_dealloc(srtp_auth_t* a) { - // Zeroize entire state - memset(reinterpret_cast(a), 0, - sizeof(ExternalHmacContext) + sizeof(srtp_auth_t)); + rtc::ExplicitZeroMemory(a, sizeof(ExternalHmacContext) + sizeof(srtp_auth_t)); // Free memory delete[] a; diff --git a/pc/srtpfilter.cc b/pc/srtpfilter.cc index 5561ada06d..751668e90b 100644 --- a/pc/srtpfilter.cc +++ b/pc/srtpfilter.cc @@ -22,6 +22,7 @@ #include "rtc_base/logging.h" #include "rtc_base/stringencode.h" #include "rtc_base/timeutils.h" +#include "rtc_base/zero_memory.h" namespace cricket { @@ -223,7 +224,7 @@ bool SrtpFilter::ApplySendParams(const CryptoParams& send_params) { return false; } - send_key_ = rtc::Buffer(send_key_len + send_salt_len); + send_key_ = rtc::ZeroOnFreeBuffer(send_key_len + send_salt_len); return ParseKeyParams(send_params.key_params, send_key_.data(), send_key_.size()); } @@ -254,7 +255,7 @@ bool SrtpFilter::ApplyRecvParams(const CryptoParams& recv_params) { return false; } - recv_key_ = rtc::Buffer(recv_key_len + recv_salt_len); + recv_key_ = rtc::ZeroOnFreeBuffer(recv_key_len + recv_salt_len); return ParseKeyParams(recv_params.key_params, recv_key_.data(), recv_key_.size()); } @@ -278,6 +279,9 @@ bool SrtpFilter::ParseKeyParams(const std::string& key_params, } memcpy(key, key_str.c_str(), len); + // TODO(bugs.webrtc.org/8905): Switch to ZeroOnFreeBuffer for storing + // sensitive data. + rtc::ExplicitZeroMemory(&key_str[0], key_str.size()); return true; } diff --git a/pc/srtpfilter.h b/pc/srtpfilter.h index dc0b37ea38..6a3644d5e9 100644 --- a/pc/srtpfilter.h +++ b/pc/srtpfilter.h @@ -17,6 +17,7 @@ #include #include +#include "api/array_view.h" #include "api/cryptoparams.h" #include "api/jsep.h" #include "api/optional.h" @@ -84,8 +85,8 @@ class SrtpFilter { rtc::Optional send_cipher_suite() { return send_cipher_suite_; } rtc::Optional recv_cipher_suite() { return recv_cipher_suite_; } - const rtc::Buffer& send_key() { return send_key_; } - const rtc::Buffer& recv_key() { return recv_key_; } + rtc::ArrayView send_key() { return send_key_; } + rtc::ArrayView recv_key() { return recv_key_; } protected: bool ExpectOffer(ContentSource source); @@ -140,8 +141,8 @@ class SrtpFilter { CryptoParams applied_recv_params_; rtc::Optional send_cipher_suite_; rtc::Optional recv_cipher_suite_; - rtc::Buffer send_key_; - rtc::Buffer recv_key_; + rtc::ZeroOnFreeBuffer send_key_; + rtc::ZeroOnFreeBuffer recv_key_; }; } // namespace cricket diff --git a/pc/srtpfilter_unittest.cc b/pc/srtpfilter_unittest.cc index 017df5baf1..342d458307 100644 --- a/pc/srtpfilter_unittest.cc +++ b/pc/srtpfilter_unittest.cc @@ -71,11 +71,17 @@ class SrtpFilterTest : public testing::Test { EXPECT_TRUE(f2_.IsActive()); } + void VerifyKeysAreEqual(ArrayView key1, + ArrayView key2) { + EXPECT_EQ(key1.size(), key2.size()); + EXPECT_EQ(0, memcmp(key1.data(), key2.data(), key1.size())); + } + void VerifyCryptoParamsMatch(const std::string& cs1, const std::string& cs2) { EXPECT_EQ(rtc::SrtpCryptoSuiteFromName(cs1), f1_.send_cipher_suite()); EXPECT_EQ(rtc::SrtpCryptoSuiteFromName(cs2), f2_.send_cipher_suite()); - EXPECT_TRUE(f1_.send_key() == f2_.recv_key()); - EXPECT_TRUE(f2_.send_key() == f1_.recv_key()); + VerifyKeysAreEqual(f1_.send_key(), f2_.recv_key()); + VerifyKeysAreEqual(f2_.send_key(), f1_.recv_key()); } cricket::SrtpFilter f1_; diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index e38b713a3e..7e6a23da57 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -259,6 +259,8 @@ rtc_source_set("rtc_base_approved_generic") { "timeutils.cc", "timeutils.h", "trace_event.h", + "zero_memory.cc", + "zero_memory.h", ] deps += [ @@ -675,8 +677,6 @@ rtc_static_library("rtc_base_generic") { "stream.h", "thread.cc", "thread.h", - "zero_memory.cc", - "zero_memory.h", ] visibility = [ @@ -1002,6 +1002,7 @@ if (rtc_include_tests) { "timestampaligner_unittest.cc", "timeutils_unittest.cc", "virtualsocket_unittest.cc", + "zero_memory_unittest.cc", ] deps = [ ":checks", @@ -1120,7 +1121,6 @@ if (rtc_include_tests) { "stream_unittest.cc", "testclient_unittest.cc", "thread_unittest.cc", - "zero_memory_unittest.cc", ] if (is_win) { sources += [ diff --git a/rtc_base/buffer.h b/rtc_base/buffer.h index eff3e40f0d..64974d3ed9 100644 --- a/rtc_base/buffer.h +++ b/rtc_base/buffer.h @@ -20,6 +20,7 @@ #include "api/array_view.h" #include "rtc_base/checks.h" #include "rtc_base/type_traits.h" +#include "rtc_base/zero_memory.h" namespace rtc { @@ -44,7 +45,10 @@ struct BufferCompat { // Basic buffer class, can be grown and shrunk dynamically. // Unlike std::string/vector, does not initialize data when increasing size. -template +// If "ZeroOnFree" is true, any memory is explicitly cleared before releasing. +// The type alias "ZeroOnFreeBuffer" below should be used instead of setting +// "ZeroOnFree" in the template manually to "true". +template class BufferT { // We want T's destructor and default constructor to be trivial, i.e. perform // no action, so that we don't have to touch the memory we allocate and @@ -108,6 +112,8 @@ class BufferT { internal::BufferCompat::value>::type* = nullptr> BufferT(U (&array)[N]) : BufferT(array, N) {} + ~BufferT() { MaybeZeroCompleteBuffer(); } + // Get a pointer to the data. Just .data() will give you a (const) T*, but if // T is a byte-sized integer, you may also use .data() for any other // byte-sized integer U. @@ -195,8 +201,12 @@ class BufferT { internal::BufferCompat::value>::type* = nullptr> void SetData(const U* data, size_t size) { RTC_DCHECK(IsConsistent()); + const size_t old_size = size_; size_ = 0; AppendData(data, size); + if (ZeroOnFree && size_ < old_size) { + ZeroTrailingData(old_size - size_); + } } template ::value>::type* = nullptr> size_t SetData(size_t max_elements, F&& setter) { RTC_DCHECK(IsConsistent()); + const size_t old_size = size_; size_ = 0; - return AppendData(max_elements, std::forward(setter)); + const size_t written = AppendData(max_elements, std::forward(setter)); + if (ZeroOnFree && size_ < old_size) { + ZeroTrailingData(old_size - size_); + } + return written; } // The AppendData functions add data to the end of the buffer. They accept @@ -301,8 +316,12 @@ class BufferT { // the existing contents will be kept and the new space will be // uninitialized. void SetSize(size_t size) { + const size_t old_size = size_; EnsureCapacityWithHeadroom(size, true); size_ = size; + if (ZeroOnFree && size_ < old_size) { + ZeroTrailingData(old_size - size_); + } } // Ensure that the buffer size can be increased to at least capacity without @@ -317,6 +336,7 @@ class BufferT { // Resets the buffer to zero size without altering capacity. Works even if the // buffer has been moved from. void Clear() { + MaybeZeroCompleteBuffer(); size_ = 0; RTC_DCHECK(IsConsistent()); } @@ -346,11 +366,29 @@ class BufferT { std::unique_ptr new_data(new T[new_capacity]); std::memcpy(new_data.get(), data_.get(), size_ * sizeof(T)); + MaybeZeroCompleteBuffer(); data_ = std::move(new_data); capacity_ = new_capacity; RTC_DCHECK(IsConsistent()); } + // Zero the complete buffer if template argument "ZeroOnFree" is true. + void MaybeZeroCompleteBuffer() { + if (ZeroOnFree && capacity_) { + // It would be sufficient to only zero "size_" elements, as all other + // methods already ensure that the unused capacity contains no sensitive + // data - but better safe than sorry. + ExplicitZeroMemory(data_.get(), capacity_ * sizeof(T)); + } + } + + // Zero the first "count" elements of unused capacity. + void ZeroTrailingData(size_t count) { + RTC_DCHECK(IsConsistent()); + RTC_DCHECK_LE(count, capacity_ - size_); + ExplicitZeroMemory(data_.get() + size_, count * sizeof(T)); + } + // Precondition for all methods except Clear and the destructor. // Postcondition for all methods except move construction and move // assignment, which leave the moved-from object in a possibly inconsistent @@ -382,6 +420,10 @@ class BufferT { // By far the most common sort of buffer. using Buffer = BufferT; +// A buffer that zeros memory before releasing it. +template +using ZeroOnFreeBuffer = BufferT; + } // namespace rtc #endif // RTC_BASE_BUFFER_H_ diff --git a/rtc_base/buffer_unittest.cc b/rtc_base/buffer_unittest.cc index 5cd18891b9..e3b7d460b0 100644 --- a/rtc_base/buffer_unittest.cc +++ b/rtc_base/buffer_unittest.cc @@ -434,4 +434,81 @@ TEST(BufferTest, TestStruct) { EXPECT_EQ(kObsidian, buf[2].stone); } +TEST(ZeroOnFreeBufferTest, TestZeroOnSetData) { + ZeroOnFreeBuffer buf(kTestData, 7); + const uint8_t* old_data = buf.data(); + const size_t old_capacity = buf.capacity(); + const size_t old_size = buf.size(); + constexpr size_t offset = 1; + buf.SetData(kTestData + offset, 2); + // Sanity checks to make sure the underlying heap memory was not reallocated. + EXPECT_EQ(old_data, buf.data()); + EXPECT_EQ(old_capacity, buf.capacity()); + // The first two elements have been overwritten, and the remaining five have + // been zeroed. + EXPECT_EQ(kTestData[offset], buf[0]); + EXPECT_EQ(kTestData[offset + 1], buf[1]); + for (size_t i = 2; i < old_size; i++) { + EXPECT_EQ(0, old_data[i]); + } +} + +TEST(ZeroOnFreeBufferTest, TestZeroOnSetDataFromSetter) { + static constexpr size_t offset = 1; + const auto setter = [](rtc::ArrayView av) { + for (int i = 0; i != 2; ++i) + av[i] = kTestData[offset + i]; + return 2; + }; + + ZeroOnFreeBuffer buf(kTestData, 7); + const uint8_t* old_data = buf.data(); + const size_t old_capacity = buf.capacity(); + const size_t old_size = buf.size(); + buf.SetData(2, setter); + // Sanity checks to make sure the underlying heap memory was not reallocated. + EXPECT_EQ(old_data, buf.data()); + EXPECT_EQ(old_capacity, buf.capacity()); + // The first two elements have been overwritten, and the remaining five have + // been zeroed. + EXPECT_EQ(kTestData[offset], buf[0]); + EXPECT_EQ(kTestData[offset + 1], buf[1]); + for (size_t i = 2; i < old_size; i++) { + EXPECT_EQ(0, old_data[i]); + } +} + +TEST(ZeroOnFreeBufferTest, TestZeroOnSetSize) { + ZeroOnFreeBuffer buf(kTestData, 7); + const uint8_t* old_data = buf.data(); + const size_t old_capacity = buf.capacity(); + const size_t old_size = buf.size(); + buf.SetSize(2); + // Sanity checks to make sure the underlying heap memory was not reallocated. + EXPECT_EQ(old_data, buf.data()); + EXPECT_EQ(old_capacity, buf.capacity()); + // The first two elements have not been modified and the remaining five have + // been zeroed. + EXPECT_EQ(kTestData[0], buf[0]); + EXPECT_EQ(kTestData[1], buf[1]); + for (size_t i = 2; i < old_size; i++) { + EXPECT_EQ(0, old_data[i]); + } +} + +TEST(ZeroOnFreeBufferTest, TestZeroOnClear) { + ZeroOnFreeBuffer buf(kTestData, 7); + const uint8_t* old_data = buf.data(); + const size_t old_capacity = buf.capacity(); + const size_t old_size = buf.size(); + buf.Clear(); + // Sanity checks to make sure the underlying heap memory was not reallocated. + EXPECT_EQ(old_data, buf.data()); + EXPECT_EQ(old_capacity, buf.capacity()); + // The underlying memory was not released but cleared. + for (size_t i = 0; i < old_size; i++) { + EXPECT_EQ(0, old_data[i]); + } +} + } // namespace rtc diff --git a/rtc_base/bytebuffer.h b/rtc_base/bytebuffer.h index 2062fbb5ed..c7aa1607c3 100644 --- a/rtc_base/bytebuffer.h +++ b/rtc_base/bytebuffer.h @@ -47,6 +47,7 @@ class ByteBufferWriter : public ByteBuffer { ~ByteBufferWriter(); const char* Data() const { return bytes_; } + char* MutableData() { return bytes_; } size_t Length() const { return end_; } size_t Capacity() const { return size_; } diff --git a/rtc_base/cryptstring.h b/rtc_base/cryptstring.h index 03b38c462f..c210487d90 100644 --- a/rtc_base/cryptstring.h +++ b/rtc_base/cryptstring.h @@ -20,7 +20,7 @@ namespace rtc { class CryptStringImpl { -public: + public: virtual ~CryptStringImpl() {} virtual size_t GetLength() const = 0; virtual void CopyTo(char * dest, bool nullterminate) const = 0; @@ -30,7 +30,7 @@ public: }; class EmptyCryptStringImpl : public CryptStringImpl { -public: + public: ~EmptyCryptStringImpl() override {} size_t GetLength() const override; void CopyTo(char* dest, bool nullterminate) const override; @@ -43,7 +43,9 @@ class CryptString { public: CryptString(); size_t GetLength() const { return impl_->GetLength(); } - void CopyTo(char * dest, bool nullterminate) const { impl_->CopyTo(dest, nullterminate); } + void CopyTo(char* dest, bool nullterminate) const { + impl_->CopyTo(dest, nullterminate); + } CryptString(const CryptString& other); explicit CryptString(const CryptStringImpl& impl); ~CryptString(); @@ -63,89 +65,6 @@ class CryptString { std::unique_ptr impl_; }; - -// Used for constructing strings where a password is involved and we -// need to ensure that we zero memory afterwards -class FormatCryptString { -public: - FormatCryptString() { - storage_ = new char[32]; - capacity_ = 32; - length_ = 0; - storage_[0] = 0; - } - - void Append(const std::string & text) { - Append(text.data(), text.length()); - } - - void Append(const char * data, size_t length) { - EnsureStorage(length_ + length + 1); - memcpy(storage_ + length_, data, length); - length_ += length; - storage_[length_] = '\0'; - } - - void Append(const CryptString * password) { - size_t len = password->GetLength(); - EnsureStorage(length_ + len + 1); - password->CopyTo(storage_ + length_, true); - length_ += len; - } - - size_t GetLength() { - return length_; - } - - const char * GetData() { - return storage_; - } - - - // Ensures storage of at least n bytes - void EnsureStorage(size_t n) { - if (capacity_ >= n) { - return; - } - - size_t old_capacity = capacity_; - char * old_storage = storage_; - - for (;;) { - capacity_ *= 2; - if (capacity_ >= n) - break; - } - - storage_ = new char[capacity_]; - - if (old_capacity) { - memcpy(storage_, old_storage, length_); - - // zero memory in a way that an optimizer won't optimize it out - old_storage[0] = 0; - for (size_t i = 1; i < old_capacity; i++) { - old_storage[i] = old_storage[i - 1]; - } - delete[] old_storage; - } - } - - ~FormatCryptString() { - if (capacity_) { - storage_[0] = 0; - for (size_t i = 1; i < capacity_; i++) { - storage_[i] = storage_[i - 1]; - } - } - delete[] storage_; - } -private: - char * storage_; - size_t capacity_; - size_t length_; -}; - class InsecureCryptStringImpl : public CryptStringImpl { public: std::string& password() { return password_; } @@ -162,6 +81,6 @@ class InsecureCryptStringImpl : public CryptStringImpl { std::string password_; }; -} +} // namespace rtc #endif // RTC_BASE_CRYPTSTRING_H_ diff --git a/rtc_base/httpcommon.cc b/rtc_base/httpcommon.cc index e60ad477a8..f23cb63a6e 100644 --- a/rtc_base/httpcommon.cc +++ b/rtc_base/httpcommon.cc @@ -28,6 +28,7 @@ #include "rtc_base/httpcommon.h" #include "rtc_base/messagedigest.h" #include "rtc_base/socketaddress.h" +#include "rtc_base/zero_memory.h" namespace rtc { namespace { @@ -775,8 +776,10 @@ HttpAuthResult HttpAuthenticate( context = new HttpAuthContext(auth_method); - // TODO: convert sensitive to a secure buffer that gets securely deleted - //std::string decoded = username + ":" + password; + // TODO(bugs.webrtc.org/8905): Convert sensitive to a CryptString and also + // return response as CryptString so contents get securely deleted + // automatically. + // std::string decoded = username + ":" + password; size_t len = username.size() + password.GetLength() + 2; char * sensitive = new char[len]; size_t pos = strcpyn(sensitive, len, username.data(), username.size()); @@ -787,7 +790,7 @@ HttpAuthResult HttpAuthenticate( response.append(" "); // TODO: create a sensitive-source version of Base64::encode response.append(Base64::Encode(sensitive)); - memset(sensitive, 0, len); + ExplicitZeroMemory(sensitive, len); delete [] sensitive; return HAR_RESPONSE; } @@ -813,8 +816,10 @@ HttpAuthResult HttpAuthenticate( bool has_qop = HttpHasAttribute(args, "qop", &qop); bool has_opaque = HttpHasAttribute(args, "opaque", &opaque); - // TODO: convert sensitive to be secure buffer - //std::string A1 = username + ":" + realm + ":" + password; + // TODO(bugs.webrtc.org/8905): Convert sensitive to a CryptString and also + // return response as CryptString so contents get securely deleted + // automatically. + // std::string A1 = username + ":" + realm + ":" + password; size_t len = username.size() + realm.size() + password.GetLength() + 3; char * sensitive = new char[len]; // A1 size_t pos = strcpyn(sensitive, len, username.data(), username.size()); @@ -832,7 +837,7 @@ HttpAuthResult HttpAuthenticate( middle = nonce; } std::string HA1 = MD5(sensitive); - memset(sensitive, 0, len); + ExplicitZeroMemory(sensitive, len); delete [] sensitive; std::string HA2 = MD5(A2); std::string dig_response = MD5(HA1 + ":" + middle + ":" + HA2); @@ -981,7 +986,7 @@ HttpAuthResult HttpAuthenticate( memcpy(passbuf, sensitive, auth_id.PasswordLength); passbuf[auth_id.PasswordLength] = 0; } - memset(sensitive, 0, len); + ExplicitZeroMemory(sensitive, len); delete [] sensitive; auth_id.User = userbuf; auth_id.Domain = domainbuf; diff --git a/rtc_base/httpcommon.h b/rtc_base/httpcommon.h index 2897c5a5a6..4dd1172403 100644 --- a/rtc_base/httpcommon.h +++ b/rtc_base/httpcommon.h @@ -442,6 +442,7 @@ enum HttpAuthResult { HAR_RESPONSE, HAR_IGNORE, HAR_CREDENTIALS, HAR_ERROR }; // 'context' is used by this function to record information between calls. // Start by passing a null pointer, then pass the same pointer each additional // call. When the authentication attempt is finished, delete the context. +// TODO(bugs.webrtc.org/8905): Change "response" to "ZeroOnFreeBuffer". HttpAuthResult HttpAuthenticate( const char * challenge, size_t len, const SocketAddress& server, diff --git a/rtc_base/socketadapters.cc b/rtc_base/socketadapters.cc index 814f23b290..5b95cac928 100644 --- a/rtc_base/socketadapters.cc +++ b/rtc_base/socketadapters.cc @@ -32,6 +32,7 @@ #include "rtc_base/socketadapters.h" #include "rtc_base/stringencode.h" #include "rtc_base/stringutils.h" +#include "rtc_base/zero_memory.h" namespace rtc { @@ -672,10 +673,14 @@ void AsyncSocksProxySocket::SendAuth() { size_t len = pass_.GetLength() + 1; char * sensitive = new char[len]; pass_.CopyTo(sensitive, true); - request.WriteString(sensitive); // Password - memset(sensitive, 0, len); + // Don't write anything to |request| afterwards to avoid potential + // reallocations where the old memory (containing the password) will not + // be cleared securely. + request.WriteBytes(sensitive, pass_.GetLength()); // Password + ExplicitZeroMemory(sensitive, len); delete [] sensitive; DirectSend(request.Data(), request.Length()); + ExplicitZeroMemory(request.MutableData(), request.Length()); state_ = SS_AUTH; } diff --git a/rtc_base/zero_memory.cc b/rtc_base/zero_memory.cc index 63600b8d2e..b9c5b380ac 100644 --- a/rtc_base/zero_memory.cc +++ b/rtc_base/zero_memory.cc @@ -26,11 +26,13 @@ void ExplicitZeroMemory(void* ptr, size_t len) { SecureZeroMemory(ptr, len); #else memset(ptr, 0, len); +#if !defined(__pnacl__) /* As best as we can tell, this is sufficient to break any optimisations that might try to eliminate "superfluous" memsets. If there's an easy way to detect memset_s, it would be better to use that. */ __asm__ __volatile__("" : : "r"(ptr) : "memory"); // NOLINT #endif +#endif // !WEBRTC_WIN } } // namespace rtc