Skip to content

Commit

Permalink
Improve resilience against memory attacks
Browse files Browse the repository at this point in the history
To reduce residual fragments of secret data in memory after
deallocation, this patch replaces the global delete operator with a
version that zeros out previously allocated memory. It makes use of
the new C++14 sized deallocation, but provides an unsized fallback
with platform-specific size deductions.

This change is only a minor mitigation and cannot protect against
buffer reallocations by the operating system or non-C++ libraries.
Thus, we still cannot guarantee all memory to be wiped after free.

As a further improvement, this patch uses libgcrypt and libsodium
to write long-lived master key component hashes into a secure
memory area and wipe it afterwards.

The patch also fixes compiler flags not being set properly on macOS.
  • Loading branch information
phoerious authored and droidmonkey committed Apr 21, 2019
1 parent c7898fd commit 13eb1c0
Show file tree
Hide file tree
Showing 14 changed files with 207 additions and 28 deletions.
18 changes: 14 additions & 4 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ option(WITH_ASAN "Enable address sanitizer checks (Linux / macOS only)" OFF)
option(WITH_COVERAGE "Use to build with coverage tests (GCC only)." OFF)
option(WITH_APP_BUNDLE "Enable Application Bundle for macOS" ON)

set(WITH_XC_ALL OFF CACHE BOOLEAN "Build in all available plugins")
set(WITH_XC_ALL OFF CACHE BOOL "Build in all available plugins")

option(WITH_XC_AUTOTYPE "Include Auto-Type." ON)
option(WITH_XC_NETWORKING "Include networking code (e.g. for downlading website icons)." OFF)
Expand Down Expand Up @@ -163,11 +163,15 @@ if("${CMAKE_SIZEOF_VOID_P}" EQUAL "4")
set(IS_32BIT TRUE)
endif()

if("${CMAKE_C_COMPILER}" MATCHES "clang$" OR "${CMAKE_C_COMPILER_ID}" STREQUAL "Clang")
if("${CMAKE_C_COMPILER}" MATCHES "clang$"
OR "${CMAKE_EXTRA_GENERATOR_C_SYSTEM_DEFINED_MACROS}" MATCHES "__clang__"
OR "${CMAKE_C_COMPILER_ID}" STREQUAL "Clang")
set(CMAKE_COMPILER_IS_CLANG 1)
endif()

