Securely clear memory containing key information / passwords before freeing.

The previously used "memset(ptr, 0, size)" can get optimized away by compilers
if "ptr" is not used afterwards.

A new class "ZeroOnFreeBuffer" is introduced that can hold sensitive data and
that automatically clears underlying memory when it's no longer used.

Bug: webrtc:8806, webrtc:8897, webrtc:8905
Change-Id: Iedddddf80790f9af0addaab3346ec5bff102917d
Reviewed-on: https://webrtc-review.googlesource.com/41941
Commit-Queue: Joachim Bauch <jbauch@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22334}

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",

pc/dtlssrtptransport.cc

@@ -165,8 +165,8 @@ void DtlsSrtpTransport::SetupRtpDtlsSrtp() {
   }
 
   int selected_crypto_suite;
-  std::vector<unsigned char> send_key;
+  rtc::ZeroOnFreeBuffer<unsigned char> send_key;
-  std::vector<unsigned char> recv_key;
+  rtc::ZeroOnFreeBuffer<unsigned char> 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<unsigned char> rtcp_send_key;
+  rtc::ZeroOnFreeBuffer<unsigned char> rtcp_send_key;
-  std::vector<unsigned char> rtcp_recv_key;
+  rtc::ZeroOnFreeBuffer<unsigned char> 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<unsigned char>* send_key,
+    rtc::ZeroOnFreeBuffer<unsigned char>* send_key,
-    std::vector<unsigned char>* recv_key) {
+    rtc::ZeroOnFreeBuffer<unsigned char>* 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<unsigned char> dtls_buffer(key_len * 2 + salt_len * 2);
+  rtc::ZeroOnFreeBuffer<unsigned char> 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<unsigned char> client_write_key(key_len + salt_len);
+  rtc::ZeroOnFreeBuffer<unsigned char> client_write_key(key_len + salt_len);
-  std::vector<unsigned char> server_write_key(key_len + salt_len);
+  rtc::ZeroOnFreeBuffer<unsigned char> 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;
+    *send_key = std::move(server_write_key);
-    *recv_key = client_write_key;
+    *recv_key = std::move(client_write_key);
   } else {
-    *send_key = client_write_key;
+    *send_key = std::move(client_write_key);
-    *recv_key = server_write_key;
+    *recv_key = std::move(server_write_key);
   }
-
   return true;
 }
 

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<unsigned char>* send_key,
+                     rtc::ZeroOnFreeBuffer<unsigned char>* send_key,
-                     std::vector<unsigned char>* recv_key);
+                     rtc::ZeroOnFreeBuffer<unsigned char>* recv_key);
   void SetDtlsTransport(cricket::DtlsTransportInternal* new_dtls_transport,
                         cricket::DtlsTransportInternal** old_dtls_transport);
   void SetRtpDtlsTransport(cricket::DtlsTransportInternal* rtp_dtls_transport);

pc/externalhmac.cc

@@ -13,6 +13,7 @@
 #include <stdlib.h>  // 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
+  rtc::ExplicitZeroMemory(a, sizeof(ExternalHmacContext) + sizeof(srtp_auth_t));
-  memset(reinterpret_cast<uint8_t*>(a), 0,
-         sizeof(ExternalHmacContext) + sizeof(srtp_auth_t));
 
   // Free memory
   delete[] a;

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<uint8_t>(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<uint8_t>(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;
 }
 

pc/srtpfilter.h

@@ -17,6 +17,7 @@
 #include <string>
 #include <vector>
 
+#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<int> send_cipher_suite() { return send_cipher_suite_; }
   rtc::Optional<int> recv_cipher_suite() { return recv_cipher_suite_; }
 
-  const rtc::Buffer& send_key() { return send_key_; }
+  rtc::ArrayView<const uint8_t> send_key() { return send_key_; }
-  const rtc::Buffer& recv_key() { return recv_key_; }
+  rtc::ArrayView<const uint8_t> recv_key() { return recv_key_; }
 
  protected:
   bool ExpectOffer(ContentSource source);
@@ -140,8 +141,8 @@ class SrtpFilter {
   CryptoParams applied_recv_params_;
   rtc::Optional<int> send_cipher_suite_;
   rtc::Optional<int> recv_cipher_suite_;
-  rtc::Buffer send_key_;
+  rtc::ZeroOnFreeBuffer<uint8_t> send_key_;
-  rtc::Buffer recv_key_;
+  rtc::ZeroOnFreeBuffer<uint8_t> recv_key_;
 };
 
 }  // namespace cricket

pc/srtpfilter_unittest.cc

@@ -71,11 +71,17 @@ class SrtpFilterTest : public testing::Test {
     EXPECT_TRUE(f2_.IsActive());
   }
 
+  void VerifyKeysAreEqual(ArrayView<const uint8_t> key1,
+                          ArrayView<const uint8_t> 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());
+    VerifyKeysAreEqual(f1_.send_key(), f2_.recv_key());
-    EXPECT_TRUE(f2_.send_key() == f1_.recv_key());
+    VerifyKeysAreEqual(f2_.send_key(), f1_.recv_key());
   }
 
   cricket::SrtpFilter f1_;

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 += [

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 <typename T>
+// 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 <typename T, bool ZeroOnFree = false>
 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<T, U>::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<U>() for any other
   // byte-sized integer U.
@@ -195,8 +201,12 @@ class BufferT {
                 internal::BufferCompat<T, U>::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 <typename U,
@@ -229,8 +239,13 @@ class BufferT {
                 internal::BufferCompat<T, U>::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<U>(max_elements, std::forward<F>(setter));
+    const size_t written = AppendData<U>(max_elements, std::forward<F>(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<T[]> 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<uint8_t>;
 
+// A buffer that zeros memory before releasing it.
+template <typename T>
+using ZeroOnFreeBuffer = BufferT<T, true>;
+
 }  // namespace rtc
 
 #endif  // RTC_BASE_BUFFER_H_

rtc_base/buffer_unittest.cc

@@ -434,4 +434,81 @@ TEST(BufferTest, TestStruct) {
   EXPECT_EQ(kObsidian, buf[2].stone);
 }
 
+TEST(ZeroOnFreeBufferTest, TestZeroOnSetData) {
+  ZeroOnFreeBuffer<uint8_t> 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<uint8_t> av) {
+    for (int i = 0; i != 2; ++i)
+      av[i] = kTestData[offset + i];
+    return 2;
+  };
+
+  ZeroOnFreeBuffer<uint8_t> 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<uint8_t> 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<uint8_t> 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

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_; }
 

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<const CryptStringImpl> 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_

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
+    // TODO(bugs.webrtc.org/8905): Convert sensitive to a CryptString and also
-    //std::string decoded = username + ":" + password;
+    // 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
+    // TODO(bugs.webrtc.org/8905): Convert sensitive to a CryptString and also
-    //std::string A1 = username + ":" + realm + ":" + password;
+    // 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;

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,

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
+  // Don't write anything to |request| afterwards to avoid potential
-  memset(sensitive, 0, len);
+  // 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;
 }
 

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