Harden winrt_encryption slightly and avoid a potential reallocation. (#…

…896)
microsoft · Oct 9, 2018 · eadff24 · eadff24
1 parent b211145
commit eadff24
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 27 deletions.
diff --git a/Release/src/json/json_parsing.cpp b/Release/src/json/json_parsing.cpp
@@ -1026,7 +1026,7 @@ std::unique_ptr<web::json::details::_Value> JSON_Parser<CharType>::_ParseObject(
         ::std::sort(elems.begin(), elems.end(), json::object::compare_pairs);
     }
 
-    return std::move(obj);
+    return obj;
 
 error:
     if (!tkn.m_error)

diff --git a/Release/src/utilities/web_utilities.cpp b/Release/src/utilities/web_utilities.cpp
@@ -12,6 +12,7 @@
 ****/
 
 #include "stdafx.h"
+#include <assert.h>
 
 #if defined(_WIN32) && !defined(__cplusplus_winrt)
 #include <Wincrypt.h>
@@ -92,23 +93,28 @@ plaintext_string winrt_encryption::decrypt() const
 win32_encryption::win32_encryption(const std::wstring &data) :
     m_numCharacters(data.size())
 {
-    // Early return because CryptProtectMemory crashs with empty string
+    // Early return because CryptProtectMemory crashes with empty string
     if (m_numCharacters == 0)
     {
         return;
     }
 
-    const auto dataNumBytes = data.size() * sizeof(std::wstring::value_type);
-    m_buffer.resize(dataNumBytes);
-    memcpy_s(m_buffer.data(), m_buffer.size(), data.c_str(), dataNumBytes);
-
-    // Buffer must be a multiple of CRYPTPROTECTMEMORY_BLOCK_SIZE
-    const auto mod = m_buffer.size() % CRYPTPROTECTMEMORY_BLOCK_SIZE;
-    if (mod != 0)
+    if (data.size() > (std::numeric_limits<DWORD>::max)() / sizeof(wchar_t))
     {
-        m_buffer.resize(m_buffer.size() + CRYPTPROTECTMEMORY_BLOCK_SIZE - mod);
+        throw std::length_error("Encryption string too long");
     }
-    if (!CryptProtectMemory(m_buffer.data(), static_cast<DWORD>(m_buffer.size()), CRYPTPROTECTMEMORY_SAME_PROCESS))
+
+    const auto dataSizeDword = static_cast<DWORD>(data.size() * sizeof(wchar_t));
+
+    // Round up dataSizeDword to be a multiple of CRYPTPROTECTMEMORY_BLOCK_SIZE
+    static_assert(CRYPTPROTECTMEMORY_BLOCK_SIZE == 16, "Power of 2 assumptions in this bit masking violated");
+    const auto mask = static_cast<DWORD>(CRYPTPROTECTMEMORY_BLOCK_SIZE - 1u);
+    const auto dataNumBytes = (dataSizeDword & ~mask) + ((dataSizeDword & mask) != 0) * CRYPTPROTECTMEMORY_BLOCK_SIZE;
+    assert((dataNumBytes % CRYPTPROTECTMEMORY_BLOCK_SIZE) == 0);
+    assert(dataNumBytes >= dataSizeDword);
+    m_buffer.resize(dataNumBytes);
+    memcpy_s(m_buffer.data(), m_buffer.size(), data.c_str(), dataNumBytes);
+    if (!CryptProtectMemory(m_buffer.data(), dataNumBytes, CRYPTPROTECTMEMORY_SAME_PROCESS))
     {
         throw ::utility::details::create_system_error(GetLastError());
     }
@@ -121,20 +127,25 @@ win32_encryption::~win32_encryption()
 
 plaintext_string win32_encryption::decrypt() const
 {
-    if (m_buffer.empty())
-        return plaintext_string(new std::wstring());
-
     // Copy the buffer and decrypt to avoid having to re-encrypt.
-    auto data = plaintext_string(new std::wstring(reinterpret_cast<const std::wstring::value_type *>(m_buffer.data()), m_buffer.size() / 2));
-    if (!CryptUnprotectMemory(
-        const_cast<std::wstring::value_type *>(data->c_str()),
-        static_cast<DWORD>(m_buffer.size()),
-        CRYPTPROTECTMEMORY_SAME_PROCESS))
-    {
-        throw ::utility::details::create_system_error(GetLastError());
+    auto result = plaintext_string(new std::wstring(
+        reinterpret_cast<const std::wstring::value_type *>(m_buffer.data()), m_buffer.size() / sizeof(wchar_t)));
+    auto& data = *result;
+    if (!m_buffer.empty()) {
+        if (!CryptUnprotectMemory(
+            &data[0],
+            static_cast<DWORD>(m_buffer.size()),
+            CRYPTPROTECTMEMORY_SAME_PROCESS))
+        {
+            throw ::utility::details::create_system_error(GetLastError());
+        }
+
+        assert(m_numCharacters <= m_buffer.size());
+        SecureZeroMemory(&data[m_numCharacters], data.size() - m_numCharacters);
+        data.erase(m_numCharacters);
     }
-    data->resize(m_numCharacters);
-    return std::move(data);
+
+    return result;
 }
 #endif
 #endif
@@ -143,12 +154,10 @@ void zero_memory_deleter::operator()(::utility::string_t *data) const
 {
     CASABLANCA_UNREFERENCED_PARAMETER(data);
 #if defined(_WIN32)
-    SecureZeroMemory(
-        const_cast<::utility::string_t::value_type *>(data->data()),
-        data->size() * sizeof(::utility::string_t::value_type));
+    SecureZeroMemory(&(*data)[0], data->size() * sizeof(::utility::string_t::value_type));
     delete data;
 #endif
 }
 }
 
-}
+}