if("${CMAKE_CXX_COMPILER}" MATCHES "clang(\\+\\+)?$" OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
if("${CMAKE_CXX_COMPILER}" MATCHES "clang(\\+\\+)?$"
OR "${CMAKE_EXTRA_GENERATOR_CXX_SYSTEM_DEFINED_MACROS}" MATCHES "__clang__"
OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
set(CMAKE_COMPILER_IS_CLANGXX 1)
endif()

Expand Down Expand Up @@ -264,6 +268,11 @@ endif()
add_gcc_compiler_cflags("-std=c99")
add_gcc_compiler_cxxflags("-std=c++11")

if((CMAKE_COMPILER_IS_GNUCXX AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 4.9.99) OR
(CMAKE_COMPILER_IS_CLANGXX AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 3.6.99))
add_gcc_compiler_cxxflags("-fsized-deallocation")
endif()

if(APPLE)
add_gcc_compiler_cxxflags("-stdlib=libc++")
endif()
Expand Down Expand Up @@ -387,14 +396,15 @@ find_package(Gcrypt 1.7.0 REQUIRED)
find_package(Argon2 REQUIRED)
find_package(ZLIB REQUIRED)
find_package(QREncode REQUIRED)
find_package(sodium 1.0.12 REQUIRED)

set(CMAKE_REQUIRED_INCLUDES ${ZLIB_INCLUDE_DIR})

if(ZLIB_VERSION_STRING VERSION_LESS "1.2.0")
message(FATAL_ERROR "zlib 1.2.0 or higher is required to use the gzip format")
endif()

include_directories(SYSTEM ${ARGON2_INCLUDE_DIR})
include_directories(SYSTEM ${ARGON2_INCLUDE_DIR} ${sodium_INCLUDE_DIR})

# Optional
if(WITH_XC_KEESHARE)
Expand Down
2 changes: 1 addition & 1 deletion INSTALL.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ The following libraries are required:
* zlib
* libmicrohttpd
* libxi, libxtst, qtx11extras (optional for auto-type on X11)
* libsodium (>= 1.0.12, optional for KeePassXC-Browser support)
* libsodium (>= 1.0.12)
* libargon2

Prepare the Building Environment
Expand Down
4 changes: 4 additions & 0 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ if(NOT ZXCVBN_LIBRARIES)
endif(NOT ZXCVBN_LIBRARIES)

set(keepassx_SOURCES
core/Alloc.cpp
core/AutoTypeAssociations.cpp
core/AutoTypeMatch.cpp
core/Compare.cpp
Expand Down Expand Up @@ -254,6 +255,8 @@ endif()

if(WITH_XC_TOUCHID)
list(APPEND keepassx_SOURCES touchid/TouchID.mm)
# TODO: Remove -Wno-error once deprecation warnings have been resolved.
set_source_files_properties(touchid/TouchID.mm PROPERTY COMPILE_FLAGS "-Wno-old-style-cast -Wno-error")
endif()

add_library(autotype STATIC ${autotype_SOURCES})
Expand All @@ -270,6 +273,7 @@ target_link_libraries(keepassx_core
Qt5::Concurrent
Qt5::Network
Qt5::Widgets
${sodium_LIBRARY_RELEASE}
${YUBIKEY_LIBRARIES}
${ZXCVBN_LIBRARIES}
${ARGON2_LIBRARIES}
Expand Down
3 changes: 1 addition & 2 deletions src/browser/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

if(WITH_XC_BROWSER)
include_directories(${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR})
find_package(sodium 1.0.12 REQUIRED)

set(keepassxcbrowser_SOURCES
BrowserAccessControlDialog.cpp
Expand All @@ -33,5 +32,5 @@ if(WITH_XC_BROWSER)
Variant.cpp)

add_library(keepassxcbrowser STATIC ${keepassxcbrowser_SOURCES})
target_link_libraries(keepassxcbrowser Qt5::Core Qt5::Concurrent Qt5::Widgets Qt5::Network sodium)
target_link_libraries(keepassxcbrowser Qt5::Core Qt5::Concurrent Qt5::Widgets Qt5::Network ${sodium_LIBRARY_RELEASE})
endif()
1 change: 1 addition & 0 deletions src/cli/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ target_link_libraries(keepassxc-cli
keepassx_core
Qt5::Core
${GCRYPT_LIBRARIES}
${sodium_LIBRARY_RELEASE}
${ARGON2_LIBRARIES}
${GPGERROR_LIBRARIES}
${ZLIB_LIBRARIES}
Expand Down
89 changes: 89 additions & 0 deletions src/core/Alloc.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
* Copyright (C) 2019 KeePassXC Team <[email protected]>
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 or (at your option)
* version 3 of the License.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

#include <QtGlobal>
#include <cstdint>
#include <sodium.h>
#ifdef Q_OS_MACOS
#include <malloc/malloc.h>
#else
#include <malloc.h>
#endif

#if defined(NDEBUG) && !defined(__cpp_sized_deallocation)
#warning "KeePassXC is being compiled without sized deallocation support. Deletes may be slow."
#endif

/**
* Custom sized delete operator which securely zeroes out allocated
* memory before freeing it (requires C++14 sized deallocation support).
*/
void operator delete(void* ptr, std::size_t size) noexcept
{
if (!ptr) {
return;
}

sodium_memzero(ptr, size);
std::free(ptr);
}

void operator delete[](void* ptr, std::size_t size) noexcept
{
::operator delete(ptr, size);
}

/**
* Custom delete operator which securely zeroes out
* allocated memory before freeing it.
*/
void operator delete(void* ptr) noexcept
{
if (!ptr) {
return;
}

#if defined(Q_OS_WIN)
::operator delete(ptr, _msize(ptr));
#elif defined(Q_OS_MACOS)
::operator delete(ptr, malloc_size(ptr));
#elif defined(Q_OS_UNIX)
::operator delete(ptr, malloc_usable_size(ptr));
#else
// whatever OS this is, give up and simply free stuff
std::free(ptr);
#endif
}

void operator delete[](void* ptr) noexcept
{
::operator delete(ptr);
}

/**
* Custom insecure delete operator that does not zero out memory before
* freeing a buffer. Can be used for better performance.
*/
void operator delete(void* ptr, bool) noexcept
{
std::free(ptr);
}

void operator delete[](void* ptr, bool) noexcept
{
::operator delete(ptr, false);
}
46 changes: 37 additions & 9 deletions src/keys/FileKey.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,35 @@

#include "FileKey.h"

#include <QFile>

#include "core/Tools.h"
#include "crypto/CryptoHash.h"
#include "crypto/Random.h"

#include <QFile>

#include <sodium.h>
#include <gcrypt.h>
#include <algorithm>
#include <cstring>

QUuid FileKey::UUID("a584cbc4-c9b4-437e-81bb-362ca9709273");

constexpr int FileKey::SHA256_SIZE;

FileKey::FileKey()
: Key(UUID)
, m_key(static_cast<char*>(gcry_malloc_secure(SHA256_SIZE)))
{
}

FileKey::~FileKey()
{
if (m_key) {
gcry_free(m_key);
m_key = nullptr;
}
}

/**
* Read key file from device while trying to detect its file format.
*
Expand Down Expand Up @@ -148,7 +164,10 @@ bool FileKey::load(const QString& fileName, QString* errorMsg)
*/
QByteArray FileKey::rawKey() const
{
return m_key;
if (!m_key) {
return {};
}
return QByteArray::fromRawData(m_key, SHA256_SIZE);
}

/**
Expand Down Expand Up @@ -223,12 +242,15 @@ bool FileKey::loadXml(QIODevice* device)
}
}

bool ok = false;
if (!xmlReader.error() && correctMeta && !data.isEmpty()) {
m_key = data;
return true;
std::memcpy(m_key, data.data(), std::min(SHA256_SIZE, data.size()));
ok = true;
}

return false;
sodium_memzero(data.data(), static_cast<std::size_t>(data.capacity()));

return ok;
}

/**
Expand Down Expand Up @@ -293,7 +315,8 @@ bool FileKey::loadBinary(QIODevice* device)
if (!Tools::readAllFromDevice(device, data) || data.size() != 32) {
return false;
} else {
m_key = data;
std::memcpy(m_key, data.data(), std::min(SHA256_SIZE, data.size()));
sodium_memzero(data.data(), static_cast<std::size_t>(data.capacity()));
return true;
}
}
Expand Down Expand Up @@ -321,12 +344,15 @@ bool FileKey::loadHex(QIODevice* device)
}

QByteArray key = QByteArray::fromHex(data);
sodium_memzero(data.data(), static_cast<std::size_t>(data.capacity()));

if (key.size() != 32) {
return false;
}

m_key = key;
std::memcpy(m_key, key.data(), std::min(SHA256_SIZE, key.size()));
sodium_memzero(key.data(), static_cast<std::size_t>(key.capacity()));

return true;
}

Expand All @@ -348,7 +374,9 @@ bool FileKey::loadHashed(QIODevice* device)
cryptoHash.addData(buffer);
} while (!buffer.isEmpty());

m_key = cryptoHash.result();
auto result = cryptoHash.result();
std::memcpy(m_key, result.data(), std::min(SHA256_SIZE, result.size()));
sodium_memzero(result.data(), static_cast<std::size_t>(result.capacity()));

return true;
}
Expand Down
5 changes: 4 additions & 1 deletion src/keys/FileKey.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ class FileKey : public Key
};

FileKey();
~FileKey() override;
bool load(QIODevice* device);
bool load(const QString& fileName, QString* errorMsg = nullptr);
QByteArray rawKey() const override;
Expand All @@ -48,14 +49,16 @@ class FileKey : public Key
static bool create(const QString& fileName, QString* errorMsg = nullptr, int size = 128);

private:
static constexpr int SHA256_SIZE = 32;

bool loadXml(QIODevice* device);
bool loadXmlMeta(QXmlStreamReader& xmlReader);
QByteArray loadXmlKey(QXmlStreamReader& xmlReader);
bool loadBinary(QIODevice* device);
bool loadHex(QIODevice* device);
bool loadHashed(QIODevice* device);

QByteArray m_key;
char* m_key = nullptr;
Type m_type = None;
};

Expand Down
24 changes: 20 additions & 4 deletions src/keys/PasswordKey.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (C) 2010 Felix Geyer <[email protected]>
* Copyright (C) 2019 KeePassXC Team <[email protected]>
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
Expand All @@ -16,35 +16,51 @@
*/

#include "PasswordKey.h"
#include "core/Tools.h"

#include "crypto/CryptoHash.h"
#include <gcrypt.h>
#include <algorithm>
#include <cstring>

QUuid PasswordKey::UUID("77e90411-303a-43f2-b773-853b05635ead");

constexpr int PasswordKey::SHA256_SIZE;

PasswordKey::PasswordKey()
: Key(UUID)
, m_key(static_cast<char*>(gcry_malloc_secure(SHA256_SIZE)))
{
}

PasswordKey::PasswordKey(const QString& password)
: Key(UUID)
, m_key(static_cast<char*>(gcry_malloc_secure(SHA256_SIZE)))
{
setPassword(password);
}

PasswordKey::~PasswordKey()
{
if (m_key) {
gcry_free(m_key);
m_key = nullptr;
}
}

QSharedPointer<PasswordKey> PasswordKey::fromRawKey(const QByteArray& rawKey)
{
auto result = QSharedPointer<PasswordKey>::create();
result->m_key = rawKey;
std::memcpy(result->m_key, rawKey.data(), std::min(SHA256_SIZE, rawKey.size()));
return result;
}

QByteArray PasswordKey::rawKey() const
{
return m_key;
return QByteArray::fromRawData(m_key, SHA256_SIZE);
}

void PasswordKey::setPassword(const QString& password)
{
m_key = CryptoHash::hash(password.toUtf8(), CryptoHash::Sha256);
std::memcpy(m_key, CryptoHash::hash(password.toUtf8(), CryptoHash::Sha256).data(), SHA256_SIZE);
}
Loading

0 comments on commit 13eb1c0

Please sign in to comment.